[Webkit-unassigned] [Bug 35072] [Mac] Move Icon::iconForFiles() code to WebChromeClient::iconForFiles().

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #51412|review?                     |review+
               Flag|                            |




--- Comment #13 from Darin Adler <darin at apple.com>  2010-03-23 14:03:16 PST ---
(From update of attachment 51412)
> -    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

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