Bug 37674 - Implement sizes attribute for link tag from HTML5
: Implement sizes attribute for link tag from HTML5
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
: HTML5
:
: 32934 125775
  Show dependency treegraph
 
Reported: 2010-04-15 14:19 PST by
Modified: 2013-12-16 10:15 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-15 14:19:46 PST
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 From 2011-07-01 10:17:08 PST -------
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 From 2011-07-01 11:04:49 PST -------
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 From 2011-07-07 16:26:44 PST -------
Created an attachment (id=100048) [details]
Patch
------- Comment #4 From 2011-07-07 16:33:59 PST -------
(In reply to comment #3)
> Created an attachment (id=100048) [details] [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 From 2011-07-07 16:52:05 PST -------
(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.
------- Comment #6 From 2011-07-07 17:04:46 PST -------
Does this patch change it so that all icons are loaded, even if the client doesn't care about those?
------- Comment #7 From 2011-07-07 17:08:33 PST -------
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 From 2011-07-07 17:20:12 PST -------
(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)
------- Comment #9 From 2011-07-08 09:35:21 PST -------
(In reply to comment #8)
> (From update of attachment 100048 [details] [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 From 2011-07-08 09:38:39 PST -------
(From update of attachment 100048 [details])
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 From 2011-07-11 10:16:44 PST -------
Created an attachment (id=100320) [details]
Patch
------- Comment #12 From 2011-07-11 15:59:47 PST -------
(From update of attachment 100320 [details])
I like the test! But let's split the allIconURLs stuff into a separate patch.
------- Comment #13 From 2011-07-11 17:42:27 PST -------
Created an attachment (id=100393) [details]
Patch
------- Comment #14 From 2011-07-12 08:33:43 PST -------
(From update of attachment 100393 [details])
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 From 2011-07-12 11:58:01 PST -------
(From update of attachment 100393 [details])
This patch is wrong. sizes should be DOMSettableTokenList.
------- Comment #16 From 2011-07-18 13:05:24 PST -------
Created an attachment (id=101192) [details]
Patch
------- Comment #17 From 2011-07-18 14:27:23 PST -------
(From update of attachment 101192 [details])
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 From 2011-07-21 17:09:29 PST -------
Created an attachment (id=101672) [details]
Patch
------- Comment #19 From 2011-07-21 17:31:43 PST -------
(From update of attachment 101672 [details])
Attachment 101672 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9209852
------- Comment #20 From 2011-07-21 17:35:43 PST -------
(From update of attachment 101672 [details])
Attachment 101672 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9199931
------- Comment #21 From 2011-07-21 21:02:06 PST -------
Created an attachment (id=101696) [details]
Patch
------- Comment #22 From 2011-07-22 10:40:02 PST -------
(From update of attachment 101696 [details])
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 From 2011-07-22 10:53:44 PST -------
> 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 From 2011-07-22 14:24:02 PST -------
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 From 2011-07-22 15:27:32 PST -------
Created an attachment (id=101778) [details]
Patch
------- Comment #26 From 2011-07-22 15:29:08 PST -------
(From update of attachment 101778 [details])
cool.
------- Comment #27 From 2011-07-27 20:01:07 PST -------
(From update of attachment 101778 [details])
Clearing flags on attachment: 101778

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