Bug 51941 - refactor HTMLLinkElement to allow Link header implementation
: refactor HTMLLinkElement to allow Link header implementation
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 51940
  Show dependency treegraph
 
Reported: 2011-01-05 11:45 PST by
Modified: 2011-06-17 12:11 PST (History)


Attachments
Patch (52.00 KB, patch)
2011-01-05 12:01 PST, Gavin Peters
no flags Review Patch | Details | Formatted Diff | Diff
Patch (56.18 KB, patch)
2011-01-05 19:38 PST, Gavin Peters
no flags Review Patch | Details | Formatted Diff | Diff
Patch (59.36 KB, patch)
2011-01-07 10:26 PST, Gavin Peters
no flags Review Patch | Details | Formatted Diff | Diff
Patch (59.00 KB, patch)
2011-01-07 11:50 PST, Gavin Peters
no flags Review Patch | Details | Formatted Diff | Diff
Patch (55.62 KB, patch)
2011-01-07 13:05 PST, Gavin Peters
no flags Review Patch | Details | Formatted Diff | Diff
Patch (31.45 KB, patch)
2011-01-12 06:02 PST, Gavin Peters
no flags Review Patch | Details | Formatted Diff | Diff
Patch (32.16 KB, patch)
2011-01-12 07:08 PST, Gavin Peters
no flags Review Patch | Details | Formatted Diff | Diff
Patch (32.71 KB, patch)
2011-01-12 09:50 PST, Gavin Peters
no flags Review Patch | Details | Formatted Diff | Diff
Patch (35.97 KB, patch)
2011-06-13 10:07 PST, Gavin Peters
no flags Review Patch | Details | Formatted Diff | Diff
Patch (36.32 KB, patch)
2011-06-14 12:43 PST, Gavin Peters
no flags Review Patch | Details | Formatted Diff | Diff
Patch (43.57 KB, patch)
2011-06-15 12:49 PST, Gavin Peters
no flags Review Patch | Details | Formatted Diff | Diff
Patch (43.00 KB, patch)
2011-06-17 07:02 PST, Gavin Peters
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.20 MB, application/zip)
2011-06-17 07:13 PST, WebKit Review Bot
no flags Details
Patch (43.25 KB, patch)
2011-06-17 10:07 PST, Gavin Peters
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-01-05 11:45:29 PST
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.
------- Comment #1 From 2011-01-05 12:01:35 PST -------
Created an attachment (id=78023) [details]
Patch
------- Comment #2 From 2011-01-05 12:03:19 PST -------
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.
------- Comment #3 From 2011-01-05 12:15:36 PST -------
Attachment 78023 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7264444
------- Comment #4 From 2011-01-05 12:20:24 PST -------
(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)
------- Comment #5 From 2011-01-05 12:29:24 PST -------
Attachment 78023 [details] did not build on win:
Build output: http://queues.webkit.org/results/7241440
------- Comment #6 From 2011-01-05 13:56:02 PST -------
(From update of attachment 78023 [details])
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?
------- Comment #7 From 2011-01-05 14:08:06 PST -------
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.
------- Comment #8 From 2011-01-05 19:38:43 PST -------
Created an attachment (id=78090) [details]
Patch
------- Comment #9 From 2011-01-05 19:43:38 PST -------
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.
------- Comment #10 From 2011-01-05 19:55:23 PST -------
Attachment 78090 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7363002
------- Comment #11 From 2011-01-05 20:06:42 PST -------
(From update of attachment 78090 [details])
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.
------- Comment #12 From 2011-01-05 20:07:54 PST -------
Attachment 78090 [details] did not build on win:
Build output: http://queues.webkit.org/results/7360002
------- Comment #13 From 2011-01-07 10:26:22 PST -------
Created an attachment (id=78248) [details]
Patch
------- Comment #14 From 2011-01-07 10:27:54 PST -------
I've added tests for the untested Link element changes from changeset 73938 now, and I am passing all the media tests.

