[webkit-reviews] review granted: [Bug 62931] Separate concerns of loading file icons and choosing files. : [Attachment 97708] Energize.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Jun 18 17:36:42 PDT 2011
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.
More information about the webkit-reviews
mailing list