Control easy unlock by a field trial.

Control easy unlock by a field trial.

BUG=395154
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285707

[Alexei Svitkine (slow), 2014-07-22 18:18:41]

https://codereview.chromium.org/410713002/diff/1/chrome/browser/extensions/co...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/410713002/diff/1/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.cc:599:
command_line->GetSwitchValuePath(switches::kEasyUnlockAppPath));
Hmm, what happens if its enabled via field trial but no path is specified on
non-cros?

https://codereview.chromium.org/410713002/diff/1/chrome/browser/signin/easy_u...
File chrome/browser/signin/easy_unlock.cc (right):

https://codereview.chromium.org/410713002/diff/1/chrome/browser/signin/easy_u...
chrome/browser/signin/easy_unlock.cc:48: switches::kEnableEasyUnlock)) {
Please follow the pattern at the bottom of go/finch-and-flags.

[Tim Song, 2014-07-22 18:36:09]

LGTM

The plan is to launch for M-38, which will involve removing the flag. Should
this field trial also be removed, or is it still useful for controlling a
gradual roll out?

https://codereview.chromium.org/410713002/diff/1/chrome/browser/extensions/co...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/410713002/diff/1/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.cc:599:
command_line->GetSwitchValuePath(switches::kEasyUnlockAppPath));
On 2014/07/22 18:18:40, Alexei Svitkine wrote:
> Hmm, what happens if its enabled via field trial but no path is specified on
> non-cros?

No component app will be installed in this case.

[xiyuan, 2014-07-22 19:53:30]

On 2014/07/22 18:36:09, Tim Song wrote:
> LGTM
> 
> The plan is to launch for M-38, which will involve removing the flag. Should
> this field trial also be removed, or is it still useful for controlling a
> gradual roll out?
> 

We want to keep this finch config after removing the flag. The purpose of having
this is to decouple the feature dependency on Android release schedule.

[xiyuan, 2014-07-22 20:00:24]

https://codereview.chromium.org/410713002/diff/1/chrome/browser/extensions/co...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/410713002/diff/1/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.cc:599:
command_line->GetSwitchValuePath(switches::kEasyUnlockAppPath));
On 2014/07/22 18:36:09, Tim Song wrote:
> On 2014/07/22 18:18:40, Alexei Svitkine wrote:
> > Hmm, what happens if its enabled via field trial but no path is specified on
> > non-cros?
> 
> No component app will be installed in this case.

The non-cros release plan is still unclear ATM. Cros is the current interesting
platform. The code is in common part because there are WIP on win/mac.

https://codereview.chromium.org/410713002/diff/1/chrome/browser/signin/easy_u...
File chrome/browser/signin/easy_unlock.cc (right):

https://codereview.chromium.org/410713002/diff/1/chrome/browser/signin/easy_u...
chrome/browser/signin/easy_unlock.cc:48: switches::kEnableEasyUnlock)) {
On 2014/07/22 18:18:41, Alexei Svitkine wrote:
> Please follow the pattern at the bottom of go/finch-and-flags.

Done.

[Alexei Svitkine (slow), 2014-07-22 20:46:42]

lgtm

[xiyuan, 2014-07-23 16:07:33]

Owners, please bless the CL. Thanks.

asargent@:
  chrome/browser/extensions/component_loader.cc

atwilson@: Could you help since Roger is OOO?
  chrome/browser/signin/*

[asargent_no_longer_on_chrome, 2014-07-23 21:24:29]

lgtm

[Andrew T Wilson (Slow), 2014-07-24 09:50:26]

lgtm with one change

https://codereview.chromium.org/410713002/diff/20001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock.cc (right):

https://codereview.chromium.org/410713002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock.cc:48:
base::FieldTrialList::FindFullName("EasyUnlock");
Move this line down after the check for the command line switch.

[Alexei Svitkine (slow), 2014-07-24 14:10:26]

https://codereview.chromium.org/410713002/diff/20001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock.cc (right):

https://codereview.chromium.org/410713002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock.cc:48:
base::FieldTrialList::FindFullName("EasyUnlock");
On 2014/07/24 09:50:25, Andrew T Wilson wrote:
> Move this line down after the check for the command line switch.

That will not produce correct results per go/finch-and-flags. Please instead add
a comment that this has to be before the command-line check to activate the
trial.

[xiyuan, 2014-07-25 17:47:27]

https://codereview.chromium.org/410713002/diff/20001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock.cc (right):

https://codereview.chromium.org/410713002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock.cc:48:
base::FieldTrialList::FindFullName("EasyUnlock");
On 2014/07/24 14:10:26, Alexei Svitkine wrote:
> On 2014/07/24 09:50:25, Andrew T Wilson wrote:
> > Move this line down after the check for the command line switch.
> 
> That will not produce correct results per go/finch-and-flags. Please instead
add
> a comment that this has to be before the command-line check to activate the
> trial.

Done.

[xiyuan, 2014-07-25 17:48:03]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2014-07-25 17:49:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xiyuan@chromium.org/410713002/60001

[commit-bot: I haz the power, 2014-07-25 19:41:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)

[commit-bot: I haz the power, 2014-07-25 20:52:17]

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

[commit-bot: I haz the power, 2014-07-25 20:52:19]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[xiyuan, 2014-07-25 20:53:28]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2014-07-25 20:54:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xiyuan@chromium.org/410713002/60001

[commit-bot: I haz the power, 2014-07-25 22:12:05]

Change committed as 285707