Any thoughts?
------- Comment #15 From 2011-01-07 11:40:51 PST -------
(From update of attachment 78248 [details])
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
------- Comment #16 From 2011-01-07 11:50:39 PST -------
Created an attachment (id=78253) [details]
Patch
------- Comment #17 From 2011-01-07 11:52:20 PST -------
(From update of attachment 78253 [details])
You didn't have to wait long, Adam.  How's this now?
------- Comment #18 From 2011-01-07 11:53:22 PST -------
(From update of attachment 78253 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=78253&action=review

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

More extra blank lines ;)
------- Comment #19 From 2011-01-07 11:58:02 PST -------
(From update of attachment 78253 [details])
Why does a refactoring only patch with an empty ChangeLog have a regression test?
------- Comment #20 From 2011-01-07 12:05:27 PST -------
(From update of attachment 78253 [details])
+    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.
------- Comment #21 From 2011-01-07 12:06:20 PST -------
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.
------- Comment #22 From 2011-01-07 12:14:35 PST -------
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.
------- Comment #23 From 2011-01-07 12:17:11 PST -------
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!
------- Comment #24 From 2011-01-07 12:41:18 PST -------
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.
------- Comment #25 From 2011-01-07 13:05:50 PST -------
Created an attachment (id=78260) [details]
Patch
------- Comment #26 From 2011-01-07 13:10:06 PST -------
(From update of attachment 78260 [details])
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.
------- Comment #27 From 2011-01-07 13:10:11 PST -------
(From update of attachment 78260 [details])
Looks like the same patch, just without a test.
------- Comment #28 From 2011-01-07 13:12:51 PST -------
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
------- Comment #29 From 2011-01-07 13:24:46 PST -------
> 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.
------- Comment #30 From 2011-01-12 06:02:26 PST -------
Created an attachment (id=78682) [details]
Patch
------- Comment #31 From 2011-01-12 06:05:11 PST -------
(From update of attachment 78682 [details])
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.
------- Comment #32 From 2011-01-12 06:30:02 PST -------
Attachment 78682 [details] did not build on win:
Build output: http://queues.webkit.org/results/7371147
------- Comment #33 From 2011-01-12 07:08:53 PST -------
Created an attachment (id=78686) [details]
Patch
------- Comment #34 From 2011-01-12 07:09:14 PST -------
(From update of attachment 78686 [details])
Hopefully this fixes the win build.
------- Comment #35 From 2011-01-12 09:50:34 PST -------
Created an attachment (id=78699) [details]
Patch
------- Comment #36 From 2011-01-12 09:51:06 PST -------
(From update of attachment 78686 [details])
Hopefully fix the efl build.
------- Comment #37 From 2011-04-26 16:25:00 PST -------
(From update of attachment 78699 [details])
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.
------- Comment #38 From 2011-06-13 10:07:50 PST -------
Created an attachment (id=96965) [details]
Patch
------- Comment #39 From 2011-06-13 10:08:46 PST -------
(From update of attachment 78699 [details])
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.
------- Comment #40 From 2011-06-13 10:09:22 PST -------
(From update of attachment 96965 [details])
Back from the dead, it's 51941!  I hope we can land this soon.
------- Comment #41 From 2011-06-13 11:12:59 PST -------
(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.

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.
------- Comment #42 From 2011-06-13 13:28:12 PST -------
(From update of attachment 96965 [details])
Attachment 96965 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8831889
------- Comment #43 From 2011-06-13 14:59:25 PST -------
(From update of attachment 96965 [details])
Attachment 96965 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8836350
------- Comment #44 From 2011-06-14 02:38:00 PST -------
(From update of attachment 96965 [details])
Attachment 96965 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8791129
------- Comment #45 From 2011-06-14 12:12:06 PST -------
(In reply to comment #41)
> (From update of attachment 96965 [details] [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.
------- Comment #46 From 2011-06-14 12:43:38 PST -------
Created an attachment (id=97156) [details]
Patch
------- Comment #47 From 2011-06-14 12:45:08 PST -------
(From update of attachment 97156 [details])
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.
------- Comment #48 From 2011-06-14 13:10:17 PST -------
> 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.
------- Comment #49 From 2011-06-15 12:49:40 PST -------
Created an attachment (id=97348) [details]
Patch
------- Comment #50 From 2011-06-15 12:50:46 PST -------
(From update of attachment 97348 [details])
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.
------- Comment #51 From 2011-06-16 15:11:52 PST -------
(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.

> 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.
------- Comment #52 From 2011-06-17 07:01:06 PST -------
(In reply to comment #51)
> (From update of attachment 97348 [details] [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.
------- Comment #53 From 2011-06-17 07:02:31 PST -------
Created an attachment (id=97593) [details]
Patch
------- Comment #54 From 2011-06-17 07:03:17 PST -------
(From update of attachment 97593 [details])
Thanks for the review Adam.  This upload is a remediation to your review.
------- Comment #55 From 2011-06-17 07:13:27 PST -------
(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
------- Comment #56 From 2011-06-17 07:13:34 PST -------
Created an attachment (id=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
------- Comment #57 From 2011-06-17 07:16:36 PST -------
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] [details])
> Attachment 97593 [details] [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
------- Comment #58 From 2011-06-17 09:40:16 PST -------
(From update of attachment 97593 [details])
Ok.  Pls watch the tree to make sure those failures are bogus.  Very strange.
------- Comment #59 From 2011-06-17 10:07:38 PST -------
Created an attachment (id=97611) [details]
Patch
------- Comment #60 From 2011-06-17 10:08:29 PST -------
(From update of attachment 97611 [details])
This is a fix on my last upload, my remediation to Adam's review unfortunately introduced a constructor that used uninitialized memory.  Now fixed.
------- Comment #61 From 2011-06-17 10:16:29 PST -------
(From update of attachment 97611 [details])
Ok.
------- Comment #62 From 2011-06-17 11:11:13 PST -------
(From update of attachment 97611 [details])
Clearing flags on attachment: 97611

Committed r89146: <http://trac.webkit.org/changeset/89146>
------- Comment #63 From 2011-06-17 11:11:24 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #64 From 2011-06-17 12:11:35 PST -------
(From update of attachment 97611 [details])
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.