Bug 105752 - Move visited link-checking (and caching) code out of SelectorChecker.
Summary: Move visited link-checking (and caching) code out of SelectorChecker.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on: 105806
Blocks: 89879
  Show dependency treegraph
 
Reported: 2012-12-25 20:33 PST by Dimitri Glazkov (Google)
Modified: 2012-12-27 19:20 PST (History)
11 users (show)

See Also:


Attachments
Patch (21.47 KB, patch)
2012-12-25 20:44 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Moved cache to document. (20.44 KB, patch)
2012-12-25 21:07 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Introduced VistedLinkState. (30.37 KB, patch)
2012-12-26 18:21 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch for landing (29.87 KB, patch)
2012-12-27 10:11 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2012-12-25 20:33:32 PST
Move visited link-checking (and caching) code out of SelectorChecker.
Comment 1 Dimitri Glazkov (Google) 2012-12-25 20:44:55 PST
Created attachment 180725 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2012-12-25 21:07:36 PST
Created attachment 180726 [details]
Moved cache to document.
Comment 3 Dimitri Glazkov (Google) 2012-12-25 21:08:58 PST
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.
Comment 4 Antti Koivisto 2012-12-26 00:35:14 PST
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.
Comment 5 Dimitri Glazkov (Google) 2012-12-26 10:19:19 PST
(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.
Comment 6 Dimitri Glazkov (Google) 2012-12-26 18:21:51 PST
Created attachment 180773 [details]
Introduced VistedLinkState.
Comment 7 Allan Sandfeld Jensen 2012-12-27 03:35:54 PST
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.
Comment 8 Antti Koivisto 2012-12-27 09:23:39 PST
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?
Comment 9 Dimitri Glazkov (Google) 2012-12-27 10:03:40 PST
(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.
Comment 10 Dimitri Glazkov (Google) 2012-12-27 10:03:52 PST
(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!
Comment 11 Dimitri Glazkov (Google) 2012-12-27 10:11:38 PST
Created attachment 180807 [details]
Patch for landing
Comment 12 WebKit Review Bot 2012-12-27 10:42:29 PST
Comment on attachment 180807 [details]
Patch for landing

Clearing flags on attachment: 180807

Committed r138509: <http://trac.webkit.org/changeset/138509>
Comment 13 WebKit Review Bot 2012-12-27 10:42:34 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Eric Seidel (no email) 2012-12-27 10:55:08 PST
Thank you again Dimitri. :)
Comment 15 WebKit Review Bot 2012-12-27 12:56:12 PST
Re-opened since this is blocked by bug 105806
Comment 16 Dimitri Glazkov (Google) 2012-12-27 14:38:35 PST
My bad. Got a bit too happy with moving ptrs. Relanding with a fix shortly...
Comment 17 Dimitri Glazkov (Google) 2012-12-27 14:58:07 PST
Committed r138515: <http://trac.webkit.org/changeset/138515>
Comment 18 Darin Adler 2012-12-27 18:05:45 PST
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?
Comment 19 Dimitri Glazkov (Google) 2012-12-27 19:20:41 PST
(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.