Start app shim when app launched.

Start app shim when app launched.

ExtensionAppShimHandler launches the relevant shim in response to
NOTIFICATION_EXTENSION_HOST_CREATED. A new field is added to the
LaunchApp IPC message: launch_now. This indicates whether to launch the
app immediately. This prevents the shim launching the app again.

BUG=168080
TEST=Find an app that has an app shim bundle.
Launch it from the launcher or new tab page.
The shim should start.

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

[tapted, 2013-05-16 04:51:00]

some initial thoughts

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/app_shim...
File apps/app_shim/app_shim_util.h (right):

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/app_shim...
apps/app_shim/app_shim_util.h:5: #ifndef APPS_APP_SHIM_APP_SHIM_UTIL_H_
this should have a _mac suffix too

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/app_shim...
apps/app_shim/app_shim_util.h:13: namespace app_shim {
should just be namespace apps. There's some rule; a correspondence between
top-level folders and top-level namespaces

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/app_shim...
File apps/app_shim/app_shim_util_mac.mm (right):

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/app_shim...
apps/app_shim/app_shim_util_mac.mm:21: const char kAppDirName[] = "Chrome
Apps.localized";
maybe now is a good time to discuss with benwells whether this should be split
into 

Chromium Apps
Chrome Canary Apps
Chrome Apps

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/app_shim...
apps/app_shim/app_shim_util_mac.mm:34: base::FilePath GetPathToShim(const
std::string& app_id,
Maybe we should consider calling this extension_id. On balance, I think it's
more consistent with the non-mac code, but I realise the app_shim_host uses
app_id (but there it's doing double duty as a "appmode" id in the OS sense).
So... when it's not ambiguous I'd say go with extension_id ;)

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/app_shim...
apps/app_shim/app_shim_util_mac.mm:40: file_util::FileEnumerator
enumerator(app_dir,
perhaps a note to say why it's iterating (i.e. because extension may have been
updated with a new name, so a search must be done to look for a matching
extension id).

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/app_shim...
apps/app_shim/app_shim_util_mac.mm:56: }
nit: doesn't need curlies

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/app_shim...
apps/app_shim/app_shim_util_mac.mm:62: const std::string& app_id,
nit: align these with the open paren (like for FindAndLaunchShimForApp in the
header)

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/app_shim...
apps/app_shim/app_shim_util_mac.mm:65: base::FilePath shim_path =
GetPathToShim(app_id, profile_name);
nit: before this, 
  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/app_shim...
apps/app_shim/app_shim_util_mac.mm:70: }
nit: blank line after for early return

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/app_shim...
apps/app_shim/app_shim_util_mac.mm:73:
base::mac::OpenApplicationWithPath(shim_path, command_line, NULL);
Do you see a reason why shim_path can't be derived from 

base::FilePath CommandLine::GetProgram() instead?

clearly OpenApplicationWithPath would need an update too. Maybe that can be
addressed along with the TODO "this opens a new broser window.."

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/app_shim...
apps/app_shim/app_shim_util_mac.mm:83: base::FilePath path =
GetWritableApplicationsDirectory();
GetWritableApplicationsDirectory might need to be called on the FILE thread

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/app_shim...
apps/app_shim/app_shim_util_mac.mm:88: const std::string& app_id,
nit: align these like the header

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/switches.h
File apps/app_shim/switches.h (right):

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/switches...
apps/app_shim/switches.h:5: #ifndef APPS_SWITCHES_H_
missing path component. Probably want a _mac.h suffix too. But...

https://chromiumcodereview.appspot.com/14579006/diff/1/apps/app_shim/switches...
apps/app_shim/switches.h:13: extern const char kNoLaunchApp[];
... I think there should just be one, top-level apps/apps_switches.h for things
under apps. But.. I think this belongs in app_mode_common.h instead. Let's ask
benwells

https://chromiumcodereview.appspot.com/14579006/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/platform_app_launcher.cc (right):

https://chromiumcodereview.appspot.com/14579006/diff/1/chrome/browser/extensi...
chrome/browser/extensions/platform_app_launcher.cc:102:
web_app::internals::LaunchShim(profile, extension, base::Bind(&DoNothing));
Would a base::Closure() with no arguments have the effect of DoNothing?

