Replace --add-to-shelf flag with kAppBanners feature.

Replace --add-to-shelf flag with kAppBanners feature.

The add to shelf flag was poorly named and doesn't use the new feature
goodness.

With this change, functionality dependent on the kBookmarkApps flag is
also enabled if the kAppBanners flag is enabled.

BUG=734897
Review-Url: https://codereview.chromium.org/2944283002
Cr-Commit-Position: refs/heads/master@{#482889}
Committed: https://chromium.googlesource.com/chromium/src/+/c421ccdbdd2c25b9d5d83eab1d8902aacb0d00ab

[benwells, 2017-06-20 08:51:34]

Description was changed from

==========
Replace --add-to-shelf flag with kAppBanners feature.

The add to shelf flag was poorly named and doesn't use the new feature
goodness.

BUG=734897
==========

to

==========
Replace --add-to-shelf flag with kAppBanners feature.

The add to shelf flag was poorly named and doesn't use the new feature
goodness.

With this change, functionality dependent on the kBookmarkApps flag is also
enabled if the kAppBanners flag is enabled.

BUG=734897
==========

[benwells, 2017-06-20 08:51:38]

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

[commit-bot: I haz the power, 2017-06-20 08:51:49]

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-06-20 09:14:22]

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

[commit-bot: I haz the power, 2017-06-20 09:14:22]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[benwells, 2017-06-20 11:34:05]

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

[commit-bot: I haz the power, 2017-06-20 11:34:20]

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-06-20 12:38:58]

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

[commit-bot: I haz the power, 2017-06-20 12:38:59]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-06-21 00:13:32]

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

[Matt Giuca, 2017-06-22 01:40:38]

https://codereview.chromium.org/2944283002/diff/60001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2944283002/diff/60001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1988: #if !defined(OS_ANDROID)
Why !defined(OS_ANDROID)?

You already have kOsDesktop. Is the compile-time check necessary?

(The difference is, without the compile-time check, this will show up in
Android's flags as "not available on this platform" which is preferable to it
being confusingly missing.)

https://codereview.chromium.org/2944283002/diff/60001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2944283002/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:270: "Enable the display of add to shelf
banners, which prompt a user to add "
s/add to shelf/app/ since that's a CrOS-specific term. Or "Progressive Web App
banners" perhaps clearer under the new terminology.

https://codereview.chromium.org/2944283002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (left):

https://codereview.chromium.org/2944283002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:22301: -  <int value="-1016202433"
label="disable-add-to-shelf"/>
Don't delete enums.

Add " (deprecated)" to the end of the name (seems to be the convention).

[Matt Giuca, 2017-06-22 01:41:04]

Also, line-wrap the last paragraph of the CL description.

[benwells, 2017-06-22 05:36:45]

Description was changed from

==========
Replace --add-to-shelf flag with kAppBanners feature.

The add to shelf flag was poorly named and doesn't use the new feature
goodness.

With this change, functionality dependent on the kBookmarkApps flag is also
enabled if the kAppBanners flag is enabled.

BUG=734897
==========

to

==========
Replace --add-to-shelf flag with kAppBanners feature.

The add to shelf flag was poorly named and doesn't use the new feature
goodness.

With this change, functionality dependent on the kBookmarkApps flag is
also enabled if the kAppBanners flag is enabled.

BUG=734897
==========

[benwells, 2017-06-22 05:57:48]

https://codereview.chromium.org/2944283002/diff/60001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2944283002/diff/60001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1988: #if !defined(OS_ANDROID)
On 2017/06/22 01:40:38, Matt Giuca wrote:
> Why !defined(OS_ANDROID)?
> 
> You already have kOsDesktop. Is the compile-time check necessary?
> 
> (The difference is, without the compile-time check, this will show up in
> Android's flags as "not available on this platform" which is preferable to it
> being confusingly missing.)

As discussed it's needed as the feature doesn't exist on Android, so otherwise
the FEATURE_VALUE_TYPE bit won't compile.

https://codereview.chromium.org/2944283002/diff/60001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2944283002/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:270: "Enable the display of add to shelf
banners, which prompt a user to add "
On 2017/06/22 01:40:38, Matt Giuca wrote:
> s/add to shelf/app/ since that's a CrOS-specific term. Or "Progressive Web App
> banners" perhaps clearer under the new terminology.

Done.

