Bug 37674 – Implement sizes attribute for link tag from HTML5

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

[Aaron Boodman]

See spec here:
http://dev.w3.org/html5/spec/Overview.html#attr-link-sizes

Basically:

  <link rel=icon href=favicon.png sizes="16x16" type="image/png">
  <link rel=icon href=windows.ico sizes="32x32 48x48" type="image/vnd.microsoft.icon">
  <link rel=icon href=mac.icns sizes="128x128 512x512 8192x8192 32768x32768">
  <link rel=icon href=iphone.png sizes="59x60" type="image/png">
  <link rel=icon href=gnome.svg sizes="any" type="image/svg+xml">

[Darin Adler]

Various web browsers and other WebKit clients do different things with the icons. The icon code built into WebKit needs to respect those policies. Adding features to the icon loading code is fine, but the features need to serve the needs of the web browsers.

Thus, the HTML5 specification tells us how to correctly parse the link element properties, but does not specify exactly what we should load based on those.

That’s what makes this a challenging project.

I presume there is some specific Chrome goal here. It would be good to know what that is and how these changes will work. Those of us at Apple will also need to consider how the changes would affect Safari as well as the many Apple and third-party applications that use WebKit on Mac OS X and iOS.

[Rachel Blum]

The short-term use case for Chromium is the new tab page, which plans to use 32x32 icons instead of 16x16. 

The proposed change is to simply collect *all* rel-icon links when WebKit does the initial tree traversal and expose those separately. WebKit already does a callback for each loaded icon. (LinkLoader::loadLink)

Any request for icons currently supports the icon types FavIcon, TouchIcon, TouchPrecomposedIcon - there would be an additional "AnyIcon" that will allow retrieving all icons collected.

WebIconURL would need to be extended to store the sizes/type/media attributes for all icons.

As a result, current clients won't be affected at all unless they request AnyIcon or examine those additional attributes - the change should be backwards compatible.

Choosing an "appropriate" icon will then entirely be at the discretion of the WebKit client. I think this is the most useful approach, since the HTML5 spec doesn't want to specify which ones to pick. (It makes vague suggestions, but there's a good case for removing that too - see e.g.  http://www.w3.org/Bugs/Public/show_bug.cgi?id=13107 )

Unless there's objection to this plan, patch forthcoming soon-ish.

[Rachel Blum]

Created attachment 100048 [details]
Patch

[Rachel Blum]

(In reply to comment #3)
> Created an attachment (id=100048) [details]
> Patch

Submitted patch stores a list of all <link rel=icon> elements, complete with size attribute. (Only as text, though, not parsed)

[Dimitri Glazkov (Google)]

Comment on attachment 100048 [details]
Patch

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

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

This won't let you land. You probably need to explain why there are no new tests for the code you adding.

[Alexey Proskuryakov]

Does this patch change it so that all icons are loaded, even if the client doesn't care about those?

[Rachel Blum]

No, it only records URLs and the size attrib. 

WebCore::Document has a new member  Vector<IconURL> allIconURLs() const;

That returns you a list of all encountered IconURLs (and IconURL has m_sizes and m_isDefault to expose additional info)

Since the HTML5 spec is deliberately unclear on what the strategy for picking icons is, I did not add any such logic to webkit - it's up to the UA clients to do something with the additional info.

[Rachel Blum]

Comment on attachment 100048 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> This won't let you land. You probably need to explain why there are no new tests for the code you adding.

There are no new tests because no observable WebKit behavior changed.

This patch simply adds a new API that allows UA's to retrieve a full list of <link rel=icon> elements encountered. The strategy for picking or downloading favicons has not been affected at all. (Deliberately left out - see bug for add'l comments)

[Dimitri Glazkov (Google)]

