Bug 51941 – refactor HTMLLinkElement to allow Link header implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Gavin Peters]

Right now a lot of Link handling behaviour is specifically buried in HTMLLinkElement.  Bug 51940 is to implement the new Link header, which will exist without there being an HTMLLinkElement.

Refactoring the HTMLLinkElement to create a new loader, a LinkLoader, is a good first step to implementing the Link header in Bug 51940.

[Gavin Peters]

Created attachment 78023 [details]
Patch

[Gavin Peters]

This patch isn't ready to land, but I wanted to solicit feedback.

RIght now: I want to cut down on passing in the Node* to LinkLoader, and use the LinkLoaderClient instead for all operations.  But CSSStyleSheet::create() is befuddling me, and I don't have a great idea how to handle it.

Passes all tests here in Mac port, with some changes in http/tests/local/link-stylesheet-load-order which I'm looking at today.

[Early Warning System Bot]

Attachment 78023 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7264444

[Gavin Peters]

(sorry about the build problems, didn't conditionalize a function used only with prefetching, now fixed on my repo, I'll upload with my next revision)

[Build Bot]

Attachment 78023 [details] did not build on win:
Build output: http://queues.webkit.org/results/7241440

[Adam Barth]

Comment on attachment 78023 [details]
Patch

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

Looks like a great start.  Some comments below.  Mostly nits / questions about existing code.

> WebCore/html/HTMLLinkElement.cpp:55
> +    , m_linkLoader(*this, document, this)

|*this| ?  We'd usually just pass |this|.

> WebCore/html/HTMLLinkElement.cpp:152
> +    RefPtr<Document> originalDocument = document();
> +    if (!dispatchBeforeLoadEvent(href))
> +        return false;
> +    // A beforeload handler might have removed us from the document or changed the document.
> +    if (!inDocument() || document() != originalDocument)
> +        return false;
> +    return true;

We should definitely test these conditions.

> WebCore/html/HTMLLinkElement.cpp:178
> +

extra space

> WebCore/html/parser/HTMLPreloadScanner.cpp:35
>  #include "HTMLLinkElement.h"

Can we remove this include?

> WebCore/loader/LinkLoader.cpp:53
> +LinkLoader::LinkLoader(LinkLoaderClient& client, Document* doc, ContainerNode* node)

LinkLoaderClient& => LinkLoaderClient*

We usually hold pointers to clients.

doc => document.

> WebCore/loader/LinkLoader.cpp:56
> +    , m_node()
> +    , m_document()

You should be able to initialize this unconditionally, right?

> WebCore/loader/LinkLoader.cpp:77
> +    if (m_cachedSheet) {
> +        m_cachedSheet->removeClient(this);
> +    if (isLoading() && !isDisabled() && !isAlternate())
> +        document()->removePendingSheet();
> +    }

Bad indent.  Tab?

> WebCore/loader/LinkLoader.cpp:89
> +

Extra blank line.

> WebCore/loader/LinkLoader.cpp:108
> +        if (baseURL.string().contains("mcafee.com/japan/", false))
> +            enforceMIMEType = false;

OMG

> WebCore/loader/LinkLoader.cpp:172
> +    relAttribute.m_isStyleSheet = false;
> +    relAttribute.m_isIcon = false;
> +    relAttribute.m_isAlternate = false;
> +    relAttribute.m_isDNSPrefetch = false;
> +#if ENABLE(LINK_PREFETCH)
> +    relAttribute.m_isLinkPrefetch = false;
> +#endif

Should this work be done in the RelAttribute constructor?

> WebCore/loader/LinkLoader.cpp:175
> +    else if (equalIgnoringCase(rel, "icon") || equalIgnoringCase(rel, "shortcut icon"))

"shortcut icon" => really exactly one space between "shortcut" and "icon"?