https://codereview.chromium.org/2944283002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (left):

https://codereview.chromium.org/2944283002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:22301: -  <int value="-1016202433"
label="disable-add-to-shelf"/>
On 2017/06/22 01:40:38, Matt Giuca wrote:
> Don't delete enums.
> 
> Add " (deprecated)" to the end of the name (seems to be the convention).

Done.

[Matt Giuca, 2017-06-22 06:51:13]

lgtm with histograms nit (please add someone from histograms to answer).

https://codereview.chromium.org/2944283002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2944283002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/enums.xml:22968: +  <int value="1350179532"
label="enable-add-to-shelf (deprecated)"/>
Ugh if you were forced to change the value of the int then this isn't going to
have an effect (the whole point was to keep tracking the old name).

I'm not fussed but I want to do it properly. Let's ask someone from histograms
team whether to:
a) Just leave the old names as-is,
b) Delete them,
c) Something else.

[benwells, 2017-06-22 06:55:03]

benwells@chromium.org changed reviewers:
+ isherman@chromium.org, thestig@chromium.org

[benwells, 2017-06-22 06:55:04]

+isherman to answer question about what to do with old flag
+thestig for little chrome/renderer change

[Lei Zhang, 2017-06-22 07:21:04]

thestig@chromium.org changed reviewers:
+ chaopeng@chromium.org

[Lei Zhang, 2017-06-22 07:21:06]

+chaopeng to look over the ScopedFeatureList usage.

https://codereview.chromium.org/2944283002/diff/120001/chrome/renderer/web_ap...
File chrome/renderer/web_apps.cc (right):

https://codereview.chromium.org/2944283002/diff/120001/chrome/renderer/web_ap...
chrome/renderer/web_apps.cc:153: #if defined(OS_MACOSX)
Side question 2: This looks just like IsNewBookmarkAppsEnabled(). Did we ever
discuss moving that to chrome/common?

https://codereview.chromium.org/2944283002/diff/120001/chrome/renderer/web_ap...
chrome/renderer/web_apps.cc:154: bool bookmark_apps_enabled =
Side question 1: Can we move this out of the for-loop?

[benwells, 2017-06-22 07:34:10]

https://codereview.chromium.org/2944283002/diff/120001/chrome/renderer/web_ap...
File chrome/renderer/web_apps.cc (right):

https://codereview.chromium.org/2944283002/diff/120001/chrome/renderer/web_ap...
chrome/renderer/web_apps.cc:153: #if defined(OS_MACOSX)
On 2017/06/22 07:21:05, Lei Zhang wrote:
> Side question 2: This looks just like IsNewBookmarkAppsEnabled(). Did we ever
> discuss moving that to chrome/common?

Good question. Currently IsNewBookmarkAppsEnabled is in
c/b/extensions/extension_util. We could create
c/common/extensions/extension_util, or do you have a better place to put it?

[Lei Zhang, 2017-06-22 07:37:17]

On 2017/06/22 07:34:10, benwells wrote:
>
https://codereview.chromium.org/2944283002/diff/120001/chrome/renderer/web_ap...
> File chrome/renderer/web_apps.cc (right):
> 
>
https://codereview.chromium.org/2944283002/diff/120001/chrome/renderer/web_ap...
> chrome/renderer/web_apps.cc:153: #if defined(OS_MACOSX)
> On 2017/06/22 07:21:05, Lei Zhang wrote:
> > Side question 2: This looks just like IsNewBookmarkAppsEnabled(). Did we
ever
> > discuss moving that to chrome/common?
> 
> Good question. Currently IsNewBookmarkAppsEnabled is in
> c/b/extensions/extension_util. We could create
> c/common/extensions/extension_util, or do you have a better place to put it?

Your call, since you are in c/c/exensions/OWNERS. :)

[Ilya Sherman, 2017-06-22 16:29:28]

https://codereview.chromium.org/2944283002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2944283002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/enums.xml:22968: +  <int value="1350179532"
label="enable-add-to-shelf (deprecated)"/>
On 2017/06/22 06:51:12, Matt Giuca wrote:
> Ugh if you were forced to change the value of the int then this isn't going to
> have an effect (the whole point was to keep tracking the old name).
> 
> I'm not fussed but I want to do it properly. Let's ask someone from histograms
> team whether to:
> a) Just leave the old names as-is,
> b) Delete them,
> c) Something else.

