Enable lucid sleep (wake on wi-fi SSID) and add to Privacy options.

Enable lucid sleep (wake on wi-fi SSID) and add to Privacy options.

* Change the wake on wifi pref to a boolean. Eventually we will need to worry about packets, but for now having an integer pref makes things too complicated, partially because if/when we do add the packets feature, we don't want to overwrite user settings.
* Make the WakeOnWifiFeatures enum bitfield-like for easier setting of the options now and in the future.
* Set the pref to enabled by default.
* Add the --disable-wake-on-wifi kill switch to WakeOnWifiManager.
* --wake-on-packet still works unless --disable-wake-on-wifi is also specified.

BUG=424719
R=stevenjb@chromium.org,asvitkine@chromium.org
CC=dbeam@chromium.org

TBR=asvitkine@chromium.org
# asvitkine OOO, but this complies with his past reviews

Committed: https://crrev.com/592b07ae355027d6128b4b2cb2c603e3159787bf
Cr-Commit-Position: refs/heads/master@{#309576}

[michaelpg, 2014-12-17 02:27:32]

Patchset #1 (id:1) has been deleted

[michaelpg, 2014-12-17 02:30:54]

steven - feel free to pre-review pending UI decision whether to include this
checkbox.

I've added notes to the particular spots that you might want nixed.

https://codereview.chromium.org/811973002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/wake_on_wifi_manager.h (right):

https://codereview.chromium.org/811973002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:25: enum WakeOnWifiFeature {
specify in octal to show we want to use bitwise operations

https://codereview.chromium.org/811973002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/811973002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.cc:612: features |=
WakeOnWifiManager::WAKE_ON_PACKET;
bitwise operations make life easier and will continue to do so if we add a
separate checkbox for wake on packet

https://codereview.chromium.org/811973002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

https://codereview.chromium.org/811973002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:188:
base::Value* value = CreateValueForPref(pref_name, controlling_pref);
maybe this isn't the best way/place to do this. but i'm ok with it. the goal is
to indicate and disable prefs that are controlled by the primary user.

[michaelpg, 2014-12-17 21:59:24]

Patchset #3 (id:60001) has been deleted

[stevenjb, 2014-12-17 22:22:55]

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/wake_on_wifi_manager.cc (right):

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:177: return;
In cases like this where we might be changing the polarity of this in a branch,
I would add a chromeos::switches::WakeOnWifiEnabled() method in
chromeos_switches.cc/h and use that.

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.cc:282:
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
We probably shouldn't even register the preference if DISABLE_WAKE_ON_WIFI is
defined?

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(left):

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:308:
if (user && user->email() != user_manager->GetPrimaryUser()->email()) {
Ah, I see this was just moved, so go ahead and leave it, but maybe add a comment
like TODO: compare User*s or add User::Compare method.

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:63:
return std::find(kPrimaryUserSettings, end, pref) != end;
Ugh, I was going to bitch about this pattern (I think a simple for loop would be
more clear than std::find here), then noticed that we already use it above, so
nevermind.

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:106:
return user && user->email() != user_manager->GetPrimaryUser()->email();
Any reason to compare email() instead of User*s, i.e.
ProfileHelper::Get()->GetUserByProfile(profile) !=
user_manager::UserManager::Get()->GetPrimaryUser() ?

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:191:
value->GetAsDictionary(&dict) && !dict->HasKey("controlledBy")) {
if (!IsSettingShared(pref_name) || !IsSecondaryUser(profile))
  return value;
base::DictionaryValue* dict;
if (!value->GetAsDictionary(&dict) || dict->HasKey("controlledBy")
  return value;

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:194:
if (primary_profile) {
early exit here too

[michaelpg, 2014-12-22 22:23:19]

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/wake_on_wifi_manager.cc (right):

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:177: return;
On 2014/12/17 22:22:54, stevenjb wrote:
> In cases like this where we might be changing the polarity of this in a
branch,
> I would add a chromeos::switches::WakeOnWifiEnabled() method in
> chromeos_switches.cc/h and use that.

I don't think we would change the polarity of it -- I thought we would just send
an update to include this flag to kill the feature rather than have to push a
binary update. or is that not something we're capable of and the default flags
are baked into the build?

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.cc:282:
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
On 2014/12/17 22:22:54, stevenjb wrote:
> We probably shouldn't even register the preference if DISABLE_WAKE_ON_WIFI is
> defined?

(no longer relevant)

The <input type="checkbox" pref="settings.wake_on_wifi_ssid"> gives us a lot for
free, but will fail a DCHECK if the pref hasn't been registered.

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:106:
return user && user->email() != user_manager->GetPrimaryUser()->email();
On 2014/12/17 22:22:55, stevenjb wrote:
> Any reason to compare email() instead of User*s, i.e.
> ProfileHelper::Get()->GetUserByProfile(profile) !=
> user_manager::UserManager::Get()->GetPrimaryUser() ?

I asked skuhne why we do this but he wasn't sure. Presumably nobody is creating
duplicate User's.

I've found the first instance of this GetPrimaryUser()->email() comparison
business: https://codereview.chromium.org/152143009#msg9. I added it on
dpolukhin's suggestion.

I'll start a thread offline to discuss it.

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:191:
value->GetAsDictionary(&dict) && !dict->HasKey("controlledBy")) {
On 2014/12/17 22:22:55, stevenjb wrote:
> if (!IsSettingShared(pref_name) || !IsSecondaryUser(profile))
>   return value;
> base::DictionaryValue* dict;
> if (!value->GetAsDictionary(&dict) || dict->HasKey("controlledBy")
>   return value;

Done.

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:194:
if (primary_profile) {
On 2014/12/17 22:22:55, stevenjb wrote:
> early exit here too

Done.

[stevenjb (google-dont-use), 2014-12-22 23:30:22]

stevenjb@google.com changed reviewers:
+ stevenjb@google.com

[stevenjb (google-dont-use), 2014-12-22 23:30:23]

lgtm

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/wake_on_wifi_manager.cc (right):

https://codereview.chromium.org/811973002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:177: return;
On 2014/12/22 22:23:18, michaelpg wrote:
> On 2014/12/17 22:22:54, stevenjb wrote:
> > In cases like this where we might be changing the polarity of this in a
> branch,
> > I would add a chromeos::switches::WakeOnWifiEnabled() method in
> > chromeos_switches.cc/h and use that.
> 
> I don't think we would change the polarity of it -- I thought we would just
send
> an update to include this flag to kill the feature rather than have to push a
> binary update. or is that not something we're capable of and the default flags
> are baked into the build?

We generally just change the executable on the branch. As far as I am aware
adding a flag isn't any easier, unless we are using a Finch experiment to
selectively enable a feature (in which case this would need to be off by default
anyway). iirc we only use this flag a couple of times so it's not a big deal,
just a suggestion.

[Chirantan Ekbote, 2014-12-23 00:01:09]

> Change the wake on wifi pref to a boolean. Eventually we will need to worry
about packets, but for now having an integer pref makes things too complicated,
partially because if/when we do add the packets feature, we don't want to
overwrite user settings.


FWIW, I believe the plan is to turn on wake-on-packets by default very soon
after the branch point.

[michaelpg, 2014-12-23 03:05:47]

On 2014/12/23 00:01:09, Chirantan Ekbote wrote:
> > Change the wake on wifi pref to a boolean. Eventually we will need to worry
> about packets, but for now having an integer pref makes things too
complicated,
> partially because if/when we do add the packets feature, we don't want to
> overwrite user settings.
> 
> 
> FWIW, I believe the plan is to turn on wake-on-packets by default very soon
> after the branch point.

That's fine, I still think having separate prefs is easier to reason about and
more flexible.

[michaelpg, 2014-12-23 03:42:51]

On 2014/12/22 23:30:23, stevenjb (google-dont-use) wrote:
> lgtm
> 
>
https://codereview.chromium.org/811973002/diff/40001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/net/wake_on_wifi_manager.cc (right):
> 
>
https://codereview.chromium.org/811973002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/net/wake_on_wifi_manager.cc:177: return;
> On 2014/12/22 22:23:18, michaelpg wrote:
> > On 2014/12/17 22:22:54, stevenjb wrote:
> > > In cases like this where we might be changing the polarity of this in a
> > branch,
> > > I would add a chromeos::switches::WakeOnWifiEnabled() method in
> > > chromeos_switches.cc/h and use that.
> > 
> > I don't think we would change the polarity of it -- I thought we would just
> send
> > an update to include this flag to kill the feature rather than have to push
a
> > binary update. or is that not something we're capable of and the default
flags
> > are baked into the build?
> 
> We generally just change the executable on the branch. As far as I am aware
> adding a flag isn't any easier, unless we are using a Finch experiment to
> selectively enable a feature (in which case this would need to be off by
default
> anyway). iirc we only use this flag a couple of times so it's not a big deal,
> just a suggestion.

oops, you LGTM'd with your Google account.

[michaelpg, 2014-12-23 03:44:23]

Patchset #6 (id:140001) has been deleted

[michaelpg, 2014-12-23 03:47:03]

Patchset #6 (id:160001) has been deleted

[stevenjb, 2014-12-23 19:27:01]

lgtm

(You can always use TBR to appease presbmit if that happens)

[michaelpg, 2014-12-23 19:29:21]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-12-23 19:29:42]

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

[commit-bot: I haz the power, 2014-12-23 19:46:40]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2014-12-23 19:48:29]

Patchset 6 (id:??) landed as
https://crrev.com/592b07ae355027d6128b4b2cb2c603e3159787bf
Cr-Commit-Position: refs/heads/master@{#309576}

[Alexei Svitkine (slow), 2015-01-05 16:38:47]

lgtm