Bug 68861 - Support for multiple <link rel="icon"> favicon elements - LayoutTests
Summary: Support for multiple <link rel="icon"> favicon elements - LayoutTests
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rachel Blum
URL:
Keywords:
Depends on:
Blocks: 65564
  Show dependency treegraph
 
Reported: 2011-09-26 18:25 PDT by Rachel Blum
Modified: 2011-12-05 05:56 PST (History)
13 users (show)

See Also:


Attachments
Patch (7.77 KB, patch)
2011-09-26 19:09 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (7.76 KB, patch)
2011-09-27 11:30 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (7.85 KB, patch)
2011-09-27 13:44 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (9.02 KB, patch)
2011-09-27 16:49 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (10.36 KB, patch)
2011-09-28 13:48 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (13.02 KB, patch)
2011-10-03 17:23 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (13.31 KB, patch)
2011-12-02 15:42 PST, Rachel Blum
aroben: review-
gustavo.noronha: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rachel Blum 2011-09-26 18:25:23 PDT
While the original issue has been fixed in #65564, that patch had no LayoutTest attached. This will be remedied here.
Comment 1 Rachel Blum 2011-09-26 19:09:01 PDT
Created attachment 108778 [details]
Patch
Comment 2 Rachel Blum 2011-09-26 19:10:00 PDT
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 3 Adam Roben (:aroben) 2011-09-27 05:47:26 PDT
Comment on attachment 108778 [details]
Patch

Yay tests!
Comment 4 Rachel Blum 2011-09-27 11:30:43 PDT
Created attachment 108874 [details]
Patch
Comment 5 Gustavo Noronha (kov) 2011-09-27 11:40:07 PDT
Comment on attachment 108874 [details]
Patch

Attachment 108874 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9877425
Comment 6 Rachel Blum 2011-09-27 13:44:46 PDT
Created attachment 108890 [details]
Patch
Comment 7 Collabora GTK+ EWS bot 2011-09-27 15:15:14 PDT
Comment on attachment 108890 [details]
Patch

Attachment 108890 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9878452
Comment 8 Rachel Blum 2011-09-27 16:49:48 PDT
Created attachment 108933 [details]
Patch
Comment 9 Collabora GTK+ EWS bot 2011-09-27 18:21:08 PDT
Comment on attachment 108933 [details]
Patch

Attachment 108933 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9878499
Comment 10 Rachel Blum 2011-09-28 13:48:47 PDT
Created attachment 109068 [details]
Patch
Comment 11 Rachel Blum 2011-09-29 11:31:51 PDT
All export definitions fixed.... Ready for review
Comment 12 Adam Roben (:aroben) 2011-10-03 06:38:41 PDT
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 13 Darin Adler 2011-10-03 07:42:01 PDT
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.
Comment 14 Rachel Blum 2011-10-03 17:23:47 PDT
Created attachment 109557 [details]
Patch
Comment 15 Rachel Blum 2011-10-03 17:26:50 PDT
* 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 16 Gustavo Noronha (kov) 2011-10-03 22:31:35 PDT
Comment on attachment 109557 [details]
Patch

Attachment 109557 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9938299
Comment 17 Darin Adler 2011-10-04 07:39:33 PDT
Comment on attachment 109557 [details]
Patch

GTK build still failing, not sure why
Comment 18 Rachel Blum 2011-10-05 13:59:39 PDT
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.
Comment 19 Martin Robinson 2011-10-05 14:13:08 PDT
(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. :(
Comment 20 Rachel Blum 2011-10-12 18:53:41 PDT
So could somebody land this patch, please?
Comment 21 WebKit Review Bot 2011-12-02 06:50:28 PST
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
Comment 22 Rachel Blum 2011-12-02 15:42:03 PST
Created attachment 117706 [details]
Patch
Comment 23 Collabora GTK+ EWS bot 2011-12-02 22:22:53 PST
Comment on attachment 117706 [details]
Patch

Attachment 117706 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10701457
Comment 24 Adam Roben (:aroben) 2011-12-05 05:56:37 PST
Comment on attachment 117706 [details]
Patch

Looks like this doesn't build on GTK.