1361274 - querySelectorAll should not call nsIDocument::FlushPendingLinkUpdates

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

-nsINode::QuerySelectorAll(nsAString const&, mozilla::ErrorResult&)                       87.7%        1.8% 
 | +moz_xmalloc                                                                             1.8%        0.0%   
 | -nsCSSRuleProcessor::SelectorListMatches()                                              10.5%        0.0% 
 | |  SelectorMatches(                                                                      3.5%        3.5%   
 | | +SelectorMatchesTree()                                                                 7.0%        3.5%   
 | -nsIDocument::FlushPendingLinkUpdates()                                                 73.7%        0.0%   
 | | -mozilla::dom::Link::LinkState() const                                                71.9%        1.8%   
 | | | +PLDHashTable::Add(void const*)                                                      1.8%        0.0%   
 | | | -mozilla::dom::HTMLAnchorElement::GetHrefURI() const                                56.1%        0.0%   
 | | | |  nsCOMPtr_base::begin_assignment()                                                 1.8%        1.8%   
 | | | | -nsGenericHTMLElement::GetHrefURIForAnchors() const                               54.4%        0.0%   
 | | | | |  mozilla::net::nsStandardURL::Release()                                          1.8%        1.8%   
 | | | | | -nsGenericHTMLElement::GetURIAttr(nsIAtom*, nsIAtom*, nsIURI**) const           52.6%        1.8%   
 | | | | | | +NS_NewURI(nsIURI**, nsAString const&, char const*, nsIURI*, nsIIOService*)   33.3%        0.0%   
 | | | | | | +nsIContent::GetBaseURI(bool) const                                           14.0%        7.0%  
 | | | | | |  nsIDocument::GetBaseURI(bool) const                                           3.5%        3.5%  
 | | | +mozilla::places::History::RegisterVisitedCallback(nsIURI*, mozilla::dom::Link*)    12.3%        1.8%  
 | |  nsDocument::AddStyleRelevantLink(mozilla::dom::Link*)                                 1.7%        1.7%

Comment 1

[Boris Zbarsky [:bzbarsky]]

Is the testcase basically inserting a bunch of links, then doing a querySelectoAll, then repeating that in a loop or something?

In any case, speeding up FlushPendingLinkUpdates is not trivial, because it has to do real work: get the attribute, parse the URI, etc, etc.

But why are we calling it from querySelectorAll at all?  The only point of FlushPendingLinkUpdates, I believe, is so we can tell apart :link and :visited.  But querySelectorAll treats all links as unvisited anyway.  It's possible the current setup was created before we stopped matching :visited in querySelectorAll.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: There's no need to update link :visited state when doing querySelectorAll, since querySelectorAll ignores that state anyway

Comment 3

[Robert Longson [:longsonr]]

Comment on attachment 8863983 [details] [diff] [review]
There's no need to update link :visited state when doing querySelectorAll, since querySelectorAll ignores that state anyway


>+  // TreeMatchContext constructor is to not call it all the time furing various

s/furing/during/

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8863983 [details] [diff] [review]
There's no need to update link :visited state when doing querySelectorAll, since querySelectorAll ignores that state anyway

s/furing/during/

I guess change to nsComputedDOMStyle::UpdateCurrentStyleSources is ok, since
we don't flush link states anyhow before layout/style flushes.
And ElementRestyler::Restyle, nsCSSFrameConstructor::ResolveStyleContext etc. do link flushing.

But don't we need link flushing for :link to work?

I don't understand eNeverMatchVisited. That seems to be for the private browsing flag control.
Isn't eRelevantLinkUnvisited the key thing here? Am I confused here now?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I guess change to nsComputedDOMStyle::UpdateCurrentStyleSources is ok

Right, because computed style never depends on :visited state: it always pretends the link only matched the :link rules.

> But don't we need link flushing for :link to work?

I don't believe we do.  Here's why, using HTMLAnchorElement as an example.

When it's first created, it calls the Link() constructor, which sets mLinkState to eLinkState_NotLink.  At his point neither NS_EVENT_STATE_VISITED nor NS_EVENT_STATE_UNVISITED is set, which means neither :link nor :visited is set, which is correct.

If the "href" attribute is now set (and in fact any time it's modified or removed), we will call Link::ResetLinkState (from HTMLAnchorElement::SetAttr/UnsetAttr).  ResetLinkState will always set mLinkState to defaultState, which is either eLinkState_Unvisited if we have an href attr or eLinkState_NotLink if we do not.  It will also make the relevant UpdateState/UpdateLinkState calls on the element to set NS_EVENT_STATE_UNVISITED on it.  So once the "href" attribute has been set, we'll be in NS_EVENT_STATE_UNVISITED and match :link.  The only thing we're waiting on FlishPendingLinkStates for at that point is maybe switching us to NS_EVENT_STATE_VISITED.

This didn't use to be the case back when <a href="something not parseable as URI"> was not considered to match :link, which is why the current setup is perhaps more complicated than it should be...

> I don't understand eNeverMatchVisited.

Ah, good catch.  This stuff could really use better documentation, which I will add.  But the summary is that eNeverMatchVisited is a promise that mVisitedHandling will _always_ be eRelevantLinkUnvisited for this TreeMatchContext.  And yes, eRelevantLinkUnvisited is the real key here.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Aha, didn't think (Un)SetAttr + ResetLinkState.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: With better comments and commit message

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: With better comments and commit message

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8864134 [details] [diff] [review]
With better comments and commit message

eNeverMatchVisited is still odd beast given how it just controls whether pb flag is checked.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06d436f9330d
There's no need to update link :visited state when doing querySelectorAll, since querySelectorAll ignores that state anyway.  r=smaug

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06d436f9330d