Fixed "Open in a Tab" option for apps in Chrome App Launcher.

Fixed "Open in a Tab" option for apps in Chrome App Launcher.

Currently, when Open in a Tab is not checked, the app will offer to open
itself in a "New Tab" as opposed to a "New Window" as desired. This CL
fixes that bug on Mac. This is only shown if the flag
--disable-new-bookmarked-apps is disabled. Otherwise the behaviour
is as before. See crbug.com/266803.

BUG=445038
Committed: https://crrev.com/dd4768622e797c5e4fa53a47200cd267d63716b2
Cr-Commit-Position: refs/heads/master@{#310057}

[mitchellj, 2015-01-05 05:03:17]

mitchelljones@chromium.org changed reviewers:
+ tapted@chromium.org

[mitchellj, 2015-01-05 05:03:17]

tapted, could you please review as owner? Thanks.

[tapted, 2015-01-05 05:35:44]

word is that bookmark apps may be rolling back in m41, so I think this will
regress http://crbug.com/266803

[mitchellj, 2015-01-05 22:55:13]

On 2015/01/05 05:35:44, tapted wrote:
> word is that bookmark apps may be rolling back in m41, so I think this will
> regress http://crbug.com/266803

I've rewritten the CL so that it will not regress crbug.com/266803. Could you
ptal?

[tapted, 2015-01-05 23:10:32]

was there another quirk where the initially-checked state of 'open in tab' was
weird on mac as well? (is that fixed by this, or is there a follow-up planned)

https://codereview.chromium.org/836673003/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_context_menu.cc (right):

https://codereview.chromium.org/836673003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_context_menu.cc:187: if (command_id ==
LAUNCH_NEW) {
nit: instead of the if/else-NOTREACHED, you can just do

DCHECK_EQ(LAUNCH_NEW, command_id);

https://codereview.chromium.org/836673003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_context_menu.cc:195: } else {
"Don't use else after return" :
http://www.chromium.org/developers/coding-style#Code_Formatting

[mitchellj, 2015-01-06 01:41:14]

> was there another quirk where the initially-checked state of 'open in tab' was
> weird on mac as well? (is that fixed by this, or is there a follow-up planned)

I'll make this a separate issue.

https://codereview.chromium.org/836673003/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_context_menu.cc (right):

https://codereview.chromium.org/836673003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_context_menu.cc:187: if (command_id ==
LAUNCH_NEW) {
On 2015/01/05 23:10:32, tapted wrote:
> nit: instead of the if/else-NOTREACHED, you can just do
> 
> DCHECK_EQ(LAUNCH_NEW, command_id);

Done.

https://codereview.chromium.org/836673003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_context_menu.cc:195: } else {
On 2015/01/05 23:10:32, tapted wrote:
> "Don't use else after return" :
> http://www.chromium.org/developers/coding-style#Code_Formatting

Done.

[tapted, 2015-01-06 01:47:11]

lgtm with the DCHECK_EQ change

https://codereview.chromium.org/836673003/diff/60001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_context_menu.cc (right):

https://codereview.chromium.org/836673003/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_context_menu.cc:187: DCHECK(command_id ==
LAUNCH_NEW);
DCHECK_EQ(LAUNCH_NEW, command_id);

(sometimes you need to cast an argument, but I don't think that's the case
here).

[mitchellj, 2015-01-06 01:50:22]

https://codereview.chromium.org/836673003/diff/60001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_context_menu.cc (right):

https://codereview.chromium.org/836673003/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_context_menu.cc:187: DCHECK(command_id ==
LAUNCH_NEW);
On 2015/01/06 01:47:10, tapted wrote:
> DCHECK_EQ(LAUNCH_NEW, command_id);
> 
> (sometimes you need to cast an argument, but I don't think that's the case
> here).

Done.

[mitchellj, 2015-01-06 01:50:43]

The CQ bit was checked by mitchelljones@chromium.org

[commit-bot: I haz the power, 2015-01-06 01:51:17]

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

[commit-bot: I haz the power, 2015-01-06 02:10:15]

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

[commit-bot: I haz the power, 2015-01-06 02:10:16]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[mitchellj, 2015-01-06 05:20:25]

The CQ bit was checked by mitchelljones@chromium.org

[commit-bot: I haz the power, 2015-01-06 05:20:45]

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

[commit-bot: I haz the power, 2015-01-06 05:52:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-06 05:53:29]

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