[webkit-reviews] review granted: [Bug 35072] [Mac] Move Icon::iconForFiles() code to WebChromeClient::iconForFiles(). : [Attachment 51412] Another approach (rev.2)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 35072: [Mac] Move Icon::iconForFiles() code to
WebChromeClient::iconForFiles().
https://bugs.webkit.org/show_bug.cgi?id=35072

Attachment 51412: Another approach (rev.2)
https://bugs.webkit.org/attachment.cgi?id=51412&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -    m_icon = Icon::createIconForFiles(m_filenames);
> -    // If synchronous icon loading failed, try asynchronous loading.
> -    if (!m_icon && m_filenames.size() && m_client)
> -	   m_client->iconForFiles(m_filenames);
> +    m_icon = 0;
> +    if (m_filenames.size() && m_client)
> +	   m_client->chooseIconForFiles(m_filenames);

Could you explain why the m_icon = 0 code is needed/helpful here?

> -    // Deprecated.  This function will be removed.
> -    // FIXME: Remove it when all implementations are moved to
ChromeClient::iconForFiles().
> +#if !PLATFORM(CHROMIUM)
>      static PassRefPtr<Icon> createIconForFiles(const Vector<String>&
filenames);
> +#endif

If Chromium is neither calling not implementing this function, it seems OK to
just leave the declaration there without an ugly conditional. It does no real
harm. Will this function eventually be removed or not? I don't understand the
status of this function. Is it just a helper for WebKit? Maybe other platforms
will later want to remove this too. Maybe new platforms won't need it. I think
having an ifdef around it is strange.

Maybe the right thing to do is to move the body of this function from WebCore
into WebKit for all platforms, and then remove it entirely from WebCore.

r=me