[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





--- Comment #14 from TAMURA, Kent <tkent at chromium.org>  2010-03-24 02:17:22 PST ---
(In reply to comment #13)
> (From update of attachment 51412 [details])
> > -    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?

It makes no sense, and should be removed.
I don't remember why I added it :-P

> > -    // 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.

I see. I'll remove the #ifdef.

> 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.

Yeah, you wrote so in Bug#32054. But bdash and eric objected in this bug.
Hmm, I'll wait for additional comments from bdash and eric for a few days, and
if no objections, I'll update the original patch in this bug (Moving Icon
loading code for Mac).

> r=me

Anyway, I'll land this patch (Another approach rev.2) with the above fixes. 
It's not harmful.

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