This is the first pass at making GetIPConfigs asynchronous.

This is the first pass at making GetIPConfigs asynchronous.

This renames the old GetIPConfigs to GetIPConfigsAndBlock, and deprecates it.

Several sites which used to use the old blocking GetIPConfigs have been rewritten to use the non-blocking asynchronous version.

There are still two places that were too complex to convert:
MoreMenuModel::InitMenuItems
SystemTrayDelegate::GetNetworkAddresses

Both of those sites rely on getting their results immediately and
would need their callers to change significantly to switch to async.

In order to allow the IPConfigs to be fetched asynchronously, we need to both get the shill properties for the service and the ipconfigs, so we have to have two layers of async calls.  This requires that we convert DictionaryValueCallbacks to use scoped_ptrs so we can avoid a deep copy of the properties.

BUG=chromium:147657
TEST=ran on device, tested network menus and settings.  Ran unit tests.


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

[Greg Spencer (Chromium), 2012-11-05 21:05:31]

http://codereview.chromium.org/11367048/diff/2002/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/11367048/diff/2002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1235:
base::Passed(&shill_properties)));
This is why the dictionary callback needs to use a scoped_ptr, so that we can
pass the shill_properties on to the next callback.  Without the scoped_ptrs, we
would have to make a deep copy of the properties here.

[stevenjb, 2012-11-05 23:22:57]

Discussed concerns about introducing scoped_ptr<> to APIs we are in the process
of deprecating in-person.

http://codereview.chromium.org/11367048/diff/2002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/11367048/diff/2002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.cc:520:
weak_pointer_factory_.GetWeakPtr()));
I'd actually rather avoid the overhead of a WeakPtrFactory for Network (which
can get constructed/destroyed pretty frequently_). I think it would be better to
bind the service path to a static callback and look up the Network from there.

[Greg Spencer (Chromium), 2012-11-12 19:28:38]

On 2012/11/05 23:22:57, stevenjb (chromium) wrote:
> Discussed concerns about introducing scoped_ptr<> to APIs we are in the
process
> of deprecating in-person.
> 
>
http://codereview.chromium.org/11367048/diff/2002/chrome/browser/chromeos/cro...
> File chrome/browser/chromeos/cros/network_library.cc (right):
> 
>
http://codereview.chromium.org/11367048/diff/2002/chrome/browser/chromeos/cro...
> chrome/browser/chromeos/cros/network_library.cc:520:
> weak_pointer_factory_.GetWeakPtr()));
> I'd actually rather avoid the overhead of a WeakPtrFactory for Network (which
> can get constructed/destroyed pretty frequently_). I think it would be better
to
> bind the service path to a static callback and look up the Network from there.

Well, but to look it up, I need an instance of NetworkLibrary, and to get that,
I need a chrome profile... Seems like it might be more complex that the weak
pointer factory.  Unless there's a way to look it up that I don't see.

[Greg Spencer (Chromium), 2012-11-12 20:17:56]

On 2012/11/12 19:28:38, Greg Spencer (Chromium) wrote:
> Well, but to look it up, I need an instance of NetworkLibrary, and to get
that,
> I need a chrome profile... Seems like it might be more complex that the weak
> pointer factory.  Unless there's a way to look it up that I don't see.

Nevermind.  CrosLibrary::Get().  Duh.

[Greg Spencer (Chromium), 2012-11-12 20:27:55]

OK, I've removed the scoped_ptr changes, and gotten rid of the weak pointer
factor in Network.  PTAL.

[stevenjb, 2012-11-12 20:40:45]

http://codereview.chromium.org/11367048/diff/10002/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/11367048/diff/10002/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library.cc:519: device_path_));
service_path_

http://codereview.chromium.org/11367048/diff/10002/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library.cc:525: const std::string&
device_path,
service_path

http://codereview.chromium.org/11367048/diff/10002/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/11367048/diff/10002/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library.h:612: const std::string&
hardware_address);
Does this get used outside of the .cc file? i.e. can we jsut make it a local
function?

[Greg Spencer (Chromium), 2012-11-12 20:56:03]

http://codereview.chromium.org/11367048/diff/10002/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/11367048/diff/10002/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library.cc:519: device_path_));
On 2012/11/12 20:40:45, stevenjb (chromium) wrote:
> service_path_

Whoops, Done.

http://codereview.chromium.org/11367048/diff/10002/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library.cc:525: const std::string&
device_path,
On 2012/11/12 20:40:45, stevenjb (chromium) wrote:
> service_path

Done.

http://codereview.chromium.org/11367048/diff/10002/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/11367048/diff/10002/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library.h:612: const std::string&
hardware_address);
On 2012/11/12 20:40:45, stevenjb (chromium) wrote:
> Does this get used outside of the .cc file? i.e. can we jsut make it a local
> function?

No, it doesn't get used outside of the .cc file, but it does set a private
member on the Network: ip_address_

I'd rather keep it a private static than add a public setter for it.

[stevenjb, 2012-11-12 21:17:27]

LGTM w/ nit

