Send Chrome's process id to the app shim.

Send Chrome's process id to the app shim.

This saves the app shim from having to look it up using Chrome's bundle
id. This also allows the shim to connect to Chrome processes started by
tests which are not associated with the bundle.

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

[tapted, 2013-08-22 05:21:49]

Some of the files didn't upload..

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

https://codereview.chromium.org/22903025/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/web_app_mac.mm:219: LOG(INFO) << "pid " <<
base::Process::Current().pid();
ohno! It's a LOG

https://codereview.chromium.org/22903025/diff/1/chrome/common/mac/app_mode_co...
File chrome/common/mac/app_mode_common.mm (right):

https://codereview.chromium.org/22903025/diff/1/chrome/common/mac/app_mode_co...
chrome/common/mac/app_mode_common.mm:9: const char kChromeProcessId[] =
"chrome-process-id";
nit: should we sort these?

[jackhou1, 2013-08-22 05:33:32]

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

https://codereview.chromium.org/22903025/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/web_app_mac.mm:219: LOG(INFO) << "pid " <<
base::Process::Current().pid();
On 2013/08/22 05:21:49, tapted wrote:
> ohno! It's a LOG

Oops, removed.

https://codereview.chromium.org/22903025/diff/1/chrome/common/mac/app_mode_co...
File chrome/common/mac/app_mode_common.mm (right):

https://codereview.chromium.org/22903025/diff/1/chrome/common/mac/app_mode_co...
chrome/common/mac/app_mode_common.mm:9: const char kChromeProcessId[] =
"chrome-process-id";
On 2013/08/22 05:21:49, tapted wrote:
> nit: should we sort these?

Moved it somewhere more 'sorted'. I think it's worth grouping them by type,
where they're used, then sort by name. I'll do another CL.

[tapted, 2013-08-22 06:17:12]

https://codereview.chromium.org/22903025/diff/9001/apps/app_shim/chrome_main_...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/22903025/diff/9001/apps/app_shim/chrome_main_...
apps/app_shim/chrome_main_app_mode_mac.mm:441: pid = -1;
This implies it will try to start a new Chrome.. Maybe LOG(FATAL) << "Invalid
PID"  (or `return EXIT_FAILURE;`)?  i.e., if this argument is present it means
it came from a running chrome, and so we should never start a new one.

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

https://codereview.chromium.org/22903025/diff/9001/chrome/common/mac/app_mode...
chrome/common/mac/app_mode_common.h:37: extern const char kNoLaunchApp[];
Hm.. do we need a separate flag for this still?

I think this could be changed to be (e.g.) kLaunchByChromeProcessId and there
will be one less flag to maintain.

[jackhou1, 2013-08-22 06:52:53]

https://codereview.chromium.org/22903025/diff/9001/apps/app_shim/chrome_main_...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/22903025/diff/9001/apps/app_shim/chrome_main_...
apps/app_shim/chrome_main_app_mode_mac.mm:441: pid = -1;
On 2013/08/22 06:17:12, tapted wrote:
> This implies it will try to start a new Chrome.. Maybe LOG(FATAL) << "Invalid
> PID"  (or `return EXIT_FAILURE;`)?  i.e., if this argument is present it means
> it came from a running chrome, and so we should never start a new one.

Done.

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

https://codereview.chromium.org/22903025/diff/9001/chrome/common/mac/app_mode...
chrome/common/mac/app_mode_common.h:37: extern const char kNoLaunchApp[];
On 2013/08/22 06:17:12, tapted wrote:
> Hm.. do we need a separate flag for this still?
> 
> I think this could be changed to be (e.g.) kLaunchByChromeProcessId and there
> will be one less flag to maintain. 

Done.

[tapted, 2013-08-22 07:25:18]

lgtm, but we should get a sanity check from security about passing the PID
around -- it's kinda IPC. You'll also need some OWNERS.

And, just musing, this CL might also affect http://crbug.com/271347 - but the
stack traces there are a bit whacky, so it's probably not related.

[jackhou1, 2013-08-23 05:12:24]

benwells, please review for OWNERS in chrome/browser/web_applications/

rsesek, please review for OWNERS in chrome/common/mac/

palmer, please advise whether sending Chrome's PID to the shim via a switch is
okay from security POV.

[benwells, 2013-08-23 13:04:51]

Web app lgtm

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

https://codereview.chromium.org/22903025/diff/17001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:23: #include
"base/strings/string_number_conversions.h"
Nit: move above string_util

[palmer, 2013-08-23 18:37:16]

lgtm

[Robert Sesek, 2013-08-23 18:47:32]

LGTM w/ a small change

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

https://codereview.chromium.org/22903025/diff/17001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:217:
base::IntToString(base::Process::Current().pid()));
Use base::GetCurrentProcessId() in base/process/process_handle.h

[jackhou1, 2013-08-26 04:29:44]

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

https://codereview.chromium.org/22903025/diff/17001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:23: #include
"base/strings/string_number_conversions.h"
On 2013/08/23 13:04:52, benwells wrote:
> Nit: move above string_util

Done.

https://codereview.chromium.org/22903025/diff/17001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:217:
base::IntToString(base::Process::Current().pid()));
On 2013/08/23 18:47:32, rsesek wrote:
> Use base::GetCurrentProcessId() in base/process/process_handle.h

Done.

[commit-bot: I haz the power, 2013-08-26 05:48:37]

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

[commit-bot: I haz the power, 2013-08-26 09:23:49]

Change committed as 219519