[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 #41 from Alexey Proskuryakov <ap at webkit.org>  2011-06-13 11:12:59 PST ---
(From update of attachment 96965)
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.

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