[Android] Add enhanced bookmark opt-in flag.

[Android] Add enhanced bookmark opt-in flag.

On Android, we provide opt-in flag for enhanced bookmark.
This is consistent with ios.

BUG=386785
Committed: https://crrev.com/948ce92d41501beacc70936f6b9104d51844ecca
Cr-Commit-Position: refs/heads/master@{#293900}

[Kibeom Kim (inactive), 2014-09-04 18:32:55]

kkimlabs@chromium.org changed reviewers:
+ sky@chromium.org, thakis@chromium.org, yfriedman@chromium.org

[Nico, 2014-09-04 18:35:25]

https://codereview.chromium.org/517713002/diff/50006/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/517713002/diff/50006/chrome/browser/about_fla...
chrome/browser/about_flags.cc:2019: return
!IsEnhancedBookmarksExperimentEnabled();
Why is this code needed?

[Kibeom Kim (inactive), 2014-09-04 18:40:49]

https://codereview.chromium.org/517713002/diff/50006/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/517713002/diff/50006/chrome/browser/about_fla...
chrome/browser/about_flags.cc:2019: return
!IsEnhancedBookmarksExperimentEnabled();
On 2014/09/04 18:35:25, Nico (hiding) wrote:
> Why is this code needed?

For desktop, we enable the experiment by chrome sync or finch (which is turned
on by default in that case), and only they can enable the feature or opt-out. So
there is no reason to show the flag to the non-experiment group.

[Nico, 2014-09-04 18:42:29]

https://codereview.chromium.org/517713002/diff/50006/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/517713002/diff/50006/chrome/browser/about_fla...
chrome/browser/about_flags.cc:2019: return
!IsEnhancedBookmarksExperimentEnabled();
On 2014/09/04 18:40:49, Kibeom Kim wrote:
> On 2014/09/04 18:35:25, Nico (hiding) wrote:
> > Why is this code needed?
> 
> For desktop, we enable the experiment by chrome sync or finch (which is turned
> on by default in that case), and only they can enable the feature or opt-out.
So
> there is no reason to show the flag to the non-experiment group.

The way this is  usually done is to have a "Enable", "Disable", "Default"
picker, with Default delegating to finch and Enable and Disable enabling and
disabling. Why can't that work here?

[Kibeom Kim (inactive), 2014-09-04 18:47:55]

On 2014/09/04 18:42:29, Nico (hiding) wrote:
>
https://codereview.chromium.org/517713002/diff/50006/chrome/browser/about_fla...
> File chrome/browser/about_flags.cc (right):
> 
>
https://codereview.chromium.org/517713002/diff/50006/chrome/browser/about_fla...
> chrome/browser/about_flags.cc:2019: return
> !IsEnhancedBookmarksExperimentEnabled();
> On 2014/09/04 18:40:49, Kibeom Kim wrote:
> > On 2014/09/04 18:35:25, Nico (hiding) wrote:
> > > Why is this code needed?
> > 
> > For desktop, we enable the experiment by chrome sync or finch (which is
turned
> > on by default in that case), and only they can enable the feature or
opt-out.
> So
> > there is no reason to show the flag to the non-experiment group.
> 
> The way this is  usually done is to have a "Enable", "Disable", "Default"
> picker, with Default delegating to finch and Enable and Disable enabling and
> disabling. Why can't that work here?

Even if the flag is "Enable" we don't actually enable it if the user is not in
the experiment group. So user can only opt-out or re-enable using this flag, and
we're showing the flag only in those cases. (The enabling logic using this
flag&finch&sync is UpdateBookmarksExperimentState function in
enhanced_bookmarks_features.cc)

[Kibeom Kim (inactive), 2014-09-04 18:50:53]

On 2014/09/04 18:47:55, Kibeom Kim wrote:
> ...
> Even if the flag is "Enable" we don't actually enable it if the user is not in
> the experiment group. So user can only opt-out or re-enable using this flag,
and
> we're showing the flag only in those cases. (The enabling logic using this
> flag&finch&sync is UpdateBookmarksExperimentState function in
> enhanced_bookmarks_features.cc)

(The above behavior is for desktop. ios has opt-in option, and Android is
following that by this CL.)

[Nico, 2014-09-04 18:53:47]

On Thu, Sep 4, 2014 at 11:47 AM, <kkimlabs@chromium.org> wrote:

> On 2014/09/04 18:42:29, Nico (hiding) wrote:
>
> https://codereview.chromium.org/517713002/diff/50006/
> chrome/browser/about_flags.cc
>
>> File chrome/browser/about_flags.cc (right):
>>
>
>
> https://codereview.chromium.org/517713002/diff/50006/
> chrome/browser/about_flags.cc#newcode2019
>
>> chrome/browser/about_flags.cc:2019: return
>> !IsEnhancedBookmarksExperimentEnabled();
>> On 2014/09/04 18:40:49, Kibeom Kim wrote:
>> > On 2014/09/04 18:35:25, Nico (hiding) wrote:
>> > > Why is this code needed?
>> >
>> > For desktop, we enable the experiment by chrome sync or finch (which is
>>
> turned
>
>> > on by default in that case), and only they can enable the feature or
>>
> opt-out.
>
>> So
>> > there is no reason to show the flag to the non-experiment group.
>>
>
>  The way this is  usually done is to have a "Enable", "Disable", "Default"
>> picker, with Default delegating to finch and Enable and Disable enabling
>> and
>> disabling. Why can't that work here?
>>
>
> Even if the flag is "Enable" we don't actually enable it if the user is
> not in
> the experiment group.


