Add a flag to enable open in window for hosted apps on Mac.

Add a flag to enable open in window for hosted apps on Mac.

Thie CL adds --[enable|disable]-hosted-apps-in-windows, which controls
whether or not open in windows is allowed for hosted apps. The flag
allows us to fix several outstanding issues with hosted apps on Mac
before enabling open in windows by default.

BUG=517682
Committed: https://crrev.com/2b10cbd741f3d355186d28bf048cefda6706e297
Cr-Commit-Position: refs/heads/master@{#344406}

[dominickn, 2015-08-18 07:41:12]

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

[dominickn, 2015-08-19 01:11:16]

PTAL, thanks!

[benwells, 2015-08-19 02:39:21]

https://codereview.chromium.org/1300763002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/launch_util.cc (right):

https://codereview.chromium.org/1300763002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/launch_util.cc:55: //  3. one of
IsNewBookmarkAppsEnabled() or CanHostedAppsOpenInWindows() are
Why does this depend on IsNewBookmarkAppsEnabled at all?

https://codereview.chromium.org/1300763002/diff/120001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/app_context_menu.cc (right):

https://codereview.chromium.org/1300763002/diff/120001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/app_context_menu.cc:120: if
(extensions::util::IsNewBookmarkAppsEnabled()) {
Ditto

[dominickn, 2015-08-19 03:01:13]

https://codereview.chromium.org/1300763002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/launch_util.cc (right):

https://codereview.chromium.org/1300763002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/launch_util.cc:55: //  3. one of
IsNewBookmarkAppsEnabled() or CanHostedAppsOpenInWindows() are
On 2015/08/19 02:39:21, benwells wrote:
> Why does this depend on IsNewBookmarkAppsEnabled at all?

I kept it like this because the new bookmark apps are disabled by default on
Mac. The intention is to remove the IsNewBookmarkAppsEnabled dependency once the
flag is flipped. Should I just remove it now?

[benwells, 2015-08-19 03:26:52]

https://codereview.chromium.org/1300763002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/launch_util.cc (right):

https://codereview.chromium.org/1300763002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/launch_util.cc:55: //  3. one of
IsNewBookmarkAppsEnabled() or CanHostedAppsOpenInWindows() are
On 2015/08/19 03:01:13, dominickn wrote:
> On 2015/08/19 02:39:21, benwells wrote:
> > Why does this depend on IsNewBookmarkAppsEnabled at all?
> 
> I kept it like this because the new bookmark apps are disabled by default on
> Mac. The intention is to remove the IsNewBookmarkAppsEnabled dependency once
the
> flag is flipped. Should I just remove it now?

I'm not sure, but it shouldn't depend on the current default state of flags.

Ideally the two flags should be orthogonal - i.e. whether hosted apps (which is
a superset of bookmark apps) can run in windows, and whether users can create
bookmark apps, feel like independent things. It seems reasonable to be able to
run hosted apps in windows while not having bookmark apps enabled.

So ideally I think they would be independent. However I'm not sure what that
means in the code. If it turns out to be complicated we could make the ability
to run hosted apps in windows only valid if the new bookmarks flow is enabled.

[dominickn, 2015-08-19 05:21:58]

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

[dominickn, 2015-08-19 05:21:58]

@tapted: similar to the patch you reviewed the other day, except now there is a
flag controlling whether or not open in window is active on Mac. I will close
the other issue and have a separate CL for the NTP.

https://codereview.chromium.org/1300763002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/launch_util.cc (right):

https://codereview.chromium.org/1300763002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/launch_util.cc:55: //  3. one of
IsNewBookmarkAppsEnabled() or CanHostedAppsOpenInWindows() are
On 2015/08/19 03:26:52, benwells wrote:
> On 2015/08/19 03:01:13, dominickn wrote:
> > On 2015/08/19 02:39:21, benwells wrote:
> > > Why does this depend on IsNewBookmarkAppsEnabled at all?
> > 
> > I kept it like this because the new bookmark apps are disabled by default on
> > Mac. The intention is to remove the IsNewBookmarkAppsEnabled dependency once
> the
> > flag is flipped. Should I just remove it now?
> 
> I'm not sure, but it shouldn't depend on the current default state of flags.
> 
> Ideally the two flags should be orthogonal - i.e. whether hosted apps (which
is
> a superset of bookmark apps) can run in windows, and whether users can create
> bookmark apps, feel like independent things. It seems reasonable to be able to
> run hosted apps in windows while not having bookmark apps enabled.
> 
> So ideally I think they would be independent. However I'm not sure what that
> means in the code. If it turns out to be complicated we could make the ability
> to run hosted apps in windows only valid if the new bookmarks flow is enabled.

Done.

https://codereview.chromium.org/1300763002/diff/120001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/app_context_menu.cc (right):

https://codereview.chromium.org/1300763002/diff/120001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/app_context_menu.cc:120: if
(extensions::util::IsNewBookmarkAppsEnabled()) {
On 2015/08/19 02:39:21, benwells wrote:
> Ditto

Done.

[benwells, 2015-08-19 05:44:54]

https://codereview.chromium.org/1300763002/diff/160001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/app_context_menu.cc (right):

https://codereview.chromium.org/1300763002/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/app_context_menu.cc:118: if
(extensions::util::CanHostedAppsOpenInWindows()) {
This logic looks nice and simple now, but I think it isn't quite what we want.
This is what i was worrying about with the complexity of keeping these two
things orthogonal ...

The complexity is because bookmark apps does some extra crazy stuff with
removing the pinned and fullscreen options, and we should keep that as it has
rolled out to stable already.

Thinking about the cases we have:

Bookmark apps enabled / hosted apps in windows enabled:
-> just want to add the extra 'window' as per line 120 of the new code

Bookmark apps enabled / hosted apps in windows disabled:
-> want to do nothing here

Bookmark apps disabled / hosted apps in windows enabled:
-> want to do what the 'else' statement here is doing, with the '#if defined
MAC' deleted

Bookmark apps disabled / hosted apps in windows disabled:
-> want to do what the else statement here is doing, with the '#if defined MAC'
statement kept (i.e. what is in that statement'.

Does that make sense?

https://codereview.chromium.org/1300763002/diff/160001/extensions/browser/api...
File extensions/browser/api/management/management_api.cc (right):

https://codereview.chromium.org/1300763002/diff/160001/extensions/browser/api...
extensions/browser/api/management/management_api.cc:85: if
(delegate->CanHostedAppsOpenInWindows())
This logic looks right - we need to make the other place match this, I think.

[dominickn, 2015-08-19 06:29:04]

Ah, I understand now. Fixed the logic in app_context_menu and
app_info_summary_panel to match this. Thanks!

https://codereview.chromium.org/1300763002/diff/160001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/app_context_menu.cc (right):

https://codereview.chromium.org/1300763002/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/app_context_menu.cc:118: if
(extensions::util::CanHostedAppsOpenInWindows()) {
On 2015/08/19 05:44:54, benwells wrote:
> This logic looks nice and simple now, but I think it isn't quite what we want.
> This is what i was worrying about with the complexity of keeping these two
> things orthogonal ...
> 
> The complexity is because bookmark apps does some extra crazy stuff with
> removing the pinned and fullscreen options, and we should keep that as it has
> rolled out to stable already.
> 
> Thinking about the cases we have:
> 
> Bookmark apps enabled / hosted apps in windows enabled:
> -> just want to add the extra 'window' as per line 120 of the new code
> 
> Bookmark apps enabled / hosted apps in windows disabled:
> -> want to do nothing here
> 
> Bookmark apps disabled / hosted apps in windows enabled:
> -> want to do what the 'else' statement here is doing, with the '#if defined
> MAC' deleted
> 
> Bookmark apps disabled / hosted apps in windows disabled:
> -> want to do what the else statement here is doing, with the '#if defined
MAC'
> statement kept (i.e. what is in that statement'.
> 
> Does that make sense?

Done.

[tapted, 2015-08-19 06:48:13]

https://codereview.chromium.org/1300763002/diff/200001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1300763002/diff/200001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1492: switches::kDisableHostedAppsInWindows)},
does anything read switches::kDisableHostedAppsInWindows? (or will it in a
follow-up?)

Perhaps just use SINGLE_VALUE_TYPE(switches::kEnableHostedAppsInWindows)}

https://codereview.chromium.org/1300763002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/1300763002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:586: if
(extensions::util::CanHostedAppsOpenInWindows())
nit: comment here like "Omit the Open As Window checkbox when that isn't
supported to avoid confusing the user."

https://codereview.chromium.org/1300763002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:611: if
(extensions::util::CanHostedAppsOpenInWindows()) {
Is this to address something specific? (I think we can remove it..)

https://codereview.chromium.org/1300763002/diff/200001/chrome/common/chrome_s...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/1300763002/diff/200001/chrome/common/chrome_s...
chrome/common/chrome_switches.h:131: extern const char
kEnableHostedAppsInWindows[];
If this is mac-only, it should be guarded by the #if defined(OS_MACOSX) down on
line 357. Same for the .cc, the about_flags.cc change and the .grd strings.
Prevents adding bloat to android and other platforms.

https://codereview.chromium.org/1300763002/diff/200001/extensions/browser/api...
File extensions/browser/api/management/management_api.cc (right):

https://codereview.chromium.org/1300763002/diff/200001/extensions/browser/api...
extensions/browser/api/management/management_api.cc:84: // TODO(dominickn)
remove check when hosted apps can open in windows on Mac.
nit: colon after TODO(..):

[dominickn, 2015-08-19 07:10:41]

Thanks!

https://codereview.chromium.org/1300763002/diff/200001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1300763002/diff/200001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1492: switches::kDisableHostedAppsInWindows)},
On 2015/08/19 06:48:12, tapted wrote:
> does anything read switches::kDisableHostedAppsInWindows? (or will it in a
> follow-up?)
> 
> Perhaps just use SINGLE_VALUE_TYPE(switches::kEnableHostedAppsInWindows)}

The follow up will be to flip this flag from checking enable to checking
disable, then removing the switch entirely once it's stable.

https://codereview.chromium.org/1300763002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/1300763002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:586: if
(extensions::util::CanHostedAppsOpenInWindows())
On 2015/08/19 06:48:13, tapted wrote:
> nit: comment here like "Omit the Open As Window checkbox when that isn't
> supported to avoid confusing the user."

Done.

https://codereview.chromium.org/1300763002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:611: if
(extensions::util::CanHostedAppsOpenInWindows()) {
On 2015/08/19 06:48:13, tapted wrote:
> Is this to address something specific? (I think we can remove it..)

Digging deeper, it looks like it doesn't doesn't have an effect. I've undone the
change.

https://codereview.chromium.org/1300763002/diff/200001/chrome/common/chrome_s...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/1300763002/diff/200001/chrome/common/chrome_s...
chrome/common/chrome_switches.h:131: extern const char
kEnableHostedAppsInWindows[];
On 2015/08/19 06:48:13, tapted wrote:
> If this is mac-only, it should be guarded by the #if defined(OS_MACOSX) down
on
> line 357. Same for the .cc, the about_flags.cc change and the .grd strings.
> Prevents adding bloat to android and other platforms.

Done.

https://codereview.chromium.org/1300763002/diff/200001/extensions/browser/api...
File extensions/browser/api/management/management_api.cc (right):

https://codereview.chromium.org/1300763002/diff/200001/extensions/browser/api...
extensions/browser/api/management/management_api.cc:84: // TODO(dominickn)
remove check when hosted apps can open in windows on Mac.
On 2015/08/19 06:48:13, tapted wrote:
> nit: colon after TODO(..):

Done.

[benwells, 2015-08-19 07:30:38]

non-mac stuff lgtm

[benwells, 2015-08-19 07:31:08]

On 2015/08/19 07:30:38, benwells wrote:
> non-mac stuff lgtm

(meaning, not the .mm file)

[dominickn, 2015-08-20 00:30:46]

dominickn@chromium.org changed reviewers:
+ isherman@chromium.org

[dominickn, 2015-08-20 00:31:20]

benwells: thanks!

tapted: do you have any further issues I should address?

isherman: please review tools/, thanks!

[Ilya Sherman, 2015-08-20 00:32:25]

histograms.xml lgtm

[tapted, 2015-08-20 00:44:02]

lgtm

https://codereview.chromium.org/1300763002/diff/200001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1300763002/diff/200001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1492: switches::kDisableHostedAppsInWindows)},
On 2015/08/19 07:10:41, dominickn wrote:
> On 2015/08/19 06:48:12, tapted wrote:
> > does anything read switches::kDisableHostedAppsInWindows? (or will it in a
> > follow-up?)
> > 
> > Perhaps just use SINGLE_VALUE_TYPE(switches::kEnableHostedAppsInWindows)}
> 
> The follow up will be to flip this flag from checking enable to checking
> disable, then removing the switch entirely once it's stable.

Hm, typically both switches are checked regardless of the default (see, e.g.,
app_list_swiches.cc). I guess it works out to be equivalent though.

https://codereview.chromium.org/1300763002/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/1300763002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:587: // When
CanHostedAppsOpenInWindows is disabled, do not show the open as window
nit: -> `CanHostedAppsOpenInWindows()` since it's a function (no parens implies
a class name)

[dominickn, 2015-08-20 00:58:02]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2015-08-20 00:58:03]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
benwells@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1300763002/#ps240001
(title: "Addressing nits")

[commit-bot: I haz the power, 2015-08-20 00:59:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1300763002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1300763002/240001

[commit-bot: I haz the power, 2015-08-20 02:09:27]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-08-20 02:10:04]

Patchset 13 (id:??) landed as
https://crrev.com/2b10cbd741f3d355186d28bf048cefda6706e297
Cr-Commit-Position: refs/heads/master@{#344406}