| Summary: | Support for multiple <link rel="icon"> favicon elements - LayoutTests | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Rachel Blum <groby> | ||||||||||||||||
| Component: | DOM | Assignee: | Rachel Blum <groby> | ||||||||||||||||
| Status: | UNCONFIRMED --- | ||||||||||||||||||
| Severity: | Normal | CC: | aa, abarth, ahmad.saleem792, ap, aroben, bfulgham, darin, dglazkov, fbeaufort, fishd, gsnedders, gustavo.noronha, gustavo, koivisto, mrobinson, rniwa, webkit.review.bot, xan.lopez | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 65564 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Rachel Blum
2011-09-26 18:25:23 PDT
Created attachment 108778 [details]
Patch
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 Comment on attachment 108778 [details]
Patch
Yay tests!
Created attachment 108874 [details]
Patch
Comment on attachment 108874 [details] Patch Attachment 108874 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9877425 Created attachment 108890 [details]
Patch
Comment on attachment 108890 [details] Patch Attachment 108890 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9878452 Created attachment 108933 [details]
Patch
Comment on attachment 108933 [details] Patch Attachment 108933 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9878499 Created attachment 109068 [details]
Patch
All export definitions fixed.... Ready for review 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?). 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. Created attachment 109557 [details]
Patch
* 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 Comment on attachment 109557 [details] Patch Attachment 109557 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9938299 Comment on attachment 109557 [details]
Patch
GTK build still failing, not sure why
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. (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. :( So could somebody land this patch, please? 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 Created attachment 117706 [details]
Patch
Comment on attachment 117706 [details] Patch Attachment 117706 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10701457 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 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! |