But we shouldn't be diving into web_app::internals here anyway.

And.. it might be preferable to not Dispatch the OnLaunched event until the shim
itself has launched. i.e. leave it up to the shim to do the launch -- so long as
we can guarantee a clear success/fail result, and fallback to dispatching the
event from the browser process directly.

But of course that might mean some delay. Let's discuss with benwells.

https://chromiumcodereview.appspot.com/14579006/diff/1/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_mac.h (right):

https://chromiumcodereview.appspot.com/14579006/diff/1/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.h:38: base::FilePath
GetShortcutPath() const;
you should get this in a rebase now ;)

https://chromiumcodereview.appspot.com/14579006/diff/1/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/14579006/diff/1/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.mm:110: const char kNoLaunchApp[] =
"no-launch-app";
we can't have this in 2 places

https://chromiumcodereview.appspot.com/14579006/diff/1/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.mm:112: const char
kChromeAppDirName[] = "Chrome Apps.localized";
this either

https://chromiumcodereview.appspot.com/14579006/diff/1/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.mm:360: base::FilePath shim_path =
shortcut_creator.GetShortcutPath();
can this logic be merged with the logic in FindAndLaunchShimOnFileThread?

[jackhou1, 2013-05-17 05:45:36]

I've removed all the duplicate functionality that was in app_shim_util so now it
just uses web_app_mac.

https://codereview.chromium.org/14579006/diff/1/apps/app_shim/switches.h
File apps/app_shim/switches.h (right):

https://codereview.chromium.org/14579006/diff/1/apps/app_shim/switches.h#newc...
apps/app_shim/switches.h:13: extern const char kNoLaunchApp[];
On 2013/05/16 04:51:00, tapted wrote:
> ... I think there should just be one, top-level apps/apps_switches.h for
things
> under apps. But.. I think this belongs in app_mode_common.h instead. Let's ask
> benwells

Moved to app_mode_common.h

https://codereview.chromium.org/14579006/diff/1/chrome/browser/extensions/pla...
File chrome/browser/extensions/platform_app_launcher.cc (right):

https://codereview.chromium.org/14579006/diff/1/chrome/browser/extensions/pla...
chrome/browser/extensions/platform_app_launcher.cc:102:
web_app::internals::LaunchShim(profile, extension, base::Bind(&DoNothing));
On 2013/05/16 04:51:00, tapted wrote:
> Would a base::Closure() with no arguments have the effect of DoNothing?

Nup, it needs to take a bool.

https://codereview.chromium.org/14579006/diff/1/chrome/browser/web_applicatio...
File chrome/browser/web_applications/web_app_mac.h (right):

https://codereview.chromium.org/14579006/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/web_app_mac.h:38: base::FilePath
GetShortcutPath() const;
On 2013/05/16 04:51:00, tapted wrote:
> you should get this in a rebase now ;)

I got it from 14760002, so might need that to land.

[jackhou1, 2013-05-17 05:49:33]

Questions for benwells.

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.cc (right):

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:102:
web_app::LaunchShim(profile, extension, base::Bind(&DoNothing));
Should we just let the shim do the launch?

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/web_applica...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_mac.mm:110: const char
kChromeAppDirName[] = "Chrome Apps.localized";
Should this be split into:
Chrome Apps
Chromium Apps
Chrome Canary Apps

https://codereview.chromium.org/14579006/diff/8001/chrome/common/mac/app_mode...
File chrome/common/mac/app_mode_common.h (right):

https://codereview.chromium.org/14579006/diff/8001/chrome/common/mac/app_mode...
chrome/common/mac/app_mode_common.h:33: extern const char kNoLaunchApp[];
Is this the right place for this switch?

[benwells, 2013-05-17 05:59:41]

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.cc (right):

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:102:
web_app::LaunchShim(profile, extension, base::Bind(&DoNothing));
On 2013/05/17 05:49:33, jackhou1 wrote:
> Should we just let the shim do the launch?

One downside is that if there is any problem with the shim, the app won't
launch.

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/web_applica...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_mac.mm:110: const char
kChromeAppDirName[] = "Chrome Apps.localized";
On 2013/05/17 05:49:33, jackhou1 wrote:
> Should this be split into:
> Chrome Apps
> Chromium Apps
> Chrome Canary Apps

