Bug 35072 – [Mac] Move Icon::iconForFiles() code to WebChromeClient::iconForFiles().

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Kent Tamura]

[Mac] Move Icon::iconForFiles() code to WebChromeClient::iconForFiles().

[Kent Tamura]

Created attachment 48969 [details]
Proposed patch

[Mark Rowe (bdash)]

Isn’t moving code from WebCore to WebKit going in completely the wrong direction?

[Kent Tamura]

(In reply to comment #2)
> Isn’t moving code from WebCore to WebKit going in completely the wrong
> direction?

The API change (Icon::iconForFiles() to ChromeClient::iconForFiles()) was agreed in Bug#32054 because Chromium can't load icons in WebCore.

[Eric Seidel (no email)]

My apologies.  I clearly didn't read the other patch closely enough to realize that code was moving from WebCore to WebKit.  I should have commented on that before.  It's obvious looking back.

Mark is right, historically the wisdom is that moving things out of WebKIt and into WebCore is a good thing.  Makes the code cross-plaform and shared among more ports.  Every WebKIt layer is a porting burden.

Although your patch added a good feature (asynchronous icon loading), I think that we may need to re-visit how it was done.

Why do we need any special per-platform code for "icon" loading.  Don't we already have plenty of async code for loading data files?  Can't we load these icons the same way we would load any other CachedImage file?

Ideally the asynchronous loading code could all be shared by all WebCore ports, and only the individual "read the bits off disk" and "turn these icon bits into an Image" would be per-port.  None of that seems like it needs to be in WebKit.

I guess the problem is that the OS might not actually be loading files directly.  In the case of Mac OS X, the library APIs hand you back NSImage pointers instead of any URL to load?  Is that correct?

Perhaps other platforms function the same in which case the *entire* load is platform specific, and it may make sense to do the load entirely in the platform-speicifc bits we call WebKit as you have done.

I think I've talked myself in circles.  Lunch time.

[Kent Tamura]

I tried an idea of icon loading via a special URL several months ago.  However it failed because of some security restrictions.

The current behavior is:
 1. FileChooser calls Icon::iconForFiles()
 2. FileChooser calls ChromeClient::iconForFiles() if the above failed.
Darin and Maciej didn't like such duplicated API situation.

How about changing it to:
 * FileChooser calls just ChromeClient::iconForFiles().
  So, all platforms need to implement ChromeClient::iconForFiles()
 
* Provide optional sub-API, Icon::iconForFiles().
  Chromium won't implement it.
  ChromeClient::iconForFiles() for other platforms call iconForFiles()
 
We don't need to move existing iconForFiles() implementations from WebCore in this idea.

[Kent Tamura]

Created attachment 51121 [details]
Another approach

Instead of moving icon loading code, just call Icon::createIconForFiles() from ChromeClient::iconForFiles().

[Kent Tamura]

Comment on attachment 51121 [details]
Another approach

> +#if !PLATFORM(Chromium)
>      static PassRefPtr<Icon> createIconForFiles(const Vector<String>& filenames);

Oops, it should be CHROMIUM.

[Darin Adler]

Comment on attachment 51121 [details]
Another approach

> -void ChromeClient::iconForFiles(const Vector<WebCore::String>&, PassRefPtr<WebCore::FileChooser>)
> +void ChromeClient::iconForFiles(const Vector<WebCore::String>& filenames, PassRefPtr<WebCore::FileChooser> chooser)

This function name is not good. The function doesn't return an icon, so it shouldn't be named "icon". I suggest the name chooseIconForFiles.

The argument types on this function may also be wrong. A function should only take a PassRefPtr if it's taking ownership of an object. This function does not take ownership of the FileChooser object.

[Adam Barth]

Comment on attachment 48969 [details]
Proposed patch

Darin's comments lead me to r- this patch.  I'm not sure we've reached consensus about whether to have this code reach out to WebKit...

[Adam Barth]

Comment on attachment 51121 [details]
Another approach

Darin's comments lead me to r- this patch.

[Kent Tamura]

Created attachment 51412 [details]
Another approach (rev.2)

Renamed iconForFiles() to chooseIconForFiles().

[Kent Tamura]

> This function name is not good. The function doesn't return an icon, so it
> shouldn't be named "icon". I suggest the name chooseIconForFiles.

Done.

> The argument types on this function may also be wrong. A function should only
> take a PassRefPtr if it's taking ownership of an object. This function does not
> take ownership of the FileChooser object.

Chromium implementation will need to take ownership because of asynchronous loading.

[Darin Adler]

Comment on attachment 51412 [details]
Another approach (rev.2)

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

[Kent Tamura]

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

[Kent Tamura]

(In reply to comment #14)
> > r=me
> 
> Anyway, I'll land this patch (Another approach rev.2) with the above fixes. 
> It's not harmful.

Landed as r56439.

[Brady Eidson]

This patch (http://trac.webkit.org/changeset/56439) caused a major regression (https://bugs.webkit.org/show_bug.cgi?id=36723)

Since:
1 - It didn't change functionality
2 - The approach was generally not liked.
3 - In general, round tripping between WebCore and WebKit is something we try to *reduce*

...I think it needs to be rolled out.

[Kent Tamura]

(In reply to comment #16)
> This patch (http://trac.webkit.org/changeset/56439) caused a major regression
> (https://bugs.webkit.org/show_bug.cgi?id=36723)
> 
> Since:
> 1 - It didn't change functionality
> 2 - The approach was generally not liked.
> 3 - In general, round tripping between WebCore and WebKit is something we try
> to *reduce*
> 
> ...I think it needs to be rolled out.

Brady, I already post a fix for the regression.  Please don't roll it out.
Even if we rolled it out, the problem would not be solved with platforms of which Icon::iconForFiles() returns 0.

[Brady Eidson]

(In reply to comment #17)

I replied to this in https://bugs.webkit.org/show_bug.cgi?id=36723 where it is more directly relevant.

[Kent Tamura]

Today I have understood moving code from WebCore to WebKit is too bad if a platform has two different WebKit layers.  So, I won't do that.

[Dimitri Glazkov (Google)]

(In reply to comment #5)
> I tried an idea of icon loading via a special URL several months ago.  However it failed because of some security restrictions.

Looking at this code now a few months later, I am becoming convinced we need to address this differently. Icon code looks complicated, brittle, and in spirit very similar to how we load images. Perhaps we could try giving another shot to reusing image plumbing?

[Dimitri Glazkov (Google)]

(In reply to comment #20)
> (In reply to comment #5)
> > I tried an idea of icon loading via a special URL several months ago.  However it failed because of some security restrictions.
> 
> Looking at this code now a few months later, I am becoming convinced we need to address this differently. Icon code looks complicated, brittle, and in spirit very similar to how we load images. Perhaps we could try giving another shot to reusing image plumbing?

Created bug 62758 to track this idea.