Add Now setting and migration logic.

Add Now setting and migration logic.

BUG=408152
Committed: https://crrev.com/f32ba96e5c273dfed76680c25752e4af7189ad6a
Cr-Commit-Position: refs/heads/master@{#317387}

[skare_, 2015-02-19 17:33:15]

skare@chromium.org changed reviewers:
+ robliao@chromium.org

[robliao, 2015-02-19 18:43:25]

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1255: * Controls the
visibility of all the hotword sections.
hotword -> Google Now section. Here and below.

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/google_now_extension.cc (right):

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/google_now_extension.cc:16: // Extension ID for
previous Now notifications component.
Since there is more than one item in this namespace, we can go ahead and add an
extra linebreak above here.

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/google_now_extension.cc:30: if
(GetGoogleNowExtensionId(&now_extension_id) == false) {
Replace == false with !GetGoogleNowExtensionId

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/google_now_extension.h (right):

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/google_now_extension.h:16: // Migrates settings from
the now component to the launcher page extension.
Let's call the now component the Now Notifications extension.

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1707: std::string
group = base::FieldTrialList::FindFullName("GoogleNowLauncher");
Turn "GoogleNowLauncher" and "Disabled" into a const char [] (same as in
component_loader.cc)

https://codereview.chromium.org/938713004/diff/40001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/938713004/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.cc:1671: // Note: this is not the notifications
component gated by ENABLE_GOOGLE_NOW.
Nit: Capitalize "this"

[skare_, 2015-02-19 20:19:29]

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1255: * Controls the
visibility of all the hotword sections.
On 2015/02/19 18:43:25, robliao wrote:
> hotword -> Google Now section. Here and below.

Done.

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/google_now_extension.cc (right):

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/google_now_extension.cc:16: // Extension ID for
previous Now notifications component.
On 2015/02/19 18:43:25, robliao wrote:
> Since there is more than one item in this namespace, we can go ahead and add
an
> extra linebreak above here.

Done.

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/google_now_extension.cc:30: if
(GetGoogleNowExtensionId(&now_extension_id) == false) {
On 2015/02/19 18:43:25, robliao wrote:
> Replace == false with !GetGoogleNowExtensionId

Done.

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/google_now_extension.h (right):

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/google_now_extension.h:16: // Migrates settings from
the now component to the launcher page extension.
On 2015/02/19 18:43:25, robliao wrote:
> Let's call the now component the Now Notifications extension.

Done.

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/938713004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1707: std::string
group = base::FieldTrialList::FindFullName("GoogleNowLauncher");
On 2015/02/19 18:43:25, robliao wrote:
> Turn "GoogleNowLauncher" and "Disabled" into a const char [] (same as in
> component_loader.cc)

VoiceTrigger (below) is using this format for its trial name and "Disabled." and
the file in general doesn't seem to use const char[]s.  I am ok switching if you
want though, let me know and reopen.

https://codereview.chromium.org/938713004/diff/40001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/938713004/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.cc:1671: // Note: this is not the notifications
component gated by ENABLE_GOOGLE_NOW.
On 2015/02/19 18:43:25, robliao wrote:
> Nit: Capitalize "this"

Done.

[robliao, 2015-02-19 20:25:50]

lgtm

[skare_, 2015-02-19 20:39:34]

skare@chromium.org changed reviewers:
+ dbeam@chromium.org, mgiuca@chromium.org

[skare_, 2015-02-19 20:44:24]

skare@chromium.org changed reviewers:
+ gab@chromium.org, noms@chromium.org

[skare_, 2015-02-19 20:44:41]

+OWNERS:
dbeam: chrome/browser (ui/webui, resources)
mgiuca: browser/ui/app_list/google_now_extension.{cc,h}
gab: browser/prefs/browser_prefs.cc
noms: browser/profiles/profile.cc
thanks!

[Matt Giuca, 2015-02-20 01:40:30]

app_list lgtm, assuming the assumption I made in my comment is actually the
case!

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/google_now_extension.cc (right):

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/google_now_extension.cc:31: if
(!GetGoogleNowExtensionId(&now_extension_id)) {
nit: No curly braces (one-line condition and one-line body).

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/google_now_extension.cc:40: if
(!enabled_pref->IsDefaultValue()) {
Nit: No curly braces.

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/google_now_extension.cc:40: if
(!enabled_pref->IsDefaultValue()) {
Can I just clarify the meaning of IsDefaultValue()? The documentation is a
little unclear.

Does this mean:
a) "the pref has not been explicitly set; it is still on its default setting",
or
b) "the pref is currently set to the value equal to its default value".

That is, if I explicitly set the pref to true, in (a), IsDefaultValue() returns
false, but in (b), IsDefaultValue() returns true because true is the default.

We do not want to migrate whenever this pref is true. We only want to migrate if
we have never set this pref before. So I will assume that (a) is the case.
Please let me know if (b) is the case, because then there is a problem!

(And perhaps this method should be called "HasBeenSet" instead of
"IsDefaultValue"!)

[Dan Beam, 2015-02-20 03:19:40]

lgtm

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1708: bool
has_field_trial = (!group.empty() && group != "Disabled");
nit: remove extra parens

[gab, 2015-02-20 15:11:22]

nit and meta-comment below, prefs/ lgtm otherwise

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:65: #include
"chrome/browser/ui/app_list/google_now_extension.h"
Make include use same ifdefs as the only code needing it.

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/google_now_extension.cc (right):

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/google_now_extension.cc:49:
prefs->SetBoolean(prefs::kGoogleNowLauncherEnabled, notifier_enabled);
User prefs should typically reflect explicit user decision, i.e. this is why we
strike a difference between IsDefaultValue() and the user value being the same
as the current default (yes Matt (a.) is correct from your previous question).

This allows us to change the default in a later release and affect only users
that haven't made an explicit choice already.

In this case looks like you're using the IsDefaultValue() check to know if
you've set it and if not unconditionally set it which is counter to this
principle.

I'm not going to block for this if you think this is correct for your feature
but just letting you know that this is slightly abusing the API and will prevent
you from changing the default value later.

[skare_, 2015-02-20 17:54:08]

New patchsets have been uploaded after l-g-t-m from
robliao@chromium.org,mgiuca@chromium.org,dbeam@chromium.org,gab@chromium.org

[skare_, 2015-02-20 17:54:49]

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/google_now_extension.cc (right):

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/google_now_extension.cc:31: if
(!GetGoogleNowExtensionId(&now_extension_id)) {
On 2015/02/20 01:40:30, Matt Giuca wrote:
> nit: No curly braces (one-line condition and one-line body).

Done.

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/google_now_extension.cc:40: if
(!enabled_pref->IsDefaultValue()) {
On 2015/02/20 01:40:30, Matt Giuca wrote:
> Nit: No curly braces.

Done.

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/google_now_extension.cc:40: if
(!enabled_pref->IsDefaultValue()) {
On 2015/02/20 01:40:30, Matt Giuca wrote:
> Can I just clarify the meaning of IsDefaultValue()? The documentation is a
> little unclear.
> 
> Does this mean:
> a) "the pref has not been explicitly set; it is still on its default setting",
> or
> b) "the pref is currently set to the value equal to its default value".
> 
> That is, if I explicitly set the pref to true, in (a), IsDefaultValue()
returns
> false, but in (b), IsDefaultValue() returns true because true is the default.
> 
> We do not want to migrate whenever this pref is true. We only want to migrate
if
> we have never set this pref before. So I will assume that (a) is the case.
> Please let me know if (b) is the case, because then there is a problem!
> 
> (And perhaps this method should be called "HasBeenSet" instead of
> "IsDefaultValue"!)

yes, it's akin to a) the pref has not been explicitly set.

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/google_now_extension.cc:49:
prefs->SetBoolean(prefs::kGoogleNowLauncherEnabled, notifier_enabled);
On 2015/02/20 15:11:21, gab wrote:
> User prefs should typically reflect explicit user decision, i.e. this is why
we
> strike a difference between IsDefaultValue() and the user value being the same
> as the current default (yes Matt (a.) is correct from your previous question).
> 
> This allows us to change the default in a later release and affect only users
> that haven't made an explicit choice already.
> 
> In this case looks like you're using the IsDefaultValue() check to know if
> you've set it and if not unconditionally set it which is counter to this
> principle.
> 
> I'm not going to block for this if you think this is correct for your feature
> but just letting you know that this is slightly abusing the API and will
prevent
> you from changing the default value later.

thanks for verifying Matt's question and the detailed explanation. Yes, this is
the intent despite making the setting explicit on migration. Verified with
kuscher/PM (cced on review) to use this approach similar to a couple other
migration paths (like autofill). We could introduce a new nowHasBeenMigrated
pref if anyone has second thoughts.

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1708: bool
has_field_trial = (!group.empty() && group != "Disabled");
On 2015/02/20 03:19:40, Dan Beam wrote:
> nit: remove extra parens

Done.

[noms (inactive), 2015-02-20 18:25:08]

profiles lgtm % nits

https://codereview.chromium.org/938713004/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/938713004/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile.cc:117: // TODO(skare): Remove or rename
ENABLE_GOOGLE_NOW: http://crbug.com/459827
nit: i think this comment should end in a . Also, could you maybe add an extra
sentence as to why this pref is intentionally outside the ifdef?

[skare_, 2015-02-20 18:33:36]

New patchsets have been uploaded after l-g-t-m from noms@chromium.org

[skare_, 2015-02-20 18:34:41]

https://codereview.chromium.org/938713004/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/938713004/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile.cc:117: // TODO(skare): Remove or rename
ENABLE_GOOGLE_NOW: http://crbug.com/459827
On 2015/02/20 18:25:08, Monica Dinculescu wrote:
> nit: i think this comment should end in a . Also, could you maybe add an extra
> sentence as to why this pref is intentionally outside the ifdef?

added a . after the URL. Clarified comment. Thank you!

[skare_, 2015-02-20 18:35:36]

The CQ bit was checked by skare@chromium.org

[commit-bot: I haz the power, 2015-02-20 18:37:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/938713004/100001

[gab, 2015-02-20 19:05:48]

The CQ bit was unchecked by gab@chromium.org

[gab, 2015-02-20 19:05:59]

still lgtm, ping on nit

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:65: #include
"chrome/browser/ui/app_list/google_now_extension.h"
On 2015/02/20 15:11:21, gab wrote:
> Make include use same ifdefs as the only code needing it.

ping.

[skare_, 2015-02-20 19:35:02]

New patchsets have been uploaded after l-g-t-m from gab@chromium.org

[skare_, 2015-02-20 19:37:13]

Patchset #7 (id:120001) has been deleted

[skare_, 2015-02-20 19:37:44]

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/938713004/diff/60001/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:65: #include
"chrome/browser/ui/app_list/google_now_extension.h"
On 2015/02/20 19:05:58, gab wrote:
> On 2015/02/20 15:11:21, gab wrote:
> > Make include use same ifdefs as the only code needing it.
> 
> ping.

apologies, missed this one

[skare_, 2015-02-20 19:39:12]

The CQ bit was checked by skare@chromium.org

[commit-bot: I haz the power, 2015-02-20 19:40:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/938713004/140001

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

Committed patchset #7 (id:140001)

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

Patchset 7 (id:??) landed as
https://crrev.com/f32ba96e5c273dfed76680c25752e4af7189ad6a
Cr-Commit-Position: refs/heads/master@{#317387}