[webkit-reviews] review denied: [Bug 68861] Support for multiple <link rel="icon"> favicon elements - LayoutTests : [Attachment 109068] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Rachel Blum
<groby at chromium.org>'s request for review:
Bug 68861: Support for multiple <link rel="icon"> favicon elements -
LayoutTests
https://bugs.webkit.org/show_bug.cgi?id=68861

Attachment 109068: Patch
https://bugs.webkit.org/attachment.cgi?id=109068&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
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?).