http://codereview.chromium.org/11367048/diff/10002/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/11367048/diff/10002/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library.h:612: const std::string&
hardware_address);
On 2012/11/12 20:56:03, Greg Spencer (Chromium) wrote:
> On 2012/11/12 20:40:45, stevenjb (chromium) wrote:
> > Does this get used outside of the .cc file? i.e. can we jsut make it a local
> > function?
> 
> No, it doesn't get used outside of the .cc file, but it does set a private
> member on the Network: ip_address_
> 
> I'd rather keep it a private static than add a public setter for it.
> 

Fair enough. I think that I regret that we used the model of private setters and
making NetworkLibrary a friend, but I don't think we should change that now :)

http://codereview.chromium.org/11367048/diff/15002/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/11367048/diff/15002/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library.h:587: void InitIPAddress();
nit: blank line here

[Greg Spencer (Chromium), 2012-11-12 21:39:35]

http://codereview.chromium.org/11367048/diff/10002/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/11367048/diff/10002/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library.h:612: const std::string&
hardware_address);
On 2012/11/12 21:17:27, stevenjb (chromium) wrote:
> On 2012/11/12 20:56:03, Greg Spencer (Chromium) wrote:
> > On 2012/11/12 20:40:45, stevenjb (chromium) wrote:
> > > Does this get used outside of the .cc file? i.e. can we jsut make it a
local
> > > function?
> > 
> > No, it doesn't get used outside of the .cc file, but it does set a private
> > member on the Network: ip_address_
> > 
> > I'd rather keep it a private static than add a public setter for it.
> > 
> 
> Fair enough. I think that I regret that we used the model of private setters
and
> making NetworkLibrary a friend, but I don't think we should change that now :)

Agreed, on both counts.  We won't repeat that pattern.

http://codereview.chromium.org/11367048/diff/15002/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/11367048/diff/15002/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library.h:587: void InitIPAddress();
On 2012/11/12 21:17:27, stevenjb (chromium) wrote:
> nit: blank line here

Done.

[Greg Spencer (Chromium), 2012-11-12 21:42:27]

Dmitry, could you please do an OWNERS review for me on this for
internet_options_handler.*?

[Dmitry Polukhin, 2012-11-12 23:02:28]

http://codereview.chromium.org/11367048/diff/6036/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/11367048/diff/6036/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1238:
network,
I haven't write this code initially so I might not understand how it works. But
this CL makes code even more async and I'm worry that network could be deleted
by the time when PopulateIPConfigsCallback is called. Comment in
network_library.h about FindNetworkByPath/FindNetworkByUniqueId said that we
shouldn't store it. I know that code was async even before this CL so it doesn't
do something new. But perhaps we should pass network by ID.

[stevenjb, 2012-11-12 23:09:43]

http://codereview.chromium.org/11367048/diff/6036/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/11367048/diff/6036/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1238:
network,
On 2012/11/12 23:02:28, Dmitry Polukhin wrote:
> I haven't write this code initially so I might not understand how it works.
But
> this CL makes code even more async and I'm worry that network could be deleted
> by the time when PopulateIPConfigsCallback is called. Comment in
> network_library.h about FindNetworkByPath/FindNetworkByUniqueId said that we
> shouldn't store it. I know that code was async even before this CL so it
doesn't
> do something new. But perhaps we should pass network by ID.

Yikes.

We should never be passing a Network* in an asynchronous call. Both
PopulateDictionaryDetailsCallback and PopulateIPConfigsCallback should be
passing service_path, not Network*. Thanks for catching this!

Let's fix this here please!

[Greg Spencer (Chromium), 2012-11-13 00:04:59]

http://codereview.chromium.org/11367048/diff/6036/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/11367048/diff/6036/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1238:
network,
On 2012/11/12 23:09:43, stevenjb (chromium) wrote:
> On 2012/11/12 23:02:28, Dmitry Polukhin wrote:
> > I haven't write this code initially so I might not understand how it works.
> But
> > this CL makes code even more async and I'm worry that network could be
deleted
> > by the time when PopulateIPConfigsCallback is called. Comment in
> > network_library.h about FindNetworkByPath/FindNetworkByUniqueId said that we
> > shouldn't store it. I know that code was async even before this CL so it
> doesn't
> > do something new. But perhaps we should pass network by ID.
> 
> Yikes.
> 
> We should never be passing a Network* in an asynchronous call. Both
> PopulateDictionaryDetailsCallback and PopulateIPConfigsCallback should be
> passing service_path, not Network*. Thanks for catching this!
> 
> Let's fix this here please!

Fixed.

[Dmitry Polukhin, 2012-11-13 00:07:55]

LGTM

Thank you for fixing network lifetime issue!

[stevenjb, 2012-11-13 00:13:39]

Thanks! LGTM

[commit-bot: I haz the power, 2012-11-13 00:55:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/11367048/12021

[commit-bot: I haz the power, 2012-11-13 02:56:01]

Retried try job too often for step(s) content_browsertests

[commit-bot: I haz the power, 2012-11-13 17:45:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/11367048/12021

[commit-bot: I haz the power, 2012-11-13 18:34:00]

Change committed as 167427