[Webkit-unassigned] [Bug 51941] refactor HTMLLinkElement to allow Link header implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=51941





--- Comment #45 from Gavin Peters <gavinp at chromium.org>  2011-06-14 12:12:06 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.