Yes, and it should be coming from the sources files. I think you can do that in
a follow up patch.

https://codereview.chromium.org/14579006/diff/8001/chrome/common/mac/app_mode...
File chrome/common/mac/app_mode_common.h (right):

https://codereview.chromium.org/14579006/diff/8001/chrome/common/mac/app_mode...
chrome/common/mac/app_mode_common.h:33: extern const char kNoLaunchApp[];
On 2013/05/17 05:49:33, jackhou1 wrote:
> Is this the right place for this switch?

I'm not sure. Does the shim have other switches, and if so where are they?

[tapted, 2013-05-17 06:44:00]

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.cc (right):

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:102:
web_app::LaunchShim(profile, extension, base::Bind(&DoNothing));
On 2013/05/17 05:59:41, benwells wrote:
> On 2013/05/17 05:49:33, jackhou1 wrote:
> > Should we just let the shim do the launch?
> 
> One downside is that if there is any problem with the shim, the app won't
> launch.

So non committal ;). But won't we need to handle that case any way? That is, if
providing icons in the dock becomes default, any situation that could cause the
icon not to appear would be a bug.

My current feeling is that, on balance, having the browser initiate the launch
might be trickier. You need to handle the race condition, where the shim
connects after the app has started and immediately closed. It might also be
weird having the app window appear before the dock icon in some cases.

Actually, I've realised another tricky aspect. It's perfectly fine launching an
app multiple times from the NTP. In most cases, you just get multiple windows
(unless the app creates windows with an `id`).

So, one question is: what should a shim do? I *think* OSX will ignore the second
launch attempt, but send the shim process an OS event instead, like OnRestart.
This could be forwarded to the already-running shim host, which I don't think
currently happens.

Back to this, I think the path via LaunchPlatformAppWithNoData also needs a
check for is-shim-already-running. If the browser is sending OnLaunched, I don't
think it should try to start another shim in this case.

In either case, the shim host needs to be aware that there could be multiple
windows open for a given connection.

