Bug 37674 - Implement sizes attribute for link tag from HTML5
Summary: Implement sizes attribute for link tag from HTML5
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: HTML5
Depends on:
Blocks: 32934 125775
  Show dependency treegraph
 
Reported: 2010-04-15 14:19 PDT by Aaron Boodman
Modified: 2013-12-16 10:15 PST (History)
12 users (show)

See Also:


Attachments
Patch (12.53 KB, patch)
2011-07-07 16:26 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (14.59 KB, patch)
2011-07-11 10:16 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (11.49 KB, patch)
2011-07-11 17:42 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (13.50 KB, patch)
2011-07-18 13:05 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (23.58 KB, patch)
2011-07-21 17:09 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (33.76 KB, patch)
2011-07-21 21:02 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (14.45 KB, patch)
2011-07-22 15:27 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Boodman 2010-04-15 14:19:46 PDT
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">
Comment 1 Darin Adler 2011-07-01 10:17:08 PDT
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.
Comment 2 Rachel Blum 2011-07-01 11:04:49 PDT
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.
Comment 3 Rachel Blum 2011-07-07 16:26:44 PDT
Created attachment 100048 [details]
Patch
Comment 4 Rachel Blum 2011-07-07 16:33:59 PDT
(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)
Comment 5 Dimitri Glazkov (Google) 2011-07-07 16:52:05 PDT
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.
Comment 6 Alexey Proskuryakov 2011-07-07 17:04:46 PDT
Does this patch change it so that all icons are loaded, even if the client doesn't care about those?
Comment 7 Rachel Blum 2011-07-07 17:08:33 PDT
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.
Comment 8 Rachel Blum 2011-07-07 17:20:12 PDT
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)
Comment 9 Dimitri Glazkov (Google) 2011-07-08 09:35:21 PDT
(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.
Comment 10 Dimitri Glazkov (Google) 2011-07-08 09:38:39 PDT
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.
Comment 11 Rachel Blum 2011-07-11 10:16:44 PDT
Created attachment 100320 [details]
Patch
Comment 12 Dimitri Glazkov (Google) 2011-07-11 15:59:47 PDT
Comment on attachment 100320 [details]
Patch

I like the test! But let's split the allIconURLs stuff into a separate patch.
Comment 13 Rachel Blum 2011-07-11 17:42:27 PDT
Created attachment 100393 [details]
Patch
Comment 14 Dimitri Glazkov (Google) 2011-07-12 08:33:43 PDT
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?
Comment 15 Dimitri Glazkov (Google) 2011-07-12 11:58:01 PDT
Comment on attachment 100393 [details]
Patch

This patch is wrong. sizes should be DOMSettableTokenList.
Comment 16 Rachel Blum 2011-07-18 13:05:24 PDT
Created attachment 101192 [details]
Patch
Comment 17 Dimitri Glazkov (Google) 2011-07-18 14:27:23 PDT
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.
Comment 18 Rachel Blum 2011-07-21 17:09:29 PDT
Created attachment 101672 [details]
Patch
Comment 19 Gyuyoung Kim 2011-07-21 17:31:43 PDT
Comment on attachment 101672 [details]
Patch

Attachment 101672 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9209852
Comment 20 Gustavo Noronha (kov) 2011-07-21 17:35:43 PDT
Comment on attachment 101672 [details]
Patch

Attachment 101672 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9199931
Comment 21 Rachel Blum 2011-07-21 21:02:06 PDT
Created attachment 101696 [details]
Patch
Comment 22 Dimitri Glazkov (Google) 2011-07-22 10:40:02 PDT
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?
Comment 23 Rachel Blum 2011-07-22 10:53:44 PDT
> 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.
Comment 24 Ian 'Hixie' Hickson 2011-07-22 14:24:02 PDT
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.
Comment 25 Rachel Blum 2011-07-22 15:27:32 PDT
Created attachment 101778 [details]
Patch
Comment 26 Dimitri Glazkov (Google) 2011-07-22 15:29:08 PDT
Comment on attachment 101778 [details]
Patch

cool.
Comment 27 WebKit Review Bot 2011-07-27 20:01:07 PDT
Comment on attachment 101778 [details]
Patch

Clearing flags on attachment: 101778

Committed r91893: <http://trac.webkit.org/changeset/91893>
Comment 28 WebKit Review Bot 2011-07-27 20:01:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Claudio Saavedra 2013-03-27 23:06:25 PDT
+#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.
Comment 30 Erik Arvidsson 2013-03-28 08:13:08 PDT
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.