Bug 62931

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

Created attachment 97706 [details] Fix Win build.

Created attachment 97708 [details] Energize.

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.

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.

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

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

(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

Created attachment 97725 [details] Patch for landing.

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

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

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