Closed
Bug 1361618
Opened 7 years ago
Closed 6 years ago
Remove unnecessary sync calls to FlushPendingLinkUpdates()
Categories
(Core :: CSS Parsing and Computation, enhancement, P2)
Tracking
()
RESOLVED
FIXED
mozilla56
People
(Reporter: bzbarsky, Assigned: mrbkap)
References
Details
Attachments
(1 file, 1 obsolete file)
No description provided.
Reporter | ||
Comment 1•7 years ago
|
||
On the other hand, maybe even the Gecko version should go away; see bug 1361709.
Depends on: 1361709
Seems related to :visited work in bug 1328509, I can investigate this. It's possible this is needed for :visited tests to pass anyway...?
Reporter | ||
Comment 3•7 years ago
|
||
If I'm right in bug 1361709 then it's not needed for those tests to pass...
Updated•7 years ago
|
Assignee: nobody → jryans
Priority: -- → P2
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=af8bbab1f363d6219343ca20a9523f7a57ba95bf I removed the two calls to `FlushPendingLinkUpdates` in nsCSSFrameConstructor and GeckoRestyleManager, and tests look good, but it's a bit tricky to have confidence about what's happening here. :bz and I looked through what the pending link code is attempting to do: * When a new anchor is bound in `BindToTree` we do: 1. Link::ResetLinkState(false, Link::ElementHasHref()); * Set link's mLinkState to default (either unvisited or not link) * Set element's link mState bits to default (either unvisited or no link state) 2. doc->RegisterPendingLinkUpdate(this); * Schedules idle dispatch to run `FlushPendingLinkUpdates` within 1 sec * In `FlushPendingLinkUpdates`: * For each pending link, call element->UpdateLinkState(link->LinkState()); 1. Register link for async history update to get potential future visited state 2. mLinkState is still unvisited / not link until we hear from history, so element state is unchanged Thus, there seems to be no need to call `FlushPendingLinkUpdates` outside of `BindToTree`, since visited state is always applied async anyway (so it doesn't work as an optimization to avoid restyling if visited, since that will trigger later).
Summary: stylo: Stylo needs some equivalent of the FlushPendingLinkUpdates() in ElementRestyler::Restyle → Remove unnecessary sync calls to FlushPendingLinkUpdates()
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8882658 [details] Bug 1361618 - Remove unnecessary calls to FlushPendingLinkUpdates. https://reviewboard.mozilla.org/r/153736/#review159094 ::: commit-message-cc903:21 (Diff revision 1) > + 1. Register link for async history update to get potential future visited > + state > + 2. mLinkState is still unvisited / not link until we hear from history, so > + element state is unchanged > + > +Thus, there seems to be no need to call `FlushPendingLinkUpdates` outside of I don't understand this comment. You explicitly remove the synchronous handling.
Comment 8•7 years ago
|
||
Comment on attachment 8882658 [details] Bug 1361618 - Remove unnecessary calls to FlushPendingLinkUpdates. I don't think I should review this. Either bz or dbaron should take a look. (I can't convince myself immediately why this change is ok.)
Attachment #8882658 -
Flags: review?(bugs)
Comment 9•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8882658 [details] Bug 1361618 - Remove unnecessary calls to FlushPendingLinkUpdates. https://reviewboard.mozilla.org/r/153736/#review159094 > I don't understand this comment. > You explicitly remove the synchronous handling. I agree it's confusing! The tricky part here is that although `FlushPendingLinkUpdates` feels synchronous, it only ever sets the default link state. It calls `LinkState()`[1] which returns that default value, and also registers a history listener for a potential _async_ visited update. [1]: http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/dom/base/Link.cpp#190
(In reply to Olli Pettay [:smaug] from comment #8) > Comment on attachment 8882658 [details] > Bug 1361618 - Remove unnecessary calls to FlushPendingLinkUpdates. > > I don't think I should review this. Either bz or dbaron should take a look. > (I can't convince myself immediately why this change is ok.) I'll try :dbaron, as he may have more context about this. :bz and I looked at this code during the SF work week, and we thought it seemed okay to remove as I've done here, but it's hard to feel confident about it!
Attachment #8882658 -
Flags: review?(dbaron)
See Also: → 1378275
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8882658 [details] Bug 1361618 - Remove unnecessary calls to FlushPendingLinkUpdates. https://reviewboard.mozilla.org/r/153736/#review161010 It's probably been too long since I've really looked at this code for me to say all that much useful here -- other than noting that link coloring becoming async was something that happened *while* I was developing (and most of the way through the work) the original patches for the :visited privacy attack, so it's entirely possible (if this goes back to that landing -- didn't check if it did) that it was the result of the code working differently when I actually wrote it, prior to landing. Other than that, I'm largely trusting your explanation here (which seems reasonable) and the fact that you and bz looked into it. Sorry for not getting to this earlier -- I'd been thinking I was actually going to really understand how this all worked again, but that's probably not really necessary to do this review.
Attachment #8882658 -
Flags: review?(dbaron) → review+
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/9a7b90cf8a63 Remove unnecessary calls to FlushPendingLinkUpdates. r=dbaron
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9a7b90cf8a63
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 15•7 years ago
|
||
Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/mozilla-central/rev/2fba314d7de7 Backed out changeset 9a7b90cf8a63 for frequently failing accessible/tests/browser/states/browser_test_link.js (bug 1385498). r=backout a=backout
Comment 16•7 years ago
|
||
Backed out for frequently failing accessible/tests/browser/states/browser_test_link.js (bug 1385498): https://hg.mozilla.org/mozilla-central/rev/2fba314d7de77ad8ab693a2ea0112c0cda5dd564 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=119118241&repo=autoland [task 2017-07-28T22:39:48.443877Z] 22:39:48 INFO - Entering test bound [task 2017-07-28T22:39:48.446603Z] 22:39:48 INFO - Buffered messages logged at 22:39:02 [task 2017-07-28T22:39:48.449340Z] 22:39:48 INFO - TEST-PASS | accessible/tests/browser/states/browser_test_link.js | Accessible document present. - [task 2017-07-28T22:39:48.452595Z] 22:39:48 INFO - TEST-PASS | accessible/tests/browser/states/browser_test_link.js | state bits should not be present in ID ['a@id="link_traversed" node', address: http://www.example.com/, role: link, name: 'example.com', address: [xpconnect wrapped nsIAccessible]]! - [task 2017-07-28T22:39:48.461912Z] 22:39:48 INFO - Buffered messages finished [task 2017-07-28T22:39:48.464412Z] 22:39:48 INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/states/browser_test_link.js | Test timed out - [task 2017-07-28T22:39:48.466754Z] 22:39:48 INFO - GECKO(1104) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration. [task 2017-07-28T22:39:48.470899Z] 22:39:48 INFO - GECKO(1104) | MEMORY STAT | vsize 863MB | residentFast 232MB | heapAllocated 92MB [task 2017-07-28T22:39:48.472347Z] 22:39:48 INFO - TEST-OK | accessible/tests/browser/states/browser_test_link.js | took 45330ms [task 2017-07-28T22:39:48.481589Z] 22:39:48 INFO - checking window state [task 2017-07-28T22:39:48.483780Z] 22:39:48 INFO - Not taking screenshot here: see the one that was previously logged [task 2017-07-28T22:39:48.485640Z] 22:39:48 INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/states/browser_test_link.js | Found a browser window after previous test timed out -
Status: RESOLVED → REOPENED
Flags: needinfo?(jryans)
Resolution: FIXED → ---
Comment hidden (Intermittent Failures Robot) |
Clearing ni? for now, but hoping to try again here when time allows.
Flags: needinfo?(jryans)
I think it's unlikely that I will return to this soon.
Assignee: jryans → nobody
Assignee | ||
Comment 21•6 years ago
|
||
I'd like to re-land this and see if we're still failing browser_test_link.js -- I've pushed to try twice and tried running the test countless times locally without ever seeing it fail. I'm hoping (optimistically, I admit) that other fixes have fixed whatever it was that made this patch bounce the first time. At the very least, pushing this again should allow me to inspect new logs and possibly push logging patches to figure out what's going on.
Assignee: nobody → mrbkap
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8882658 -
Attachment is obsolete: true
Assignee | ||
Comment 23•6 years ago
|
||
David, this patch is equivalent to the one that you reviewed 7 months ago, I needed a new mozreview that I could re-open and interact with.
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8950428 [details] Bug 1361618 - Remove unnecessary calls to FlushPendingLinkUpdates. https://reviewboard.mozilla.org/r/219674/#review225430
Attachment #8950428 -
Flags: review?(dbaron) → review+
Comment 25•6 years ago
|
||
Pushed by mrbkap@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/37b91f33d4f3 Remove unnecessary calls to FlushPendingLinkUpdates. r=dbaron
Comment 26•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/37b91f33d4f3
Status: REOPENED → RESOLVED
Closed: 7 years ago → 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•