> WebCore/loader/LinkLoader.cpp:183
> +    else if (equalIgnoringCase(rel, "alternate stylesheet") || equalIgnoringCase(rel, "stylesheet alternate")) {

Same question.  I realize you're just moving this code, but might be worth investigating.

> WebCore/loader/LinkLoader.cpp:257
> +        if (!m_client.linkBeforeLoad(m_url))
> +          return;

Bad indent.

> WebCore/loader/LinkLoader.cpp:271
> +

Extra blank line.

> WebCore/loader/LinkLoader.cpp:279
> +        if (m_cachedSheet)
> +            m_cachedSheet->addClient(this);
> +        else {

Prefer early return.

> WebCore/loader/LinkLoader.h:70
> +            { 
> +            }

Bad indent.

> WebCore/loader/LinkLoader.h:80
> +    //

Dangling comment.

> WebCore/loader/LinkLoader.h:99
> +    { m_title = title;

These should be on separate lines.  Also, this probably shouldn't be inline.

> WebCore/loader/LinkLoader.h:112
> +    //    virtual void parseMappedAttribute(Attribute*);

Please don't comment out code.

> WebCore/loader/LinkLoader.h:149
> +

extra blank line.

> WebCore/loader/LinkLoaderClient.h:48
> +    virtual bool linkBeforeLoad(const KURL&) { return true; }
> +
> +    virtual void linkLoaded() { }
> +    virtual void linkError() { }
> +
> +    virtual bool linkInDocument() { return true; }

Seems like this should be a pure virtual interface, right?

[Gavin Peters]

Thanks for the review, Adam!

In my reply, I'm deleting nits I'll address on my next upload, and only leaving in questions from you that get an answer other than "Done."


(In reply to comment #6)
>
> > WebCore/html/HTMLLinkElement.cpp:152
> > +    RefPtr<Document> originalDocument = document();
> > +    if (!dispatchBeforeLoadEvent(href))
> > +        return false;
> > +    // A beforeload handler might have removed us from the document or changed the document.
> > +    if (!inDocument() || document() != originalDocument)
> > +        return false;
> > +    return true;
> 
> We should definitely test these conditions.

Yes.   This was a bug fix I copied in from r74995; if none of those tests touch this, I'll add one.

> > WebCore/loader/LinkLoader.cpp:56
> > +    , m_node()
> > +    , m_document()
> 
> You should be able to initialize this unconditionally, right?

I think so.  I'm still waffling about how to handle the m_node* in the LinkLoader: not always present, but needed in CSSStyleSheet::create (called twice).  Perhaps a method in LinkLoaderClient that returns a Node*, or calls CSSStyleSheet::create itself?  This is a bigger deal if we add stylesheet rel types.

> 
> > WebCore/loader/LinkLoader.cpp:108
> > +        if (baseURL.string().contains("mcafee.com/japan/", false))
> > +            enforceMIMEType = false;
> 
> OMG

I know.

> > WebCore/loader/LinkLoader.cpp:175
> > +    else if (equalIgnoringCase(rel, "icon") || equalIgnoringCase(rel, "shortcut icon"))
> 
> "shortcut icon" => really exactly one space between "shortcut" and "icon"?
> 
> > WebCore/loader/LinkLoader.cpp:183
> > +    else if (equalIgnoringCase(rel, "alternate stylesheet") || equalIgnoringCase(rel, "stylesheet alternate")) {
> 
> Same question.  I realize you're just moving this code, but might be worth investigating.

Will do, as it is puzzling.

> 
> > WebCore/loader/LinkLoaderClient.h:48
> > +    virtual bool linkBeforeLoad(const KURL&) { return true; }
> > +
> > +    virtual void linkLoaded() { }
> > +    virtual void linkError() { }
> > +
> > +    virtual bool linkInDocument() { return true; }
> 
> Seems like this should be a pure virtual interface, right?

I think only in the case of linkInDocument().  Each of linkBeforeLoad, linkLoaded and linkError may not be called (say in the Link header case, I'm thinking the first rev doesn't put JS in the HTTP headers at least...), and so a reasonable default makes sense.  linkInDocument not being pure though is just sloppiness on my part.

[Gavin Peters]

Created attachment 78090 [details]
Patch

[Gavin Peters]

This second update doesn't fix everything, but it does address most of your abovestated concerns Adam.

Still open:

- we're failing http/tests/local/link-stylesheet-load-order*.html, and not in a major way, just over failed stylesheet loads.  I'm very curious!

- I removed the m_node from LinkLoader, and added getContainerNode to LinkLoaderClient.  I'm on the fence about this, so please offer better suggestions as available.  :)

- I have reviewed the tests in Bug 74995 which added the document modified by beforeload, and no test was in that bug.  So I'll add a test for that condition.

- I have no answer yet about why things like "stylesheet alternate" are string compared in the rel type parser.

- I've added LinkLoaderClient.cpp, to avoid link bloating.

[WebKit Review Bot]

Attachment 78090 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7363002

[Adam Barth]

Comment on attachment 78090 [details]
Patch

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

> WebCore/loader/LinkLoaderClient.cpp:54
> +ContainerNode* LinkLoaderClient::getContainerNode()

Usually we omit the "get" prefix (although we do use the "set" prefix).  We'd usually call this method just "containerNode" or "asContainerNode" if you're thinking about this as a dynamic cast.

[Build Bot]

Attachment 78090 [details] did not build on win:
Build output: http://queues.webkit.org/results/7360002

[Gavin Peters]

Created attachment 78248 [details]
Patch

[Gavin Peters]

I've added tests for the untested Link element changes from changeset 73938 now, and I am passing all the media tests.

Any thoughts?

[Adam Barth]

Comment on attachment 78248 [details]
Patch

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

This looks great.  Some extremely minor nits below.  I look forward to the next patch.  :)

> WebCore/WebCore.xcodeproj/project.pbxproj:-22334
> -			developmentRegion = English;

Probably better not to fight the developmentRegion war.

> WebCore/html/HTMLLinkElement.h:60
> +    //    bool isIcon() const { return m_relAttribute.m_isIcon; }

Please remove commented out code.

> WebCore/html/HTMLLinkElement.h:77
> +

extra blank line.

> WebCore/loader/LinkLoaderClient.h:37
> +

extra space

[Gavin Peters]

Created attachment 78253 [details]
Patch

[Gavin Peters]

Comment on attachment 78253 [details]
Patch

You didn't have to wait long, Adam.  How's this now?

[Adam Barth]

Comment on attachment 78253 [details]
Patch

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

> WebCore/loader/LinkLoader.h:144
> +

More extra blank lines ;)

[Alexey Proskuryakov]

Comment on attachment 78253 [details]
Patch

Why does a refactoring only patch with an empty ChangeLog have a regression test?

[Alexey Proskuryakov]

Comment on attachment 78253 [details]
Patch

+    RefPtr<CSSStyleSheet> m_sheet;
+    bool m_loading;
+    bool m_disabled;
+    KURL m_url;
+    RelAttribute m_relAttribute;
+    String m_type;
+    String m_media;
+    String m_charset;
+    String m_title;

It seems extremely counter-intuitive to have these in a loader class. 

+LinkLoader::~LinkLoader()
+{
+    if (sheet())
+        sheet()->clearOwnerNode();

Link loader is not a node, why does is its destructor call clearOwnerNode()?

Maybe it's only the "LinkLoader" class name, and the refactoring is good otherwise, but the new mix of loading, DOM and CSSOM code feels like a wrong direction to take to me.

[Gavin Peters]

Alexey, re the unit test:

It's for changes in changeset 74995, bug 8852.  I didn't initially see the test fast/dom/beforeload/remove-link-in-beforeload-listener.html, and so I added this test since I thought that logic was untested.  My new test is somewhat, but not entirely, duplicative of that one.

[Alexey Proskuryakov]

OK - as this test is unrelated to refactoring, and if you still think that it's good to add it, please file a new bug with it. Refactorings tend to attract much attention later (when regressions are found), so saving others from the confusion I had would be helpful.

[Gavin Peters]

Alexey,

First, about the test: thanks.  I'll remove it in my next upload.

I'm open to other names and suggestions about where to go.  The context for this change is described in the bug 51940; my hope is to implement the HTTP Link header for the rel=prefetch case (leaving other cases for future work), and I wanted to share as much code as possible.  I brought this up in WebKit-dev, and the suggestions came back to try to keep as much code in common as possible between the two paths: that is what this CL tries to do, provide a foundation for an alternative entry to loading and applying links to documents that can be used by the (yet to be written/applied) HTTP header parsing code. 

The LinkLoader name may be confusing: conceptually a LinkLoader is not just loading the link, it's applying it to the document as well.  And of course, in the HTMLLinkElement link case, document mutations affect the loader too.

Any suggestions about how to make a better refactoring, change the names etc...?

Thanks!

[Alexey Proskuryakov]

Link prefetch doesn't parse a style sheet, or load stylesheets that it imports, so a CSSStyleSheet makes no sense in LinkLoader.

Bug 51940 is about the less controversial parts of Link header field, and rel=stylesheet is definitely not one of those.

> I brought this up in WebKit-dev, and the suggestions came back to try to keep as much code in common as possible between the two paths

Yes, that makes good sense. What are the common parts between the two forms of link prefetch?

Firing beforeload is probably one of those, but it means that loads initiated from HTTP headers need to be delayed until after a beforeload handler has a chance to be installed. Is that something that's going to be implemented?

On the other hand, linkLoaded() and linkError() seem unclear. What's the use for those for a prefetch load initialed from an HTTP header? HTMLLinkElement::linkLoaded() only dispatches an event, but there is no target to dispatch this event for an HTTP header field.

[Gavin Peters]

Created attachment 78260 [details]
Patch

[Adam Barth]

Comment on attachment 78260 [details]
Patch

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

> WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This line will prevent the patch from being landed because of the OOPS.

[Alexey Proskuryakov]

Comment on attachment 78260 [details]
Patch

Looks like the same patch, just without a test.

[Gavin Peters]

Thanks for your comments Alexey.  I've uploaded another patch, this one incorporates what you've said to date: I remove the extraneous test, and I move the clearOwnerNode() call to the proper destructor.

(In reply to comment #24)
> Link prefetch doesn't parse a style sheet, or load stylesheets that it imports, so a CSSStyleSheet makes no sense in LinkLoader.
> 
> Bug 51940 is about the less controversial parts of Link header field, and rel=stylesheet is definitely not one of those.

Definitely not.  I included it in this refactoring because I didn't want to close that option to us in the future; but I am right now among the skeptical that rel=stylesheet will make sense.  That having been said, the HTMLLinkElement had a lot of logic that felt like it was more connected to resource loading than HTML parsing in it, and so I moved that out as I did.

> > I brought this up in WebKit-dev, and the suggestions came back to try to keep as much code in common as possible between the two paths
> 
> Yes, that makes good sense. What are the common parts between the two forms of link prefetch?

It's all in the ::process() method now for both, and the rel type parser.  As well, since a link can have multiple rel types, any HTMLLinkElement can be both a stylesheet and a preload, for instance.

> Firing beforeload is probably one of those, but it means that loads initiated from HTTP headers need to be delayed until after a beforeload handler has a chance to be installed. Is that something that's going to be implemented?

Probably not.

> On the other hand, linkLoaded() and linkError() seem unclear. What's the use for those for a prefetch load initialed from an HTTP header? HTMLLinkElement::linkLoaded() only dispatches an event, but there is no target to dispatch this event for an HTTP header field.

I doubt that we'll put javascript in HTTP headers; that seems like a bad plan.  The default implementation for those methods in LinkLoaderClient foreshadows this: the intention is that in the header case we'll just use the default implementation for these methods.

- Gavin

[Alexey Proskuryakov]

> I move the clearOwnerNode() call to the proper destructor.

Sorry, I got fooled by a diff in HTMLLinkElement::~HTMLLinkElement().

I think that this patch needs further scaling down in the direction outlined above, but I can't afford the time for a line by line review right now, sorry.

However, RefPtr<CSSStyleSheet> m_sheet should definitely stay in HTMLLinkElement.

>> Firing beforeload is probably one of those, but it means that loads initiated from HTTP headers need
>> to be delayed until after a beforeload handler has a chance to be installed. Is that something that's 
>> going to be implemented?
>
> Probably not.

This makes me quite suspicious about adding even rel=prefetch. Beforeload exists to let one block or change all subresource loads.

[Gavin Peters]

Created attachment 78682 [details]
Patch

[Gavin Peters]

Comment on attachment 78682 [details]
Patch

Alexey, is this more what you were thinking?

I've left stylesheet logic in HTMLLinkElement, and only moved dns-prefetch, prefetch & icon rel types into the LinkLoader.  I think this change is much more modest.

There's also the question about beforeload: I'll start a discussion in webkit-dev about that like we spoke about, but is this issue dependant on that (this issue doesn't change how beforeload works for those three rel types, right now it just doesn't, and after this CL, it still won't).  I'm no beforeload expert, and I look forward to hearing what people say there.

[Build Bot]

Attachment 78682 [details] did not build on win:
Build output: http://queues.webkit.org/results/7371147

[Gavin Peters]

Created attachment 78686 [details]
Patch

[Gavin Peters]

Comment on attachment 78686 [details]
Patch

Hopefully this fixes the win build.

[Gavin Peters]

Created attachment 78699 [details]
Patch

[Gavin Peters]

Comment on attachment 78686 [details]
Patch

Hopefully fix the efl build.

[Adam Barth]

Comment on attachment 78699 [details]
Patch

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

Ok.  I think it's time for this patch now.  Some minor nits below.

> Source/WebCore/html/HTMLLinkElement.h:76
> +    // from LinkLoaderClient

We don't really need this comment.

> Source/WebCore/loader/LinkLoaderClient.h:44
> +    virtual ~LinkLoaderClient();

Do we need this?  No one should be deleting subclasses via this pointer, right?

> Source/WebCore/loader/LinkLoaderClient.h:47
> +    virtual void linkLoaded();
> +    virtual void linkError();

It seems like these should be pure virtual.

[Gavin Peters]

Created attachment 96965 [details]
Patch

[Gavin Peters]

Comment on attachment 78699 [details]
Patch

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

>> Source/WebCore/loader/LinkLoaderClient.h:44
>> +    virtual ~LinkLoaderClient();
> 
> Do we need this?  No one should be deleting subclasses via this pointer, right?

Sure, but I tried without it, and the Mac build elevated the no-virtual-destructor warning to an error.  See my latest upload: do you have any advice?

>> Source/WebCore/loader/LinkLoaderClient.h:47
>> +    virtual void linkError();
> 
> It seems like these should be pure virtual.

Done.

[Gavin Peters]

Comment on attachment 96965 [details]
Patch

Back from the dead, it's 51941!  I hope we can land this soon.

[Alexey Proskuryakov]

Comment on attachment 96965 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        This change moves the rel type parser for link elements into a
> +        linkloader, and pulls the loader code for icons, dns prefetching

Moving parsing code into a loader class sounds really strange.

Perhaps you want to extract it into a separate class instead? Or maybe you actually don't want to have "loader" in the name?

> Source/WebCore/html/HTMLLinkElement.cpp:55
> +    , LinkLoaderClient()

This line is unnecessary, please remove it.

> Source/WebCore/html/HTMLLinkElement.cpp:69
> -    return adoptRef(new HTMLLinkElement(tagName, document, createdByParser));
> +    HTMLLinkElement* elem = new HTMLLinkElement(tagName, document, createdByParser);
> +    return adoptRef(elem);

Why this change? The old code has style that's common throughout WebCore.

> Source/WebCore/loader/LinkLoader.cpp:141
> +    if (m_cachedLinkResource->errorOccurred())
> +        m_client->linkError();
> +    else if (!m_cachedLinkResource->wasCanceled())
> +        m_client->linkLoaded();

So, the client will never be notified on cancellation. This might be something to add a comment about in LinkLoaderClient.h.

> Source/WebCore/loader/LinkLoader.cpp:149
> +void LinkLoader::notifyFinished(CachedResource* resource)
> +{
> +    ASSERT_UNUSED(resource, m_cachedLinkResource.get() == resource);
> +    m_linkLoadedTimer.startOneShot(0);

I'm wondering why we have a timer here. Seems like it should be bad for performance.

> Source/WebCore/loader/LinkLoader.cpp:156
> +    // IE extension: location of small icon for locationbar / bookmarks
> +    // We'll record this URL per document, even if we later only use it in top level frames

I know that you are only moving this code, but calling favicons "IE extension" makes no sense in 2011.

> Source/WebCore/loader/LinkLoader.h:61
> +    ~LinkLoader();

virtual?

> Source/WebCore/loader/LinkLoader.h:67
> +    bool process(const RelAttribute&, const String& type, const KURL&, Document*);
> +private:

There should be an empty line before "private".

What does "process" mean? Can there be a more descriptive name?

> Source/WebCore/loader/LinkLoaderClient.h:40
> +    virtual bool checkBeforeLoadEvent() = 0;

This seems very specific. How do we know that the client will only send the beforeload event, and not have any other logic? The client callback should have a name that explains when it's called, or how the loader uses its result, and not what it expects the client to do.

> Source/WebCore/loader/LinkLoaderClient.h:43
> +    virtual void linkLoaded() = 0;
> +    virtual void linkError() = 0;

It's not clear if "error" is necessarily a loading error, especially with parsing implemented inside this class now.

Why doesn't this method take an error parameter?

Also, "link loaded" and "link error" do not match grammatically.

[Gyuyoung Kim]

Comment on attachment 96965 [details]
Patch

Attachment 96965 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8831889

[Gustavo Noronha (kov)]

Comment on attachment 96965 [details]
Patch

Attachment 96965 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8836350

[Early Warning System Bot]

Comment on attachment 96965 [details]
Patch

Attachment 96965 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8791129

[Gavin Peters]

(In reply to comment #41)
> (From update of attachment 96965 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96965&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        This change moves the rel type parser for link elements into a
> > +        linkloader, and pulls the loader code for icons, dns prefetching
> 
> Moving parsing code into a loader class sounds really strange.

Hrm.  The RelAttribute class is used by the Loader, and by HTMLLinkElement; all  the parsing that's happening is just of the rel attribute, not parsing of the HTMLLinkElement itself.  This parser/tokenizer will also be needed by the parser for the Link header; I thought it was a good practice to move the helper class to the common include, which is the LinkLoader.  I hesitate to add another cpp file just for this.  What do you think?   I'm open to any specific suggestions, but I'm at a loss what to do other than leave it there, since users of the link loader will need to cons up a relattribute, so it might as well be there.

> Perhaps you want to extract it into a separate class instead? Or maybe you actually don't want to have "loader" in the name?
> 
> > Source/WebCore/html/HTMLLinkElement.cpp:55
> > +    , LinkLoaderClient()
> 
> This line is unnecessary, please remove it.

Done.

> > Source/WebCore/html/HTMLLinkElement.cpp:69
> > -    return adoptRef(new HTMLLinkElement(tagName, document, createdByParser));
> > +    HTMLLinkElement* elem = new HTMLLinkElement(tagName, document, createdByParser);
> > +    return adoptRef(elem);
> 
> Why this change? The old code has style that's common throughout WebCore.

Sorry, cruft from debugging.  Fixed.

> > Source/WebCore/loader/LinkLoader.cpp:141
> > +    if (m_cachedLinkResource->errorOccurred())
> > +        m_client->linkError();
> > +    else if (!m_cachedLinkResource->wasCanceled())
> > +        m_client->linkLoaded();
> 
> So, the client will never be notified on cancellation. This might be something to add a comment about in LinkLoaderClient.h.

Done.

> > Source/WebCore/loader/LinkLoader.cpp:149
> > +void LinkLoader::notifyFinished(CachedResource* resource)
> > +{
> > +    ASSERT_UNUSED(resource, m_cachedLinkResource.get() == resource);
> > +    m_linkLoadedTimer.startOneShot(0);
> 
> I'm wondering why we have a timer here. Seems like it should be bad for performance.

The timer is being copied from HTMLLinkElement.  When I've experimented with removing it, Link onload events didn't reliably occur.  Certainly there's
a one shot timer for Images, and other elements with onload events as well.

> > Source/WebCore/loader/LinkLoader.cpp:156
> > +    // IE extension: location of small icon for locationbar / bookmarks
> > +    // We'll record this URL per document, even if we later only use it in top level frames
> 
> I know that you are only moving this code, but calling favicons "IE extension" makes no sense in 2011.

Removed.

> > Source/WebCore/loader/LinkLoader.h:61
> > +    ~LinkLoader();
> 
> virtual?

virtual!

> > Source/WebCore/loader/LinkLoader.h:67
> > +    bool process(const RelAttribute&, const String& type, const KURL&, Document*);
> > +private:
> 
> There should be an empty line before "private".

Done.

> What does "process" mean? Can there be a more descriptive name?
> 

It means the same thing as in HTMLLinkElement.  But I've renamed the method loadLink.  :)

> > Source/WebCore/loader/LinkLoaderClient.h:40
> > +    virtual bool checkBeforeLoadEvent() = 0;
> 
> This seems very specific. How do we know that the client will only send the beforeload event, and not have any other logic? The client callback should have a name that explains when it's called, or how the loader uses its result, and not what it expects the client to do.

Renamed to linkCanLoad().

> > Source/WebCore/loader/LinkLoaderClient.h:43
> > +    virtual void linkLoaded() = 0;
> > +    virtual void linkError() = 0;
> 
> It's not clear if "error" is necessarily a loading error, especially with parsing implemented inside this class now.

Renamed to make it clear.

> Why doesn't this method take an error parameter?

Right now its only use is to fire onerror events, and DOM onerror events don't have parameters.

> Also, "link loaded" and "link error" do not match grammatically.

Renamed.

[Gavin Peters]

Created attachment 97156 [details]
Patch

[Gavin Peters]

Comment on attachment 97156 [details]
Patch

This patch addresses most of Alexey's concerns, and should fix the build on other platforms.

However, I'm still torn about what to do about the RelAttribute helper class.  Right now I really do think that LinkLoader is the best location, but I appreciate arguments against it.  Specific suggestions welcome.

[Alexey Proskuryakov]

> I hesitate to add another cpp file just for this.

I don't have a design or names to suggest, but I do not think that a "loader" class should do parsing.

> Renamed to linkCanLoad().

We usually start those names with "can", "will" or "may", as you can see e.g. in ResourceHandleClient.h.

[Gavin Peters]

Created attachment 97348 [details]
Patch

[Gavin Peters]

Comment on attachment 97348 [details]
Patch

Thanks for the reviews Alexey.  I renamed the method in LinkLoaderClient to shouldLoadLink(), which is more consistant with existing practice.

As well, I moved the RelAttribute helper to its own file in html.

[Adam Barth]

Comment on attachment 97348 [details]
Patch

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

> Source/WebCore/html/LinkRelAttribute.cpp:48
> +            {
> +            }

Bad indent.

> Source/WebCore/html/LinkRelAttribute.cpp:52
> +    *this = tokenize(rel);

Crazy!  I would just make this a private member function rather than doing this bizarre thing.  The one caller of this method can just use the constructor and the assignment operator.

> Source/WebCore/html/LinkRelAttribute.h:54
> +    static LinkRelAttribute tokenize(const AtomicString&);

This probably doesn't need to be an AtomicString.  You're using equalIgnoringCase, so the atomicity isn't really helping.

> Source/WebCore/loader/LinkLoader.h:45
> +/* The LinkLoader can load link rel types icon, dns-prefetch, subresource, prefetch and prerender
> + */

C++ style comments pls.

[Gavin Peters]

(In reply to comment #51)
> (From update of attachment 97348 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97348&action=review
> 
> > Source/WebCore/html/LinkRelAttribute.cpp:48
> > +            {
> > +            }
> 
> Bad indent.

Fixed.


> > Source/WebCore/html/LinkRelAttribute.cpp:52
> > +    *this = tokenize(rel);
> 
> Crazy!  I would just make this a private member function rather than doing this bizarre thing.  The one caller of this method can just use the constructor and the assignment operator.

I just got rid of the method entirely, now there's just a constructor from a string.

> > Source/WebCore/html/LinkRelAttribute.h:54
> > +    static LinkRelAttribute tokenize(const AtomicString&);
> 
> This probably doesn't need to be an AtomicString.  You're using equalIgnoringCase, so the atomicity isn't really helping.

Now moot, since there's no method and no AtomicString.  So fixed.

> 
> > Source/WebCore/loader/LinkLoader.h:45
> > +/* The LinkLoader can load link rel types icon, dns-prefetch, subresource, prefetch and prerender
> > + */
> 
> C++ style comments pls.

Done.

[Gavin Peters]

Created attachment 97593 [details]
Patch

[Gavin Peters]

Comment on attachment 97593 [details]
Patch

Thanks for the review Adam.  This upload is a remediation to your review.

[WebKit Review Bot]

Comment on attachment 97593 [details]
Patch

Attachment 97593 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8879566

New failing tests:
css2.1/t040103-escapes-05-c.html
css2.1/t040103-escapes-06-b.html
css2.1/t040103-escapes-02-d.html
css2.1/t040103-case-01-c.html
css2.1/t040103-escapes-07-b.html
css2.1/t010403-shand-font-02-b.html
css2.1/t040102-keywords-00-b.html
css2.1/t040102-keywords-01-b.html
css2.1/t010403-shand-font-03-b.html
css2.1/t040103-ident-00-c.html
css2.1/t010403-shand-font-00-b.html
css2.1/t040103-escapes-04-b.html
css2.1/t040103-escapes-01-b.html
css2.1/t040103-escapes-03-b.html
css2.1/t040103-case-00-b.html
css2.1/t040103-escapes-00-b.html
css2.1/t040103-ident-01-c.html
css2.1/t040103-escapes-08-b.html
css2.1/t010403-shand-border-00-c.html
css2.1/t010403-shand-font-01-b.html

[WebKit Review Bot]

Created attachment 97596 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Gavin Peters]

I'm pretty sure that these layout test failures are unrelated.  Is the tree in some freaky state?

(In reply to comment #55)
> (From update of attachment 97593 [details])
> Attachment 97593 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8879566
> 
> New failing tests:
> css2.1/t040103-escapes-05-c.html
> css2.1/t040103-escapes-06-b.html
> css2.1/t040103-escapes-02-d.html
> css2.1/t040103-case-01-c.html
> css2.1/t040103-escapes-07-b.html
> css2.1/t010403-shand-font-02-b.html
> css2.1/t040102-keywords-00-b.html
> css2.1/t040102-keywords-01-b.html
> css2.1/t010403-shand-font-03-b.html
> css2.1/t040103-ident-00-c.html
> css2.1/t010403-shand-font-00-b.html
> css2.1/t040103-escapes-04-b.html
> css2.1/t040103-escapes-01-b.html
> css2.1/t040103-escapes-03-b.html
> css2.1/t040103-case-00-b.html
> css2.1/t040103-escapes-00-b.html
> css2.1/t040103-ident-01-c.html
> css2.1/t040103-escapes-08-b.html
> css2.1/t010403-shand-border-00-c.html
> css2.1/t010403-shand-font-01-b.html

[Adam Barth]

Comment on attachment 97593 [details]
Patch

Ok.  Pls watch the tree to make sure those failures are bogus.  Very strange.

[Gavin Peters]

Created attachment 97611 [details]
Patch

[Gavin Peters]

Comment on attachment 97611 [details]
Patch

This is a fix on my last upload, my remediation to Adam's review unfortunately introduced a constructor that used uninitialized memory.  Now fixed.

[Adam Barth]

Comment on attachment 97611 [details]
Patch

Ok.

[WebKit Review Bot]

Comment on attachment 97611 [details]
Patch

Clearing flags on attachment: 97611

Committed r89146: <http://trac.webkit.org/changeset/89146>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 97611 [details]
Patch

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

> Source/WebCore/loader/LinkLoader.h:42
> +class LinkRelAttribute;

LinkRelAttribute is a struct, not a class.

This mismatch breaks building with clang. Not clear why it didn’t break compiling on Windows.