Remove unnecessary sync calls to FlushPendingLinkUpdates()

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P2
normal
3 months ago
15 days ago

People

(Reporter: bz, Assigned: jryans)

Tracking

(Blocks: 1 bug)

53 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
(Reporter)

Comment 1

3 months ago
On the other hand, maybe even the Gecko version should go away; see bug 1361709.
Depends on: 1361709
(Assignee)

Comment 2

3 months ago
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

3 months ago
If I'm right in bug 1361709 then it's not needed for those tests to pass...
Assignee: nobody → jryans
Priority: -- → P2
(Assignee)

Comment 4

26 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=af8bbab1f363d6219343ca20a9523f7a57ba95bf
(Assignee)

Comment 5

25 days ago
(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

23 days 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

23 days 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)
(Assignee)

Comment 9

20 days 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!
(Assignee)

Updated

20 days ago
Attachment #8882658 - Flags: review?(dbaron)
(Assignee)

Updated

20 days ago
See Also: → bug 1378275

Comment 11

15 days 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+
You need to log in before you can comment on or make changes to this bug.