Closed Bug 1265876 Opened 4 years ago Closed 4 years ago

replace Timer.jsm

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Iteration:
49.1 - May 9
Tracking Status
firefox49 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file, 1 obsolete file)

Replace Timer.jsm for the devtools de-chrome-ification project.
See also bug 1265840.

One idea might be to have the loader define setTimeout, setImmediate,
and clearTimeout, as it does for console.
Note that modules using the BrowserLoader should be able to use window.setTimeout.

Our loader is based on the SDK one. Irakli explicitely didn't included setTimeout, as, at the time he wrote the loader, setTimeout and others weren't part of the globals defined in ES6 modules.

Jorendorff seems to say it will have access to generic document globals:
https://hacks.mozilla.org/2015/08/es6-in-depth-modules/
He only mention document and XMLHttpRequest, but I imagine setTimeout will be part of it?!

If so, yes, we should just expose them (and possible others) in Loader's globals.
Perhaps porting the timeout-related parts of DevToolsUtils.js should be part of this bug.
MozReview-Commit-ID: HzbnNvTc5db

This removes Timer.jsm nearly entirely from devtools.  Exceptions are:

* Tests (no biggie IMO)
* Loader.jsm (required)
* shared/worker/loader.js (maybe required and anyway unusual)
* bootstrap.js
* ViewHelpers.jsm .. which I think has to be converted to a .js first
  anyhow
Not sure about DevToolsUtils any more, but this should also handle timer.js.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Attachment #8744008 - Attachment is obsolete: true
Comment on attachment 8744364 [details]
MozReview Request: Bug 1265876 - don't directly use Timer.jsm or timers.js in devtools; r=ochameau

https://reviewboard.mozilla.org/r/48489/#review45555

Looks good, thanks.

Do you plan to address the rest in followups?
Did you had a green try for these changes?
Attachment #8744364 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #6)

> Do you plan to address the rest in followups?

I'm not sure what remains.  Do you mean DevToolsUtils?  If so then yes,
I'll file a follow-up, since that file has a number of things requiring
investigation.  We may want to split it up.

> Did you had a green try for these changes?

No, normally I do the try runs post-review; mostly since that's what I
was told to do when I joined.
I did do some smoke tests here, so it's at least not completely broken :)
(In reply to Tom Tromey :tromey from comment #7)
> (In reply to Alexandre Poirot [:ochameau] from comment #6)
> 
> > Do you plan to address the rest in followups?
> 
> I'm not sure what remains.  Do you mean DevToolsUtils?

Yes, and ViewHelpers.jsm.
I was wondering what is the priority of bug 1182194 in devtools.html plans.
I converted most server side jsm to modules in this bug but never jumped to client ones.
ViewHelpers is bug 1266828.
I think the first goal is to be able to load the inspector in a tab.
So, we'll replace or rewrite any .jsm on that path.
Other ones will come later.
That try run looks terrible but it's because I neglected one update in timeline.js.
Comment on attachment 8744364 [details]
MozReview Request: Bug 1265876 - don't directly use Timer.jsm or timers.js in devtools; r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48489/diff/1-2/
Attachment #8744364 - Attachment description: MozReview Request: Bug 1265876 - don't directly use Timer.jsm or timers.js in devtools; r?ochameau → MozReview Request: Bug 1265876 - don't directly use Timer.jsm or timers.js in devtools; r=ochameau
Iteration: --- → 49.1 - May 9
Flags: qe-verify-
Priority: -- → P1
Keywords: checkin-needed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9039640&repo=fx-team
Flags: needinfo?(ttromey)
Looking.
Unclear to me how this could possibly cause a leak.
Flags: needinfo?(ttromey)
Comment on attachment 8744364 [details]
MozReview Request: Bug 1265876 - don't directly use Timer.jsm or timers.js in devtools; r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48489/diff/2-3/
The bug was that a test was using one clearTimeout but the code was changed to use
a different setTimeout.  Fixed by updating the test.
Keywords: checkin-needed
failed to apply:

applying 347f2154ac23
patching file devtools/client/inspector/shared/utils.js
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/inspector/shared/utils.js.rej
patching file devtools/client/shared/widgets/CubicBezierWidget.js
Hunk #1 FAILED at 20
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/shared/widgets/CubicBezierWidget.js.rej
Flags: needinfo?(ttromey)
Keywords: checkin-needed
I rebased and pushed it.
Flags: needinfo?(ttromey)
https://hg.mozilla.org/mozilla-central/rev/0f32a1437164
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.