So, that's a fairly non-committal answer as well. Maybe we start with whatever
works (ie: this), and get feedback when people try it out.

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/web_applica...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app.h:102: void LaunchShim(Profile* profile,
This can probably go into web_app_mac.h. But you still need a non-#ifdefed-out
declaration in here otherwise platform_app_launcher.cc will only compile on Mac.

I think a wrapper is best, without the callback. Like:

MaybeLaunchShortcut(Profile*, const Extension*)

which is just a no-op on non-mac.

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/web_applica...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_mac.mm:311: base::Callback<void(bool)>
completed) {
Is there only one entrypoint now? It looks like |completed| is only ever
`DoNothing`, in which case it should be dropped.

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_mac.mm:327: void LaunchShim(Profile*
profile,
I think this should first check with the shim manager to ensure there isn't
already a shim running.

[jackhou1, 2013-05-21 03:13:22]

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/web_applica...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app.h:102: void LaunchShim(Profile* profile,
I just if-defined this into a no-op for non-mac. If it's in web_app_mac.h, you
need to include that file to call it. Not sure of a way around that.

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/web_applica...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_mac.mm:311: base::Callback<void(bool)>
completed) {
On 2013/05/17 06:44:00, tapted wrote:
> Is there only one entrypoint now? It looks like |completed| is only ever
> `DoNothing`, in which case it should be dropped.

Done.

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_mac.mm:327: void LaunchShim(Profile*
profile,
ATM the shim manager doesn't keep track of shims after starting them. After the
refactor (15269003) we can probably do this in the extension app shim handler.
Alternatively, we can just make AppShimHostManager track app shims.

[benwells, 2013-05-21 03:14:47]

Arg sorry I wrote this on Monday morning but didn't publish it :-/

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.cc (right):

https://codereview.chromium.org/14579006/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:102:
web_app::LaunchShim(profile, extension, base::Bind(&DoNothing));
On 2013/05/17 06:44:00, tapted wrote:
> On 2013/05/17 05:59:41, benwells wrote:
> > On 2013/05/17 05:49:33, jackhou1 wrote:
> > > Should we just let the shim do the launch?
> > 
> > One downside is that if there is any problem with the shim, the app won't
> > launch.
> 
> So non committal ;). But won't we need to handle that case any way? That is,
if
> providing icons in the dock becomes default, any situation that could cause
the
> icon not to appear would be a bug.
> 
> My current feeling is that, on balance, having the browser initiate the launch
> might be trickier. You need to handle the race condition, where the shim
> connects after the app has started and immediately closed. It might also be
> weird having the app window appear before the dock icon in some cases.
> 
> Actually, I've realised another tricky aspect. It's perfectly fine launching
an
> app multiple times from the NTP. In most cases, you just get multiple windows
> (unless the app creates windows with an `id`).
> 
> So, one question is: what should a shim do? I *think* OSX will ignore the
second
> launch attempt, but send the shim process an OS event instead, like OnRestart.
> This could be forwarded to the already-running shim host, which I don't think
> currently happens.
> 
> Back to this, I think the path via LaunchPlatformAppWithNoData also needs a
> check for is-shim-already-running. If the browser is sending OnLaunched, I
don't
> think it should try to start another shim in this case.
> 
> In either case, the shim host needs to be aware that there could be multiple
> windows open for a given connection.
> 
> So, that's a fairly non-committal answer as well. Maybe we start with whatever
> works (ie: this), and get feedback when people try it out.

Well, I trust you guys know more about the ins and outs of implementing this ...
as you've just demonstrated :)

But, your explanations have helped me get more committal:
- I'd prefer to keep launching apps independent of shims. Just from a
flexibility point of view: it is better to be able to remove or replace shims
and have everything else just work.
- If there are races avoiding them like this will probably* just make them
appear somewhere else.
- What happens if the user deletes the shim?

* generalization which may be incorrect in this case.

Also, we should make NTP / launcher / shim behavior consistent across platforms
(or decide not to). I wouldn't worry about it for now.

[tapted, 2013-05-21 08:33:25]

https://codereview.chromium.org/14579006/diff/19001/chrome/app/chrome_main_ap...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/14579006/diff/19001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:91: return;
Since we're going down this path, it still needs to send a message, otherwise
the shim host won't know what extension it's hosting in order to close it when
the extension is unloaded.

You should be able to change AppShimHostMsg_LaunchApp to have a third, bool
argument indicating to the host whether it should launch, or just register
itself with the shimhandler (which will also need that flag, or a new method on
the abstract interface). So, we should get that other change landed first, then
base this change off it to add the "just register" flag.

And, I think editing the existing message is preferable to the alternative of a
new, 'SilentLaunchApp' message, since it's extensible to more/non-orthogonal
flags later.

https://codereview.chromium.org/14579006/diff/19001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/14579006/diff/19001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app.h:11: #include "base/callback.h"
this can go

https://codereview.chromium.org/14579006/diff/19001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app.h:103: #if defined(OS_MACOSX)
I think it will be a bit cleaner to just have the first case here, but in
web_app.cc, have a no-op definition guarded by !defined(OS_MACOSX)

https://codereview.chromium.org/14579006/diff/19001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/14579006/diff/19001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:9: #include "base/callback.h"
drop?

[jackhou1, 2013-05-28 06:50:33]

PTAL

The LaunchApp IPC message now includes whether to launch the app. Also, the shim
is now started by ExtensionAppShimHandler when it observes a
NOTIFICATION_EXTENSION_HOST_CREATED.

https://codereview.chromium.org/14579006/diff/19001/chrome/app/chrome_main_ap...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/14579006/diff/19001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:91: return;
On 2013/05/21 08:33:25, tapted wrote:
> Since we're going down this path, it still needs to send a message, otherwise
> the shim host won't know what extension it's hosting in order to close it when
> the extension is unloaded.
> 
> You should be able to change AppShimHostMsg_LaunchApp to have a third, bool
> argument indicating to the host whether it should launch, or just register
> itself with the shimhandler (which will also need that flag, or a new method
on
> the abstract interface). So, we should get that other change landed first,
then
> base this change off it to add the "just register" flag.
> 
> And, I think editing the existing message is preferable to the alternative of
a
> new, 'SilentLaunchApp' message, since it's extensible to more/non-orthogonal
> flags later.

Done.

https://codereview.chromium.org/14579006/diff/19001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/14579006/diff/19001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app.h:11: #include "base/callback.h"
On 2013/05/21 08:33:25, tapted wrote:
> this can go

Done.

https://codereview.chromium.org/14579006/diff/19001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app.h:103: #if defined(OS_MACOSX)
On 2013/05/21 08:33:25, tapted wrote:
> I think it will be a bit cleaner to just have the first case here, but in
> web_app.cc, have a no-op definition guarded by !defined(OS_MACOSX)

Done.

https://codereview.chromium.org/14579006/diff/19001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/14579006/diff/19001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:9: #include "base/callback.h"
On 2013/05/21 08:33:25, tapted wrote:
> drop?

Done.

[tapted, 2013-05-29 04:20:30]

Also, make sure you update the CL description, BUG=, TEST=

https://codereview.chromium.org/14579006/diff/40001/apps/app_shim/app_shim_me...
File apps/app_shim/app_shim_messages.h (right):

https://codereview.chromium.org/14579006/diff/40001/apps/app_shim/app_shim_me...
apps/app_shim/app_shim_messages.h:18: // Signals to the main Chrome process that
a shim has started indicating the
nit: 'started indicating' -> 'started. Indicates'

https://codereview.chromium.org/14579006/diff/40001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/14579006/diff/40001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_mac.mm:276: bool launch_now) {
I've noticed the app list will need some logic added here. E.g., when the app
list is re-shown after switching profiles (or displayed after an extension
install), it will also want to start its shim. Otherwise, clicking the icon
again doesn't hide it.

Let's leave more for a follow-up though.

https://codereview.chromium.org/14579006/diff/40001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/14579006/diff/40001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app.h:102: void MaybeLaunchShortcut(Profile*
profile,
Can this be moved to web_app_mac.h now? (and remove the non-osx stub?)

https://codereview.chromium.org/14579006/diff/40001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/14579006/diff/40001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:308: void
LaunchShimOnFileThread(Profile* profile,
can you move this to an anonymous namespace?

https://codereview.chromium.org/14579006/diff/40001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:325:
base::Bind(&LaunchShimOnFileThread, profile, extension));
I've just realised.. posting Extension pointers to non-UI threads isn't safe. If
the extension is uninstalled in parallel, you can get a crash. (E.g., browser
starts in response to a shim, Chrome sync kicks in, notices you've uninstalled
the app elsewhere, removes the local copy, deletes extension pointer, then tries
to launch via deleted pointer).

(aside: the function ShortcutInfoForExtensionAndProfile should probably have a
DCHECK(..CurrentlyOn(UI))..).

ShortcutInfo is copyable, so I'd bind one of those in the closure, to keep it
generic.

Note the app list will want to pass a ShortcutInfo in to a public API to launch
its shim, since it doesn't have an Extension*. We can make that a new function,
or replace this one, and call web_app::ShortcutInfoForExtensionAndProfile(..) in
ExtensionAppShimHandler::StartShim(..). Not sure which is better.

[jackhou1, 2013-05-29 05:53:04]

https://codereview.chromium.org/14579006/diff/40001/apps/app_shim/app_shim_me...
File apps/app_shim/app_shim_messages.h (right):

https://codereview.chromium.org/14579006/diff/40001/apps/app_shim/app_shim_me...
apps/app_shim/app_shim_messages.h:18: // Signals to the main Chrome process that
a shim has started indicating the
On 2013/05/29 04:20:30, tapted wrote:
> nit: 'started indicating' -> 'started. Indicates'

Done.

https://codereview.chromium.org/14579006/diff/40001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/14579006/diff/40001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app.h:102: void MaybeLaunchShortcut(Profile*
profile,
On 2013/05/29 04:20:30, tapted wrote:
> Can this be moved to web_app_mac.h now? (and remove the non-osx stub?)

chrome/browser/web_applications/web_app_mac.h is not in apps/DEPS, should I add
it? or leave this?

https://codereview.chromium.org/14579006/diff/40001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/14579006/diff/40001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:308: void
LaunchShimOnFileThread(Profile* profile,
On 2013/05/29 04:20:30, tapted wrote:
> can you move this to an anonymous namespace?

Done.

https://codereview.chromium.org/14579006/diff/40001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:325:
base::Bind(&LaunchShimOnFileThread, profile, extension));
On 2013/05/29 04:20:30, tapted wrote:
> I've just realised.. posting Extension pointers to non-UI threads isn't safe.
If
> the extension is uninstalled in parallel, you can get a crash. (E.g., browser
> starts in response to a shim, Chrome sync kicks in, notices you've uninstalled
> the app elsewhere, removes the local copy, deletes extension pointer, then
tries
> to launch via deleted pointer).
> 
> (aside: the function ShortcutInfoForExtensionAndProfile should probably have a
> DCHECK(..CurrentlyOn(UI))..).
> 
> ShortcutInfo is copyable, so I'd bind one of those in the closure, to keep it
> generic.
> 
> Note the app list will want to pass a ShortcutInfo in to a public API to
launch
> its shim, since it doesn't have an Extension*. We can make that a new
function,
> or replace this one, and call web_app::ShortcutInfoForExtensionAndProfile(..)
in
> ExtensionAppShimHandler::StartShim(..). Not sure which is better.

Done.

Changed MaybeCreateShortcut since
chrome/browser/ui/web_applications/web_app_ui.h is already in apps/DEPS.

[tapted, 2013-05-29 06:16:01]

https://codereview.chromium.org/14579006/diff/40001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/14579006/diff/40001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app.h:102: void MaybeLaunchShortcut(Profile*
profile,
On 2013/05/29 05:53:04, jackhou1 wrote:
> On 2013/05/29 04:20:30, tapted wrote:
> > Can this be moved to web_app_mac.h now? (and remove the non-osx stub?)
> 
> chrome/browser/web_applications/web_app_mac.h is not in apps/DEPS, should I
add
> it? or leave this?

I think it makes sense to add web_app_mac.h to DEPS at this stage.

https://codereview.chromium.org/14579006/diff/53001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/14579006/diff/53001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:21: #include
"chrome/common/chrome_switches.h"
nit: don't think this is used

https://codereview.chromium.org/14579006/diff/53001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:22: #include
"chrome/browser/profiles/profile.h"
or this

https://codereview.chromium.org/14579006/diff/53001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:296: void
LaunchShimOnFileThread(ShellIntegration::ShortcutInfo shortcut_info) {
can this be a const-reference? I think the closure will hold on to a copy until
it returns.

[jackhou1, 2013-05-29 07:36:06]

Updated description.

https://codereview.chromium.org/14579006/diff/53001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/14579006/diff/53001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:21: #include
"chrome/common/chrome_switches.h"
On 2013/05/29 06:16:01, tapted wrote:
> nit: don't think this is used

Done.

https://codereview.chromium.org/14579006/diff/53001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:22: #include
"chrome/browser/profiles/profile.h"
On 2013/05/29 06:16:01, tapted wrote:
> or this

Done.

https://codereview.chromium.org/14579006/diff/53001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:296: void
LaunchShimOnFileThread(ShellIntegration::ShortcutInfo shortcut_info) {
On 2013/05/29 06:16:01, tapted wrote:
> can this be a const-reference? I think the closure will hold on to a copy
until
> it returns.

Done.

[tapted, 2013-05-29 07:48:15]

lgtm with a nit

We will need an IPC security review here too. And some OWNERS.

btw - if you want to run jobs before lkgr updates now that
http://crrev.com/202791 has landed you can do `git cl try -rHEAD`

https://codereview.chromium.org/14579006/diff/58001/chrome/common/mac/app_mod...
File chrome/common/mac/app_mode_common.h (right):

https://codereview.chromium.org/14579006/diff/58001/chrome/common/mac/app_mod...
chrome/common/mac/app_mode_common.h:32: // Instructs the app shim not to send a
LaunchApp message.
nit: update comment

[jackhou1, 2013-05-29 07:58:14]

https://codereview.chromium.org/14579006/diff/58001/chrome/common/mac/app_mod...
File chrome/common/mac/app_mode_common.h (right):

https://codereview.chromium.org/14579006/diff/58001/chrome/common/mac/app_mod...
chrome/common/mac/app_mode_common.h:32: // Instructs the app shim not to send a
LaunchApp message.
On 2013/05/29 07:48:15, tapted wrote:
> nit: update comment

Done.

[benwells, 2013-05-29 08:00:57]

apps\DEPS lgtm

[jackhou1, 2013-05-29 08:03:30]

Adding extra reviewers:

benwells, for OWNERS in apps/, chrome/browser/web_applications

thakis, for OWNERS in chrome/app/, chrome/common/mac

palmer, for security review of app shim IPC changes

Thanks.

[benwells, 2013-05-29 08:13:17]

chrome/browser/web_applications lgtm.

[palmer, 2013-05-29 21:20:43]

https://codereview.chromium.org/14579006/diff/69001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_mac.cc (right):

https://codereview.chromium.org/14579006/diff/69001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_mac.cc:76: if (!(profile_ =
FetchProfileForDirectory(profile_dir))) {
Can |profile_dir| just be a |base::FilePath|? The callee,
|FetchProfileForDirectory|, just turns it into a FilePath immediately anyway.

[jackhou1, 2013-05-30 00:06:28]

https://codereview.chromium.org/14579006/diff/69001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_mac.cc (right):

https://codereview.chromium.org/14579006/diff/69001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_mac.cc:76: if (!(profile_ =
FetchProfileForDirectory(profile_dir))) {
On 2013/05/29 21:20:43, Chromium Palmer wrote:
> Can |profile_dir| just be a |base::FilePath|? The callee,
> |FetchProfileForDirectory|, just turns it into a FilePath immediately anyway.

Done.

[Nico, 2013-05-30 00:14:44]

Looks mostly good.

https://codereview.chromium.org/14579006/diff/75001/chrome/app/chrome_main_ap...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/14579006/diff/75001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:79: LOG(ERROR) << "Cannot get user data
directory for browser bundle.";
Logging isn't useful. If this important enough that we want to learn about this,
add a CHECK or DCHECK. If it isn't, do nothing. Logspam doesn't help.

https://codereview.chromium.org/14579006/diff/75001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:92:
!CommandLine::ForCurrentProcess()->HasSwitch(app_mode::kNoLaunchApp)));
Not clear what the bool does here. Consider using a named bool or a
/*param_name=*/ comment to make that clearer.

https://codereview.chromium.org/14579006/diff/75001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:112: LOG(ERROR) << "App launch failed.";
see above

https://codereview.chromium.org/14579006/diff/75001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/14579006/diff/75001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_mac.mm:65: bool launch_now)
OVERRIDE;
enum params tend to be more readable at the call site (compared to bool params).
consider using an enum instead.

[jackhou1, 2013-05-30 01:43:30]

Added a file for the enum because it's used in both the IPC interface and the
AppShimHandler interface.

https://codereview.chromium.org/14579006/diff/75001/chrome/app/chrome_main_ap...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/14579006/diff/75001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:79: LOG(ERROR) << "Cannot get user data
directory for browser bundle.";
On 2013/05/30 00:14:44, Nico wrote:
> Logging isn't useful. If this important enough that we want to learn about
this,
> add a CHECK or DCHECK. If it isn't, do nothing. Logspam doesn't help.

It's probably not important enough. I've also changed the one in OnChannelError
to a DLOG.

https://codereview.chromium.org/14579006/diff/75001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:92:
!CommandLine::ForCurrentProcess()->HasSwitch(app_mode::kNoLaunchApp)));
On 2013/05/30 00:14:44, Nico wrote:
> Not clear what the bool does here. Consider using a named bool or a
> /*param_name=*/ comment to make that clearer.

