Remove WebFileChooserParams::initialValue

Remove WebFileChooserParams::initialValue

On the Chromium side, FileSelectHelper::RunFileChooser asserts that
default filenames should only be specified for "Save" choosers. Blink
doesn't use those ("download" is a browser thing) so just remove
the field which would be ignored anyway. (Remembering the last used
directory is done through other means in the browser.)

BUG=632454, 655298
Committed: https://crrev.com/14785d90f691b2b20d07612a62ba35a630bbb9ac
Cr-Commit-Position: refs/heads/master@{#427814}

[jsbell, 2016-10-24 21:25:56]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 21:27:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2016-10-24 23:34:37]

FYI, I'll add a browser test that at least verifies that the value is not
specified for blink-initiated file pickers. New patch coming soon.

[jsbell, 2016-10-25 00:11:54]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 00:12:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2016-10-25 00:12:57]

Description was changed from

==========
Don't unnecessarily populate WebFileChooserParams::initialValue

On the Chromium side, FileSelectHelper::RunFileChooser asserts that
default filenames should only be specified for "Save" choosers. Blink
doesn't use those ("download" is a browser thing) so don't bother
populating the field which would be ignored anyway. (Remembering the
last used directory is done through other means in the browser.)

BUG=632454
==========

to

==========
Don't unnecessarily populate WebFileChooserParams::initialValue

On the Chromium side, FileSelectHelper::RunFileChooser asserts that
default filenames should only be specified for "Save" choosers. Blink
doesn't use those ("download" is a browser thing) so don't bother
populating the field which would be ignored anyway. (Remembering the
last used directory is done through other means in the browser.)

BUG=632454,655298
==========

[jsbell, 2016-10-25 00:18:05]

jsbell@chromium.org changed reviewers:
+ dcheng@chromium.org, kinuko@chromium.org

[jsbell, 2016-10-25 00:18:05]

kinuko@ - can you specifically review the content/ changes and the overall
approach? (i.e. does this seem right to you?)

dcheng@ - can you review third_party/WebKit/Source/web change?

Thanks!

[dcheng, 2016-10-25 00:27:35]

https://codereview.chromium.org/2446743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (left):

https://codereview.chromium.org/2446743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:715: params.initialValue =
params.selectedFiles[0];
Is it possible to remove initialValue completely?

[commit-bot: I haz the power, 2016-10-25 01:55:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 01:55:59]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[jsbell, 2016-10-25 16:24:45]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[jsbell, 2016-10-25 16:24:55]

https://codereview.chromium.org/2446743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (left):

https://codereview.chromium.org/2446743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:715: params.initialValue =
params.selectedFiles[0];
On 2016/10/25 00:27:35, dcheng wrote:
> Is it possible to remove initialValue completely?

Used in PepperFileChooserHost::OnShow apparently, which does have a "save as"
path. We could refactor it to use FileChooserParams directly  and split up
runFileChooser() - not sure that would be cleaner.

I've renamed the param (|defaultName|), updated the docs and added a DCHECK.

[commit-bot: I haz the power, 2016-10-25 16:25:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2016-10-25 17:30:00]

It feels a bit odd to have a value in the Blink public API that isn't consumed
used by anything in Blink. Maybe we can just use ScheduleFileChooser directly
and move the IsHidden() check in there to avoid code duplication?

[commit-bot: I haz the power, 2016-10-25 17:59:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 17:59:32]

Dry run: This issue passed the CQ dry run.

[jsbell, 2016-10-25 18:29:29]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 18:30:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 19:48:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 19:48:43]

Dry run: This issue passed the CQ dry run.

[jsbell, 2016-10-25 19:59:56]

On 2016/10/25 17:30:00, dcheng wrote:
> It feels a bit odd to have a value in the Blink public API that isn't consumed
> used by anything in Blink. Maybe we can just use ScheduleFileChooser directly
> and move the IsHidden() check in there to avoid code duplication?

Yeah, you convinced me. Refactored as suggested.

I left the dependency by content::PepperFileChooserHost on
blink::WebFileChooserCompletion though that's obviously a target for untangling
as well.

[dcheng, 2016-10-25 20:17:39]

Blink LGTM

[kinuko, 2016-10-26 02:35:01]

lgtm % nits

https://codereview.chromium.org/2446743002/diff/60001/content/browser/fileapi...
File content/browser/fileapi/file_system_browsertest.cc (right):

