Refactor WifiNetwork, CellularNetwork, and EthernetNetwork into classes to ma...

Refactor WifiNetwork, CellularNetwork, and EthernetNetwork into classes to make the code cleaner.
BUG=none
TEST=ran browser tests and also manually made sure everything still works.

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

[Charlie Lee, 2010-05-05 23:25:43]

Scott, can you take a look. Let me know if the class implementation in the .h
file should be moved to the .cc file.

Thanks,
Charlie

[sky, 2010-05-05 23:44:33]

http://codereview.chromium.org/2010001/diff/1/10
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/2010001/diff/1/10#newcode238
chrome/browser/chromeos/cros/network_library.cc:238: if (service.type ==
TYPE_ETHERNET) {
The new code is so much clearer:)

http://codereview.chromium.org/2010001/diff/1/11
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/2010001/diff/1/11#newcode22
chrome/browser/chromeos/cros/network_library.h:22: protected:
Style guide says protected should be after public.

http://codereview.chromium.org/2010001/diff/1/11#newcode39
chrome/browser/chromeos/cros/network_library.h:39: virtual void Clear() {
As you asked, the implementation of this and SetService should be in the .cc.

http://codereview.chromium.org/2010001/diff/1/11#newcode47
chrome/browser/chromeos/cros/network_library.h:47: virtual void SetService(const
ServiceInfo& service) {
I would have thought a method like this was setting service_. Should this be
ConfigureFromService or SetFromService?

http://codereview.chromium.org/2010001/diff/1/11#newcode83
chrome/browser/chromeos/cros/network_library.h:83: protected:
after public.

http://codereview.chromium.org/2010001/diff/1/11#newcode146
chrome/browser/chromeos/cros/network_library.h:146: encryption_ = encryption; }
put the } on a newline.

http://codereview.chromium.org/2010001/diff/1/11#newcode148
chrome/browser/chromeos/cros/network_library.h:148: passphrase_ = passphrase; }
put the } on a newline.

[Charlie Lee, 2010-05-06 00:00:59]

http://codereview.chromium.org/2010001/diff/1/11
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/2010001/diff/1/11#newcode22
chrome/browser/chromeos/cros/network_library.h:22: protected:
On 2010/05/05 23:44:33, sky wrote:
> Style guide says protected should be after public.

Done.

http://codereview.chromium.org/2010001/diff/1/11#newcode39
chrome/browser/chromeos/cros/network_library.h:39: virtual void Clear() {
On 2010/05/05 23:44:33, sky wrote:
> As you asked, the implementation of this and SetService should be in the .cc.

Done.

http://codereview.chromium.org/2010001/diff/1/11#newcode47
chrome/browser/chromeos/cros/network_library.h:47: virtual void SetService(const
ServiceInfo& service) {
On 2010/05/05 23:44:33, sky wrote:
> I would have thought a method like this was setting service_. Should this be
> ConfigureFromService or SetFromService?

Done.

http://codereview.chromium.org/2010001/diff/1/11#newcode83
chrome/browser/chromeos/cros/network_library.h:83: protected:
On 2010/05/05 23:44:33, sky wrote:
> after public.

Done.

http://codereview.chromium.org/2010001/diff/1/11#newcode146
chrome/browser/chromeos/cros/network_library.h:146: encryption_ = encryption; }
On 2010/05/05 23:44:33, sky wrote:
> put the } on a newline.

Done.

http://codereview.chromium.org/2010001/diff/1/11#newcode148
chrome/browser/chromeos/cros/network_library.h:148: passphrase_ = passphrase; }
On 2010/05/05 23:44:33, sky wrote:
> put the } on a newline.

Done.

[sky, 2010-05-06 00:03:25]

LGTM

[Nikita (slow), 2010-05-06 13:40:34]

Great, I hope at some point we'll refactor how network list is represented in
NetworkMenuButton and InternetPageView.
NetworkList has such TODO :)

[Charlie Lee, 2010-05-06 16:01:31]

Yes. I looked at it but it was too hard to do that also in the same
CL. I will get to that soon.

On Thursday, May 6, 2010,  <nkostylev@chromium.org> wrote:
> Great, I hope at some point we'll refactor how network list is represented in
> NetworkMenuButton and InternetPageView.
> NetworkList has such TODO :)
>
>
> http://codereview.chromium.org/2010001/show
>

-- 

- Charlie

[tfarina (gmail-do not use), 2010-05-06 16:21:07]