Right. I'm asking "why?"


> So user can only opt-out or re-enable using this flag, and
> we're showing the flag only in those cases. (The enabling logic using this
> flag&finch&sync is UpdateBookmarksExperimentState function in
> enhanced_bookmarks_features.cc)
>
> https://codereview.chromium.org/517713002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Kibeom Kim (inactive), 2014-09-04 20:35:31]

On 2014/09/04 18:53:47, Nico (hiding) wrote:
> > ...
> 
> Right. I'm asking "why?"
> 
> 

+cc yefim, mcolbert for desktop

I wasn't involved in the desktop side, but I think it's because this feature was
developed internally and confidential (and still not public although we're
checking in some parts in the public repository). I don't know if there are
other reasons.

[Mark, 2014-09-04 21:23:52]

On 2014/09/04 20:35:31, Kibeom Kim wrote:
> On 2014/09/04 18:53:47, Nico (hiding) wrote:
> > > ...
> > 
> > Right. I'm asking "why?"
> > 
> > 
> 
> +cc yefim, mcolbert for desktop
> 
> I wasn't involved in the desktop side, but I think it's because this feature
was
> developed internally and confidential (and still not public although we're
> checking in some parts in the public repository). I don't know if there are
> other reasons.

Long story short, until Finch sends down the extension id, the desktop would not
do anything since everything lives in an external component extension, so having
this flag be enabled without the finch experiment is effectively a NOOP.  Now on
Clank and Bling, this is different.  The reason we only want a controlled
rollout (with a flag to opt-out) is that we are hitting new backend services
that we need to ensure perform gracefully with the new load.  Allowing for an
opt-in launch could result in our services possibly being over taxed and under
provisioned, resulting in fails left and right.  So this flag should only exist
as a method to opt-out of Stars.  Does that make sense?

[Yaron, 2014-09-05 04:48:44]

https://codereview.chromium.org/517713002/diff/50006/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main_android.cc (right):

https://codereview.chromium.org/517713002/diff/50006/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_android.cc:62: SigninManagerBase* signin =
SigninManagerFactory::GetForProfile(main_profile);
Please pull this out from here and external_component_loader and define a
function in enhanced_bookmarks_features (e.g. MaybeInitEnhancedBookmarks or
similar)

[Kibeom Kim (inactive), 2014-09-08 09:26:59]

https://codereview.chromium.org/517713002/diff/50006/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main_android.cc (right):

https://codereview.chromium.org/517713002/diff/50006/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_android.cc:62: SigninManagerBase* signin =
SigninManagerFactory::GetForProfile(main_profile);
On 2014/09/05 04:48:43, Yaron wrote:
> Please pull this out from here and external_component_loader and define a
> function in enhanced_bookmarks_features (e.g. MaybeInitEnhancedBookmarks or
> similar)

Done. But I had to increase chrome/browser/bookmarks/DEPS list

[Yaron, 2014-09-08 23:19:58]

lgtm

https://codereview.chromium.org/517713002/diff/160001/chrome/browser/bookmark...
File chrome/browser/bookmarks/enhanced_bookmarks_features.h (right):

https://codereview.chromium.org/517713002/diff/160001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.h:35: // , Finch
experiments, and command line flag.
leading "," on previous line

https://codereview.chromium.org/517713002/diff/160001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.h:43: //
BOOKMARKS_EXPERIMENT_ENABLED_FROM_SYNC_UNKNOWN.
You could add: "Intended for performing initial configuration of bookmarks
experiments when the browser is first initialized."

[sky, 2014-09-08 23:55:02]

LGTM

[Kibeom Kim (inactive), 2014-09-09 00:32:22]

https://codereview.chromium.org/517713002/diff/160001/chrome/browser/bookmark...
File chrome/browser/bookmarks/enhanced_bookmarks_features.h (right):

https://codereview.chromium.org/517713002/diff/160001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.h:35: // , Finch
experiments, and command line flag.
On 2014/09/08 23:19:58, Yaron wrote:
> leading "," on previous line

Done.

https://codereview.chromium.org/517713002/diff/160001/chrome/browser/bookmark...
chrome/browser/bookmarks/enhanced_bookmarks_features.h:43: //
BOOKMARKS_EXPERIMENT_ENABLED_FROM_SYNC_UNKNOWN.
On 2014/09/08 23:19:58, Yaron wrote:
> You could add: "Intended for performing initial configuration of bookmarks
> experiments when the browser is first initialized."

Done.

[Kibeom Kim (inactive), 2014-09-09 00:32:25]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2014-09-09 00:34:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/517713002/180001

[commit-bot: I haz the power, 2014-09-09 06:51:30]

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

[commit-bot: I haz the power, 2014-09-09 06:51:31]

Exceeded time limit waiting for builds to trigger.

[Kibeom Kim (inactive), 2014-09-09 07:08:42]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2014-09-09 07:12:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/517713002/180001

[commit-bot: I haz the power, 2014-09-09 07:57:06]

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

[commit-bot: I haz the power, 2014-09-09 07:57:07]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Kibeom Kim (inactive), 2014-09-09 07:58:53]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2014-09-09 07:59:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/517713002/200001

[commit-bot: I haz the power, 2014-09-09 09:08:50]

Committed patchset #12 (id:200001) as a64a0a976213cfc057f97a125b8a0277ccb5161a

[commit-bot: I haz the power, 2014-09-10 03:52:09]

Patchset 12 (id:??) landed as
https://crrev.com/948ce92d41501beacc70936f6b9104d51844ecca
Cr-Commit-Position: refs/heads/master@{#293900}