[webkit-reviews] review granted: [Bug 62931] Separate concerns of loading file icons and choosing files. : [Attachment 97708] Energize.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 62931: Separate concerns of loading file icons and choosing files.
https://bugs.webkit.org/show_bug.cgi?id=62931

Attachment 97708: Energize.
https://bugs.webkit.org/attachment.cgi?id=97708&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.