Rewritten parts of NetworkLibrary to work around memory corruption that prev...

Rewritten parts of NetworkLibrary to work around memory corruption that prevented proper dbus functioning for 3G actrivation.

The principle behind this change is pretty simple - there is only one instance of chromeos::Network specialization representing a network service at the time. Please note that there are parts of obsolete UI elements where object copies were still made. 

The cellular plan part is still work in progress, but there is enough to be reviewed now so I want to get an early start right now.

BUG=chromium-os:7619
TEST=none


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

[Charlie Lee, 2010-10-27 01:08:07]

Just had time to look at the first 2 files.
Will take a look in detail Tomorrow.

http://codereview.chromium.org/4169001/diff/20022/81002
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/4169001/diff/20022/81002#newcode108
chrome/browser/chromeos/cros/network_library.h:108: bool operator()(const
WirelessNetwork* a) {
No need to indent this method

http://codereview.chromium.org/4169001/diff/20022/81002#newcode395
chrome/browser/chromeos/cros/network_library.h:395: // Returns the list of
remembered celular networks.
Comment wrong. Why was remembered cellular networks removed?

[stevenjb, 2010-10-27 19:18:10]

http://codereview.chromium.org/4169001/diff/76003/15003
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/4169001/diff/76003/15003#newcode252
chrome/browser/chromeos/cros/network_library.cc:252: prl_version_ =
network.prl_version();
Copy data_plans_

http://codereview.chromium.org/4169001/diff/76003/15003#newcode687
chrome/browser/chromeos/cros/network_library.cc:687: const std::string&
certpath) {
DCHECK(network)

http://codereview.chromium.org/4169001/diff/76003/15003#newcode753
chrome/browser/chromeos/cros/network_library.cc:753: if (!EnsureCrosLoaded() ||
!network)
DCHECK() or !network, don't need both. (or, do both checks everywhere if we want
to crash on debug and NOOP on release)

