Bug 68861 – Support for multiple <link rel="icon"> favicon elements - LayoutTests

• Component: DOM
• Status: UNCONFIRMED

[Rachel Blum]

While the original issue has been fixed in #65564, that patch had no LayoutTest attached. This will be remedied here.

[Rachel Blum]

Created attachment 108778 [details]
Patch

[Rachel Blum]

First patch uploaded simply to get an error message telling me what the mangled name for the .def file should be. Not ready to commit

[Adam Roben (:aroben)]

Comment on attachment 108778 [details]
Patch

Yay tests!

[Rachel Blum]

Created attachment 108874 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 108874 [details]
Patch

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

[Rachel Blum]

Created attachment 108890 [details]
Patch

[Collabora GTK+ EWS bot]

Comment on attachment 108890 [details]
Patch

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

[Rachel Blum]

Created attachment 108933 [details]
Patch

[Collabora GTK+ EWS bot]

Comment on attachment 108933 [details]
Patch

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

[Rachel Blum]

Created attachment 109068 [details]
Patch

[Rachel Blum]

All export definitions fixed.... Ready for review

[Adam Roben (:aroben)]

Comment on attachment 109068 [details]
Patch

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

It is awesome to have a test for this! It might be worth adding tests that have zero and one icons specified.

A few small comments below.

> Source/WebCore/testing/Internals.cpp:400
> +    for (Vector<IconURL>::const_iterator i(iconURLs.begin()); i != iconURLs.end(); ++i) {

We usually use indices to iterate over Vectors rather than iterators.

> Source/WebCore/testing/Internals.cpp:406
> +        if (!result.isEmpty())
> +            result += ", ";
> +        result += "{ ";
> +        result += i->m_iconURL.string() + ", ";
> +        result += i->m_mimeType + ", ";
> +        result += i->m_sizes + " }";

I think our convention is to omit the "m_" prefix for public data members. (I might be wrong, though; you should check for examples in our source tree.)

All the += in here is pretty inefficient. I'm not sure what the recommended pattern is these days (StringBuilder?).

[Darin Adler]

Comment on attachment 109068 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:393
> +    String result;

Yes, as Adam says, this should be a StringBuilder. String's += operation is extremely inefficient and even for test code would be better to not use it.

[Rachel Blum]

Created attachment 109557 [details]
Patch

[Rachel Blum]

* iterator => indices fixed
* left m_ convention intact. Several spotchecks in dom directory (KeyboardEvent.h, MediaStream.h) also have an m_
* Added tests for zero/none
* Switched to StringBuilder

This _might_ need a further revision if any StringBuilder symbols need to be exported to be used in Internals.cpp

[Gustavo Noronha (kov)]

Comment on attachment 109557 [details]
Patch

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

[Darin Adler]

Comment on attachment 109557 [details]
Patch

GTK build still failing, not sure why

[Rachel Blum]

GTK build breakage happened before - it seems to occasionally not pick up the export list. See e.g. the 4th & 5th patch. Same export list for GTK, 4 works, 5 fails. Looping in mrobinson, hoping he can solve that riddle.

[Martin Robinson]

(In reply to comment #18)
> GTK build breakage happened before - it seems to occasionally not pick up the export list. See e.g. the 4th & 5th patch. Same export list for GTK, 4 works, 5 fails. Looping in mrobinson, hoping he can solve that riddle.

I don't think it should block this patch from landing. If it turns out that the GTK+ build breaks, we'll just force a clean rebuild. I'm not sure what the issue is that's causing the flakiness. :(

[Rachel Blum]

So could somebody land this patch, please?

[WebKit Review Bot]

Comment on attachment 109557 [details]
Patch

Rejecting attachment 109557 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t2.def
Hunk #2 succeeded at 149 with fuzz 2 (offset 2 lines).
patching file Source/WebKit2/win/WebKit2CFLite.def
Hunk #1 succeeded at 141 with fuzz 2 (offset 3 lines).
Hunk #2 FAILED at 148.
1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit2/win/WebKit2CFLite.def.rej
patching file Source/autotools/symbols.filter
Hunk #1 succeeded at 75 (offset 4 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Roben', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10695374

[Rachel Blum]

Created attachment 117706 [details]
Patch

[Collabora GTK+ EWS bot]

Comment on attachment 117706 [details]
Patch

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

[Adam Roben (:aroben)]

Comment on attachment 117706 [details]
Patch

Looks like this doesn't build on GTK.

It would be great to support multiple link[rel=icon] as specified in the HTML spec[1]. Without it, it is impossible for web developers to use media features[2] for multiple equally appropriate icons.

It seems like it's something web developers want according to https://twitter.com/tomayac/status/1367075186428833793

[1] https://html.spec.whatwg.org/#rel-icon
[2] https://html.spec.whatwg.org/multipage/semantics.html#processing-the-media-attribute

[Ahmad Saleem]

There are test coverage available for these:

https://wpt.fyi/results/html/links/icon?label=master&label=experimental&aligned&view=subtest&q=icon

Do we need to add these tests from the patch? Thanks!

NOTE - One test was removed in this commit due to non-standard behavior:

https://github.com/WebKit/WebKit/commit/a48cc016912799c3dd65e94737947c2a519dbc53

Appreciate if someone can if anything further is required and mark / tag this bug accordingly. Thanks!