While the original issue has been fixed in #65564, that patch had no LayoutTest attached. This will be remedied here.
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!