On 2010/05/06 16:01:31, chocobo wrote:
> Yes. I looked at it but it was too hard to do that also in the same
> CL. I will get to that soon.
> 
Cool, thanks Charlie. Also for futher reference, I tried to step in, in this CL
(http://codereview.chromium.org/1755021).

http://codereview.chromium.org/2010001/diff/16001/17009
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/2010001/diff/16001/17009#newcode303
chrome/browser/chromeos/cros/network_library.cc:303: if (service.type ==
TYPE_ETHERNET) {
nit: I'd prefer to not have {} around single line if statements.

http://codereview.chromium.org/2010001/diff/16001/17010
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/2010001/diff/16001/17010#newcode1
chrome/browser/chromeos/cros/network_library.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/2010001/diff/16001/17010#newcode51
chrome/browser/chromeos/cros/network_library.h:51: };
Is this class and the others below allowed to be copied and assigned (I didn't
search), otherwise DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/2010001/diff/16001/17010#newcode361
chrome/browser/chromeos/cros/network_library.h:361: return available_devices_ &
(1 << TYPE_ETHERNET); }
nit: I'd like to put the } into the following line, below too. But is up to you.

http://codereview.chromium.org/2010001/diff/16001/17006
File chrome/browser/chromeos/options/internet_page_view.cc (right):

http://codereview.chromium.org/2010001/diff/16001/17006#newcode1
chrome/browser/chromeos/options/internet_page_view.cc:1: // Copyright (c) 2009
The Chromium Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/2010001/diff/16001/17004
File chrome/browser/chromeos/options/network_config_view.cc (right):

http://codereview.chromium.org/2010001/diff/16001/17004#newcode1
chrome/browser/chromeos/options/network_config_view.cc:1: // Copyright (c) 2009
The Chromium Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/2010001/diff/16001/17005
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

http://codereview.chromium.org/2010001/diff/16001/17005#newcode1
chrome/browser/chromeos/options/wifi_config_view.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
nit: 2010

[Charlie Lee, 2010-05-06 17:48:17]

http://codereview.chromium.org/2010001/diff/16001/17009
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/2010001/diff/16001/17009#newcode303
chrome/browser/chromeos/cros/network_library.cc:303: if (service.type ==
TYPE_ETHERNET) {
On 2010/05/06 16:21:07, tfarina wrote:
> nit: I'd prefer to not have {} around single line if statements.

Done.

http://codereview.chromium.org/2010001/diff/16001/17010
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/2010001/diff/16001/17010#newcode1
chrome/browser/chromeos/cros/network_library.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
On 2010/05/06 16:21:07, tfarina wrote:
> nit: 2010

Done.

http://codereview.chromium.org/2010001/diff/16001/17010#newcode51
chrome/browser/chromeos/cros/network_library.h:51: };
This is actually copied. I plan to refactor it to not do any copying. Once
that's done, I will add the DISALLOW_COPY_AND_ASSIGN.

On 2010/05/06 16:21:07, tfarina wrote:
> Is this class and the others below allowed to be copied and assigned (I didn't
> search), otherwise DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/2010001/diff/16001/17010#newcode361
chrome/browser/chromeos/cros/network_library.h:361: return available_devices_ &
(1 << TYPE_ETHERNET); }
On 2010/05/06 16:21:07, tfarina wrote:
> nit: I'd like to put the } into the following line, below too. But is up to
you.

Done.

http://codereview.chromium.org/2010001/diff/16001/17006
File chrome/browser/chromeos/options/internet_page_view.cc (right):

http://codereview.chromium.org/2010001/diff/16001/17006#newcode1
chrome/browser/chromeos/options/internet_page_view.cc:1: // Copyright (c) 2009
The Chromium Authors. All rights reserved.
On 2010/05/06 16:21:07, tfarina wrote:
> nit: 2010

Done.

http://codereview.chromium.org/2010001/diff/16001/17004
File chrome/browser/chromeos/options/network_config_view.cc (right):

http://codereview.chromium.org/2010001/diff/16001/17004#newcode1
chrome/browser/chromeos/options/network_config_view.cc:1: // Copyright (c) 2009
The Chromium Authors. All rights reserved.
On 2010/05/06 16:21:07, tfarina wrote:
> nit: 2010

Done.

http://codereview.chromium.org/2010001/diff/16001/17005
File chrome/browser/chromeos/options/wifi_config_view.cc (right):

http://codereview.chromium.org/2010001/diff/16001/17005#newcode1
chrome/browser/chromeos/options/wifi_config_view.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
On 2010/05/06 16:21:07, tfarina wrote:
> nit: 2010

Done.