Files.app: Let Files.app pass mutliple files to file handlers.

Files.app: Let Files.app pass mutliple files to file handlers.

Previously if multiple files are selected for launching a file handler,
Files.app sends multiple launch events for each file. But according to the
specification of the launch event, we can put multiple entries in a single
launch event. This CL lets Files.app follow the specificaiton.

BUG=358694
TEST=manually
R=benwells@chromium.org, kinaba@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274755

[hirono, 2014-05-27 05:29:07]

PTAL the CL? Thank you!

[kinaba, 2014-05-27 07:08:26]

Please rebase over https://codereview.chromium.org/294163010/ that'll be landing
soon :).

https://codereview.chromium.org/300063006/diff/1/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/300063006/diff/1/apps/launcher.cc#newcode208
apps/launcher.cc:208: if (!base::PathExists(file_paths_[0]) ||
0 => mime_types.size() ? (I'd rather recommend you to declare a new variable for
the path.)

[hirono, 2014-05-28 02:23:27]

Thank you!

https://codereview.chromium.org/300063006/diff/1/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/300063006/diff/1/apps/launcher.cc#newcode208
apps/launcher.cc:208: if (!base::PathExists(file_paths_[0]) ||
On 2014/05/27 07:08:26, kinaba wrote:
> 0 => mime_types.size() ? (I'd rather recommend you to declare a new variable
for
> the path.)

Done.

[kinaba, 2014-05-28 03:19:29]

lgtm

[hirono, 2014-05-28 04:13:21]

@benwells - PTAL apps directory? Thank you!

[benwells, 2014-05-28 22:46:13]

https://codereview.chromium.org/300063006/diff/40001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/300063006/diff/40001/apps/launcher.cc#newcode163
apps/launcher.cc:163: file_paths_.erase(it);
should this be it = file_path_.erase(it)?

I'd be happy to jump to LaunchWithLaunchNoData at this point anyway.

https://codereview.chromium.org/300063006/diff/40001/apps/launcher.cc#newcode180
apps/launcher.cc:180: void StepToFillMimeTypes() {
This logic is more complicated than it needs to be on non-Chromeos. There we can
just fill in all the mime types in one go on the FILE thread, there is no need
to keep bouncing back to the UI thread.

https://codereview.chromium.org/300063006/diff/40001/apps/launcher.cc#newcode327
apps/launcher.cc:327: void GrantAccessToFileAndLaunch(ExtensionHost* host) {
Nit should be GrantAccessToFilesAndLaunch

https://codereview.chromium.org/300063006/diff/40001/apps/launcher.h
File apps/launcher.h (right):

https://codereview.chromium.org/300063006/diff/40001/apps/launcher.h#newcode45
apps/launcher.h:45: // |file_path| available through the launch data.
|handler_id| corresponds to
Nit: comment needs to be updated.

[hirono, 2014-05-29 04:08:03]

Thank you!

https://codereview.chromium.org/300063006/diff/40001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/300063006/diff/40001/apps/launcher.cc#newcode163
apps/launcher.cc:163: file_paths_.erase(it);
On 2014/05/28 22:46:14, benwells wrote:
> should this be it = file_path_.erase(it)?
> 
> I'd be happy to jump to LaunchWithLaunchNoData at this point anyway.

Done.

https://codereview.chromium.org/300063006/diff/40001/apps/launcher.cc#newcode180
apps/launcher.cc:180: void StepToFillMimeTypes() {
On 2014/05/28 22:46:14, benwells wrote:
> This logic is more complicated than it needs to be on non-Chromeos. There we
can
> just fill in all the mime types in one go on the FILE thread, there is no need
> to keep bouncing back to the UI thread.

Done.

https://codereview.chromium.org/300063006/diff/40001/apps/launcher.cc#newcode327
apps/launcher.cc:327: void GrantAccessToFileAndLaunch(ExtensionHost* host) {
On 2014/05/28 22:46:14, benwells wrote:
> Nit should be GrantAccessToFilesAndLaunch

Done.

https://codereview.chromium.org/300063006/diff/40001/apps/launcher.h
File apps/launcher.h (right):

https://codereview.chromium.org/300063006/diff/40001/apps/launcher.h#newcode45
apps/launcher.h:45: // |file_path| available through the launch data.
|handler_id| corresponds to
On 2014/05/28 22:46:14, benwells wrote:
> Nit: comment needs to be updated.

Done.

[benwells, 2014-05-30 00:59:34]

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc#newcode179
apps/launcher.cc:179: StepToFillMimeTypes();
Sorry to be so nitpicky. I'm finding this logic pretty confusing. I don't think
it will deal with mixed non-native and native files on chromeos but maybe i'm
wrong. I don't think it has to deal with that case but that should be more
explicit.

Can you do this:
- remove the functionStepToFillMimeTypes
- instead, in OnFileValid, if onchromeos check if all files are nonnative or
native. dcheck if mixed.
- if they are all nonnative call a function GetNextNonNativeMimeType (like
StepToFillMimeTypes but just for the next non native chromeos path)
- otherwise, in OnFileValid pass off to GetMimeTypesAndLaunch
- GetNextNonNativeMimeType and OnGotMimeType is chromeos only

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc#newcode218
apps/launcher.cc:218: void GetMimeTypeAndLaunch() {
Nit: GetMimeTypesAndLaunch

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc#newcode222
apps/launcher.cc:222: if (!this->mime_types_[i].empty())
I think this would be a programming error. Maybe DCHECK.

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc#newcode278
apps/launcher.cc:278: void LaunchWithMimeType() {
Nit: LaunchWithMimeTypes

[hirono, 2014-05-30 02:53:43]

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc#newcode179
apps/launcher.cc:179: StepToFillMimeTypes();
On 2014/05/30 00:59:35, benwells wrote:
> Sorry to be so nitpicky. I'm finding this logic pretty confusing. I don't
think
> it will deal with mixed non-native and native files on chromeos but maybe i'm
> wrong. I don't think it has to deal with that case but that should be more
> explicit.
> 
> Can you do this:
> - remove the functionStepToFillMimeTypes
> - instead, in OnFileValid, if onchromeos check if all files are nonnative or
> native. dcheck if mixed.
> - if they are all nonnative call a function GetNextNonNativeMimeType (like
> StepToFillMimeTypes but just for the next non native chromeos path)
> - otherwise, in OnFileValid pass off to GetMimeTypesAndLaunch
> - GetNextNonNativeMimeType and OnGotMimeType is chromeos only

Currently the mixed case should not happen, but it can after Files.app support a
cross-volume search feature. In the search result, users can select non-native
and native files and launch an application.

The current logic handles the mixed case as followings:

1. Prepare empty strings by mime_types_.resize(file_paths.size());
2. Check if file_types[i] is non-native. If so, it assigns a mime type string to
file_types_[i]. Otherwise it keeps file_types[i] as empty.
3. After checking all file_paths, it checks if all the mime_types_ are filled or
not. (filled variables shows it) If there is empty strings in mime_types_, it
calls GetMimeTypeAndLaunch to fill remaining mime_types_. Otherwise it call
LaunchWithMimeType directly.

The logic is a bit confusing. But I think this logic minimizes the number of
hopping threads for the three cases: native files, non-native files, mixed
files. WDYT?

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc#newcode222
apps/launcher.cc:222: if (!this->mime_types_[i].empty())
On 2014/05/30 00:59:35, benwells wrote:
> I think this would be a programming error. Maybe DCHECK.

It can happen, if the given files are mixed (native and non-native).
The previous logic fills mime types for non native paths, but does not for
native paths.

[benwells, 2014-05-30 06:33:56]

Can we get some tests for this change?

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc#newcode179
apps/launcher.cc:179: StepToFillMimeTypes();
On 2014/05/30 02:53:44, hirono wrote:
> On 2014/05/30 00:59:35, benwells wrote:
> > Sorry to be so nitpicky. I'm finding this logic pretty confusing. I don't
> think
> > it will deal with mixed non-native and native files on chromeos but maybe
i'm
> > wrong. I don't think it has to deal with that case but that should be more
> > explicit.
> > 
> > Can you do this:
> > - remove the functionStepToFillMimeTypes
> > - instead, in OnFileValid, if onchromeos check if all files are nonnative or
> > native. dcheck if mixed.
> > - if they are all nonnative call a function GetNextNonNativeMimeType (like
> > StepToFillMimeTypes but just for the next non native chromeos path)
> > - otherwise, in OnFileValid pass off to GetMimeTypesAndLaunch
> > - GetNextNonNativeMimeType and OnGotMimeType is chromeos only
> 
> Currently the mixed case should not happen, but it can after Files.app support
a
> cross-volume search feature. In the search result, users can select non-native
> and native files and launch an application.
> 
> The current logic handles the mixed case as followings:
> 
> 1. Prepare empty strings by mime_types_.resize(file_paths.size());
> 2. Check if file_types[i] is non-native. If so, it assigns a mime type string
to
> file_types_[i]. Otherwise it keeps file_types[i] as empty.
> 3. After checking all file_paths, it checks if all the mime_types_ are filled
or
> not. (filled variables shows it) If there is empty strings in mime_types_, it
> calls GetMimeTypeAndLaunch to fill remaining mime_types_. Otherwise it call
> LaunchWithMimeType directly.
> 
> The logic is a bit confusing. But I think this logic minimizes the number of
> hopping threads for the three cases: native files, non-native files, mixed
> files. WDYT?

Ah, OK. Hmm. Maybe with some comments / changed names this will be easier to
understand ...

Non-chromeos platforms don't need to do any of that iteration, they should be
able to jump straight to GetMimeTypesAndLaunch (maybe it should be called
GetNativeMimeTypesAndLaunch).

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc#newcode186
apps/launcher.cc:186: void StepToFillMimeTypes() {
I think the name GetNextNonNativeMimeType would be clearer.

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc#newcode189
apps/launcher.cc:189: bool filled = true;
Changing filled to any_native_files would be clearer.

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc#newcode222
apps/launcher.cc:222: if (!this->mime_types_[i].empty())
On 2014/05/30 02:53:44, hirono wrote:
> On 2014/05/30 00:59:35, benwells wrote:
> > I think this would be a programming error. Maybe DCHECK.
> 
> It can happen, if the given files are mixed (native and non-native).
> The previous logic fills mime types for non native paths, but does not for
> native paths.

OK, great, thanks for explaining.

[hirono, 2014-06-02 03:33:25]

Thank you!

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc#newcode179
apps/launcher.cc:179: StepToFillMimeTypes();
On 2014/05/30 06:33:57, benwells wrote:
> On 2014/05/30 02:53:44, hirono wrote:
> > On 2014/05/30 00:59:35, benwells wrote:
> > > Sorry to be so nitpicky. I'm finding this logic pretty confusing. I don't
> > think
> > > it will deal with mixed non-native and native files on chromeos but maybe
> i'm
> > > wrong. I don't think it has to deal with that case but that should be more
> > > explicit.
> > > 
> > > Can you do this:
> > > - remove the functionStepToFillMimeTypes
> > > - instead, in OnFileValid, if onchromeos check if all files are nonnative
or
> > > native. dcheck if mixed.
> > > - if they are all nonnative call a function GetNextNonNativeMimeType (like
> > > StepToFillMimeTypes but just for the next non native chromeos path)
> > > - otherwise, in OnFileValid pass off to GetMimeTypesAndLaunch
> > > - GetNextNonNativeMimeType and OnGotMimeType is chromeos only
> > 
> > Currently the mixed case should not happen, but it can after Files.app
support
> a
> > cross-volume search feature. In the search result, users can select
non-native
> > and native files and launch an application.
> > 
> > The current logic handles the mixed case as followings:
> > 
> > 1. Prepare empty strings by mime_types_.resize(file_paths.size());
> > 2. Check if file_types[i] is non-native. If so, it assigns a mime type
string
> to
> > file_types_[i]. Otherwise it keeps file_types[i] as empty.
> > 3. After checking all file_paths, it checks if all the mime_types_ are
filled
> or
> > not. (filled variables shows it) If there is empty strings in mime_types_,
it
> > calls GetMimeTypeAndLaunch to fill remaining mime_types_. Otherwise it call
> > LaunchWithMimeType directly.
> > 
> > The logic is a bit confusing. But I think this logic minimizes the number of
> > hopping threads for the three cases: native files, non-native files, mixed
> > files. WDYT?
> 
> Ah, OK. Hmm. Maybe with some comments / changed names this will be easier to
> understand ...
> 
> Non-chromeos platforms don't need to do any of that iteration, they should be
> able to jump straight to GetMimeTypesAndLaunch (maybe it should be called
> GetNativeMimeTypesAndLaunch).

Done.

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc#newcode186
apps/launcher.cc:186: void StepToFillMimeTypes() {
On 2014/05/30 06:33:57, benwells wrote:
> I think the name GetNextNonNativeMimeType would be clearer.

Done.

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc#newcode189
apps/launcher.cc:189: bool filled = true;
On 2014/05/30 06:33:57, benwells wrote:
> Changing filled to any_native_files would be clearer.

Done.

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc#newcode218
apps/launcher.cc:218: void GetMimeTypeAndLaunch() {
On 2014/05/30 00:59:35, benwells wrote:
> Nit: GetMimeTypesAndLaunch

Done.

https://codereview.chromium.org/300063006/diff/80001/apps/launcher.cc#newcode278
apps/launcher.cc:278: void LaunchWithMimeType() {
On 2014/05/30 00:59:35, benwells wrote:
> Nit: LaunchWithMimeTypes

Done.

[benwells, 2014-06-02 23:11:29]

I think this needs tests.

https://codereview.chromium.org/300063006/diff/120001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/300063006/diff/120001/apps/launcher.cc#newcod...
apps/launcher.cc:211: if (!any_native_files) {
Shouldn't this be if (any_native_files)?

[hirono, 2014-06-03 06:42:14]

Sorry, I missed about the test. Could you take another look? Thank you.

https://codereview.chromium.org/300063006/diff/120001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/300063006/diff/120001/apps/launcher.cc#newcod...
apps/launcher.cc:211: if (!any_native_files) {
On 2014/06/02 23:11:29, benwells wrote:
> Shouldn't this be if (any_native_files)?

Done.

[benwells, 2014-06-03 07:40:05]

On 2014/06/03 06:42:14, hirono wrote:
> Sorry, I missed about the test. Could you take another look? Thank you.

Awesome, and thank you for your patience.

I think you should get someone familiar with those tests to review them.

> 
> https://codereview.chromium.org/300063006/diff/120001/apps/launcher.cc
> File apps/launcher.cc (right):
> 
>
https://codereview.chromium.org/300063006/diff/120001/apps/launcher.cc#newcod...
> apps/launcher.cc:211: if (!any_native_files) {
> On 2014/06/02 23:11:29, benwells wrote:
> > Shouldn't this be if (any_native_files)?
> 
> Done.

Is the mixed native / non native case covered in your tests?

[hirono, 2014-06-03 09:32:30]

@benwells - I added a test for mixed entries.
@kinaba - Please take another look at the tests?

Thank you!

[benwells, 2014-06-04 00:58:24]

Thanks for bearing with me!

apps/ lgtm++

[kinaba, 2014-06-04 02:29:59]

lgtm for the tests, but one thing

https://codereview.chromium.org/300063006/diff/190001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/300063006/diff/190001/apps/launcher.cc#newcod...
apps/launcher.cc:182: GetMimeTypesAndLaunch();
PostTask(FILE) is needed, I guess.

[benwells, 2014-06-04 02:55:38]

https://codereview.chromium.org/300063006/diff/190001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/300063006/diff/190001/apps/launcher.cc#newcod...
apps/launcher.cc:182: GetMimeTypesAndLaunch();
On 2014/06/04 02:29:59, kinaba wrote:
> PostTask(FILE) is needed, I guess.

Yes, good catch. Looking back at the last patch to have try jobs there were
actually a lot of failures in this stuff, this is probably the problem.

[hirono, 2014-06-04 04:02:23]

https://codereview.chromium.org/300063006/diff/190001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/300063006/diff/190001/apps/launcher.cc#newcod...
apps/launcher.cc:182: GetMimeTypesAndLaunch();
On 2014/06/04 02:55:38, benwells wrote:
> On 2014/06/04 02:29:59, kinaba wrote:
> > PostTask(FILE) is needed, I guess.
> 
> Yes, good catch. Looking back at the last patch to have try jobs there were
> actually a lot of failures in this stuff, this is probably the problem.

Fixed. Thank you!

[kinaba, 2014-06-04 04:04:50]

lgtm

[hirono, 2014-06-04 04:06:03]

The CQ bit was checked by hirono@chromium.org

[commit-bot: I haz the power, 2014-06-04 04:06:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hirono@chromium.org/300063006/200001

[hirono, 2014-06-04 09:25:53]

The CQ bit was unchecked by hirono@chromium.org

[hirono, 2014-06-04 09:25:54]

The CQ bit was checked by hirono@chromium.org

[commit-bot: I haz the power, 2014-06-04 09:26:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hirono@chromium.org/300063006/200001

[hirono, 2014-06-04 09:41:13]

Committed patchset #10 manually as r274755 (presubmit successful).