https://codereview.chromium.org/2446743002/diff/60001/content/browser/fileapi...
content/browser/fileapi/file_system_browsertest.cc:98:
IN_PROC_BROWSER_TEST_F(FileSystemBrowserTest, FileInputChooserParams) {
This is not really about FileSystem test, could we put this in its own file or
maybe we could just rename this file/tests to FileAPIBrowserTest?

https://codereview.chromium.org/2446743002/diff/60001/content/browser/fileapi...
content/browser/fileapi/file_system_browsertest.cc:116: // Click again, to
verify what state was maintained and what was not.
nit: Maybe add a few more words to note why we test this / why default_file_name
must be empty?

https://codereview.chromium.org/2446743002/diff/60001/content/test/content_br...
File content/test/content_browser_test_utils_internal.h (right):

https://codereview.chromium.org/2446743002/diff/60001/content/test/content_br...
content/test/content_browser_test_utils_internal.h:156: FileChooserParams
params() { return params_; }
nit: make it a const method?

[jsbell, 2016-10-26 15:48:54]

Description was changed from

==========
Don't unnecessarily populate WebFileChooserParams::initialValue

On the Chromium side, FileSelectHelper::RunFileChooser asserts that
default filenames should only be specified for "Save" choosers. Blink
doesn't use those ("download" is a browser thing) so don't bother
populating the field which would be ignored anyway. (Remembering the
last used directory is done through other means in the browser.)

BUG=632454,655298
==========

to

==========
Remove WebFileChooserParams::initialValue

On the Chromium side, FileSelectHelper::RunFileChooser asserts that
default filenames should only be specified for "Save" choosers. Blink
doesn't use those ("download" is a browser thing) so just remove
the field which would be ignored anyway. (Remembering the last used
directory is done through other means in the browser.)

BUG=632454,655298
==========

[jsbell, 2016-10-26 17:21:13]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2016-10-26 17:21:13]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2446743002/#ps80001
(title: "Split test into own file; const sprinkles")

[commit-bot: I haz the power, 2016-10-26 17:21:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2016-10-26 17:21:44]

Thanks!

https://codereview.chromium.org/2446743002/diff/60001/content/browser/fileapi...
File content/browser/fileapi/file_system_browsertest.cc (right):

https://codereview.chromium.org/2446743002/diff/60001/content/browser/fileapi...
content/browser/fileapi/file_system_browsertest.cc:98:
IN_PROC_BROWSER_TEST_F(FileSystemBrowserTest, FileInputChooserParams) {
On 2016/10/26 02:35:01, kinuko wrote:
> This is not really about FileSystem test, could we put this in its own file or
> maybe we could just rename this file/tests to FileAPIBrowserTest?

I split it up. The other tests are filesystem specific, so I tweaked the comment
in the original file.

https://codereview.chromium.org/2446743002/diff/60001/content/browser/fileapi...
content/browser/fileapi/file_system_browsertest.cc:116: // Click again, to
verify what state was maintained and what was not.
On 2016/10/26 02:35:01, kinuko wrote:
> nit: Maybe add a few more words to note why we test this / why
default_file_name
> must be empty?

Done.

https://codereview.chromium.org/2446743002/diff/60001/content/test/content_br...
File content/test/content_browser_test_utils_internal.h (right):

https://codereview.chromium.org/2446743002/diff/60001/content/test/content_br...
content/test/content_browser_test_utils_internal.h:156: FileChooserParams
params() { return params_; }
On 2016/10/26 02:35:01, kinuko wrote:
> nit: make it a const method?

Done.

[commit-bot: I haz the power, 2016-10-26 17:31:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 17:31:13]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[jsbell, 2016-10-26 19:40:14]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2016-10-26 19:41:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 21:37:52]

Description was changed from

==========
Remove WebFileChooserParams::initialValue

On the Chromium side, FileSelectHelper::RunFileChooser asserts that
default filenames should only be specified for "Save" choosers. Blink
doesn't use those ("download" is a browser thing) so just remove
the field which would be ignored anyway. (Remembering the last used
directory is done through other means in the browser.)

BUG=632454,655298
==========

to

==========
Remove WebFileChooserParams::initialValue

On the Chromium side, FileSelectHelper::RunFileChooser asserts that
default filenames should only be specified for "Save" choosers. Blink
doesn't use those ("download" is a browser thing) so just remove
the field which would be ignored anyway. (Remembering the last used
directory is done through other means in the browser.)

BUG=632454,655298
==========

[commit-bot: I haz the power, 2016-10-26 21:37:53]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-26 21:42:17]

Description was changed from

==========
Remove WebFileChooserParams::initialValue

On the Chromium side, FileSelectHelper::RunFileChooser asserts that
default filenames should only be specified for "Save" choosers. Blink
doesn't use those ("download" is a browser thing) so just remove
the field which would be ignored anyway. (Remembering the last used
directory is done through other means in the browser.)

BUG=632454,655298
==========

to

==========
Remove WebFileChooserParams::initialValue

On the Chromium side, FileSelectHelper::RunFileChooser asserts that
default filenames should only be specified for "Save" choosers. Blink
doesn't use those ("download" is a browser thing) so just remove
the field which would be ignored anyway. (Remembering the last used
directory is done through other means in the browser.)

BUG=632454,655298

Committed: https://crrev.com/14785d90f691b2b20d07612a62ba35a630bbb9ac
Cr-Commit-Position: refs/heads/master@{#427814}
==========

[commit-bot: I haz the power, 2016-10-26 21:42:18]

Patchset 5 (id:??) landed as
https://crrev.com/14785d90f691b2b20d07612a62ba35a630bbb9ac
Cr-Commit-Position: refs/heads/master@{#427814}