Bug 51941

Created attachment 78023 [details] Patch

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.

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

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

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

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?

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.

Created attachment 78090 [details] Patch

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.

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

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.

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

Created attachment 78248 [details] Patch

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

Created attachment 78253 [details] Patch

Comment on attachment 78253 [details] Patch You didn't have to wait long, Adam. How's this now?

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 ;)

Comment on attachment 78253 [details] Patch Why does a refactoring only patch with an empty ChangeLog have a regression test?

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.

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.

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.

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!

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.

Created attachment 78260 [details] Patch

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.

Comment on attachment 78260 [details] Patch Looks like the same patch, just without a test.

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

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

Created attachment 78682 [details] Patch

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.

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

Created attachment 78686 [details] Patch

Comment on attachment 78686 [details] Patch Hopefully this fixes the win build.

Created attachment 78699 [details] Patch

Comment on attachment 78686 [details] Patch Hopefully fix the efl build.

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.

Created attachment 96965 [details] Patch

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.

Comment on attachment 96965 [details] Patch Back from the dead, it's 51941! I hope we can land this soon.

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.

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

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

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

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

Created attachment 97156 [details] Patch

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.

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

Created attachment 97348 [details] Patch

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.

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.

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

Created attachment 97593 [details] Patch

Comment on attachment 97593 [details] Patch Thanks for the review Adam. This upload is a remediation to your review.

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

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

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

Comment on attachment 97593 [details] Patch Ok. Pls watch the tree to make sure those failures are bogus. Very strange.

Created attachment 97611 [details] Patch

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.

Comment on attachment 97611 [details] Patch Ok.

Comment on attachment 97611 [details] Patch Clearing flags on attachment: 97611 Committed r89146: <http://trac.webkit.org/changeset/89146>

All reviewed patches have been landed. Closing bug.

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.