AppShell: pass command-line files to app as launch data

AppShell: pass command-line files to app as launch data

An app that registers file handlers can be passed filenames on the
command line to use those files.

BUG=679870
TBR=benwells@chromium.org # DEPS

Review-Url: https://codereview.chromium.org/2824553002
Cr-Commit-Position: refs/heads/master@{#469827}
Committed: https://chromium.googlesource.com/chromium/src/+/b7b29db1011a7b8e0e8816449e2209305402a46a

[michaelpg, 2017-04-21 20:53:13]

michaelpg@chromium.org changed reviewers:
+ rkc@chromium.org

[michaelpg, 2017-04-21 20:53:14]

rkc: PTAL.

[rkc, 2017-05-02 23:49:34]

lgtm

https://codereview.chromium.org/2824553002/diff/20001/extensions/shell/browse...
File extensions/shell/browser/default_shell_browser_main_delegate.cc (right):

https://codereview.chromium.org/2824553002/diff/20001/extensions/shell/browse...
extensions/shell/browser/default_shell_browser_main_delegate.cc:61:
apps::LaunchPlatformAppWithCommandLineAndLaunchId(
Out of curiousity, what's the difference between
ShellExtensionSystem::LaunchApp() and
apps::LaunchPlatformAppWithCommandLineAndLaunchId() other than of course being
able to provide the extra parameters?

[michaelpg, 2017-05-05 20:55:37]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2017-05-05 20:55:37]

The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org
Link to the patchset: https://codereview.chromium.org/2824553002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-05 20:56:36]

CQ is trying da patch.

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

[michaelpg, 2017-05-05 21:01:40]

https://codereview.chromium.org/2824553002/diff/20001/extensions/shell/browse...
File extensions/shell/browser/default_shell_browser_main_delegate.cc (right):

https://codereview.chromium.org/2824553002/diff/20001/extensions/shell/browse...
extensions/shell/browser/default_shell_browser_main_delegate.cc:61:
apps::LaunchPlatformAppWithCommandLineAndLaunchId(
On 2017/05/02 23:49:34, rkc wrote:
> Out of curiousity, what's the difference between
> ShellExtensionSystem::LaunchApp() and
> apps::LaunchPlatformAppWithCommandLineAndLaunchId() other than of course being
> able to provide the extra parameters?

^ being able to provide the extra parameters. The 500 lines of launcher.cc are
where the app-specific launching code is, so it makes sense to reuse that like
Chrome does rather than try to implement our own.

Looks like the shell's LaunchApp is only used in tests now, btw.

[rkc, 2017-05-05 21:05:20]

https://codereview.chromium.org/2824553002/diff/20001/extensions/shell/browse...
File extensions/shell/browser/default_shell_browser_main_delegate.cc (right):

https://codereview.chromium.org/2824553002/diff/20001/extensions/shell/browse...
extensions/shell/browser/default_shell_browser_main_delegate.cc:61:
apps::LaunchPlatformAppWithCommandLineAndLaunchId(
On 2017/05/05 21:01:40, michaelpg wrote:
> On 2017/05/02 23:49:34, rkc wrote:
> > Out of curiousity, what's the difference between
> > ShellExtensionSystem::LaunchApp() and
> > apps::LaunchPlatformAppWithCommandLineAndLaunchId() other than of course
being
> > able to provide the extra parameters?
> 
> ^ being able to provide the extra parameters. The 500 lines of launcher.cc are
> where the app-specific launching code is, so it makes sense to reuse that like
> Chrome does rather than try to implement our own.
> 
> Looks like the shell's LaunchApp is only used in tests now, btw.

Ah - can we just delete it then?

[commit-bot: I haz the power, 2017-05-05 21:09:51]

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

[commit-bot: I haz the power, 2017-05-05 21:09:52]

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

[michaelpg, 2017-05-05 21:12:43]

Description was changed from

==========
AppShell: pass command-line files to app as launch data

An app that registers file handlers can be passed filenames on the
command line to use those files.

BUG=679870
==========

to

==========
AppShell: pass command-line files to app as launch data

An app that registers file handlers can be passed filenames on the
command line to use those files.

BUG=679870
TBR=benwells@chromium.org # DEPS
==========

[michaelpg, 2017-05-05 21:12:54]

michaelpg@chromium.org changed reviewers:
+ benwells@chromium.org

[michaelpg, 2017-05-05 21:12:58]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2017-05-05 21:14:07]

CQ is trying da patch.

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

[michaelpg, 2017-05-05 21:14:45]

TBRing benwells for the +apps moving from
  extensions/shell/DEPS
to
  extensions/shell/browser/DEPS

[commit-bot: I haz the power, 2017-05-06 00:26:46]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494018778720160,
"parent_rev": "6dfb54fcb774508b6679b8d83f0d935232b3a450", "commit_rev":
"b7b29db1011a7b8e0e8816449e2209305402a46a"}

[commit-bot: I haz the power, 2017-05-06 00:27:33]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494018778720160,
"parent_rev": "6dfb54fcb774508b6679b8d83f0d935232b3a450", "commit_rev":
"b7b29db1011a7b8e0e8816449e2209305402a46a"}

