Bug 105752 – Move visited link-checking (and caching) code out of SelectorChecker.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dimitri Glazkov (Google)]

Move visited link-checking (and caching) code out of SelectorChecker.

[Dimitri Glazkov (Google)]

Created attachment 180725 [details]
Patch

[Dimitri Glazkov (Google)]

Created attachment 180726 [details]
Moved cache to document.

[Dimitri Glazkov (Google)]

Sorry, I chickened out and decided to move visited-link cache to Document, which is where it was (life/scope-wise) before. This change now has no behavior changes.

[Antti Koivisto]

Comment on attachment 180726 [details]
Moved cache to document.

View in context: https://bugs.webkit.org/attachment.cgi?id=180726&action=review

This doesn't seem like a progression to me. The code get spread out and the already bloaty Document grows further.

 Why not factor this into a type? LinkStyleInvalidator or similar.

> Source/WebCore/html/HTMLAnchorElement.h:107
> +    static EInsideLink determineLinkState(Element*);
> +    static LinkHash linkHashForElement(Element*);

Hanging generic Element functions to HTMLAnchorElement is confusing.

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> (From update of attachment 180726 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180726&action=review
> 
> This doesn't seem like a progression to me. The code get spread out and the already bloaty Document grows further.
> 
>  Why not factor this into a type? LinkStyleInvalidator or similar.

Okay. How about dom/VisitedLinkState? I'll also move the determineLinkState, et al. there, so now it will truly represent the visited link state for the Document.

[Dimitri Glazkov (Google)]

Created attachment 180773 [details]
Introduced VistedLinkState.

[Allan Sandfeld Jensen]

Comment on attachment 180773 [details]
Introduced VistedLinkState.

View in context: https://bugs.webkit.org/attachment.cgi?id=180773&action=review

> Source/WebCore/html/HTMLAnchorElement.h:101
> -    
> +

Please try to not include this change in the patch, since it would record the file as changed when it is otherwise not.

[Antti Koivisto]

Comment on attachment 180773 [details]
Introduced VistedLinkState.

View in context: https://bugs.webkit.org/attachment.cgi?id=180773&action=review

Looks good.

> Source/WebCore/dom/VisitedLinkState.h:49
> +    void invalidateStylesForAllLinks();
> +    void invalidateStylesForLink(LinkHash);

Maybe singular Style?

[Dimitri Glazkov (Google)]

(In reply to comment #7)
> (From update of attachment 180773 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180773&action=review
> 
> > Source/WebCore/html/HTMLAnchorElement.h:101
> > -    
> > +
> 
> Please try to not include this change in the patch, since it would record the file as changed when it is otherwise not.

Will remove when landing.

[Dimitri Glazkov (Google)]

(In reply to comment #8)
> (From update of attachment 180773 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180773&action=review
> 
> Looks good.
> 
> > Source/WebCore/dom/VisitedLinkState.h:49
> > +    void invalidateStylesForAllLinks();
> > +    void invalidateStylesForLink(LinkHash);
> 
> Maybe singular Style?

Sure thing!

[Dimitri Glazkov (Google)]

Created attachment 180807 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 180807 [details]
Patch for landing

Clearing flags on attachment: 180807

Committed r138509: <http://trac.webkit.org/changeset/138509>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Thank you again Dimitri. :)

[WebKit Review Bot]

Re-opened since this is blocked by bug 105806

[Dimitri Glazkov (Google)]

My bad. Got a bit too happy with moving ptrs. Relanding with a fix shortly...

[Dimitri Glazkov (Google)]

Committed r138515: <http://trac.webkit.org/changeset/138515>

[Darin Adler]

Comment on attachment 180807 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=180807&action=review

> Source/WebCore/dom/VisitedLinkState.cpp:48
> +inline static const AtomicString* linkAttribute(Element* element)

A little strange to use "inline static" rather than the usual "static inline" that we use elsewhere.

> Source/WebCore/dom/VisitedLinkState.cpp:66
> +VisitedLinkState::VisitedLinkState(Document* document)
> +    :m_document(document)
> +{ }

Two formatting errors:

1) Missing space after colon.
2) Braces should not be on a single line.

> Source/WebCore/dom/VisitedLinkState.cpp:83
> +    if (const AtomicString* attr = linkAttribute(element))
> +        return WebCore::visitedLinkHash(document->baseURL(), *attr);

Would be better to use the word "attribute" instead of the abbreviation "attr".

> Source/WebCore/dom/VisitedLinkState.cpp:108
> +    // An empty href refers to the document itself which is always visited. It is useful to check this explicitly so
> +    // that visited links can be tested in platform independent manner, without explicit support in the test harness.
> +    if (attribute->isEmpty())
> +        return InsideVisitedLink;

What about an href that consists only of whitespace? Does the visitedLinkHash function trim whitespace when resolving the URL?

[Dimitri Glazkov (Google)]

(In reply to comment #18)
> (From update of attachment 180807 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180807&action=review
> 
> > Source/WebCore/dom/VisitedLinkState.cpp:48
> > +inline static const AtomicString* linkAttribute(Element* element)
> 
> A little strange to use "inline static" rather than the usual "static inline" that we use elsewhere.
> 
> > Source/WebCore/dom/VisitedLinkState.cpp:66
> > +VisitedLinkState::VisitedLinkState(Document* document)
> > +    :m_document(document)
> > +{ }
> 
> Two formatting errors:
> 
> 1) Missing space after colon.
> 2) Braces should not be on a single line.
> 
> > Source/WebCore/dom/VisitedLinkState.cpp:83
> > +    if (const AtomicString* attr = linkAttribute(element))
> > +        return WebCore::visitedLinkHash(document->baseURL(), *attr);
> 
> Would be better to use the word "attribute" instead of the abbreviation "attr".
>

Thank you for spotting these! Fixed in http://trac.webkit.org/changeset/138519.

> > Source/WebCore/dom/VisitedLinkState.cpp:108
> > +    // An empty href refers to the document itself which is always visited. It is useful to check this explicitly so
> > +    // that visited links can be tested in platform independent manner, without explicit support in the test harness.
> > +    if (attribute->isEmpty())
> > +        return InsideVisitedLink;
> 
> What about an href that consists only of whitespace? Does the visitedLinkHash function trim whitespace when resolving the URL?

I don't know. I was just moving the pipes around.