Aura on Android: Stub implementation of SelectFileDialog.

Aura on Android: Stub implementation of SelectFileDialog.

BUG=507792
Committed: https://crrev.com/cddfeaa40018e47e8d0e1b2851cf117aee06ecca
Cr-Commit-Position: refs/heads/master@{#356857}

[mfomitchev, 2015-10-15 19:52:43]

mfomitchev@chromium.org changed reviewers:
+ sievers@chromium.org, sky@chromium.org

[mfomitchev, 2015-10-15 19:52:43]

I will log a bug to wire this up properly. It's non-trivial because the Android
implementation relies on gfx::NativeWindow to be defined as WindowAndroid.

[sky, 2015-10-15 21:51:47]

The lack of _ between aura and android bothers me, but we seem to do that more
often than not. *SIGH*

https://codereview.chromium.org/1409793002/diff/1/ui/shell_dialogs/BUILD.gn
File ui/shell_dialogs/BUILD.gn (right):

https://codereview.chromium.org/1409793002/diff/1/ui/shell_dialogs/BUILD.gn#n...
ui/shell_dialogs/BUILD.gn:25: "select_file_dialog_auraandroid.cc",
Can you conditionally include below rather than listing for all?

[no sievers, 2015-10-15 22:05:19]

On 2015/10/15 19:52:43, mfomitchev wrote:
> I will log a bug to wire this up properly. It's non-trivial because the
Android
> implementation relies on gfx::NativeWindow to be defined as WindowAndroid.

Sounds good. Looks like we need to get to the activity for sending the intent.

[mfomitchev, 2015-10-15 22:12:40]

https://codereview.chromium.org/1409793002/diff/1/ui/shell_dialogs/BUILD.gn
File ui/shell_dialogs/BUILD.gn (right):

https://codereview.chromium.org/1409793002/diff/1/ui/shell_dialogs/BUILD.gn#n...
ui/shell_dialogs/BUILD.gn:25: "select_file_dialog_auraandroid.cc",
On 2015/10/15 21:51:47, sky wrote:
> Can you conditionally include below rather than listing for all?

Are there some guidelines which explain when it's better to exclude vs. include
files conditionally? The only thing I was able to find was the GYP
documentation, which says excluding is generally the preferred style
(https://gyp.gsrc.io/docs/UserDocumentation.md#Add-new-source-files).
It doesn't matter much to me either way, but it would be nice to have something
concrete we can use in the future in similar cases.

Actually, perhaps it would be worth adding a rule for _auraandroid in
BUILDCONFIG.gn
(https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD...
We do have several other cases where we add _auraandroid files (which we haven't
upstreamed yet). Incidentally, that would also rationalize the fact that we use
_auraandroid vs. _aura_android suffix.

[sky, 2015-10-15 23:07:34]

sky@chromium.org changed reviewers:
+ brettw@chromium.org

[sky, 2015-10-15 23:09:06]

+brettw for his comments on but of these issues. Specifically:

1. Should we favor including conditional sources vs excluding? GYP recommended
the later.

2. Should we be adding more pattern matching (_auraandroid)?

[brettw, 2015-10-20 02:49:27]

https://codereview.chromium.org/1409793002/diff/1/ui/shell_dialogs/BUILD.gn
File ui/shell_dialogs/BUILD.gn (right):

https://codereview.chromium.org/1409793002/diff/1/ui/shell_dialogs/BUILD.gn#n...
ui/shell_dialogs/BUILD.gn:25: "select_file_dialog_auraandroid.cc",
You should conditionally include now unless for some reason the reverse is
better.

GYP recommended this because it allowed all files to be included in generated VC
solution files, even if they applied to other platforms. But this facility isn't
really used any more so we stopped recommending this.

This is documented in the GN style guide:
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...

The GN rule for filename exclusion rules is for platforms (like _android) only
and not for features and things like Aura. This is because the GYP rules were
spiraling out of control and there are relatively few of these cases anyway.

[mfomitchev, 2015-10-20 17:55:44]

https://codereview.chromium.org/1409793002/diff/1/ui/shell_dialogs/BUILD.gn
File ui/shell_dialogs/BUILD.gn (right):

https://codereview.chromium.org/1409793002/diff/1/ui/shell_dialogs/BUILD.gn#n...
ui/shell_dialogs/BUILD.gn:25: "select_file_dialog_auraandroid.cc",
On 2015/10/20 02:49:26, brettw wrote:
> You should conditionally include now unless for some reason the reverse is
> better.
> 
> GYP recommended this because it allowed all files to be included in generated
VC
> solution files, even if they applied to other platforms. But this facility
isn't
> really used any more so we stopped recommending this.
> 
> This is documented in the GN style guide:
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...
> 
> The GN rule for filename exclusion rules is for platforms (like _android) only
> and not for features and things like Aura. This is because the GYP rules were
> spiraling out of control and there are relatively few of these cases anyway.

Thanks Brett! Done.

[brettw, 2015-10-21 02:51:03]

lgtm

https://codereview.chromium.org/1409793002/diff/20001/ui/shell_dialogs/BUILD.gn
File ui/shell_dialogs/BUILD.gn (right):

https://codereview.chromium.org/1409793002/diff/20001/ui/shell_dialogs/BUILD....
ui/shell_dialogs/BUILD.gn:55: if (is_android && !use_aura) {
What I would do for this:

if (is_android) {
  if (use_aura) {
    sources += [ <stuff you have below> ]
  } else {
    sources += [
      <the android files you remove below, so there are no sources -= calls>
    ]
    <rest of the stuff here>
  }
}

Obviously remove the duplicated files from the sources list above.

https://codereview.chromium.org/1409793002/diff/20001/ui/shell_dialogs/select...
File ui/shell_dialogs/select_file_dialog_auraandroid.cc (right):

https://codereview.chromium.org/1409793002/diff/20001/ui/shell_dialogs/select...
ui/shell_dialogs/select_file_dialog_auraandroid.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
This is a new file, so use the current year :)

Also, remove the "(c)" which we don't use any more.

Ditto for the other file.

[mfomitchev, 2015-10-21 22:24:22]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2015-10-21 22:24:43]

The CQ bit was unchecked by mfomitchev@chromium.org

[mfomitchev, 2015-10-21 22:44:04]

https://codereview.chromium.org/1409793002/diff/20001/ui/shell_dialogs/BUILD.gn
File ui/shell_dialogs/BUILD.gn (right):

https://codereview.chromium.org/1409793002/diff/20001/ui/shell_dialogs/BUILD....
ui/shell_dialogs/BUILD.gn:55: if (is_android && !use_aura) {
Done - I think. There's nothing in <rest of the stuff here>, so I've just left
two separate ifs. I think it's also nicer to avoid an extra nesting level.

https://codereview.chromium.org/1409793002/diff/20001/ui/shell_dialogs/select...
File ui/shell_dialogs/select_file_dialog_auraandroid.cc (right):

https://codereview.chromium.org/1409793002/diff/20001/ui/shell_dialogs/select...
ui/shell_dialogs/select_file_dialog_auraandroid.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2015/10/21 02:51:03, brettw wrote:
> This is a new file, so use the current year :)
> 
> Also, remove the "(c)" which we don't use any more.
> 
> Ditto for the other file.

Done and done

[mfomitchev, 2015-10-22 15:01:45]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2015-10-22 15:01:45]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1409793002/#ps60001
(title: "Cleaning up copyright comments.")

[commit-bot: I haz the power, 2015-10-22 15:02:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409793002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409793002/60001

[commit-bot: I haz the power, 2015-10-22 15:11:18]

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

[commit-bot: I haz the power, 2015-10-22 15:11:18]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[no sievers, 2015-10-26 23:40:25]

On 2015/10/22 15:11:18, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

** Presubmit ERRORS **
The following files appear to be using DISALLOW_* macros.
Please #include "base/macros.h" in them.
  ui/shell_dialogs/select_file_dialog_auraandroid.h

[mfomitchev, 2015-10-29 15:28:33]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2015-10-29 15:28:33]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1409793002/#ps80001
(title: "Addding a missing import.")

[commit-bot: I haz the power, 2015-10-29 15:28:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409793002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409793002/80001

[commit-bot: I haz the power, 2015-10-29 15:59:56]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-29 16:00:52]

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