Done.

https://codereview.chromium.org/14579006/diff/75001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:112: LOG(ERROR) << "App launch failed.";
On 2013/05/30 00:14:44, Nico wrote:
> see above

Done.

https://codereview.chromium.org/14579006/diff/75001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/14579006/diff/75001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_mac.mm:65: bool launch_now)
OVERRIDE;
On 2013/05/30 00:14:44, Nico wrote:
> enum params tend to be more readable at the call site (compared to bool
params).
> consider using an enum instead.

Done.

[tapted, 2013-05-30 03:25:06]

Some thoughts: I'm not sure there's precedent for sending enums over IPC - an
int with some validation might be a preferred technique. I'm not an expert on
this, but maybe there's something similar in another message group that you can
model it off.

It crossed my mind that app_shim_messages.h should maybe have a _mac suffix, but
then it couldn't play with the others in common_message_generator.h, so perhaps
that's intentional.

https://codereview.chromium.org/14579006/diff/94004/apps/app_shim/app_shim_la...
File apps/app_shim/app_shim_launch.h (right):

https://codereview.chromium.org/14579006/diff/94004/apps/app_shim/app_shim_la...
apps/app_shim/app_shim_launch.h:13: REGISTER_WITHOUT_LAUNCH = 0,  // Process the
app shim's LaunchApp message and
It's typical for enum values to share a common prefix (or suffix). Maybe 