[commit-bot: I haz the power, 2017-05-06 00:27:45]

Description was changed from

==========
AppShell: pass command-line files to app as launch data

An app that registers file handlers can be passed filenames on the
command line to use those files.

BUG=679870
TBR=benwells@chromium.org # DEPS
==========

to

==========
AppShell: pass command-line files to app as launch data

An app that registers file handlers can be passed filenames on the
command line to use those files.

BUG=679870
TBR=benwells@chromium.org # DEPS

Review-Url: https://codereview.chromium.org/2824553002
Cr-Commit-Position: refs/heads/master@{#469827}
Committed:
https://chromium.googlesource.com/chromium/src/+/b7b29db1011a7b8e0e8816449e22...
==========

[commit-bot: I haz the power, 2017-05-06 00:27:46]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b7b29db1011a7b8e0e8816449e22...

[benwells, 2017-05-06 04:05:07]

This wasn't moving a dependency, it was adding a dependency. Now there is a
circular dependency between extensions and apps.

Can you do this without adding a dependency from extensions to apps?

[michaelpg, 2017-05-06 23:42:22]

On 2017/05/06 04:05:07, benwells wrote:
> This wasn't moving a dependency, it was adding a dependency.

You're right, my mistake. I opened this a while ago, yesterday I mistook the -
for a + when TBRing you.

I'll revert it, no reason not to.

> Now there is a
> circular dependency between extensions and apps.
> 
> Can you do this without adding a dependency from extensions to apps?

//apps isn't meant to depend on //extensions/shell specifically. Perhaps we
could express that with more DEPS rules, but I don't see why app_shell should
necessarily live in //extensions anyway. (Maybe //apps/shell....)

We could add some sort of AppLauncher delegate that //apps implements, but that
seems kinda silly to me.

[michaelpg, 2017-05-06 23:43:03]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2864603007/ by michaelpg@chromium.org.

The reason for reverting is: TBR was insufficient for commit.

[benwells, 2017-05-08 07:58:48]

On 2017/05/06 23:42:22, michaelpg wrote:
> On 2017/05/06 04:05:07, benwells wrote:
> > This wasn't moving a dependency, it was adding a dependency.
> 
> You're right, my mistake. I opened this a while ago, yesterday I mistook the -
> for a + when TBRing you.
> 
> I'll revert it, no reason not to.
> 
> > Now there is a
> > circular dependency between extensions and apps.
> > 
> > Can you do this without adding a dependency from extensions to apps?
> 
> //apps isn't meant to depend on //extensions/shell specifically. Perhaps we
> could express that with more DEPS rules, but I don't see why app_shell should
> necessarily live in //extensions anyway. (Maybe //apps/shell....)
> 
> We could add some sort of AppLauncher delegate that //apps implements, but
that
> seems kinda silly to me.

I agree. Put another way, it totally makes sense that the app shell depends on
apps but it's weird that app shell lives in extensions.

I'd be happy with this relanding, if you add an extra "-extensions/shell" in the
apps deps either as part of this CL or a follow up. Longer term maybe app shell
should move out of extensions and into apps.