Typically we leave the old name as is.  Having the " (deprecated)" annotation is
kind of nice though; so if you feel motivated to fix the test to, say, ignore
such an annotation when computing the appropriate number, I'm on board with
that.

[Ilya Sherman, 2017-06-22 16:36:22]

Incidentally...

https://codereview.chromium.org/2944283002/diff/120001/chrome/browser/install...
File chrome/browser/installable/installable_manager_browsertest.cc (right):

https://codereview.chromium.org/2944283002/diff/120001/chrome/browser/install...
chrome/browser/installable/installable_manager_browsertest.cc:185:
feature_list_.InitAndDisableFeature(features::kAppBanners);
I'm not 100% sure, but I think this might be running too late to have any
effect.  Did you verify that removing this line affects how the test runs?

[chaopeng, 2017-06-22 16:46:12]

On 2017/06/22 16:36:22, Ilya Sherman wrote:
> Incidentally...
> 
>
https://codereview.chromium.org/2944283002/diff/120001/chrome/browser/install...
> File chrome/browser/installable/installable_manager_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2944283002/diff/120001/chrome/browser/install...
> chrome/browser/installable/installable_manager_browsertest.cc:185:
> feature_list_.InitAndDisableFeature(features::kAppBanners);
> I'm not 100% sure, but I think this might be running too late to have any
> effect.  Did you verify that removing this line affects how the test runs?

I think maybe benwells should use ScopedFeatureList in SetUpCommandLine or use
command_line->AppendSwitchASCII(switches::kEnableFeatures, *) at this time. I
will move to SetUp later.

[benwells, 2017-06-23 02:08:52]

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

[commit-bot: I haz the power, 2017-06-23 02:09:12]

Dry run: CQ is trying da patch.

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

[benwells, 2017-06-23 02:09:27]

Removed '(deprecated)' strings and the change in web_apps.cc.

isherman - lgty?

https://codereview.chromium.org/2944283002/diff/120001/chrome/browser/install...
File chrome/browser/installable/installable_manager_browsertest.cc (right):

https://codereview.chromium.org/2944283002/diff/120001/chrome/browser/install...
chrome/browser/installable/installable_manager_browsertest.cc:185:
feature_list_.InitAndDisableFeature(features::kAppBanners);
On 2017/06/22 16:36:22, Ilya Sherman wrote:
> I'm not 100% sure, but I think this might be running too late to have any
> effect.  Did you verify that removing this line affects how the test runs?

There is no problem with this, it does affect the tests in the correct way. It
isn't very clear to do it via a feature though, I have another patch lined up
which does this same thing with a static ForTesting style method.

[commit-bot: I haz the power, 2017-06-23 02:22:54]

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

[commit-bot: I haz the power, 2017-06-23 02:22:55]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[benwells, 2017-06-23 22:34:29]

ping isherman, did you want another look?

[Ilya Sherman, 2017-06-26 18:52:27]

On 2017/06/23 22:34:29, benwells wrote:
> ping isherman, did you want another look?

Sorry, was out of the office on Friday.  LGTM, and thanks for your patience.

[benwells, 2017-06-26 22:11:18]

The CQ bit was checked by benwells@chromium.org

[benwells, 2017-06-26 22:11:18]

The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org
Link to the patchset: https://codereview.chromium.org/2944283002/#ps160001
(title: "Rebase and remove web_apps change")

[commit-bot: I haz the power, 2017-06-26 22:11:40]

CQ is trying da patch.

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

[Lei Zhang, 2017-06-26 22:23:34]

On 2017/06/22 16:46:12, chaopeng wrote:
> On 2017/06/22 16:36:22, Ilya Sherman wrote:
> > Incidentally...
> > 
> >
>
https://codereview.chromium.org/2944283002/diff/120001/chrome/browser/install...
> > File chrome/browser/installable/installable_manager_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2944283002/diff/120001/chrome/browser/install...
> > chrome/browser/installable/installable_manager_browsertest.cc:185:
> > feature_list_.InitAndDisableFeature(features::kAppBanners);
> > I'm not 100% sure, but I think this might be running too late to have any
> > effect.  Did you verify that removing this line affects how the test runs?
> 
> I think maybe benwells should use ScopedFeatureList in SetUpCommandLine or use
> command_line->AppendSwitchASCII(switches::kEnableFeatures, *) at this time. I
> will move to SetUp later.

