Hardcode extensions::util::IsNewBookmarkAppsEnabled() to true, except on Mac.

Hardcode extensions::util::IsNewBookmarkAppsEnabled() to true, except on Mac.

On Mac, make it a Feature controlled by chrome://flags/#bookmark-apps.
(Currently one needs to "Enable" #disable-new-bookmark-apps which is
confusing). Feature flags have other benefits over command line flags
too (e.g. Finch integration).

On other platforms, it's been on by default for a long time and doesn't
need a disable flag. Hardcoding it to `true` means we have some more
obviously dead code around IDC_CREATE_SHORTCUTS which was never
implemented on Mac, and replaced by bookmark apps elsewhere.

BUG=702551
Review-Url: https://codereview.chromium.org/2765083002
Cr-Commit-Position: refs/heads/master@{#459209}
Committed: https://chromium.googlesource.com/chromium/src/+/70cdc81ee387422102b362949621882df0bacd25

[tapted, 2017-03-21 23:56:36]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 23:57:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-22 02:26:02]

Patchset #3 (id:40001) has been deleted

[tapted, 2017-03-22 02:26:10]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 02:27:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-22 02:32:28]

Description was changed from

==========
cl format


Alter flag

BUG=
==========

to

==========
Hardcode extensions::util::IsNewBookmarkAppsEnabled() to true, except on Mac.

On Mac, make it a Feature controlled by chrome://flags/#bookmark-apps.
(Currently one needs to "Enable" #disable-new-bookmark-apps which is
confusing). Feature flags have other benefits over command line flags
too (e.g. Finch integration).

On other platforms, it's been on by default for a long time and doesn't
need a disable flag. Hardcoding it to `true` means we have some more
obviously dead code around IDC_CREATE_SHORTCUTS which was never
implemented on Mac, and replaced by bookmark apps elsewhere.

BUG=702551
==========

[tapted, 2017-03-22 02:32:28]

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

[tapted, 2017-03-22 02:34:02]

Hi Dom, please take a look.

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (left):

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1865:
switches::kDisableNewBookmarkApps,
Using base::Feature allows this stuff to happen automatically when
RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer(..) does
BrowserChildProcessHostImpl::CopyFeatureAndFieldTrialFlags(..)

[dominickn, 2017-03-22 04:18:12]

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/apps/app...
File chrome/browser/apps/app_shim/app_shim_interactive_uitest_mac.mm (right):

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/apps/app...
chrome/browser/apps/app_shim/app_shim_interactive_uitest_mac.mm:60:
command_line->AppendSwitchASCII(switches::kEnableFeatures,
Can you carry a base::ScopedFeatureList as a member on this class, and
initialize it in the setup with features::kBookmarkApps? Not sure if there's a
weird restriction here that means you can't use it.

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_util.cc (right):

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_util.cc:12: #include "base/values.h"
#include "base/feature_list.h"

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm
(right):

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:48:
command_line->AppendSwitchASCII(switches::kEnableFeatures,
Ditto base::ScopedFeatureList

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/hosted_app_browsertest.cc (right):

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/hosted_app_browsertest.cc:49:
switches::kEnableFeatures, features::kBookmarkApps.name);
Can you use base::FeatureList::RegisterOverride or a ScopedFeatureList?

https://codereview.chromium.org/2765083002/diff/60001/chrome/renderer/web_app...
File chrome/renderer/web_apps.cc (right):

https://codereview.chromium.org/2765083002/diff/60001/chrome/renderer/web_app...
chrome/renderer/web_apps.cc:12: #include "base/command_line.h"
#include "base/feature_list.h"

[commit-bot: I haz the power, 2017-03-22 04:28:41]

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

[commit-bot: I haz the power, 2017-03-22 04:28:41]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
build hasn't started yet, builder probably lacks capacity)

[tapted, 2017-03-23 00:37:14]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 00:38:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 02:05:13]

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