APP_SHIM_LAUNCH_REGISTER_ONLY
APP_SHIM_LAUNCH_REGULAR

with the leading `APP_` perhaps optional, since it's a bit redundant with the
namespace.

https://codereview.chromium.org/14579006/diff/94004/chrome/app/chrome_main_ap...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/14579006/diff/94004/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:106: DLOG(ERROR) << "App shim channel
error.";
Let's just remove this. It's usually a lie, since there's no "you should exit
gracefully now" IPC message -- the domain socket is just closed.

[jackhou1, 2013-05-30 05:10:05]

There was one case that used SimilarTypeTraits, but after looking around a bit,
it seems there's a macro for using enums with IPC.

https://codereview.chromium.org/14579006/diff/94004/apps/app_shim/app_shim_la...
File apps/app_shim/app_shim_launch.h (right):

https://codereview.chromium.org/14579006/diff/94004/apps/app_shim/app_shim_la...
apps/app_shim/app_shim_launch.h:13: REGISTER_WITHOUT_LAUNCH = 0,  // Process the
app shim's LaunchApp message and
On 2013/05/30 03:25:06, tapted wrote:
> It's typical for enum values to share a common prefix (or suffix). Maybe 
> 
> APP_SHIM_LAUNCH_REGISTER_ONLY
> APP_SHIM_LAUNCH_REGULAR
> 
> with the leading `APP_` perhaps optional, since it's a bit redundant with the
> namespace.

