Really fix chromium-os:14180 this time.

Really fix chromium-os:14180 this time.

Add the device change property manager in the correct branch
of the if statement.

BUG=chromium-os:14180
TEST=Bring up the cellular device tab, and see that it is now
populated with actual values instead of being blank.

R=stevenjb@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85338

[Eric Shienbrood, 2011-05-12 21:48:00]

This is fairly urgent, and will need to be merged to M12.

[stevenjb, 2011-05-12 22:33:06]

LGTM (but see comment)
Sorry I didn't look closely at this the first time around, might have caught
that.

http://codereview.chromium.org/7011023/diff/3001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/7011023/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3674: new
NetworkDeviceObserverList(this, device_path);
I think that only adding the observer when creating the NetworkDevice (in
ParseNetworkDevice) is sufficient and would be better (i.e. remove this line),
but I don't think this will cause any problems and should be fine for R12.

[Eric Shienbrood, 2011-05-12 22:41:18]

No apologies needed, this was totally my fault for trying to rush things and not
being careful enough.

http://codereview.chromium.org/7011023/diff/3001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/7011023/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3674: new
NetworkDeviceObserverList(this, device_path);
Since we're requesting device info immediately below, I'm concerned that there
would be a window during which we could miss property changes. It seems like the
only safe way to do things is to make sure we're monitoring properties *before*
we request device info. Do you disagree?

I'll hold off committing until I hear back from you.

On 2011/05/12 22:33:06, Steven Bennetts wrote:
> I think that only adding the observer when creating the NetworkDevice (in
> ParseNetworkDevice) is sufficient and would be better (i.e. remove this line),
> but I don't think this will cause any problems and should be fine for R12.

[stevenjb, 2011-05-12 23:00:24]

http://codereview.chromium.org/7011023/diff/3001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/7011023/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3674: new
NetworkDeviceObserverList(this, device_path);
On 2011/05/12 22:41:18, Eric Shienbrood wrote:
> Since we're requesting device info immediately below, I'm concerned that there
> would be a window during which we could miss property changes. It seems like
the
> only safe way to do things is to make sure we're monitoring properties
*before*
> we request device info. Do you disagree?
> 
> I'll hold off committing until I hear back from you.
> 
> On 2011/05/12 22:33:06, Steven Bennetts wrote:
> > I think that only adding the observer when creating the NetworkDevice (in
> > ParseNetworkDevice) is sufficient and would be better (i.e. remove this
line),
> > but I don't think this will cause any problems and should be fine for R12.
> 
Well, since the device doesn't exist yet (i.e. it hasn't been allocated), then
any property changes received wouldn't do anything anyway, so if that is really
happening we would need to keep track of property changes for devices that don't
exist yet... But it seems like when we receive a new device, it's properties
ought to be up to date.

What we might want to do however is call NotifyNetworkDeviceChanged(device) in
ParseNetworkDevice() to notify any UI monitoring devices but not the manager.

http://codereview.chromium.org/7011023/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3735:
NotifyNetworkManagerChanged(false);  // Not forced.
NotifyNetworkDeviceChanged(device); ?

[Eric Shienbrood, 2011-05-13 21:48:48]

http://codereview.chromium.org/7011023/diff/3001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/7011023/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3674: new
NetworkDeviceObserverList(this, device_path);
If they're not monitoring the manager, then they won't have found out when the
device was added, and thus wouldn't have set up an observer for the device. If
they are monitoring the manager, then calling NotifyNetworkDeviceChanged is
almost certainly redundant.
On 2011/05/12 23:00:24, Steven Bennetts wrote:
> On 2011/05/12 22:41:18, Eric Shienbrood wrote:
> > Since we're requesting device info immediately below, I'm concerned that
there
> > would be a window during which we could miss property changes. It seems like
> the
> > only safe way to do things is to make sure we're monitoring properties
> *before*
> > we request device info. Do you disagree?
> > 
> > I'll hold off committing until I hear back from you.
> > 
> > On 2011/05/12 22:33:06, Steven Bennetts wrote:
> > > I think that only adding the observer when creating the NetworkDevice (in
> > > ParseNetworkDevice) is sufficient and would be better (i.e. remove this
> line),
> > > but I don't think this will cause any problems and should be fine for R12.
> > 
> Well, since the device doesn't exist yet (i.e. it hasn't been allocated), then
> any property changes received wouldn't do anything anyway, so if that is
really
> happening we would need to keep track of property changes for devices that
don't
> exist yet... But it seems like when we receive a new device, it's properties
> ought to be up to date.
> 
> What we might want to do however is call NotifyNetworkDeviceChanged(device) in
> ParseNetworkDevice() to notify any UI monitoring devices but not the manager.

http://codereview.chromium.org/7011023/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3674: new
NetworkDeviceObserverList(this, device_path);
I still think there's a theoretical window of vulnerability, but in practice the
order of the signals sent out from flimflam is preserved, and there's only one
thread handling all the signals, so I think it's safe to ignore this. In other
words, I'm taking your suggestion.

[stevenjb, 2011-05-13 22:15:39]

LGTM

http://codereview.chromium.org/7011023/diff/1005/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/7011023/diff/1005/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3488:
active_profile_path_.c_str(), &ProfileUpdate, this);
nit: { }

[Eric Shienbrood, 2011-05-13 22:29:24]

http://codereview.chromium.org/7011023/diff/1005/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/7011023/diff/1005/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3488:
active_profile_path_.c_str(), &ProfileUpdate, this);
On 2011/05/13 22:15:39, Steven Bennetts wrote:
> nit: { }

Done.