[Webkit-unassigned] [Bug 60247] Handle the touch icon in WebKit/Chromium

[bugzilla-daemon at webkit.org]

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


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

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




--- Comment #29 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2011-05-10 18:34:19 PST ---
(From update of attachment 93022)
View in context: https://bugs.webkit.org/attachment.cgi?id=93022&action=review

> Source/WebKit/chromium/public/WebFrame.h:137
> +    // the document loaded in this frame. The iconTypes could be a bit-mask of

Sorry for all the back-n-forth on this.  Being out on vacation has meant that I haven't
had the time to give this the focus I should.  Thanks for putting up with all the cycles.

nit: "The iconTypes [is] a bit-mask of WebIconURL::Type values, used to select from
the available set of icon URLs."

nit: Since this method returns possibly many WebIconURL objects, please pluralize the
method name.  It should be favIconURLs instead of favIconURL.

nit: Since this method returns an array of WebIconURL and not WebFavIconURL, the method
should not have the "Fav" part in its name.  I think it should just be iconURLs.

> Source/WebKit/chromium/public/WebFrameClient.h:206
> +    virtual void didChangeIcons(WebFrame*, WebIconURL::Type) { }

I think this method name could be improved.  Since it is only reporting the
change of a single icon type, it should not be pluralized.  It should just
be didChangeIcon.

> Source/WebKit/chromium/public/WebIconURL.h:67
> +

you can define a constructor here that takes a WebCore::IconURL object.
wrap the constructor in #if WEBKIT_IMPLEMENTATION so that it will not
be visible to consumers of the API (we don't want them to see WebCore
stuff).

with the above constructor, it is now possible to implicitly convert
WTF::Vector<WebCore::IconURL> to WebVector<WebIconURL>.  Magic!

> Source/WebKit/chromium/src/WebFrameImpl.cpp:531
> +WebVector<WebIconURL> WebFrameImpl::favIconURL(int webIconTypes) const

nit: parameter name should just be iconTypes.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:537
> +        Vector<WebCore::IconURL> iconURLs = frameLoader->iconURLs(webIconTypes);

with the constructor on WebIconURL that i mentioned, you can condense this down to:

  return frameLoader->iconURLs(iconTypes);

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