Clean up wake on wifi handling

Clean up wake on wifi handling

Unify all the wake-on-wifi related code into one place.  Since the entire shill
API moved over into the Device interface, make the corresponding change on the
chrome side.  Finally, add a new preference that will be hooked up to the
settings UI and will be responsible for controlling shill's behavior.

BUG=424719
Committed: https://crrev.com/1c4090c9467c8ffcf01beec023686529cb8cd521
Cr-Commit-Position: refs/heads/master@{#304684}

[Chirantan Ekbote, 2014-11-13 02:50:42]

chirantan@chromium.org changed reviewers:
+ stevenjb@chromium.org, thestig@chromium.org, zea@chromium.org

[Chirantan Ekbote, 2014-11-13 02:50:43]

Hi, this is my first crack at cleaning up all the wake-on-wifi stuff in chrome
and providing a place for all future changes to go.  Please take a look.  Lei,
please review chrome_browser.gypi and chrome/common.  Nicolas, please review
gcm.  Steven, please review the chromeos bits.

Thanks :-)

[Chirantan Ekbote, 2014-11-13 02:51:39]

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

https://codereview.chromium.org/722043004/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:838: // A integer pref that controls the wake on
wifi features that should be
Should be "An integer".  Will fix in the next patch set.

[michaelpg, 2014-11-13 03:18:48]

