Closed
Bug 1384041
Opened 7 years ago
Closed 7 years ago
Label the use of setTimeout of Timer.jsm in content-sessionStore.js
Categories
(Firefox :: Session Restore, enhancement)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: bevis, Assigned: bevis)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
According to the analysis in bug 1382172 comment 9, the usage of ThrottleTimeoutsCallback is high. We need this to be labeled to gain more benefit from qDOM scheduler.
There is one thing that block this labeling is to expose proper SchedulerGroup event target to content-sessionStore.js then we can pass this event target to Timer.jsm to set before initiating the timer.
Just FYI, bug 1383328 will expose labeled event targets to JS.
Assignee | ||
Comment 2•7 years ago
|
||
After further discussion with :wiwang who is working on content-sessionStore.js recently, to label these timercallbacks in content script, we can just replace the use of setTimeout() provided from Timer.jsm with content.setTimeout() in which the callback will be labeled internally by the TimeoutManager of the content window.
I'll take this bug to follow up!
Assignee: nobody → btseng
Flags: needinfo?(wiwang)
Assignee | ||
Comment 4•7 years ago
|
||
Some failures captured in the tests of devtools if using content.setTimeout():
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1efbdbf11b558ede20a0ef1843ed1d1f203a30bd&selectedJob=118870652
It might be related to the throttling in the TimeoutManager internally.
The alternative solution is to pass tabEventTarget to the setTimeout in Timer.jsm.
Assignee | ||
Comment 5•7 years ago
|
||
1. Define set{Timeout|Internal}WithTarget in Timer.jsm and give them names.
3. Labeling the use of setTimeout in content-sessionStore.js using tabEventTarget.
Treeherder result looks fine, btw,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=596041f4d44fd83c62ca82a84c60af4c192db11d
Attachment #8891909 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 6•7 years ago
|
||
1. Merge the logic of setTimeout()/setInternal() into _setTimeoutOrIsInterval().
2. Define set{Timeout|Internal}WithTarget() in Timer.jsm and give them names.
3. Label the use of setTimeout in content-sessionStore.js using tabEventTarget.
Attachment #8891909 -
Attachment is obsolete: true
Attachment #8891909 -
Flags: review?(wmccloskey)
Attachment #8891951 -
Flags: review?(wmccloskey)
Comment on attachment 8891951 [details] [diff] [review]
(v2) Patch: Label the use of setTimeout of Timer.jsm in content-sessionStore.js
Review of attachment 8891951 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/modules/Timer.jsm
@@ +32,5 @@
> + timer.target = aTarget;
> + }
> +
> + let callback = {
> + QueryInterface(iid) {
Two-space indent please. Also, it's easier to use XPCOMUtils.generateQI for this.
@@ +61,5 @@
> gTimerTable.set(id, timer);
> return id;
> }
>
> +this.setTimeout = function setTimeout(aCallback, aMilliseconds) {
Please add |...args| for these (i.e., rest parameters). Using |arguments| is slow and old-fashioned.
Attachment #8891951 -
Flags: review?(wmccloskey) → review+
Pushed by btseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef75a524b850
Label the use of setTimeout of Timer.jsm in content-sessionStore.js. r=billm
Comment 9•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in
before you can comment on or make changes to this bug.
Description
•