Page MenuHomePhabricator

[performance] Interval not cleared: 2 Intervals per file
Closed, ResolvedPublic

Description

Simply test it: During the whole upload process 2 intervalls *per file* are not cleared:

var se = window.setInterval;
window.setInterval = function (fn, t) {
  return se(function() {
    console.log(fn, t);
    fn();
  }, t)
};

If you upload 50 files, this means 100 events fire (all the time) within 500ms, executing a function that is computing some numbers from the DOM (object position) and setting CSS. Not good for old machines and slow browsers.

Culprit:
/extensions/UploadWizard/resources/mw.UploadWizardUploadInterface.js

moveFileInputToCover: function( selector ) {
//...

this.moveFileInputInterval = window.setInterval(function() {
  update();
  }, 500); 
}

Version: unspecified
Severity: major

Details

Reference
bz37997

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 12:31 AM
bzimport added a project: UploadWizard.
bzimport set Reference to bz37997.
bzimport added a subscriber: Unknown Object (MLST).

Change 75122 had a related patch set uploaded by Rillke:
Improved file-control positioning

https://gerrit.wikimedia.org/r/75122

Change 75122 abandoned by Rillke:
Improved file-control positioning

Reason:
Too much work to do. No time fixing boring white-space.

https://gerrit.wikimedia.org/r/75122

Change 75122 restored by Alex Monk:
Improved file-control positioning

Reason:
Don't abandon changes for such silly reasons.

https://gerrit.wikimedia.org/r/75122

Created attachment 13056
Screenshot showing Firebug, UploadWizard and Mozilla Firefox. CSS has been tweaked to make the input visible.

Attached:

UpWiz_fileinput_bug.png (980×1 px, 54 KB)

Change 75122 merged by jenkins-bot:
Improved file-control positioning

https://gerrit.wikimedia.org/r/75122

still an issue; timeouts must not be used for adjusting positions; use resize events or/and mutation observers

Rillke set Security to None.
MarkTraceur lowered the priority of this task from Medium to Low.Dec 3 2015, 6:26 PM

I don't believe this is an issue anymore. The interval is cleared once the upload is filled, and a new interval is created in the new empty upload. We could use events to do this, but there's no reason to rush to that right now.

matmarex claimed this task.
matmarex closed subtask T126712: Kill moveFileInputToCover as Resolved.
matmarex subscribed.

This should be gone for good with T126712.