Some questions around who actually controls the preference.

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/pref...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/pref...
chrome/browser/chromeos/preferences.cc:574: if (reason != REASON_PREF_CHANGED ||
Seems like we need to clarify how the machine setting/owner-controlled
preference works.

This CL makes kWakeOnWiFiEnabled a per-profile preference that only the owner
can set -- that's kind of weird. For example, when a non-owner signs in, the
initialization here will use the default value (e.g., PACKET_AND_SSID) because
the check on 575 means a non-owner can never set his preference. That doesn't
seem right, since we're not respecting the owner's preference OR the user's
preference.

Since this isn't a profile setting but a per-machine (owner) setting, we
probably don't want to add this preference to the user's pref store, but to the
local state. (That said, I'm not convinced this shouldn't be a per-user
decision, like the sleep settings on Windows.)

Would it make more sense to register the pref in WakeOnWifiManager so we don't
need to involve it in the Preferences class?

You could then observe this pref directly by using
WakeOnWifiManager::OnPreferenceChanged as the callback when initing the pref in
WakeOnWifiManager. You could still check that the user is the owner, but you
would be setting the local state preference, not the current user's preference.

The question then is what to do about a managed device (it has no owner). Stick
with the default? make this a policy setting? allow any user to change the
setting?

https://codereview.chromium.org/722043004/diff/1/chrome/chrome_browser_chrome...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/722043004/diff/1/chrome/chrome_browser_chrome...
chrome/chrome_browser_chromeos.gypi:660:
'browser/chromeos/net/wake_on_wifi_manager.cc',
alphabetize

https://codereview.chromium.org/722043004/diff/1/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/722043004/diff/1/chrome/common/pref_names.h#n...
chrome/common/pref_names.h:271: extern const char kWakeOnWiFiEnabled[];
If you decide to make this a local state pref, you'd add it in the local state
section (line 411).

[Nicolas Zea, 2014-11-13 21:23:39]

gcm lgtm

[Chirantan Ekbote, 2014-11-13 21:32:14]

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/pref...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/pref...
chrome/browser/chromeos/preferences.cc:574: if (reason != REASON_PREF_CHANGED ||
On 2014/11/13 03:18:48, michaelpg wrote:
> Seems like we need to clarify how the machine setting/owner-controlled
> preference works.
> 
> This CL makes kWakeOnWiFiEnabled a per-profile preference that only the owner
> can set -- that's kind of weird. For example, when a non-owner signs in, the
> initialization here will use the default value (e.g., PACKET_AND_SSID) because
> the check on 575 means a non-owner can never set his preference. That doesn't
> seem right, since we're not respecting the owner's preference OR the user's
> preference.
> 
> Since this isn't a profile setting but a per-machine (owner) setting, we
> probably don't want to add this preference to the user's pref store, but to
the
> local state. (That said, I'm not convinced this shouldn't be a per-user
> decision, like the sleep settings on Windows.)
> 

So I discussed this again with Sameer and I think you're right.  This makes more
sense as a per-user thing rather than an owner thing, especially considering the
managed device case.  I'm going to run it by drewry@, who was the one who had
initially asked for the flag to be owner only but I'm going to go ahead right
now with the assumption that making it per-user is ok.

> Would it make more sense to register the pref in WakeOnWifiManager so we don't
> need to involve it in the Preferences class?
> 

I'd like to do that as well but I was unsure about how to plumb it in so that
the WakeOnWifiManager will get access to the PrefRegistry and the PrefService
for the user.

[michaelpg, 2014-11-13 21:40:21]

On 2014/11/13 21:32:14, chirantan wrote:
>
https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/pref...
> File chrome/browser/chromeos/preferences.cc (right):
> 
>
https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/pref...
> chrome/browser/chromeos/preferences.cc:574: if (reason != REASON_PREF_CHANGED
||
> On 2014/11/13 03:18:48, michaelpg wrote:
> > Seems like we need to clarify how the machine setting/owner-controlled
> > preference works.
> > 
> > This CL makes kWakeOnWiFiEnabled a per-profile preference that only the
owner
> > can set -- that's kind of weird. For example, when a non-owner signs in, the
> > initialization here will use the default value (e.g., PACKET_AND_SSID)
because
> > the check on 575 means a non-owner can never set his preference. That
doesn't
> > seem right, since we're not respecting the owner's preference OR the user's
> > preference.
> > 
> > Since this isn't a profile setting but a per-machine (owner) setting, we
> > probably don't want to add this preference to the user's pref store, but to
> the
> > local state. (That said, I'm not convinced this shouldn't be a per-user
> > decision, like the sleep settings on Windows.)
> > 
> 
> So I discussed this again with Sameer and I think you're right.  This makes
more
> sense as a per-user thing rather than an owner thing, especially considering
the
> managed device case.  I'm going to run it by drewry@, who was the one who had
> initially asked for the flag to be owner only but I'm going to go ahead right
> now with the assumption that making it per-user is ok.
> 
> > Would it make more sense to register the pref in WakeOnWifiManager so we
don't
> > need to involve it in the Preferences class?
> > 
> 
> I'd like to do that as well but I was unsure about how to plumb it in so that
> the WakeOnWifiManager will get access to the PrefRegistry and the PrefService
> for the user.

If you make the pref per-user, keeping it in Preferences probably makes more
sense. I don't see a reason to make WakeOnWifiManager user-aware if the
WakeOnWiFiFeaturesEnabled state is global in shill. stevenjb@ might have a
different/similar opinion.

[stevenjb, 2014-11-13 22:20:17]

Thanks for doing this. Feel free to ping me if we need to discuss how we should
handle the preference.

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

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:31: namespace {
nit: WS

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:48: default:
No default so the compiler can catch missing enums. Put the NOTREACHED outside
the switch.

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:56:
chromeos::WakeOnWifiManager* g_wake_on_wifi_manager = NULL;
Rather than exposing a weak global pointer and making Preferences depend on this
class, I think you can use PrefChangeRegistrar to register and observe changes
to the preference within this class.

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:62: // this class.
I believe that the correct behavior is that there should only be one instance of
this for the primary user profile. All Shill configuration applies to the
primary user.

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:83: //
gcm::GCMConnectionObserver overrides.
When inlining overrides, they need to be spaced apart (i.e. WS between line 93
and 94), so it is also better to put a blank line after the base class comment.

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:157: return;
DCHECK instead since this should never be empty.

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:199: }
We shouldn't need any of the above, there should just be the one connection
observer for the primary profile.

(Note: as written, if this class is constructed after the primary profile
exists, no PROFILE_ADDED notification will occur and so no observer will be
created, but again I think there should only be one observer for the primary
profile).

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/pref...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/pref...
chrome/browser/chromeos/preferences.cc:574: if (reason != REASON_PREF_CHANGED ||
On 2014/11/13 21:32:14, chirantan wrote:
> On 2014/11/13 03:18:48, michaelpg wrote:
> > Seems like we need to clarify how the machine setting/owner-controlled
> > preference works.
> > 
> > This CL makes kWakeOnWiFiEnabled a per-profile preference that only the
owner
> > can set -- that's kind of weird. For example, when a non-owner signs in, the
> > initialization here will use the default value (e.g., PACKET_AND_SSID)
because
> > the check on 575 means a non-owner can never set his preference. That
doesn't
> > seem right, since we're not respecting the owner's preference OR the user's
> > preference.
> > 
> > Since this isn't a profile setting but a per-machine (owner) setting, we
> > probably don't want to add this preference to the user's pref store, but to
> the
> > local state. (That said, I'm not convinced this shouldn't be a per-user
> > decision, like the sleep settings on Windows.)
> > 
> 
> So I discussed this again with Sameer and I think you're right.  This makes
more
> sense as a per-user thing rather than an owner thing, especially considering
the
> managed device case.  I'm going to run it by drewry@, who was the one who had
> initially asked for the flag to be owner only but I'm going to go ahead right
> now with the assumption that making it per-user is ok.
> 
> > Would it make more sense to register the pref in WakeOnWifiManager so we
don't
> > need to involve it in the Preferences class?
> > 
> 
> I'd like to do that as well but I was unsure about how to plumb it in so that
> the WakeOnWifiManager will get access to the PrefRegistry and the PrefService
> for the user.
> 
> 

I am not entirely up to speed on how our pref store works, but essencially we
want to associate WakeOnWifiManager with the primary user profile only, and use
the preference in that user's pref store.

I believe that there should always be a "primary user profile" even on managed /
kisok devices.

https://codereview.chromium.org/722043004/diff/1/chromeos/dbus/fake_shill_dev...
File chromeos/dbus/fake_shill_device_client.cc (right):

https://codereview.chromium.org/722043004/diff/1/chromeos/dbus/fake_shill_dev...
chromeos/dbus/fake_shill_device_client.cc:271: const ErrorCallback&
error_callback) {
We may want to trivially map device_path -> set<IPEndPoint> and trigger the
error callback here if |device_path| doesn't exist and in Remove if the endpoint
doesn't exist.

https://codereview.chromium.org/722043004/diff/1/chromeos/network/network_dev...
File chromeos/network/network_device_handler_impl.cc (right):

https://codereview.chromium.org/722043004/diff/1/chromeos/network/network_dev...
chromeos/network/network_device_handler_impl.cc:453: }
This entire clause should be moved to a file local helper function shared by all
three methods, e.g.:

const DeviceState* device_state = GetWifiDeviceState(error_callback);
if (!device_state)
  return;

[Chirantan Ekbote, 2014-11-13 23:12:38]

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

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:56:
chromeos::WakeOnWifiManager* g_wake_on_wifi_manager = NULL;
On 2014/11/13 22:20:16, stevenjb wrote:
> Rather than exposing a weak global pointer and making Preferences depend on
this
> class, I think you can use PrefChangeRegistrar to register and observe changes
> to the preference within this class.

PrefChangeRegistrar still needs a PrefService before any observers can be
registered.  So I would need to hook this into ProfileImpl if we don't want it
in Preferences.

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:62: // this class.
On 2014/11/13 22:20:17, stevenjb wrote:
> I believe that the correct behavior is that there should only be one instance
of
> this for the primary user profile. All Shill configuration applies to the
> primary user.

So I don't know much about how profiles work.  Is the primary user the one who
signs in from the login screen?  Assuming it is then this is what I think you
are saying:

- A user signs in via the login screen and we configure shill with that user's
preferences.  We also set up a GCMConnectionObserver to observe connections to
the GCM server and send those down to shill.
- The user then adds another account via the status area.  We ignore this
profiles's wake on wifi preferences and we don't observe this profile's
connection to the GCM server.

So even if the primary profile chooses to wake on GCM packets we still will not
wake up for packets sent to the secondary profile.  It seems to me that what we
should do is a combination of the two: respect the primary profile's preference
but still wake on packets for the secondary profile if the primary profile has
enabled it.

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:199: }
On 2014/11/13 22:20:16, stevenjb wrote:
> We shouldn't need any of the above, there should just be the one connection
> observer for the primary profile.
> 
> (Note: as written, if this class is constructed after the primary profile
> exists, no PROFILE_ADDED notification will occur and so no observer will be
> created, but again I think there should only be one observer for the primary
> profile).

This is why I construct this class in PreMainMessageLoopRun, which I think runs
after the browser process is created but before any profiles are created.  If
that's not actually how it works then I need to fix this.

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/pref...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/pref...
chrome/browser/chromeos/preferences.cc:574: if (reason != REASON_PREF_CHANGED ||
On 2014/11/13 22:20:17, stevenjb wrote:
> On 2014/11/13 21:32:14, chirantan wrote:
> > On 2014/11/13 03:18:48, michaelpg wrote:
> > > Seems like we need to clarify how the machine setting/owner-controlled
> > > preference works.
> > > 
> > > This CL makes kWakeOnWiFiEnabled a per-profile preference that only the
> owner
> > > can set -- that's kind of weird. For example, when a non-owner signs in,
the
> > > initialization here will use the default value (e.g., PACKET_AND_SSID)
> because
> > > the check on 575 means a non-owner can never set his preference. That
> doesn't
> > > seem right, since we're not respecting the owner's preference OR the
user's
> > > preference.
> > > 
> > > Since this isn't a profile setting but a per-machine (owner) setting, we
> > > probably don't want to add this preference to the user's pref store, but
to
> > the
> > > local state. (That said, I'm not convinced this shouldn't be a per-user
> > > decision, like the sleep settings on Windows.)
> > > 
> > 
> > So I discussed this again with Sameer and I think you're right.  This makes
> more
> > sense as a per-user thing rather than an owner thing, especially considering
> the
> > managed device case.  I'm going to run it by drewry@, who was the one who
had
> > initially asked for the flag to be owner only but I'm going to go ahead
right
> > now with the assumption that making it per-user is ok.
> > 
> > > Would it make more sense to register the pref in WakeOnWifiManager so we
> don't
> > > need to involve it in the Preferences class?
> > > 
> > 
> > I'd like to do that as well but I was unsure about how to plumb it in so
that
> > the WakeOnWifiManager will get access to the PrefRegistry and the
PrefService
> > for the user.
> > 
> > 
> 
> I am not entirely up to speed on how our pref store works, but essencially we
> want to associate WakeOnWifiManager with the primary user profile only, and
use
> the preference in that user's pref store.
> 
> I believe that there should always be a "primary user profile" even on managed
/
> kisok devices.

We can change this to

    if (user_is_primary_ && (reason != REASON_PREF_CHANGED ||
                             pref_name == prefs::kWakeOnWifiEnabled))

which I think covers all the cases.

[stevenjb, 2014-11-13 23:24:46]

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

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:62: // this class.
On 2014/11/13 23:12:37, chirantan wrote:
> On 2014/11/13 22:20:17, stevenjb wrote:
> > I believe that the correct behavior is that there should only be one
instance
> of
> > this for the primary user profile. All Shill configuration applies to the
> > primary user.
> 
> So I don't know much about how profiles work.  Is the primary user the one who
> signs in from the login screen?  Assuming it is then this is what I think you
> are saying:
> 
> - A user signs in via the login screen and we configure shill with that user's
> preferences.  We also set up a GCMConnectionObserver to observe connections to
> the GCM server and send those down to shill.
> - The user then adds another account via the status area.  We ignore this
> profiles's wake on wifi preferences and we don't observe this profile's
> connection to the GCM server.
> 
> So even if the primary profile chooses to wake on GCM packets we still will
not
> wake up for packets sent to the secondary profile.  It seems to me that what
we
> should do is a combination of the two: respect the primary profile's
preference
> but still wake on packets for the secondary profile if the primary profile has
> enabled it.

Yes, "primary" refers to the profile used to initially sign in.

I see, GCMConnectionObserver is per-profile. So yes, you are correct, we want a
single Manager instance using the preference associated with the primary
profile, with an observer for each profile (primary and secondary). Apologies
for my confusion.

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:199: }
On 2014/11/13 23:12:37, chirantan wrote:
> On 2014/11/13 22:20:16, stevenjb wrote:
> > We shouldn't need any of the above, there should just be the one connection
> > observer for the primary profile.
> > 
> > (Note: as written, if this class is constructed after the primary profile
> > exists, no PROFILE_ADDED notification will occur and so no observer will be
> > created, but again I think there should only be one observer for the primary
> > profile).
> 
> This is why I construct this class in PreMainMessageLoopRun, which I think
runs
> after the browser process is created but before any profiles are created.  If
> that's not actually how it works then I need to fix this.

OK, I think that is valid, but if we make that sort of assumption we should
document or ideally CHECK in the constructor that we are not logged in, e.g.
CHECK(!LoginState::IsUserLoggedIn()).

[Chirantan Ekbote, 2014-11-14 02:47:43]

ptal

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

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:31: namespace {
On 2014/11/13 22:20:17, stevenjb wrote:
> nit: WS

Done.

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:48: default:
On 2014/11/13 22:20:16, stevenjb wrote:
> No default so the compiler can catch missing enums. Put the NOTREACHED outside
> the switch.

Done.

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:83: //
gcm::GCMConnectionObserver overrides.
On 2014/11/13 22:20:16, stevenjb wrote:
> When inlining overrides, they need to be spaced apart (i.e. WS between line 93
> and 94), so it is also better to put a blank line after the base class
comment.

Done.

https://codereview.chromium.org/722043004/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:157: return;
On 2014/11/13 22:20:17, stevenjb wrote:
> DCHECK instead since this should never be empty.

Done.

https://codereview.chromium.org/722043004/diff/1/chrome/chrome_browser_chrome...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/722043004/diff/1/chrome/chrome_browser_chrome...
chrome/chrome_browser_chromeos.gypi:660:
'browser/chromeos/net/wake_on_wifi_manager.cc',
On 2014/11/13 03:18:48, michaelpg wrote:
> alphabetize

Done.

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

https://codereview.chromium.org/722043004/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:838: // A integer pref that controls the wake on
wifi features that should be
On 2014/11/13 02:51:39, chirantan wrote:
> Should be "An integer".  Will fix in the next patch set.

Done.

https://codereview.chromium.org/722043004/diff/1/chromeos/dbus/fake_shill_dev...
File chromeos/dbus/fake_shill_device_client.cc (right):

https://codereview.chromium.org/722043004/diff/1/chromeos/dbus/fake_shill_dev...
chromeos/dbus/fake_shill_device_client.cc:271: const ErrorCallback&
error_callback) {
On 2014/11/13 22:20:17, stevenjb wrote:
> We may want to trivially map device_path -> set<IPEndPoint> and trigger the
> error callback here if |device_path| doesn't exist and in Remove if the
endpoint
> doesn't exist. 

Done.

https://codereview.chromium.org/722043004/diff/1/chromeos/network/network_dev...
File chromeos/network/network_device_handler_impl.cc (right):

https://codereview.chromium.org/722043004/diff/1/chromeos/network/network_dev...
chromeos/network/network_device_handler_impl.cc:453: }
On 2014/11/13 22:20:17, stevenjb wrote:
> This entire clause should be moved to a file local helper function shared by
all
> three methods, e.g.:
> 
> const DeviceState* device_state = GetWifiDeviceState(error_callback);
> if (!device_state)
>   return;
> 

Done.

[stevenjb, 2014-11-14 18:57:57]

stevenjb@chromium.org changed reviewers:
+ michaelpg@chromium.org

[stevenjb, 2014-11-14 18:57:57]

stevenjb@chromium.org changed required reviewers:
+ michaelpg@chromium.org

[stevenjb, 2014-11-14 18:57:58]

This lgtm pending signoff by michaelpg@ and resolution of any preference related
issues.

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

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:133: // so this check should
only run on actual chromebooks.
nit: Use "Chrome OS device" or just "device" (since device can include
"chromebox" and potentially other designators)

[michaelpg, 2014-11-18 01:32:00]

lgtm w/ nits

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

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:38: // Should be called
whenever the user changes their preference for the
nit: whenever the *primary* user changes their preference

(although technically the UI shouldn't allow non-primary users to change their
preference.)

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:52: std::map<Profile*,
linked_ptr<WakeOnPacketConnectionObserver> >
nit: use >> without space

[Chirantan Ekbote, 2014-11-18 01:54:35]

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

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:133: // so this check should
only run on actual chromebooks.
On 2014/11/14 18:57:57, stevenjb wrote:
> nit: Use "Chrome OS device" or just "device" (since device can include
> "chromebox" and potentially other designators)

Done.

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

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:38: // Should be called
whenever the user changes their preference for the
On 2014/11/18 01:31:59, michaelpg wrote:
> nit: whenever the *primary* user changes their preference
> 
> (although technically the UI shouldn't allow non-primary users to change their
> preference.)

Done.

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:52: std::map<Profile*,
linked_ptr<WakeOnPacketConnectionObserver> >
On 2014/11/18 01:31:59, michaelpg wrote:
> nit: use >> without space

Done.

[Chirantan Ekbote, 2014-11-18 01:54:47]

The CQ bit was checked by chirantan@chromium.org

[commit-bot: I haz the power, 2014-11-18 01:55:43]

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

[Lei Zhang, 2014-11-18 01:57:55]

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

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:14: #include
"chrome/browser/browser_process.h"
not used?

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:42:
chromeos::WakeOnWifiManager::WakeOnWifiFeature feature) {
no need for chromeos:: inside namespace chromeos.

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:100: if (ip_endpoint_ ==
net::IPEndPoint()) {
I wonder if net::IPEndPoint should have a valid() method, but that's a
discussion for a different CL.

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:208:
connection_observers_.erase(iter);
As is, this entire function can just be: connection_observers_.erase(profile);

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

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: no (c), same in .cc file.

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:11: #include
"base/memory/linked_ptr.h"
Can you use base/containers/scoped_ptr_hash_map.h and avoid using linked_ptrs?
See http://crbug.com/137767

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:23: // owned by
ChromeBrowserMainPartsChromeos.
Is this class single-threaded? It runs on the UI thread, right?

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:33: static WakeOnWifiManager*
Get();
Do you really need this and |g_wake_on_wifi_manager| ? Can the caller just go
through |g_browser_process| ?

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

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.cc:287: registry->RegisterIntegerPref(
nit: you can use the if-block to figure out the value, and then do a single
RegisterIntegerPref() call, rather than repeating most of the code from the
if-block here.

[Chirantan Ekbote, 2014-11-18 01:59:32]

The CQ bit was unchecked by chirantan@chromium.org

[Chirantan Ekbote, 2014-11-18 02:16:17]

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

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:11: #include
"base/memory/linked_ptr.h"
On 2014/11/18 01:57:54, Lei Zhang wrote:
> Can you use base/containers/scoped_ptr_hash_map.h and avoid using linked_ptrs?
> See http://crbug.com/137767

Ok

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:23: // owned by
ChromeBrowserMainPartsChromeos.
On 2014/11/18 01:57:55, Lei Zhang wrote:
> Is this class single-threaded? It runs on the UI thread, right?

Yes, this class is not thread-safe.  I will add a comment to that effect.

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:33: static WakeOnWifiManager*
Get();
On 2014/11/18 01:57:55, Lei Zhang wrote:
> Do you really need this and |g_wake_on_wifi_manager| ? Can the caller just go
> through |g_browser_process| ?

I'm not sure I follow what you mean.  How do you get to BrowserMainParts from
|g_browser_process|?  From what I can tell it is a private member variable of
BrowserMainLoop with no accessor.  |g_wake_on_wifi_manager| is local to the .cc
file and I don't know how Get() can access the pointer to the class without it.

[Lei Zhang, 2014-11-18 02:22:38]

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

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:23: // owned by
ChromeBrowserMainPartsChromeos.
On 2014/11/18 02:16:17, chirantan wrote:
> On 2014/11/18 01:57:55, Lei Zhang wrote:
> > Is this class single-threaded? It runs on the UI thread, right?
> 
> Yes, this class is not thread-safe.  I will add a comment to that effect.

Please sprinkle in a few DCHECK_CURRENTLY_ON() calls to enforce this.

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:33: static WakeOnWifiManager*
Get();
On 2014/11/18 02:16:17, chirantan wrote:
> On 2014/11/18 01:57:55, Lei Zhang wrote:
> > Do you really need this and |g_wake_on_wifi_manager| ? Can the caller just
go
> > through |g_browser_process| ?
> 
> I'm not sure I follow what you mean.  How do you get to BrowserMainParts from
> |g_browser_process|?  From what I can tell it is a private member variable of
> BrowserMainLoop with no accessor.  |g_wake_on_wifi_manager| is local to the
.cc
> file and I don't know how Get() can access the pointer to the class without
it.

Sorry, I was thinking of BrowserProcess owning this. Nevermind.

[Chirantan Ekbote, 2014-11-18 20:37:39]

ptal

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

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:14: #include
"chrome/browser/browser_process.h"
On 2014/11/18 01:57:54, Lei Zhang wrote:
> not used?

Done.

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:42:
chromeos::WakeOnWifiManager::WakeOnWifiFeature feature) {
On 2014/11/18 01:57:54, Lei Zhang wrote:
> no need for chromeos:: inside namespace chromeos.

Done.

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:100: if (ip_endpoint_ ==
net::IPEndPoint()) {
On 2014/11/18 01:57:54, Lei Zhang wrote:
> I wonder if net::IPEndPoint should have a valid() method, but that's a
> discussion for a different CL.

I had a look at this to see if it would be easy to do. 
net::IPEndPoint::ToString claims that it will return an empty string if the
address is invalid so that was promising.  But then I tracked it to
net_util::IPAddressToString
(https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_util....),
which simply checks if the address length matches an ipv4 or ipv6 length and
then hits a CHECK(false) if it doesn't match either. 
net::IPEndPoint::GetSockAddrFamily does the same thing.

So the takeaway is (1) the docstring for net::IPEndPoint::ToString is a dirty
liar and (2) the only way to check if an endpoint is valid is to see if the
address length matches an ipv4 or ipv6 address length.

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.cc:208:
connection_observers_.erase(iter);
On 2014/11/18 01:57:54, Lei Zhang wrote:
> As is, this entire function can just be: connection_observers_.erase(profile);

Done.

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

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/11/18 01:57:54, Lei Zhang wrote:
> nit: no (c), same in .cc file.

Done.

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:11: #include
"base/memory/linked_ptr.h"
On 2014/11/18 02:16:17, chirantan wrote:
> On 2014/11/18 01:57:54, Lei Zhang wrote:
> > Can you use base/containers/scoped_ptr_hash_map.h and avoid using
linked_ptrs?
> > See http://crbug.com/137767
> 
> Ok

Done.

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/wake_on_wifi_manager.h:23: // owned by
ChromeBrowserMainPartsChromeos.
On 2014/11/18 02:22:38, Lei Zhang wrote:
> On 2014/11/18 02:16:17, chirantan wrote:
> > On 2014/11/18 01:57:55, Lei Zhang wrote:
> > > Is this class single-threaded? It runs on the UI thread, right?
> > 
> > Yes, this class is not thread-safe.  I will add a comment to that effect.
> 
> Please sprinkle in a few DCHECK_CURRENTLY_ON() calls to enforce this.

Done.

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

https://codereview.chromium.org/722043004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.cc:287: registry->RegisterIntegerPref(
On 2014/11/18 01:57:55, Lei Zhang wrote:
> nit: you can use the if-block to figure out the value, and then do a single
> RegisterIntegerPref() call, rather than repeating most of the code from the
> if-block here.

Done.

[Lei Zhang, 2014-11-18 21:10:59]

Thanks for addressing my nits. lgtm++

[Chirantan Ekbote, 2014-11-18 21:13:33]

The CQ bit was checked by chirantan@chromium.org

[commit-bot: I haz the power, 2014-11-18 21:14:38]

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

[commit-bot: I haz the power, 2014-11-18 22:34:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-18 22:35:11]

Patchset 5 (id:??) landed as
https://crrev.com/1c4090c9467c8ffcf01beec023686529cb8cd521
Cr-Commit-Position: refs/heads/master@{#304684}