(In reply to comment #8)
> (From update of attachment 100048 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100048&action=review
> 
> >> Source/WebCore/ChangeLog:8
> >> +        No new tests. (OOPS!)
> > 
> > This won't let you land. You probably need to explain why there are no new tests for the code you adding.
> 
> There are no new tests because no observable WebKit behavior changed.
> 
> This patch simply adds a new API that allows UA's to retrieve a full list of <link rel=icon> elements encountered. The strategy for picking or downloading favicons has not been affected at all. (Deliberately left out - see bug for add'l comments)

If we're adding code, we should ensure it's tested somehow.

[Dimitri Glazkov (Google)]

Comment on attachment 100048 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Implement sizes attribute for link tag from HTML5, collects *ALL* <link rel="icon"> elements

It seems like there are two patches (and possibly two tests):

* Add collection of all icons
* Add sizes attribute. I wonder if the code to parse the sizes should be somewhere here too.

[Rachel Blum]

Created attachment 100320 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 100320 [details]
Patch

I like the test! But let's split the allIconURLs stuff into a separate patch.

[Rachel Blum]

Created attachment 100393 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 100393 [details]
Patch

r=me. I assume the sizes parsing code, based on http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#attr-link-sizes is coming in next?

[Dimitri Glazkov (Google)]

Comment on attachment 100393 [details]
Patch

This patch is wrong. sizes should be DOMSettableTokenList.

[Rachel Blum]

Created attachment 101192 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 101192 [details]
Patch

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

This is not a complete implementation, since the validating parser isn't yet implemented. This means that if I set sizes = "foo 10x10", the icon.sizes.length would report a size of 2, instead of 1. I wonder if you could add the parser into this patch? This way, we wouldn't be landing incomplete functionality.

> LayoutTests/fast/dom/icon-size-property.html:14
> +      var icon=document.getElementById("rel-icon");

I think we need a few more tests here. Let's test for an empty sizes value, for instance.

[Rachel Blum]

Created attachment 101672 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 101672 [details]
Patch

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

[Gustavo Noronha (kov)]

Comment on attachment 101672 [details]
Patch

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

[Rachel Blum]

Created attachment 101696 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 101696 [details]
Patch

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

the bots are all confused, probably need to re-run the patch. Great progress!

> LayoutTests/fast/dom/icon-size-property.html:42
> +

Let's also test setting the sizes attribute from script.

> Source/WebCore/html/DOMSettableTokenList.cpp:103
> +    return DOMTokenList::validateToken(token, ec);

Could we just override validateToken and not do the fooInternal dance?

> Source/WebCore/html/SizesList.cpp:56
> +                localEc = SYNTAX_ERR;

Are we really supposed to throw SYNTAX_ERR in these cases?

> Source/WebCore/html/SizesList.cpp:85
> +    ExceptionCode ec;
> +    DOMSettableTokenList::setValue("");
> +    for (unsigned i = 0; i < tokens.size(); ++i)
> +        add(tokens[i], ec);

This seems inefficient. It's almost like SpaceSplitString wants to have a validating function parameter. But I guess it's ok as is.

> Source/WebCore/html/SizesList.h:37
> +class SizesList : public DOMSettableTokenList {

Should we be more specific in naming, like IconSizeList?

[Rachel Blum]

> the bots are all confused, probably need to re-run the patch. 

Yes, a merge conflict. Resolved locally, testing right now.

> Let's also test setting the sizes attribute from script.

That's already in there, see icon-sizes-property.html:23-25

      icon.sizes="10x10 20x20 30x30 40x40";
      shouldEvaluateTo('icon.sizes.length',4);
      shouldBeEqualToString('icon.sizes.value',"10x10 20x20 30x30 40x40");

> Could we just override validateToken and not do the fooInternal dance?

We possibly could, except it's currently a static on DOMTokenList, not a member. I'll check if there's any reason to keep it that way, otherwise it just earned itself a 'virtual' ;)

> Are we really supposed to throw SYNTAX_ERR in these cases?

That's how I read the spec - can't have more than one 'x'/'X', and it must separate two numbers.  It's not really an invalid character per se. 


> This seems inefficient. It's almost like SpaceSplitString wants to have a validating function parameter. 

The entire chain right now is inefficient - SizesList::validateTokenInternal also traverses the token twice. On the upside, it's a clear separation of responsibilities. 

> Should we be more specific in naming, like IconSizeList?
Probably a better name, so will do.

[Ian 'Hixie' Hickson]

Don't confuse authoring conformance requirements with implementor requirements. Whether something is allowed or not has *absolutely no effect* on what browsers should do. Unless you can point to a specific requirement that says to fire an exception, you should not fire an exception per spec.

[Rachel Blum]

Created attachment 101778 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 101778 [details]
Patch

cool.

[WebKit Review Bot]

Comment on attachment 101778 [details]
Patch

Clearing flags on attachment: 101778

Committed r91893: <http://trac.webkit.org/changeset/91893>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Claudio Saavedra]

+#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
+        attribute [Custom] DOMSettableTokenList sizes;
+#endif

Why was this made JavaScript only? I see the first revisions of the patch were adding this unconditionally but it was later made JavaScript only.

I'd like to be able to use this from GTK+'s dom bindings.

[Erik Arvidsson]

This uses a custom binding which doesn't seem needed. Please file a new bug to not use custom bindings and/or add GTK custom bindings.