Convert Extension* to extension id in AppLaunchParams.

Convert Extension* to extension id in AppLaunchParams.

Having raw Extension pointers is problematic because an extension may be
reloaded during the lifetime of AppLaunchParams, which will invalidate the
pointer.

This also converts CommandLine* to CommandLine and lets the empty command
line represent "no command line".

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

[koz (OOO until 15th September), 2013-12-13 02:21:29]

sky: for chrome/browser/ui/startup/startup_browser_creator_impl.cc

tapted: general review
benewlls: owners

[tapted, 2013-12-13 03:10:58]

https://codereview.chromium.org/114263005/diff/1/chrome/browser/ui/extensions...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/114263005/diff/1/chrome/browser/ui/extensions...
chrome/browser/ui/extensions/application_launch.cc:185: const
extensions::Extension* const extension = GetExtension(params);
nit: probably don't need the extra 'extension::' on Extension. Same below x3

https://codereview.chromium.org/114263005/diff/1/chrome/browser/ui/extensions...
chrome/browser/ui/extensions/application_launch.cc:186: if (!extension)
I think extension is allowed to be NULL in this function -- see how it's called
from OpenAppShortcutWindow. Probably it means the AppLaunchParams constructor
also needs some care not to dereference NULL

https://codereview.chromium.org/114263005/diff/1/chrome/browser/ui/extensions...
chrome/browser/ui/extensions/application_launch.cc:240: if (!extension)
This has already been checked in OpenEnabledApplication.. You could perhaps make
the Extension* an argument, or CHECK here. I'm probably leaning towards the
former, although part of me is sad that it loses symmetry with
OpenApplicationWindow, so I could be convinced otherwise.

https://codereview.chromium.org/114263005/diff/1/chrome/browser/ui/extensions...
File chrome/browser/ui/extensions/application_launch.h (right):

https://codereview.chromium.org/114263005/diff/1/chrome/browser/ui/extensions...
chrome/browser/ui/extensions/application_launch.h:71: // If non-NULL,
information from the command line may be passed on to the
nit: update comment (re: non-NULL)

[koz (OOO until 15th September), 2013-12-13 03:54:42]

https://codereview.chromium.org/114263005/diff/1/chrome/browser/ui/extensions...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/114263005/diff/1/chrome/browser/ui/extensions...
chrome/browser/ui/extensions/application_launch.cc:185: const
extensions::Extension* const extension = GetExtension(params);
On 2013/12/13 03:10:58, tapted wrote:
> nit: probably don't need the extra 'extension::' on Extension. Same below x3

Done.

https://codereview.chromium.org/114263005/diff/1/chrome/browser/ui/extensions...
chrome/browser/ui/extensions/application_launch.cc:186: if (!extension)
On 2013/12/13 03:10:58, tapted wrote:
> I think extension is allowed to be NULL in this function -- see how it's
called
> from OpenAppShortcutWindow. Probably it means the AppLaunchParams constructor
> also needs some care not to dereference NULL

Done.

https://codereview.chromium.org/114263005/diff/1/chrome/browser/ui/extensions...
chrome/browser/ui/extensions/application_launch.cc:240: if (!extension)
On 2013/12/13 03:10:58, tapted wrote:
> This has already been checked in OpenEnabledApplication.. You could perhaps
make
> the Extension* an argument, or CHECK here. I'm probably leaning towards the
> former, although part of me is sad that it loses symmetry with
> OpenApplicationWindow, so I could be convinced otherwise.

I think a CHECK is slightly better here because otherwise it's possible to call
OpenApplicationTab() with mismatched extension id and Extension*.

https://codereview.chromium.org/114263005/diff/1/chrome/browser/ui/extensions...
File chrome/browser/ui/extensions/application_launch.h (right):

https://codereview.chromium.org/114263005/diff/1/chrome/browser/ui/extensions...
chrome/browser/ui/extensions/application_launch.h:71: // If non-NULL,
information from the command line may be passed on to the
On 2013/12/13 03:10:58, tapted wrote:
> nit: update comment (re: non-NULL)

Done.

[tapted, 2013-12-13 05:16:46]

lgtm

