Bug 105752

Created attachment 180725 [details] Patch

Created attachment 180726 [details] Moved cache to document.

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 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.

(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.

Created attachment 180773 [details] Introduced VistedLinkState.

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 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?

(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.

(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!

Created attachment 180807 [details] Patch for landing

Comment on attachment 180807 [details] Patch for landing Clearing flags on attachment: 180807 Committed r138509: <http://trac.webkit.org/changeset/138509>

All reviewed patches have been landed. Closing bug.

Thank you again Dimitri. :)

Re-opened since this is blocked by bug 105806

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

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

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?

(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.