http://codereview.chromium.org/4169001/diff/76003/15003#newcode758
chrome/browser/chromeos/cros/network_library.cc:758: virtual void
DisconnectFromWirelessNetwork(const WirelessNetwork* network) {
DCHECK(network)

http://codereview.chromium.org/4169001/diff/76003/15003#newcode782
chrome/browser/chromeos/cros/network_library.cc:782: virtual void
SaveCellularNetwork(const CellularNetwork* network) {
DCHECK(network)

http://codereview.chromium.org/4169001/diff/76003/15003#newcode789
chrome/browser/chromeos/cros/network_library.cc:789: virtual void
SaveWifiNetwork(const WifiNetwork* network) {
DCHECK(network)

http://codereview.chromium.org/4169001/diff/76003/15003#newcode810
chrome/browser/chromeos/cros/network_library.cc:810:
remembered_wifi_networks_.end());
I believe this will delete the range [found_iter, end), which is not what you
want.
I think what you want to do is:
iter = remove_if(...)  // this re-orders the list so that the matching entries
are at the end of the list
STLDeleteContainerPointers(iter, end())
erase(iter, end();

http://codereview.chromium.org/4169001/diff/76003/15003#newcode988
chrome/browser/chromeos/cros/network_library.cc:988: DCHECK(network &&
network->cellular_network());
A bug in Cashew (the data plan service) could cause it to trigger this callback,
even if we don't have a cellular network. We might want to just exit here if
cellular_network() is NULL.

http://codereview.chromium.org/4169001/diff/76003/15003#newcode1004
chrome/browser/chromeos/cros/network_library.cc:1004: *ethernet = NULL;
This should be handled in ClearNetworks(); DCHECK(*ethernet == NULL).

http://codereview.chromium.org/4169001/diff/76003/15003#newcode1021
chrome/browser/chromeos/cros/network_library.cc:1021: if (service->type ==
TYPE_ETHERNET && !((*ethernet)->connected()))
*ethernet == NULL here; (*ethernet)->connected() is bogus.

http://codereview.chromium.org/4169001/diff/76003/15003#newcode1222
chrome/browser/chromeos/cros/network_library.cc:1222: if (ethernet_)
if () not strictly necessary.

http://codereview.chromium.org/4169001/diff/76003/15003#newcode1250
chrome/browser/chromeos/cros/network_library.cc:1250: std::string
prev_service_path = cellular_ ?
prev_cellular_path? Wasn't clear right away that this was just a cellular thing.

http://codereview.chromium.org/4169001/diff/76003/15004
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/4169001/diff/76003/15004#newcode72
chrome/browser/chromeos/cros/network_library.h:72: explicit Network(const
chromeos::Network& network);
chromeos:: not needed here and elsewhere since all of this is in the chromeos
namespace.

http://codereview.chromium.org/4169001/diff/76003/15004#newcode447
chrome/browser/chromeos/cros/network_library.h:447: // Forget the passed in
wireless (either cellular or wifi) network.
Fix comment: // Forget the wifi network corresponding to service_path.

http://codereview.chromium.org/4169001/diff/76003/15005
File chrome/browser/chromeos/dom_ui/internet_options_handler.cc (right):

http://codereview.chromium.org/4169001/diff/76003/15005#newcode485
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:485: if ((network =
cros->FindWifiNetworkByPath(service_path))) {
Would prefer assignment outside of if() here

http://codereview.chromium.org/4169001/diff/76003/15005#newcode549
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:549: const
chromeos::Network* net, chromeos::NetworkLibrary* cros) {
DCHECK(net)

http://codereview.chromium.org/4169001/diff/76003/15005#newcode574
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:574: if (!(wireless =
cros->FindWifiNetworkByPath(net->service_path()))) {
Prefer assignment outside of if()

http://codereview.chromium.org/4169001/diff/76003/15005#newcode603
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:603: if (!(cellular =
cros->FindCellularNetworkByPath(net->service_path()))) {
Prefer assignment outside of if()

http://codereview.chromium.org/4169001/diff/76003/15005#newcode679
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:679: if ((network =
cros->FindWifiNetworkByPath(service_path))) {
Prefer assignment outside of if()

http://codereview.chromium.org/4169001/diff/76003/15005#newcode707
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:707: if ((network =
cros->FindWifiNetworkByPath(service_path))) {
Prefer assignment outside of if()

http://codereview.chromium.org/4169001/diff/76003/15005#newcode777
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:777: if ((cellular =
cros->FindCellularNetworkByPath(service_path))) {
Prefer assignment outside of if()

http://codereview.chromium.org/4169001/diff/76003/15005#newcode806
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:806: if ((cellular =
cros->FindCellularNetworkByPath(service_path))) {
Prefer assignment outside of if()

http://codereview.chromium.org/4169001/diff/76003/15007
File chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc (right):

http://codereview.chromium.org/4169001/diff/76003/15007#newcode360
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:360:
EvaluateCellularNetwork(network);
passing (NULL) explicitly might make it a little clearer that Evaluate handles a
NULL argument?

http://codereview.chromium.org/4169001/diff/76003/15007#newcode388
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:388: PLAN_ACTIVATION_ERROR,
std::string());
Same comment about explicit NULL to ChangeState()

http://codereview.chromium.org/4169001/diff/76003/15007#newcode398
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:398: //                       
 this);
// TODO() ?

http://codereview.chromium.org/4169001/diff/76003/15013
File chrome/browser/chromeos/options/cellular_config_view.cc (right):

http://codereview.chromium.org/4169001/diff/76003/15013#newcode73
chrome/browser/chromeos/options/cellular_config_view.cc:73: if (cellular_)
if() not strictly necessary

http://codereview.chromium.org/4169001/diff/76003/15014
File chrome/browser/chromeos/options/cellular_config_view.h (right):

http://codereview.chromium.org/4169001/diff/76003/15014#newcode53
chrome/browser/chromeos/options/cellular_config_view.h:53: CellularNetwork*
cellular_;
Strictly speaking, since we always copy the input network here, it doesn't need
to be a pointer. Alternately you could use a scoped_ptr<>.

http://codereview.chromium.org/4169001/diff/76003/15017
File chrome/browser/chromeos/options/network_config_view.h (right):

http://codereview.chromium.org/4169001/diff/76003/15017#newcode121
chrome/browser/chromeos/options/network_config_view.h:121: CellularNetwork*
cellular_;
Scoped pointers?

http://codereview.chromium.org/4169001/diff/76003/15019
File chrome/browser/chromeos/options/wifi_config_view.h (right):

http://codereview.chromium.org/4169001/diff/76003/15019#newcode89
chrome/browser/chromeos/options/wifi_config_view.h:89: WifiNetwork* wifi_;
scoped_ptr<> (never mind that parent_ also contains a pointer to, presumably,
the same network. Eventually we're hoping to deprecate these anyway...)

[zel, 2010-10-27 19:55:17]

http://codereview.chromium.org/4169001/diff/20022/81002
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/4169001/diff/20022/81002#newcode108
chrome/browser/chromeos/cros/network_library.h:108: bool operator()(const
WirelessNetwork* a) {
On 2010/10/27 01:08:07, Charlie Lee wrote:
> No need to indent this method

Done.

http://codereview.chromium.org/4169001/diff/20022/81002#newcode395
chrome/browser/chromeos/cros/network_library.h:395: // Returns the list of
remembered celular networks.
On 2010/10/27 01:08:07, Charlie Lee wrote:
> Comment wrong. Why was remembered cellular networks removed?

Done.

http://codereview.chromium.org/4169001/diff/76003/15003
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/4169001/diff/76003/15003#newcode782
chrome/browser/chromeos/cros/network_library.cc:782: virtual void
SaveCellularNetwork(const CellularNetwork* network) {
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> DCHECK(network)

Done.

http://codereview.chromium.org/4169001/diff/76003/15003#newcode789
chrome/browser/chromeos/cros/network_library.cc:789: virtual void
SaveWifiNetwork(const WifiNetwork* network) {
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> DCHECK(network)

Done.

http://codereview.chromium.org/4169001/diff/76003/15003#newcode810
chrome/browser/chromeos/cros/network_library.cc:810:
remembered_wifi_networks_.end());
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> I believe this will delete the range [found_iter, end), which is not what you
> want.
> I think what you want to do is:
> iter = remove_if(...)  // this re-orders the list so that the matching entries
> are at the end of the list
> STLDeleteContainerPointers(iter, end())
> erase(iter, end();
> 

this will first call "delete iter->second" of all found elements. remove_if
below will take care of remembered_wifi_networks_ collection

http://codereview.chromium.org/4169001/diff/76003/15003#newcode988
chrome/browser/chromeos/cros/network_library.cc:988: DCHECK(network &&
network->cellular_network());
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> A bug in Cashew (the data plan service) could cause it to trigger this
callback,
> even if we don't have a cellular network. We might want to just exit here if
> cellular_network() is NULL.

jason is rewriting this part now

http://codereview.chromium.org/4169001/diff/76003/15003#newcode1004
chrome/browser/chromeos/cros/network_library.cc:1004: *ethernet = NULL;
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> This should be handled in ClearNetworks(); DCHECK(*ethernet == NULL).
> 

the method should clear its output params

http://codereview.chromium.org/4169001/diff/76003/15003#newcode1021
chrome/browser/chromeos/cros/network_library.cc:1021: if (service->type ==
TYPE_ETHERNET && !((*ethernet)->connected()))
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> *ethernet == NULL here; (*ethernet)->connected() is bogus. 

Done.

http://codereview.chromium.org/4169001/diff/76003/15003#newcode1222
chrome/browser/chromeos/cros/network_library.cc:1222: if (ethernet_)
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> if () not strictly necessary.

i know, but i still like it

http://codereview.chromium.org/4169001/diff/76003/15003#newcode1250
chrome/browser/chromeos/cros/network_library.cc:1250: std::string
prev_service_path = cellular_ ?
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> prev_cellular_path? Wasn't clear right away that this was just a cellular
thing.

Done.

http://codereview.chromium.org/4169001/diff/76003/15004
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/4169001/diff/76003/15004#newcode72
chrome/browser/chromeos/cros/network_library.h:72: explicit Network(const
chromeos::Network& network);
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> chromeos:: not needed here and elsewhere since all of this is in the chromeos
> namespace.
> 

Done.

http://codereview.chromium.org/4169001/diff/76003/15004#newcode447
chrome/browser/chromeos/cros/network_library.h:447: // Forget the passed in
wireless (either cellular or wifi) network.
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> Fix comment: // Forget the wifi network corresponding to service_path.
> 

Done.

http://codereview.chromium.org/4169001/diff/76003/15005
File chrome/browser/chromeos/dom_ui/internet_options_handler.cc (right):

http://codereview.chromium.org/4169001/diff/76003/15005#newcode485
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:485: if ((network =
cros->FindWifiNetworkByPath(service_path))) {
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> Would prefer assignment outside of if() here

Done.

http://codereview.chromium.org/4169001/diff/76003/15005#newcode549
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:549: const
chromeos::Network* net, chromeos::NetworkLibrary* cros) {
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> DCHECK(net)

Done.

http://codereview.chromium.org/4169001/diff/76003/15005#newcode574
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:574: if (!(wireless =
cros->FindWifiNetworkByPath(net->service_path()))) {
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> Prefer assignment outside of if()

Done.

http://codereview.chromium.org/4169001/diff/76003/15005#newcode603
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:603: if (!(cellular =
cros->FindCellularNetworkByPath(net->service_path()))) {
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> Prefer assignment outside of if()

Done.

http://codereview.chromium.org/4169001/diff/76003/15005#newcode679
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:679: if ((network =
cros->FindWifiNetworkByPath(service_path))) {
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> Prefer assignment outside of if()

Done.

http://codereview.chromium.org/4169001/diff/76003/15005#newcode707
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:707: if ((network =
cros->FindWifiNetworkByPath(service_path))) {
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> Prefer assignment outside of if()

Done.

http://codereview.chromium.org/4169001/diff/76003/15005#newcode777
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:777: if ((cellular =
cros->FindCellularNetworkByPath(service_path))) {
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> Prefer assignment outside of if()

Done.

http://codereview.chromium.org/4169001/diff/76003/15005#newcode806
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:806: if ((cellular =
cros->FindCellularNetworkByPath(service_path))) {
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> Prefer assignment outside of if()

Done.

http://codereview.chromium.org/4169001/diff/76003/15007
File chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc (right):

http://codereview.chromium.org/4169001/diff/76003/15007#newcode360
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:360:
EvaluateCellularNetwork(network);
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> passing (NULL) explicitly might make it a little clearer that Evaluate handles
a
> NULL argument? 

Done.

http://codereview.chromium.org/4169001/diff/76003/15007#newcode388
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:388: PLAN_ACTIVATION_ERROR,
std::string());
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> Same comment about explicit NULL to ChangeState()

Done.

http://codereview.chromium.org/4169001/diff/76003/15007#newcode398
chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:398: //                       
 this);
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> // TODO() ?

Done.

http://codereview.chromium.org/4169001/diff/76003/15013
File chrome/browser/chromeos/options/cellular_config_view.cc (right):

http://codereview.chromium.org/4169001/diff/76003/15013#newcode73
chrome/browser/chromeos/options/cellular_config_view.cc:73: if (cellular_)
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> if() not strictly necessary

i know, still prefer to have it...

http://codereview.chromium.org/4169001/diff/76003/15014
File chrome/browser/chromeos/options/cellular_config_view.h (right):

http://codereview.chromium.org/4169001/diff/76003/15014#newcode53
chrome/browser/chromeos/options/cellular_config_view.h:53: CellularNetwork*
cellular_;
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> Strictly speaking, since we always copy the input network here, it doesn't
need
> to be a pointer. Alternately you could use a scoped_ptr<>.
> 

Done.

http://codereview.chromium.org/4169001/diff/76003/15019
File chrome/browser/chromeos/options/wifi_config_view.h (right):

http://codereview.chromium.org/4169001/diff/76003/15019#newcode89
chrome/browser/chromeos/options/wifi_config_view.h:89: WifiNetwork* wifi_;
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> scoped_ptr<> (never mind that parent_ also contains a pointer to, presumably,
> the same network. Eventually we're hoping to deprecate these anyway...)
> 

Done.

[Charlie Lee, 2010-10-27 22:47:55]

Zel, what was the reasoning behind removing the remembered cellular
networks? Did we decide to not bother with that because most users will only
have 1 cellular network set up anyways?

- Charlie


On Wed, Oct 27, 2010 at 12:55 PM, <zelidrag@chromium.org> wrote:

>
> http://codereview.chromium.org/4169001/diff/20022/81002
>
> File chrome/browser/chromeos/cros/network_library.h (right):
>
> http://codereview.chromium.org/4169001/diff/20022/81002#newcode108
> chrome/browser/chromeos/cros/network_library.h:108: bool
> operator()(const WirelessNetwork* a) {
> On 2010/10/27 01:08:07, Charlie Lee wrote:
>
>> No need to indent this method
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/20022/81002#newcode395
> chrome/browser/chromeos/cros/network_library.h:395: // Returns the list
> of remembered celular networks.
> On 2010/10/27 01:08:07, Charlie Lee wrote:
>
>> Comment wrong. Why was remembered cellular networks removed?
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15003
> File chrome/browser/chromeos/cros/network_library.cc (right):
>
> http://codereview.chromium.org/4169001/diff/76003/15003#newcode782
> chrome/browser/chromeos/cros/network_library.cc:782: virtual void
> SaveCellularNetwork(const CellularNetwork* network) {
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> DCHECK(network)
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15003#newcode789
> chrome/browser/chromeos/cros/network_library.cc:789: virtual void
> SaveWifiNetwork(const WifiNetwork* network) {
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> DCHECK(network)
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15003#newcode810
> chrome/browser/chromeos/cros/network_library.cc:810:
> remembered_wifi_networks_.end());
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> I believe this will delete the range [found_iter, end), which is not
>>
> what you
>
>> want.
>> I think what you want to do is:
>> iter = remove_if(...)  // this re-orders the list so that the matching
>>
> entries
>
>> are at the end of the list
>> STLDeleteContainerPointers(iter, end())
>> erase(iter, end();
>>
>
>
> this will first call "delete iter->second" of all found elements.
> remove_if below will take care of remembered_wifi_networks_ collection
>
>
> http://codereview.chromium.org/4169001/diff/76003/15003#newcode988
> chrome/browser/chromeos/cros/network_library.cc:988: DCHECK(network &&
> network->cellular_network());
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> A bug in Cashew (the data plan service) could cause it to trigger this
>>
> callback,
>
>> even if we don't have a cellular network. We might want to just exit
>>
> here if
>
>> cellular_network() is NULL.
>>
>
> jason is rewriting this part now
>
>
> http://codereview.chromium.org/4169001/diff/76003/15003#newcode1004
> chrome/browser/chromeos/cros/network_library.cc:1004: *ethernet = NULL;
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> This should be handled in ClearNetworks(); DCHECK(*ethernet == NULL).
>>
>
>
> the method should clear its output params
>
>
> http://codereview.chromium.org/4169001/diff/76003/15003#newcode1021
> chrome/browser/chromeos/cros/network_library.cc:1021: if (service->type
> == TYPE_ETHERNET && !((*ethernet)->connected()))
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> *ethernet == NULL here; (*ethernet)->connected() is bogus.
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15003#newcode1222
> chrome/browser/chromeos/cros/network_library.cc:1222: if (ethernet_)
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> if () not strictly necessary.
>>
>
> i know, but i still like it
>
>
> http://codereview.chromium.org/4169001/diff/76003/15003#newcode1250
> chrome/browser/chromeos/cros/network_library.cc:1250: std::string
> prev_service_path = cellular_ ?
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> prev_cellular_path? Wasn't clear right away that this was just a
>>
> cellular thing.
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15004
> File chrome/browser/chromeos/cros/network_library.h (right):
>
> http://codereview.chromium.org/4169001/diff/76003/15004#newcode72
> chrome/browser/chromeos/cros/network_library.h:72: explicit
> Network(const chromeos::Network& network);
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> chromeos:: not needed here and elsewhere since all of this is in the
>>
> chromeos
>
>> namespace.
>>
>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15004#newcode447
> chrome/browser/chromeos/cros/network_library.h:447: // Forget the passed
> in wireless (either cellular or wifi) network.
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> Fix comment: // Forget the wifi network corresponding to service_path.
>>
>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15005
> File chrome/browser/chromeos/dom_ui/internet_options_handler.cc (right):
>
> http://codereview.chromium.org/4169001/diff/76003/15005#newcode485
> chrome/browser/chromeos/dom_ui/internet_options_handler.cc:485: if
> ((network = cros->FindWifiNetworkByPath(service_path))) {
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> Would prefer assignment outside of if() here
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15005#newcode549
> chrome/browser/chromeos/dom_ui/internet_options_handler.cc:549: const
> chromeos::Network* net, chromeos::NetworkLibrary* cros) {
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> DCHECK(net)
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15005#newcode574
> chrome/browser/chromeos/dom_ui/internet_options_handler.cc:574: if
> (!(wireless = cros->FindWifiNetworkByPath(net->service_path()))) {
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> Prefer assignment outside of if()
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15005#newcode603
> chrome/browser/chromeos/dom_ui/internet_options_handler.cc:603: if
> (!(cellular = cros->FindCellularNetworkByPath(net->service_path()))) {
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> Prefer assignment outside of if()
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15005#newcode679
> chrome/browser/chromeos/dom_ui/internet_options_handler.cc:679: if
> ((network = cros->FindWifiNetworkByPath(service_path))) {
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> Prefer assignment outside of if()
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15005#newcode707
> chrome/browser/chromeos/dom_ui/internet_options_handler.cc:707: if
> ((network = cros->FindWifiNetworkByPath(service_path))) {
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> Prefer assignment outside of if()
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15005#newcode777
> chrome/browser/chromeos/dom_ui/internet_options_handler.cc:777: if
> ((cellular = cros->FindCellularNetworkByPath(service_path))) {
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> Prefer assignment outside of if()
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15005#newcode806
> chrome/browser/chromeos/dom_ui/internet_options_handler.cc:806: if
> ((cellular = cros->FindCellularNetworkByPath(service_path))) {
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> Prefer assignment outside of if()
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15007
> File chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc (right):
>
> http://codereview.chromium.org/4169001/diff/76003/15007#newcode360
> chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:360:
> EvaluateCellularNetwork(network);
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> passing (NULL) explicitly might make it a little clearer that Evaluate
>>
> handles a
>
>> NULL argument?
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15007#newcode388
> chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:388:
> PLAN_ACTIVATION_ERROR, std::string());
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> Same comment about explicit NULL to ChangeState()
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15007#newcode398
> chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc:398: //
>         this);
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> // TODO() ?
>>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15013
> File chrome/browser/chromeos/options/cellular_config_view.cc (right):
>
> http://codereview.chromium.org/4169001/diff/76003/15013#newcode73
> chrome/browser/chromeos/options/cellular_config_view.cc:73: if
> (cellular_)
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> if() not strictly necessary
>>
>
> i know, still prefer to have it...
>
>
> http://codereview.chromium.org/4169001/diff/76003/15014
> File chrome/browser/chromeos/options/cellular_config_view.h (right):
>
> http://codereview.chromium.org/4169001/diff/76003/15014#newcode53
> chrome/browser/chromeos/options/cellular_config_view.h:53:
> CellularNetwork* cellular_;
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> Strictly speaking, since we always copy the input network here, it
>>
> doesn't need
>
>> to be a pointer. Alternately you could use a scoped_ptr<>.
>>
>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/diff/76003/15019
> File chrome/browser/chromeos/options/wifi_config_view.h (right):
>
> http://codereview.chromium.org/4169001/diff/76003/15019#newcode89
> chrome/browser/chromeos/options/wifi_config_view.h:89: WifiNetwork*
> wifi_;
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
>
>> scoped_ptr<> (never mind that parent_ also contains a pointer to,
>>
> presumably,
>
>> the same network. Eventually we're hoping to deprecate these
>>
> anyway...)
>
>
> Done.
>
>
> http://codereview.chromium.org/4169001/show
>

[stevenjb, 2010-10-28 01:42:03]

http://codereview.chromium.org/4169001/diff/76003/15003
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/4169001/diff/76003/15003#newcode753
chrome/browser/chromeos/cros/network_library.cc:753: if (!EnsureCrosLoaded() ||
!network)
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> DCHECK() or !network, don't need both. (or, do both checks everywhere if we
want
> to crash on debug and NOOP on release)
?

http://codereview.chromium.org/4169001/diff/76003/15003#newcode758
chrome/browser/chromeos/cros/network_library.cc:758: virtual void
DisconnectFromWirelessNetwork(const WirelessNetwork* network) {
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> DCHECK(network)
?

http://codereview.chromium.org/4169001/diff/76003/15003#newcode810
chrome/browser/chromeos/cros/network_library.cc:810:
remembered_wifi_networks_.end());
On 2010/10/27 19:55:17, zel wrote:
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
> > I believe this will delete the range [found_iter, end), which is not what
you
> > want.
> > I think what you want to do is:
> > iter = remove_if(...)  // this re-orders the list so that the matching
entries
> > are at the end of the list
> > STLDeleteContainerPointers(iter, end())
> > erase(iter, end();
> > 
> 
> this will first call "delete iter->second" of all found elements. remove_if
> below will take care of remembered_wifi_networks_ collection
> 

find_if returns the first item found, which might be begin(), which would result
in STLDeleteCountainerPointers(begin(), end()), which will delete the entire
list, not just the first element.
remove_if() will actually reorder the list and do the right thing; if you call
remove_if() in the STLDelete call, and find_if() in the erase call, that would
work, although it might be less confusing to save the results of remove_if().

http://codereview.chromium.org/4169001/diff/76003/15003#newcode1004
chrome/browser/chromeos/cros/network_library.cc:1004: *ethernet = NULL;
On 2010/10/27 19:55:17, zel wrote:
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
> > This should be handled in ClearNetworks(); DCHECK(*ethernet == NULL).
> > 
> 
> the method should clear its output params

OK, but it looks like a potential memory leak if *ethernet != NULL, even though
in practice that would never be true.

http://codereview.chromium.org/4169001/diff/76003/15003#newcode1222
chrome/browser/chromeos/cros/network_library.cc:1222: if (ethernet_)
On 2010/10/27 19:55:17, zel wrote:
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
> > if () not strictly necessary.
> 
> i know, but i still like it
Fair enough :)

http://codereview.chromium.org/4169001/diff/107002/58002#newcode253
chrome/browser/chromeos/cros/network_library.cc:253: type_ = TYPE_CELLULAR;
We need to copy data_plans_ here, don't we?

http://codereview.chromium.org/4169001/diff/107002/58002#newcode687
chrome/browser/chromeos/cros/network_library.cc:687: const std::string&
certpath) {
 DCHECK(network)

[stevenjb, 2010-10-28 18:47:23]

I believe this will do what you want for ForgetWifiNetwork().


virtual void ForgetWifiNetwork(const std::string& service_path) {
  if (EnsureCrosLoaded()) {
    if (DeleteRememberedService(service_path.c_str())) {
      // Update local cache.
      // remove_if moves the matching entries to the end of the list and
      // returns an iterator pointing to the first matching entry (the new
'end').
      WifiNetworkVector::iterator iter =
          std::remove_if(remembered_wifi_networks_.begin(),
                         remembered_wifi_networks_.end(),
                         WirelessNetwork::ServicePathEq(service_path));
      // Delete the pointers.
      STLDeleteContainerPointers(iter, remembered_wifi_networks_.end());
      // Erase the entries from the list.
      remembered_wifi_networks_.erase(iter, remembered_wifi_networks_.end());
      // Notify listeners.
      NotifyNetworkChanged();
    }
  }
}

[zel, 2010-10-28 20:22:34]

http://codereview.chromium.org/4169001/diff/76003/15003
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/4169001/diff/76003/15003#newcode753
chrome/browser/chromeos/cros/network_library.cc:753: if (!EnsureCrosLoaded() ||
!network)
On 2010/10/27 19:18:10, Steven Bennetts wrote:
> DCHECK() or !network, don't need both. (or, do both checks everywhere if we
want
> to crash on debug and NOOP on release)

I added the same type of check everywhere else.

http://codereview.chromium.org/4169001/diff/76003/15003#newcode758
chrome/browser/chromeos/cros/network_library.cc:758: virtual void
DisconnectFromWirelessNetwork(const WirelessNetwork* network) {
On 2010/10/28 01:42:03, Steven Bennetts wrote:
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
> > DCHECK(network)
> ?
> 

Done.

http://codereview.chromium.org/4169001/diff/76003/15003#newcode758
chrome/browser/chromeos/cros/network_library.cc:758: virtual void
DisconnectFromWirelessNetwork(const WirelessNetwork* network) {
On 2010/10/28 01:42:03, Steven Bennetts wrote:
> On 2010/10/27 19:18:10, Steven Bennetts wrote:
> > DCHECK(network)
> ?
> 

Done.

http://codereview.chromium.org/4169001/diff/76003/15003#newcode810
chrome/browser/chromeos/cros/network_library.cc:810:
remembered_wifi_networks_.end());
On 2010/10/28 01:42:03, Steven Bennetts wrote:
> On 2010/10/27 19:55:17, zel wrote:
> > On 2010/10/27 19:18:10, Steven Bennetts wrote:
> > > I believe this will delete the range [found_iter, end), which is not what
> you
> > > want.
> > > I think what you want to do is:
> > > iter = remove_if(...)  // this re-orders the list so that the matching
> entries
> > > are at the end of the list
> > > STLDeleteContainerPointers(iter, end())
> > > erase(iter, end();
> > > 
> > 
> > this will first call "delete iter->second" of all found elements. remove_if
> > below will take care of remembered_wifi_networks_ collection
> > 
> 
> find_if returns the first item found, which might be begin(), which would
result
> in STLDeleteCountainerPointers(begin(), end()), which will delete the entire
> list, not just the first element.
> remove_if() will actually reorder the list and do the right thing; if you call
> remove_if() in the STLDelete call, and find_if() in the erase call, that would
> work, although it might be less confusing to save the results of remove_if().
> 

I have rewritten this part

http://codereview.chromium.org/4169001/diff/76003/15003#newcode1004
chrome/browser/chromeos/cros/network_library.cc:1004: *ethernet = NULL;
On 2010/10/28 01:42:03, Steven Bennetts wrote:
> On 2010/10/27 19:55:17, zel wrote:
> > On 2010/10/27 19:18:10, Steven Bennetts wrote:
> > > This should be handled in ClearNetworks(); DCHECK(*ethernet == NULL).
> > > 
> > 
> > the method should clear its output params
> 
> OK, but it looks like a potential memory leak if *ethernet != NULL, even
though
> in practice that would never be true.
> 

I will just assert that *ethernet is NULL already then

[stevenjb, 2010-10-28 22:00:30]

LGTM with minor nit

http://codereview.chromium.org/4169001/diff/40003/75004
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/4169001/diff/40003/75004#newcode1036
chrome/browser/chromeos/cros/network_library.cc:1036: // Create placeholder
network for etherner even if the service is not
nit: s/etherner/ethernet/

[zel, 2010-10-28 22:03:28]

will push it when trybots cycle green

http://codereview.chromium.org/4169001/diff/40003/75004
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/4169001/diff/40003/75004#newcode1036
chrome/browser/chromeos/cros/network_library.cc:1036: // Create placeholder
network for etherner even if the service is not
On 2010/10/28 22:00:30, Steven Bennetts wrote:
> nit: s/etherner/ethernet/
> 

Done.