[Webkit-unassigned] [Bug 68861] Support for multiple <link rel="icon"> favicon elements - LayoutTests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=68861


Adam Roben (:aroben) <aroben at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #109068|review?                     |review-
               Flag|                            |




--- Comment #12 from Adam Roben (:aroben) <aroben at apple.com>  2011-10-03 06:38:41 PST ---
(From update of attachment 109068)
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?).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.