Bug 62931 - Separate concerns of loading file icons and choosing files.
: Separate concerns of loading file icons and choosing files.
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 62069
  Show dependency treegraph
 
Reported: 2011-06-18 10:45 PST by
Modified: 2011-06-19 10:00 PST (History)


Attachments
WIP: Cook on bots. (57.82 KB, patch)
2011-06-18 10:45 PST, Dimitri Glazkov (Google)
no flags Review Patch | Details | Formatted Diff | Diff
Fix Win build. (58.29 KB, patch)
2011-06-18 14:19 PST, Dimitri Glazkov (Google)
no flags Review Patch | Details | Formatted Diff | Diff
Energize. (59.55 KB, patch)
2011-06-18 16:24 PST, Dimitri Glazkov (Google)
darin: review+
Review Patch | Details | Formatted Diff | Diff
Patch for landing. (59.90 KB, patch)
2011-06-19 08:55 PST, Dimitri Glazkov (Google)
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-06-18 10:45:03 PST
Separate concerns of loading file icons from choosing files.
------- Comment #1 From 2011-06-18 10:45:24 PST -------
Created an attachment (id=97702) [details]
WIP: Cook on bots.
------- Comment #2 From 2011-06-18 14:19:10 PST -------
Created an attachment (id=97706) [details]
Fix Win build.
------- Comment #3 From 2011-06-18 16:24:14 PST -------
Created an attachment (id=97708) [details]
Energize.
------- Comment #4 From 2011-06-18 16:27:28 PST -------
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 #5 From 2011-06-18 17:36:43 PST -------
(From update of attachment 97708 [details])
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 #6 From 2011-06-18 17:48:53 PST -------
(From update of attachment 97708 [details])
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
------- Comment #7 From 2011-06-18 17:53:33 PST -------
(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.
------- Comment #8 From 2011-06-18 17:54:26 PST -------
(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
------- Comment #9 From 2011-06-19 08:55:52 PST -------
Created an attachment (id=97725) [details]
Patch for landing.
------- Comment #10 From 2011-06-19 09:01:58 PST -------
(From update of attachment 97725 [details])
Attachment 97725 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8914144
------- Comment #11 From 2011-06-19 09:23:18 PST -------
(From update of attachment 97725 [details])
Attachment 97725 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8909160
------- Comment #12 From 2011-06-19 10:00:37 PST -------
Committed r89216: <http://trac.webkit.org/changeset/89216>