[commit-bot: I haz the power, 2017-03-23 02:05:14]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-03-23 02:59:59]

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/apps/app...
File chrome/browser/apps/app_shim/app_shim_interactive_uitest_mac.mm (right):

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/apps/app...
chrome/browser/apps/app_shim/app_shim_interactive_uitest_mac.mm:60:
command_line->AppendSwitchASCII(switches::kEnableFeatures,
On 2017/03/22 04:18:12, dominickn wrote:
> Can you carry a base::ScopedFeatureList as a member on this class, and
> initialize it in the setup with features::kBookmarkApps? Not sure if there's a
> weird restriction here that means you can't use it.

Done.

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_util.cc (right):

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_util.cc:12: #include "base/values.h"
On 2017/03/22 04:18:12, dominickn wrote:
> #include "base/feature_list.h"

Done.

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm
(right):

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:48:
command_line->AppendSwitchASCII(switches::kEnableFeatures,
On 2017/03/22 04:18:12, dominickn wrote:
> Ditto base::ScopedFeatureList

Done.

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/hosted_app_browsertest.cc (right):

https://codereview.chromium.org/2765083002/diff/60001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/hosted_app_browsertest.cc:49:
switches::kEnableFeatures, features::kBookmarkApps.name);
On 2017/03/22 04:18:12, dominickn wrote:
> Can you use base::FeatureList::RegisterOverride or a ScopedFeatureList?

Done.

https://codereview.chromium.org/2765083002/diff/60001/chrome/renderer/web_app...
File chrome/renderer/web_apps.cc (right):

https://codereview.chromium.org/2765083002/diff/60001/chrome/renderer/web_app...
chrome/renderer/web_apps.cc:12: #include "base/command_line.h"
On 2017/03/22 04:18:12, dominickn wrote:
> #include "base/feature_list.h"

Done.

[dominickn, 2017-03-23 03:44:39]

lgtm, thanks

[tapted, 2017-03-23 05:46:28]

tapted@chromium.org changed reviewers:
+ benwells@chromium.org, isherman@chromium.org, jochen@chromium.org

[tapted, 2017-03-23 05:46:29]

+jochen for chrome/renderer/web_apps.cc
+benwells for extensions (hosted_app_browsertest.cc,extension_util.cc
+isherman for histograms.xml

[benwells, 2017-03-23 06:22:04]

lgtm

[jochen (gone - plz use gerrit), 2017-03-23 15:52:42]

chrome/renderer/web_apps.cc lgtm

[Ilya Sherman, 2017-03-23 19:57:27]

histograms.xml lgtm

[tapted, 2017-03-23 20:41:59]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-03-23 20:42:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 20:48:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490301719007650,
"parent_rev": "dffda5006efcf5d473530268ff2da14f43e41cc3", "commit_rev":
"70cdc81ee387422102b362949621882df0bacd25"}

[commit-bot: I haz the power, 2017-03-23 20:49:11]

Description was changed from

==========
Hardcode extensions::util::IsNewBookmarkAppsEnabled() to true, except on Mac.

On Mac, make it a Feature controlled by chrome://flags/#bookmark-apps.
(Currently one needs to "Enable" #disable-new-bookmark-apps which is
confusing). Feature flags have other benefits over command line flags
too (e.g. Finch integration).

On other platforms, it's been on by default for a long time and doesn't
need a disable flag. Hardcoding it to `true` means we have some more
obviously dead code around IDC_CREATE_SHORTCUTS which was never
implemented on Mac, and replaced by bookmark apps elsewhere.

BUG=702551
==========

to

==========
Hardcode extensions::util::IsNewBookmarkAppsEnabled() to true, except on Mac.

On Mac, make it a Feature controlled by chrome://flags/#bookmark-apps.
(Currently one needs to "Enable" #disable-new-bookmark-apps which is
confusing). Feature flags have other benefits over command line flags
too (e.g. Finch integration).

On other platforms, it's been on by default for a long time and doesn't
need a disable flag. Hardcoding it to `true` means we have some more
obviously dead code around IDC_CREATE_SHORTCUTS which was never
implemented on Mac, and replaced by bookmark apps elsewhere.

BUG=702551

Review-Url: https://codereview.chromium.org/2765083002
Cr-Commit-Position: refs/heads/master@{#459209}
Committed:
https://chromium.googlesource.com/chromium/src/+/70cdc81ee387422102b362949621...
==========

[commit-bot: I haz the power, 2017-03-23 20:49:13]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/70cdc81ee387422102b362949621...