Bug 62931 – Separate concerns of loading file icons and choosing files.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dimitri Glazkov (Google)]

Separate concerns of loading file icons from choosing files.

[Dimitri Glazkov (Google)]

Created attachment 97702 [details]
WIP: Cook on bots.

[Dimitri Glazkov (Google)]

Created attachment 97706 [details]
Fix Win build.

[Dimitri Glazkov (Google)]

Created attachment 97708 [details]
Energize.

[Dimitri Glazkov (Google)]

This patch also resolves nicely the conundrum from bug 36723, which was fixed by introducing "am I a real boy yet?" boolean. It's brittle, and I am happy to report we no longer need this workaround.

[Darin Adler]

Comment on attachment 97708 [details]
Energize.

View in context: https://bugs.webkit.org/attachment.cgi?id=97708&action=review

Seems like a nice improvement.

> Source/WebCore/page/ChromeClient.h:230
> -        virtual void chooseIconForFiles(const Vector<String>&, FileChooser*) = 0;
> +        virtual void loadIconForFiles(const Vector<String>&, PassRefPtr<FileIconLoader>) = 0;

I think that FileIconLoader should be a raw pointer, not a PassRefPtr. The caller is giving an icon loader for use, not handing off ownership. Even if the recipient might choose to take a reference to it.

> Source/WebCore/rendering/RenderFileUploadControl.cpp:82
> +    if (Chrome* chromePointer = chrome())
> +        chromePointer->loadIconForFiles(filenames, m_iconLoader);

Our style for this is normally:

    if (Chrome* chrome = this->chrome())

That way you can use the word “chrome” and not have to say “chromePointer”.

> Source/WebCore/rendering/RenderFileUploadControl.h:70
> +    // FileIconLoaderClient methods.
> +    void updateRendering(PassRefPtr<Icon>);

I know the other comment uses the word “methods”, but that is not a C++ term. The C++ term is virtual member functions or just virtual functions.

These should be explicitly marked virtual too. Again, FileChooserClient functions above are not done that way, but that’s a mistake.

At some point we need to add a feature to clang so we can use something explicitly for overriding, like virtual but a compile error or warning if you are not overriding something inherited from a base class. In other compilers we’d just use “virtual”. I wonder if someone has already designed that and whether the clang folks would be open to it or not.

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:791
> -void WebChromeClient::chooseIconForFiles(const Vector<String>& filenames, FileChooser* chooser)
> +void WebChromeClient::loadIconForFiles(const Vector<String>& filenames, PassRefPtr<FileIconLoader> iconLoader)
>  {
> -    chooser->iconLoaded(Icon::createIconForFiles(filenames));
> +    iconLoader->notifyFinished(Icon::createIconForFiles(filenames));
>  }

I’d just use the name loader here instead of iconLoader; in this local scope the extra word doesn’t add clarity, so brevity is better.

[Dimitri Glazkov (Google)]

Comment on attachment 97708 [details]
Energize.

View in context: https://bugs.webkit.org/attachment.cgi?id=97708&action=review

Thanks for your review! I will address the feedback and land.

>> Source/WebCore/page/ChromeClient.h:230
>> +        virtual void loadIconForFiles(const Vector<String>&, PassRefPtr<FileIconLoader>) = 0;
> 
> I think that FileIconLoader should be a raw pointer, not a PassRefPtr. The caller is giving an icon loader for use, not handing off ownership. Even if the recipient might choose to take a reference to it.

I could've sworn you've given me a nearly opposite feedback once... Here it is: https://bugs.webkit.org/show_bug.cgi?id=46015#c15

[Darin Adler]

(In reply to comment #6)
> I could've sworn you've given me a nearly opposite feedback once... Here it is: https://bugs.webkit.org/show_bug.cgi?id=46015#c15

I’m not sure whether I was wrong that time or wrong this time. I just wrote a message to webkit-dev about this subject.

[Dimitri Glazkov (Google)]

(In reply to comment #7)
> (In reply to comment #6)
> > I could've sworn you've given me a nearly opposite feedback once... Here it is: https://bugs.webkit.org/show_bug.cgi?id=46015#c15
> 
> I’m not sure whether I was wrong that time or wrong this time. I just wrote a message to webkit-dev about this subject.

It's the ghost of the past battling the ghost of the future!!! :P

[Dimitri Glazkov (Google)]

Created attachment 97725 [details]
Patch for landing.

[WebKit Review Bot]

Comment on attachment 97725 [details]
Patch for landing.

Attachment 97725 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8914144

[WebKit Review Bot]

Comment on attachment 97725 [details]
Patch for landing.

Attachment 97725 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8909160

[Dimitri Glazkov (Google)]

Committed r89216: <http://trac.webkit.org/changeset/89216>