Done.

But _NORMAL instead of _REGULAR, because it looks less like REGISTER.

https://codereview.chromium.org/14579006/diff/94004/chrome/app/chrome_main_ap...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/14579006/diff/94004/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:106: DLOG(ERROR) << "App shim channel
error.";
On 2013/05/30 03:25:06, tapted wrote:
> Let's just remove this. It's usually a lie, since there's no "you should exit
> gracefully now" IPC message -- the domain socket is just closed.

Done.

[tapted, 2013-05-30 05:41:34]

lgtm

[palmer, 2013-05-30 20:51:44]

LGTM

https://codereview.chromium.org/14579006/diff/105004/apps/app_shim/app_shim_l...
File apps/app_shim/app_shim_launch.h (right):

https://codereview.chromium.org/14579006/diff/105004/apps/app_shim/app_shim_l...
apps/app_shim/app_shim_launch.h:8: #include "ipc/ipc_param_traits.h"
I don't know, but is actually necessary here? You don't seem to use it.

[jackhou1, 2013-05-30 22:58:56]

https://codereview.chromium.org/14579006/diff/105004/apps/app_shim/app_shim_l...
File apps/app_shim/app_shim_launch.h (right):

https://codereview.chromium.org/14579006/diff/105004/apps/app_shim/app_shim_l...
apps/app_shim/app_shim_launch.h:8: #include "ipc/ipc_param_traits.h"
On 2013/05/30 20:51:44, Chromium Palmer wrote:
> I don't know, but is actually necessary here? You don't seem to use it.

Done.

[Nico, 2013-05-30 23:02:06]

chrome/ lgtm

[commit-bot: I haz the power, 2013-05-30 23:24:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/14579006/51003

[commit-bot: I haz the power, 2013-05-31 02:09:41]

Change committed as 203309