Minor refactoring of nsRefreshDriver::Tick
Categories
(Core :: Layout, task)
Tracking
()
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
Attachments
(9 files)
Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r?tnikkel
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r=tnikkel
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-esr115+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-esr115+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-esr115+
|
Details | Review |
Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r?tnikkel
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-beta+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-beta+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-beta+
|
Details | Review |
Filing this bug to do some refactoring in nsRefreshDriver::Tick, to make that massive function slightly-less-massive and to make debugging a bit more straightforward.
Assignee | ||
Comment 1•1 year ago
|
||
This doesn't affect behavior; it's just a pure refactoring, moving code from
being inline to being in a helper function.
Assignee | ||
Comment 2•1 year ago
|
||
This is a bit gross, but it helps us make better use of the backtrace to figure
out what's going on in a given crash report.
Depends on D191908
Assignee | ||
Comment 3•1 year ago
|
||
Try (static-analysis to answer the annotation question in part 1, and linux-reftests to get some minimal functional testing, even though this shouldn't change behavior):
https://treeherder.mozilla.org/jobs?repo=try&revision=d04e24ce6e3d25131babb2819c90e194ac71ac1e
Assignee | ||
Comment 4•1 year ago
|
||
This patch doesn't change behavior; it just removes some redundant checks.
(Note that the tight-scoping around nsAutoMicroTask/ReduceAnimations is
important for the RAII object to be torn down at the intended time; but it
doesn't need its own if-check.)
Depends on D191909
Comment 6•1 year ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7eddf33b21eb
https://hg.mozilla.org/mozilla-central/rev/e92559c7af9c
https://hg.mozilla.org/mozilla-central/rev/f19db7c059b3
Assignee | ||
Comment 7•1 year ago
|
||
This doesn't affect behavior; it's just a pure refactoring, moving code from
being inline to being in a helper function.
Original Revision: https://phabricator.services.mozilla.com/D191908
Updated•1 year ago
|
Assignee | ||
Comment 8•1 year ago
|
||
This patch doesn't change behavior.
This is a bit gross, but it helps us make better use of the backtrace to figure
out what's going on in a given crash report.
Original Revision: https://phabricator.services.mozilla.com/D191909
Updated•1 year ago
|
Assignee | ||
Comment 9•1 year ago
|
||
This patch doesn't change behavior; it just removes some redundant checks.
(Note that the tight-scoping around nsAutoMicroTask/ReduceAnimations is
important for the RAII object to be torn down at the intended time; but it
doesn't need its own if-check.)
Original Revision: https://phabricator.services.mozilla.com/D191911
Updated•1 year ago
|
Comment 11•1 year ago
|
||
Uplift Approval Request
- Risk associated with taking this patch: Low
- Explanation of risk level: No behavior change; this patch-stack is purely a refatoring of existing code.
- Needs manual QE test: no
- Fix verified in Nightly: yes
- Steps to reproduce for manual QE testing: N/A
- User impact if declined: None
- Code covered by automated testing: yes
- String changes made/needed: None
- Is Android affected?: no
Comment 12•1 year ago
|
||
We should take this on Beta as well if it's wanted on ESR this cycle.
Assignee | ||
Comment 13•1 year ago
|
||
Sounds good. For whatever reasons, the crashes that this might help with are most-prevalent on ESR so that's where I ultimately care about this reaching; but there's no problem having it on beta as well. I'll request uplift there.
Assignee | ||
Comment 14•1 year ago
|
||
This doesn't affect behavior; it's just a pure refactoring, moving code from
being inline to being in a helper function.
Original Revision: https://phabricator.services.mozilla.com/D191908
Updated•1 year ago
|
Assignee | ||
Comment 15•1 year ago
|
||
This patch doesn't change behavior.
This is a bit gross, but it helps us make better use of the backtrace to figure
out what's going on in a given crash report.
Original Revision: https://phabricator.services.mozilla.com/D191909
Updated•1 year ago
|
Assignee | ||
Comment 16•1 year ago
|
||
This patch doesn't change behavior; it just removes some redundant checks.
(Note that the tight-scoping around nsAutoMicroTask/ReduceAnimations is
important for the RAII object to be torn down at the intended time; but it
doesn't need its own if-check.)
Original Revision: https://phabricator.services.mozilla.com/D191911
Updated•1 year ago
|
Comment 17•1 year ago
|
||
Uplift Approval Request
- Is Android affected?: no
- String changes made/needed: None
- Code covered by automated testing: yes
- User impact if declined: None
- Steps to reproduce for manual QE testing: N/A
- Fix verified in Nightly: yes
- Needs manual QE test: no
- Explanation of risk level: No behavior change; this patch-stack is purely a refatoring of existing code.
- Risk associated with taking this patch: Low
Comment 18•1 year ago
|
||
Comment on attachment 9360456 [details]
Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r?tnikkel
Approved for 120.0b4
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Comment 19•1 year ago
|
||
uplift |
Comment 20•1 year ago
|
||
Comment on attachment 9360450 [details]
Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r=tnikkel
Approved for 115.5esr
Updated•1 year ago
|
Updated•1 year ago
|
Comment 21•1 year ago
|
||
Comment on attachment 9360450 [details]
Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r=tnikkel
:dholbert the esr115 patches need to be rebased with esr115 due to conflicts with bug 1830820
Updated•1 year ago
|
Assignee | ||
Comment 22•1 year ago
•
|
||
(In reply to Dianna Smith [:diannaS] from comment #21)
:dholbert the esr115 patches need to be rebased with esr115 due to conflicts with bug 1830820
Hmm, it's not actually a conflict with that bug; it's rather just that the contextual code is different on ESR115. (The uplift for bug 1830820 actulaly removed a difference from esr115 vs. central, and hence made for a cleaner rebase operation here, not a messier one. But there are still other contextual differences that lando didn't detect when generating the ESR-specific uplift revisions.)
(I don't expect lando/phabricator to be able to resolve those conflicts, but it's odd that it didn't notice them.)
Assignee | ||
Comment 23•1 year ago
|
||
Specifically: esr115-rebase-wise, this snippet has a EvaluateMediaQueriesAndReportChanges()
call on mozilla-central (and beta) but not on ESR115, because it was added in bug 1451717:
https://searchfox.org/mozilla-central/rev/4f15c940fa206cb29e4c6992a9d8ba2eb4e7df88/layout/base/nsRefreshDriver.cpp#2619-2621
Here's the equivalent snippet on esr115, missing that middle line (because bug 1451717 didn't land there):
https://searchfox.org/mozilla-esr115/rev/d39c91545f0f5086bb26e2dee46d31ae5344b2b0/layout/base/nsRefreshDriver.cpp#2616-2617
This is ~fine, but it does cause a merge conflict -- the "part 1" patch here is moving that whole chunk of code from one place to another, so that trivially creates a merge conflict. (Trivial for a human to work around, but should have been detected/flagged when generating the lando ESR uplift request, presumably.)
Assignee | ||
Comment 24•1 year ago
•
|
||
The other merge-conflict is from some telemetry code that we removed on central in https://hg.mozilla.org/mozilla-central/rev/65677e6aa05d#l1.57 but still exists on ESR115.
That change is slightly awkward to rebase around (since it involves some state that straddles code that's now split between two functions).
Given that the lines in question are just for unused telemetry (per the commit message in the commit that removed it from central), it seems like we should just uplift that patch as well, rather than trying to delicately edit this bug's patch to preserve those lines to still collect apparently-unused telemetry.
Assignee | ||
Comment 25•1 year ago
•
|
||
So I'd like to request ESR115 uplift for bug 1843534's https://phabricator.services.mozilla.com/D185007 specifically [EDIT: here's the esr115 uplift version of that patch] as a prereq to the patch stack here. (It's the patch that's creating a merge conflict per comment 24). It's a pure unused-code-removal (with the removed code being otherwise slightly awkward to work around).
Not sure what the cleanest way to do that is. I'll start out by trying to do a specific uplift request for that patch via moz-phab if I can figure out how to do that without creating uplift-requests for the rest of the patches on that bug...
Assignee | ||
Comment 26•1 year ago
|
||
ESR115 patches (part 1 in particular) is now updated to:
(1) work around the missing line in the code-that's-being-moved-with-no-changes (per comment 23)
(2) be based off of the code-removal referenced in comment 24 - comment 25 (which makes parts 1 and 2 here simpler to rebase & reduces opportunity for human error). I've incorporated that into the ESR115 patch stack via "edit related revisions".
I've verified that the patch stack here now applies cleanly onto up-to-date mozilla-esr115 (with moz-phab patch D191987 --apply-to esr115
).
DiannaS: sorry for the trouble; the ball's back in your court, and this is good to land assuming you're OK to approve the uplift request on the supporting code-removal patch.
Assignee | ||
Comment 27•1 year ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #22)
(I don't expect lando/phabricator to be able to resolve those conflicts, but it's odd that it didn't notice them.)
side note: I filed bug 1861780 on making the lando-based uplift process more helpful here, FWIW.
Comment 28•1 year ago
•
|
||
Thank you!!!
The only issue i have now is that https://phabricator.services.mozilla.com/D191985 seems to be for m-c instead of esr115. not sure what happened otherwise i could land them all at once base on the dependencies you set. Im gonna take bug 1843534 for now.
nvm i think i fixed it will land shortly
Updated•1 year ago
|
Assignee | ||
Comment 29•1 year ago
|
||
Thanks!
Comment 30•1 year ago
|
||
uplift |
Updated•1 year ago
|
Updated•1 year ago
|
Comment 31•1 year ago
|
||
(In reply to Dianna Smith [:diannaS] from comment #28)
Thank you!!!
The only issue i have now is that https://phabricator.services.mozilla.com/D191985 seems to be for m-c instead of esr115. not sure what happened otherwise i could land them all at once base on the dependencies you set. Im gonna take bug 1843534 for now.nvm i think i fixed it will land shortly
The repo was probably set to mozilla-central when the patch was updated due to bug 1828731
Assignee | ||
Comment 32•1 year ago
|
||
Yup, that sounds likely - thanks for the reference. (I did use moz-phab submit
when updating the uplift patch here.)
Description
•