chaopeng: Ben says the current approach works. Are you ok with the
*browsertest.cc changes in the latest patch set, or do you strongly prefer one
of your suggestions? If you have a strong preference, which of your 2
suggestions do you prefer?

[benwells, 2017-06-26 22:30:00]

The CQ bit was unchecked by benwells@chromium.org

[benwells, 2017-06-26 22:34:09]

On 2017/06/26 22:23:34, Lei Zhang wrote:
> On 2017/06/22 16:46:12, chaopeng wrote:
> > On 2017/06/22 16:36:22, Ilya Sherman wrote:
> > > Incidentally...
> > > 
> > >
> >
>
https://codereview.chromium.org/2944283002/diff/120001/chrome/browser/install...
> > > File chrome/browser/installable/installable_manager_browsertest.cc
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2944283002/diff/120001/chrome/browser/install...
> > > chrome/browser/installable/installable_manager_browsertest.cc:185:
> > > feature_list_.InitAndDisableFeature(features::kAppBanners);
> > > I'm not 100% sure, but I think this might be running too late to have any
> > > effect.  Did you verify that removing this line affects how the test runs?
> > 
> > I think maybe benwells should use ScopedFeatureList in SetUpCommandLine or
use
> > command_line->AppendSwitchASCII(switches::kEnableFeatures, *) at this time.
I
> > will move to SetUp later.
> 
> chaopeng: Ben says the current approach works. Are you ok with the
> *browsertest.cc changes in the latest patch set, or do you strongly prefer one
> of your suggestions? If you have a strong preference, which of your 2
> suggestions do you prefer?

Oops, sorry I forgot to wait for this. Unchecked box to give chaopeng@ a chance
to wait for this.

To explain why it works as is - disabling the feature in this way does not
prevent an AppBannerManager being created[1] as the feature is initialized too
late, but it stops it interfering[2].

1:
https://cs.chromium.org/chromium/src/chrome/browser/ui/tab_helpers.cc?rcl=ec5...
2:
https://cs.chromium.org/chromium/src/chrome/browser/banners/app_banner_manage...
and
https://cs.chromium.org/chromium/src/chrome/browser/banners/app_banner_manage...

Let me know if that needs more explanation.

I do think it is unclear to use a feature in this way and will change it in a
follow up CL.

[chaopeng, 2017-06-27 21:12:27]

lgtm

[Lei Zhang, 2017-06-27 21:14:15]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2017-06-27 21:14:15]

lgtm

[commit-bot: I haz the power, 2017-06-27 21:14:40]

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-06-27 21:22:41]

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

[commit-bot: I haz the power, 2017-06-27 21:22:43]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[benwells, 2017-06-28 04:05:08]

The CQ bit was checked by benwells@chromium.org

[benwells, 2017-06-28 04:05:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, chaopeng@chromium.org, isherman@chromium.org,
mgiuca@chromium.org
Link to the patchset: https://codereview.chromium.org/2944283002/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2017-06-28 04:05:21]

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-06-28 05:10:34]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1498622708257450,
"parent_rev": "bb019e3460527a01f3c8a34f8819001755c949f5", "commit_rev":
"c421ccdbdd2c25b9d5d83eab1d8902aacb0d00ab"}

[commit-bot: I haz the power, 2017-06-28 05:10:48]

Description was changed from

==========
Replace --add-to-shelf flag with kAppBanners feature.

The add to shelf flag was poorly named and doesn't use the new feature
goodness.

With this change, functionality dependent on the kBookmarkApps flag is
also enabled if the kAppBanners flag is enabled.

BUG=734897
==========

to

==========
Replace --add-to-shelf flag with kAppBanners feature.

The add to shelf flag was poorly named and doesn't use the new feature
goodness.

With this change, functionality dependent on the kBookmarkApps flag is
also enabled if the kAppBanners flag is enabled.

BUG=734897

Review-Url: https://codereview.chromium.org/2944283002
Cr-Commit-Position: refs/heads/master@{#482889}
Committed:
https://chromium.googlesource.com/chromium/src/+/c421ccdbdd2c25b9d5d83eab1d89...
==========

[commit-bot: I haz the power, 2017-06-28 05:10:50]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/c421ccdbdd2c25b9d5d83eab1d89...