Bug 32473 - [Chromium] Add 2 parameters to WebViewClient::runFileChooser()
Summary: [Chromium] Add 2 parameters to WebViewClient::runFileChooser()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on: 32429
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-12 11:59 PST by Kent Tamura
Modified: 2010-01-14 09:39 PST (History)
6 users (show)

See Also:


Attachments
Proposed patch (4.03 KB, patch)
2009-12-12 12:01 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.2) (8.33 KB, patch)
2010-01-13 22:54 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.3) (8.33 KB, patch)
2010-01-13 23:06 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.4) (7.96 KB, patch)
2010-01-14 00:04 PST, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2009-12-12 11:59:27 PST
We had better add parameters to WebViewClient::runFileChooser() in order to improve multi-selected file upload control UI and support for HTML5's accept attribute.
Comment 1 Kent Tamura 2009-12-12 12:01:55 PST
Created attachment 44740 [details]
Proposed patch

The patch should not be committed before http://codereview.chromium.org/497001 is committed.
Comment 2 WebKit Review Bot 2009-12-12 12:07:07 PST
style-queue ran check-webkit-style on attachment 44740 [details] without any errors.
Comment 3 Eric Seidel (no email) 2009-12-21 00:23:03 PST
I think Darin Fisher or Dimitri Glazkov are your peeps here.
Comment 4 Darin Fisher (:fishd, Google) 2010-01-13 11:01:09 PST
Comment on attachment 44740 [details]
Proposed patch

> +++ b/WebKit/chromium/public/WebViewClient.h
...
>      virtual bool runFileChooser(
>          bool multiSelect, const WebString& title,
> -        const WebString& initialValue, WebFileChooserCompletion*) { return false; }
> +        const WebString& initialValue,
> +        const WebVector<WebString>& selectedFiles, const WebString& acceptTypes,
> +        WebFileChooserCompletion*) { return false; }

Instead of dumping more parameters on this function, I think we should
create a WebFileChooserParams structure (much like WebPluginParams).

This way it'll be easier to add additional parameters in the future, and
we can document the parameters in WebFileChooserParams.h instead of here
in WebViewClient.h (which should improve readability a bit).

To avoid the issue of breaking the chromium build, please preserve the old
WebViewClient::runFileChooser method temporarily.  Add a comment to declare
that that old method is deprecated.

You can even use the return value of runFileChooser to failover to the old
deprecated method if the new method returns false.  Then, once the chromium
side picks up this new version of WebKit, you should go back and remove the
deprecated function.  This way there is no burden on the chromium tree sheriffs.
Comment 5 Kent Tamura 2010-01-13 22:54:25 PST
Created attachment 46541 [details]
Proposed patch (rev.2)
Comment 6 Kent Tamura 2010-01-13 22:56:08 PST
Thank you for reviewing.

(In reply to comment #4)
> Instead of dumping more parameters on this function, I think we should
> create a WebFileChooserParams structure (much like WebPluginParams).

Ok, I introduced WebFileChooserParams in the updated patch.

> You can even use the return value of runFileChooser to failover to the old
> deprecated method if the new method returns false.  Then, once the chromium
> side picks up this new version of WebKit, you should go back and remove the
> deprecated function.  This way there is no burden on the chromium tree
> sheriffs.

That's good idea.  I updated the code so.
Comment 7 WebKit Review Bot 2010-01-13 23:00:26 PST
Attachment 46541 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/public/WebFileChooserParams.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1
Comment 8 Kent Tamura 2010-01-13 23:06:19 PST
Created attachment 46542 [details]
Proposed patch (rev.3)
Comment 9 Darin Fisher (:fishd, Google) 2010-01-13 23:43:13 PST
Comment on attachment 46542 [details]
Proposed patch (rev.3)

> +++ b/WebKit/chromium/public/WebFileChooserParams.h
...
> +struct WebFileChooserParams {
...
> +    WebFileChooserCompletion* chooserCompletion;

I'm sorry for not mentioning this earlier, but I think that this
member should not be part of this structure since it is not a
parameter that effects the way the dialog is rendered.  It is
conceptually separate from those parameters since it is just
a mechanism for indicating completion of the runFileChooser call.

Sorry to add another round to this patch, but I think this would
be a good change to make.


> +++ b/WebKit/chromium/src/ChromeClientImpl.cpp
...
> +    WebFileChooserParams params;
> +    params.multiSelect = fileChooser->allowsMultipleFiles();
> +    params.acceptTypes = fileChooser->acceptTypes();
> +    const Vector<String>& selectedFiles = fileChooser->filenames();
> +    params.selectedFiles = WebVector<WebString>(selectedFiles);

you should be able to write:

       params.selectedFiles = fileChooser->filenames();

does that not work for you?
Comment 10 Kent Tamura 2010-01-14 00:04:50 PST
Created attachment 46545 [details]
Proposed patch (rev.4)
Comment 11 Kent Tamura 2010-01-14 00:06:24 PST
> > +struct WebFileChooserParams {
> ...
> > +    WebFileChooserCompletion* chooserCompletion;
> 
> I'm sorry for not mentioning this earlier, but I think that this
> member should not be part of this structure since it is not a
> parameter that effects the way the dialog is rendered.  It is
> conceptually separate from those parameters since it is just
> a mechanism for indicating completion of the runFileChooser call.

ok, done.

> > +    const Vector<String>& selectedFiles = fileChooser->filenames();
> > +    params.selectedFiles = WebVector<WebString>(selectedFiles);
> 
> you should be able to write:
> 
>        params.selectedFiles = fileChooser->filenames();
> 
> does that not work for you?

It works.  Fixed.
Probably I had a reason in the last year though I can't remember it :-)
Comment 12 WebKit Commit Bot 2010-01-14 09:39:03 PST
Comment on attachment 46545 [details]
Proposed patch (rev.4)

Clearing flags on attachment: 46545

Committed r53269: <http://trac.webkit.org/changeset/53269>
Comment 13 WebKit Commit Bot 2010-01-14 09:39:09 PST
All reviewed patches have been landed.  Closing bug.