https://codereview.chromium.org/114263005/diff/20001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/114263005/diff/20001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/application_launch.cc:403: extension_id(extension ?
extension->id() : ""),
nit: "" -> std::string()  [it's much faster, since "" often needs a malloc()]

https://codereview.chromium.org/114263005/diff/20001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/application_launch.cc:415: extension_id(extension ?
extension->id() : ""),
I think this overload, and the next, actually can't be called with a NULL
extension: it looks like there are no checks leading up to GetAppLaunchInfo(..)
in app_launch_info.cc where it would dereference NULL.

So, I'd maybe leave these, so we die closer to the caller.

[benwells, 2013-12-13 06:31:58]

Description nit: you are also changing CommandLine* to CommandLine, can you
update the description to mention that somewhere.

https://codereview.chromium.org/114263005/diff/20001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/114263005/diff/20001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/application_launch.cc:124: const Extension*
extension = service->GetExtensionById(
Nit: There is a GetExtensionById variant that takes an include mask. Did you
consider using that instead of this one and then GetTerminatedExtension?

[benwells, 2013-12-13 06:32:31]

On 2013/12/13 06:31:58, benwells wrote:
> Description nit: you are also changing CommandLine* to CommandLine, can you
> update the description to mention that somewhere.
> 
>
https://codereview.chromium.org/114263005/diff/20001/chrome/browser/ui/extens...
> File chrome/browser/ui/extensions/application_launch.cc (right):
> 
>
https://codereview.chromium.org/114263005/diff/20001/chrome/browser/ui/extens...
> chrome/browser/ui/extensions/application_launch.cc:124: const Extension*
> extension = service->GetExtensionById(
> Nit: There is a GetExtensionById variant that takes an include mask. Did you
> consider using that instead of this one and then GetTerminatedExtension?

oops forgot. lgtm.

[sky, 2013-12-13 17:03:33]

LGTM

[koz (OOO until 15th September), 2013-12-16 00:21:44]

Thanks all.

https://codereview.chromium.org/114263005/diff/20001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/114263005/diff/20001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/application_launch.cc:124: const Extension*
extension = service->GetExtensionById(
On 2013/12/13 06:31:58, benwells wrote:
> Nit: There is a GetExtensionById variant that takes an include mask. Did you
> consider using that instead of this one and then GetTerminatedExtension?

I didn't - that would be clearer. Done.

https://codereview.chromium.org/114263005/diff/20001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/application_launch.cc:403: extension_id(extension ?
extension->id() : ""),
On 2013/12/13 05:16:47, tapted wrote:
> nit: "" -> std::string()  [it's much faster, since "" often needs a malloc()]

Done.

https://codereview.chromium.org/114263005/diff/20001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/application_launch.cc:415: extension_id(extension ?
extension->id() : ""),
On 2013/12/13 05:16:47, tapted wrote:
> I think this overload, and the next, actually can't be called with a NULL
> extension: it looks like there are no checks leading up to
GetAppLaunchInfo(..)
> in app_launch_info.cc where it would dereference NULL.
> 
> So, I'd maybe leave these, so we die closer to the caller.

I think it's simpler if we just have extension_id.empty() be equivalent to
passing a NULL Extension* in the old code. Besides, we didn't use to have a
CHECK(extension) in the old constructors so I'd rather not introduce one now.

[commit-bot: I haz the power, 2013-12-16 00:22:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/114263005/40001

[commit-bot: I haz the power, 2013-12-16 00:35:24]

Retried try job too often on linux_rel for step(s) cacheinvalidation_unittests,
cc_unittests, check_deps, chromedriver2_unittests, components_unittests,
content_browsertests, content_unittests, crypto_unittests,
google_apis_unittests, gpu_unittests, ipc_tests, jingle_unittests,
media_unittests, nacl_integration, ppapi_unittests, printing_unittests,
remoting_unittests, sandbox_linux_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-16 01:56:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/114263005/60001

[commit-bot: I haz the power, 2013-12-16 02:16:08]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-16 03:01:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/114263005/80001

[commit-bot: I haz the power, 2013-12-16 03:19:25]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-16 04:24:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/114263005/100001

[commit-bot: I haz the power, 2013-12-16 05:24:36]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-16 22:53:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/114263005/100001

[commit-bot: I haz the power, 2013-12-17 02:06:11]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-17 02:21:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/114263005/100001

[commit-bot: I haz the power, 2013-12-17 03:49:20]

Change committed as 241143