Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21961003:
Request updates when network state changes (Closed)
|
Created:
7 years, 4 months ago by stevenjb Modified:
7 years, 4 months ago CC:
chromium-reviews, gauravsh+watch_chromium.org, gspencer+watch_chromium.org, stevenjb+watch_chromium.org, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRequest updates when network state changes
This addresses three things:
* Properties like IPConfig may change when Service.State changes, so properties need to be requested.
* FavoriteState needs to be created when Service.Profile changes, which is triggered from a property request.
* FavoriteState updates do not need to trigger NetworkListChanged event since they will always follow a corresponding NetworkState update which will trigger the event.
BUG=256810, 268270, 263568
R=gauravsh@chromium.org, pneubeck@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216404
Patch Set 1 #Patch Set 2 : Rebase + Request update when Profile changes #Patch Set 3 : . #
Total comments: 5
Patch Set 4 : Rebase + fix comment #Messages
Total messages: 11 (0 generated)
Small change to ensure properties like IPConfig get updated on connect, etc. There are a few bugs which might be caused by not having this code.
lgtm
On 2013/08/02 22:36:15, stevenjb (chromium) wrote: > Small change to ensure properties like IPConfig get updated on connect, etc. > There are a few bugs which might be caused by not having this code. you could add this explanation to the commit message. Not sure why the api tests break, maybe it's caused by RequestUpdateForNetwork calling ShillServiceClientStub::GetProperties, which returns the _current properties_: https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/dbus/shil... Other Set* calls of the NetworkConnectionHandler might not be done yet. This Stub behavior can again be different from Shill. Shill would answer other pending DBus calls before this GetProperties call is handled.
On 2013/08/05 20:39:47, pneubeck wrote: > On 2013/08/02 22:36:15, stevenjb (chromium) wrote: > > Small change to ensure properties like IPConfig get updated on connect, etc. > > There are a few bugs which might be caused by not having this code. > > you could add this explanation to the commit message. > > Not sure why the api tests break, maybe it's caused by RequestUpdateForNetwork > calling ShillServiceClientStub::GetProperties, which returns the _current > properties_: > https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/dbus/shil... > Other Set* calls of the NetworkConnectionHandler might not be done yet. > > This Stub behavior can again be different from Shill. Shill would answer other > pending DBus calls before this GetProperties call is handled. The test is failing because this triggers an extra list update which breaks the tests somewhat fragile expectations. I'm re-factoring that code to sort correctly, hopefully making the tests less fragile.
On 2013/08/05 20:54:50, stevenjb (chromium) wrote: > On 2013/08/05 20:39:47, pneubeck wrote: > > On 2013/08/02 22:36:15, stevenjb (chromium) wrote: > > > Small change to ensure properties like IPConfig get updated on connect, etc. > > > There are a few bugs which might be caused by not having this code. > > > > you could add this explanation to the commit message. > > > > Not sure why the api tests break, maybe it's caused by RequestUpdateForNetwork > > calling ShillServiceClientStub::GetProperties, which returns the _current > > properties_: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/dbus/shil... > > Other Set* calls of the NetworkConnectionHandler might not be done yet. > > > > This Stub behavior can again be different from Shill. Shill would answer other > > pending DBus calls before this GetProperties call is handled. > > The test is failing because this triggers an extra list update which breaks the > tests somewhat fragile expectations. I'm re-factoring that code to sort > correctly, hopefully making the tests less fragile. Hm. Let me try to understand this: 1. NetworkConn*Handler gets a connect call. In turn it does some calls to the Shill*Clients. 2. StateHandler observes that ConnectionState changed (i.e. ConnectionStateChanged(network, prev_connection_state)) 3. StateHandler requests more properties 4. After receiving the requested properties, StateHandler triggers a network list changed, which is identical to the list before ConnectionState changed. 5. StateHandler observes that ServiceList changed. 6. StateHandler triggers another network list changed, which is now matching the the ConnectionState. I find it surprising that 4. comes before 5./6. and not afterwards, in particular as step 4. is triggered by the callback from a GetProperties call.
I believe that is correct, with some notes / clarifications: 4: We may be able to fix this. We want to send a ListChanged signal when the list changes, but *after* the properties for new entries are received, so that the list will be complete/correct when queried after the signal. This is why we trigger the ListChanged signal only when there are no more pending updates. That code was written before we started requesting individual updates. We should be able to differentiate between the two (keeping in mind that an individual update night happen during a list update). 5/6: Because the state transition triggers a list re-order, right. I don't think we have any guarantee whether Shill emits a property change for Manger.Services before or after Service.State (i.e. whether 4 comes before or after 5/6). I don't think we should rely on any specific ordering there. The "*Changed" signals are really all "MayHaveChanged". Reducing the number of signals should be an optimization not a requirement. However, reducing the number of uninteresting signals is certainly worthwhile. In this particular case the stub code was pretty brittle and the NetworkingPrivateApi tests are making some poor assumptions about the stub behavior (e.g. that the first ListChanged signal after a Connect will have the new ordering). For now I've improved the Stub behavior, but I've made a note to improve the NetworkingPrivateApi tests (specifically to check expectations against the *last* ListChanged event, not the first one). On Tue, Aug 6, 2013 at 1:10 AM, <pneubeck@chromium.org> wrote: > On 2013/08/05 20:54:50, stevenjb (chromium) wrote: > >> On 2013/08/05 20:39:47, pneubeck wrote: >> > On 2013/08/02 22:36:15, stevenjb (chromium) wrote: >> > > Small change to ensure properties like IPConfig get updated on >> connect, >> > etc. > >> > > There are a few bugs which might be caused by not having this code. >> > >> > you could add this explanation to the commit message. >> > >> > Not sure why the api tests break, maybe it's caused by >> > RequestUpdateForNetwork > >> > calling ShillServiceClientStub::**GetProperties, which returns the >> _current >> > properties_: >> > >> > > https://code.google.com/p/**chromium/codesearch#chromium/** > src/chromeos/dbus/shill_**service_client_stub.cc&sq=** > package:chromium&type=cs&rcl=**1375686820&l=76<https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/dbus/shill_service_client_stub.cc&sq=package:chromium&type=cs&rcl=1375686820&l=76> > >> > Other Set* calls of the NetworkConnectionHandler might not be done yet. >> > >> > This Stub behavior can again be different from Shill. Shill would answer >> > other > >> > pending DBus calls before this GetProperties call is handled. >> > > The test is failing because this triggers an extra list update which >> breaks >> > the > >> tests somewhat fragile expectations. I'm re-factoring that code to sort >> correctly, hopefully making the tests less fragile. >> > > Hm. Let me try to understand this: > > 1. NetworkConn*Handler gets a connect call. In turn it does some calls to > the > Shill*Clients. > 2. StateHandler observes that ConnectionState changed (i.e. > ConnectionStateChanged(**network, prev_connection_state)) > 3. StateHandler requests more properties > 4. After receiving the requested properties, StateHandler triggers a > network > list changed, which is identical to the list before ConnectionState > changed. > 5. StateHandler observes that ServiceList changed. > 6. StateHandler triggers another network list changed, which is now > matching the > the ConnectionState. > > I find it surprising that 4. comes before 5./6. and not afterwards, in > particular as step 4. is triggered by the callback from a GetProperties > call. > > > https://codereview.chromium.**org/21961003/<https://codereview.chromium.org/2... >
PTAL. I made a similar change to fix issue 263568.
lgtm but please include in the commit message why this fixes 263568
lgtm https://codereview.chromium.org/21961003/diff/12002/chromeos/network/network_... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/21961003/diff/12002/chromeos/network/network_... chromeos/network/network_state_handler.cc:541: key == shill::kWifiFrequencyListProperty; nit: I don't say you should, but you could remove kWifiFrequencyListProperty, as it isn't parsed by NetworkState anymore. https://codereview.chromium.org/21961003/diff/12002/chromeos/network/network_... chromeos/network/network_state_handler.cc:560: log_level = network_event_log::LOG_LEVEL_EVENT; nit: you could initialize log_level with LOG_LEVEL_EVENT and remove the else. https://codereview.chromium.org/21961003/diff/12002/chromeos/network/network_... chromeos/network/network_state_handler.cc:613: // This will always get triggered for TYPE_NETWORK if the favorite list I know the reason for this change, but I don't quite understand the comment. The commit message is clearer, or maybe: This will always get triggered if the NetworkList changed, so do not notify the observers about NetworkListChanged again.
https://codereview.chromium.org/21961003/diff/12002/chromeos/network/network_... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/21961003/diff/12002/chromeos/network/network_... chromeos/network/network_state_handler.cc:560: log_level = network_event_log::LOG_LEVEL_EVENT; On 2013/08/08 07:40:56, pneubeck wrote: > nit: you could initialize log_level with LOG_LEVEL_EVENT and remove the else. I had that first but for some reason it looked less clear to me. https://codereview.chromium.org/21961003/diff/12002/chromeos/network/network_... chromeos/network/network_state_handler.cc:613: // This will always get triggered for TYPE_NETWORK if the favorite list On 2013/08/08 07:40:56, pneubeck wrote: > I know the reason for this change, but I don't quite understand the comment. > > The commit message is clearer, or maybe: > This will always get triggered if the NetworkList changed, so do not notify the > observers about NetworkListChanged again. Clarified.
Message was sent while issue was closed.
Committed patchset #4 manually as r216404 (presubmit successful). |
