This factors out all of the parsing code from the network library

This factors out all of the parsing code from the network library
network classes into a separate hierarchy of parsers.  This is
in preparation for creating a second parser hierarchy that can be
used to create networks from a configuration file instead of from
flimflam.

In the course of doing this, I updated much of the style
inconsistencies in this code (mainly having to do with passing
arguments).

BUG=chromium-os:15457
TEST=ran on device, connected to new network, forgot a network.

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

[Greg Spencer (Chromium), 2011-08-04 23:02:14]

Steven,

Here's that enormous yak-shaving CL I warned you about.

Charlie, I've CC'd you on this one so you can take a look, since you'll be
taking it over.  You're welcome to review too if you want, but I didn't want to
commit you to it unilaterally. :)

-Greg.

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

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:435:
EthernetNetwork::EthernetNetwork(const std::string& service_path)
When we want to support a new parser, we can add a second constructor here that
takes the parser as well.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3737: case
PROPERTY_INDEX_ARP_GATEWAY:
I added this to stop the "unhandled key" warning below, although it's still
unhandled, really, we just know about it.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3808:
NativeNetworkParser::ParseConnectionType(technology);
I think this call to the native parser is unfortunate, but necessary.  The
implementation deals directly with the native network layer, so it really should
be using it directly here, in my opinion.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.h (left):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:254: SIMPinRequire
sim_pin_required() const { return sim_pin_required_; }
The capitalization of this one was even internally inconsistent (SIM and PIN are
both acronyms, but were capitalized differently).

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:323: enum ProviderType {
I moved this out of the VirtualNetwork class because it was more consistent with
our other enums for the network library.

I considered adding all of the enums to their own namespace, but decided it
wasn't necessary as long as the enum names were prefixed already.

[stevenjb, 2011-08-05 18:46:58]

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/native_network_constants.h (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/native_network_constants.h:11: #include
"chrome/browser/chromeos/cros/network_library.h"
We should forward declare ConnectionType, etc, instead of including
network_library.h here.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/native_network_parser.cc (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/native_network_parser.cc:118: const
EnumMapper<PropertyIndex>* get_mapper() {
nit: get_native_mapper for clarity?

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/native_network_parser.h (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/native_network_parser.h:25: virtual ConnectionType
ParseType(const std::string& type) OVERRIDE;
Are the rest of these overrides (in which case we should add OVERRIDE) or not
(in which case we should add a comment to separate them).

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/native_network_parser.h:66: virtual
~NativeEthernetNetworkParser();
Maybe add ParseValue() here, even if it just calls the base class parser, to
make that explicit?

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/native_network_parser.h:90: protected:
Do want the following to be virtual, i.e. would it ever make sense to
intentionally override them? Same for the new virtual methods in Cellular and
Virtual.

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

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3161: if (key == NULL || value
== NULL)
Add a NULL check for path since string conversion would crash on NULL.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3163:
networklib->UpdateNetworkStatus(path, key, *value);
nit: Even though we do a NULL check here, we should still avoid explicit string
conversion, so add std::string(path), etc, here and elsewhere.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3204: if
(!device->UpdateStatus(key, value, &index)) {
This logic is a little hard to follow now - index, which is set in the first if
clause, gets used in the second if clause. I would either invert the logic, or
pull the UpdateStatus call out of the if clause.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3737: case
PROPERTY_INDEX_ARP_GATEWAY:
On 2011/08/04 23:02:14, Greg Spencer (Chromium) wrote:
> I added this to stop the "unhandled key" warning below, although it's still
> unhandled, really, we just know about it. 
Thanks, that's why there is a warning here, so we know which manager properties
we are ignoring.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3808:
NativeNetworkParser::ParseConnectionType(technology);
On 2011/08/04 23:02:14, Greg Spencer (Chromium) wrote:
> I think this call to the native parser is unfortunate, but necessary.  The
> implementation deals directly with the native network layer, so it really
should
> be using it directly here, in my opinion.

Just a thought, but the 'Native' has been bothering me a little. I wonder if
CrosNetworkParser would be better, since really it is libcros (or, ultimately,
chromeos) specific, just as NetworkLibraryImplCros is.

Ultimately I think that CellularDataPlan should also be re-factored to also do
its parsing in Chrome instead of libcros, at which point this code would be in
the parser. For now I think this is fine.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:4132: << " Type: " <<
ConnectionTypeToString(remembered->type());
nit: align <<

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:4234: LOG(WARNING) << " NULL
Device Dictionary: " << json;
I think we should move the detailed output to the parser code and keep json out
of NetworkLibrary, just logging device_path here.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:54: friend class
NativeVirtualNetworkParser;
On the one hand this is convenient, on the other hand, it might be better to
explicitly pair friendships with the specific parser type. This could help catch
places where we are parsing a WirelessNetwork property only for WifiNetwork,
etc?

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_parser.cc (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_parser.cc:15: const
EnumMapper<PropertyIndex>* mapper) : mapper_(mapper) {
Should CHECK(mapper) here since we access it without checking it elsewhere.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_parser.cc:63: } else if (VLOG_IS_ON(2)) {
nit: No need for else here.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_parser.cc:75: : mapper_(mapper) {
CHECK(mapper)

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_parser.cc:141:
base::JSONWriter::Write(&info, true, &json);
json unused here

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_parser.cc:157: } else if (VLOG_IS_ON(2)) {
nit: no else

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_parser.h (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_parser.h:111: // Called from the
NetworkLibrary when a new network is encountered.
More generic description (since it might be used from the policy code)? Maybe ad
a similar comment to NetworkDeviceParser?

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_parser.h:114: 
Should probably comment the following methods also, for this and
NetworkDeviceParser.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_parser.h:128: const std::string&
service_path);
Does this need to be a static member, or could it be moved into a local
namespace in the .cc file?

[Greg Spencer (Chromium), 2011-08-11 22:19:31]

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/native_network_constants.h (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/native_network_constants.h:11: #include
"chrome/browser/chromeos/cros/network_library.h"
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> We should forward declare ConnectionType, etc, instead of including
> network_library.h here.

I would, but they are enums, so not forward-declarable.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/native_network_parser.cc (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/native_network_parser.cc:118: const
EnumMapper<PropertyIndex>* get_mapper() {
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> nit: get_native_mapper for clarity?

Done.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/native_network_parser.h (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/native_network_parser.h:25: virtual ConnectionType
ParseType(const std::string& type) OVERRIDE;
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> Are the rest of these overrides (in which case we should add OVERRIDE) or not
> (in which case we should add a comment to separate them).

Done.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/native_network_parser.h:66: virtual
~NativeEthernetNetworkParser();
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> Maybe add ParseValue() here, even if it just calls the base class parser, to
> make that explicit?

Well, but that adds an additional virtual function call that isn't needed. 
Maybe I can just add a comment to that effect?

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/native_network_parser.h:90: protected:
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> Do want the following to be virtual, i.e. would it ever make sense to
> intentionally override them? Same for the new virtual methods in Cellular and
> Virtual.

You're right.  And they're just helper functions anyhow. Done.

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

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3163:
networklib->UpdateNetworkStatus(path, key, *value);
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> nit: Even though we do a NULL check here, we should still avoid explicit
string
> conversion, so add std::string(path), etc, here and elsewhere.

You mean implicit, right?

OK, I've added these in the callbacks where they're needed.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3204: if
(!device->UpdateStatus(key, value, &index)) {
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> This logic is a little hard to follow now - index, which is set in the first
if
> clause, gets used in the second if clause. I would either invert the logic, or
> pull the UpdateStatus call out of the if clause.

OK, I've switched it, it should be more clear now.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3808:
NativeNetworkParser::ParseConnectionType(technology);
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> Just a thought, but the 'Native' has been bothering me a little.
> I wonder if CrosNetworkParser would be better, since really it
> is libcros (or, ultimately, chromeos) specific, just as 
> NetworkLibraryImplCros is.

Well, but we're already in the chromeos/cros directory, and I wanted to avoid
"FlimFlam" because it's going to change.  I can change it if it really bothers
you though.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:4132: << " Type: " <<
ConnectionTypeToString(remembered->type());
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> nit: align <<

Done.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:4234: LOG(WARNING) << " NULL
Device Dictionary: " << json;
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> I think we should move the detailed output to the parser code and keep json
out
> of NetworkLibrary, just logging device_path here.

Sorry, that was some debugging code I accidentally left in there.  It's gone
now.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:54: friend class
NativeVirtualNetworkParser;
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> On the one hand this is convenient, on the other hand, it might be better to
> explicitly pair friendships with the specific parser type. This could help
catch
> places where we are parsing a WirelessNetwork property only for WifiNetwork,
> etc?

Yeah, I guess... The Wireless parser needs internals of the NetworkParser
though, for instance, so it has to be in each place, and I was trying to
simplify the management when you added a new set of parsers, and keep there from
being big stanzas of similar-but-not-identical friends in each class.

I'll split them out and you can decide if I should go back or not. :)

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_parser.cc (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_parser.cc:63: } else if (VLOG_IS_ON(2)) {
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> nit: No need for else here.

Duh. Removed.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_parser.cc:141:
base::JSONWriter::Write(&info, true, &json);
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> json unused here

Removed.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_parser.cc:157: } else if (VLOG_IS_ON(2)) {
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> nit: no else

Done.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_parser.h (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_parser.h:114: 
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> Should probably comment the following methods also, for this and
> NetworkDeviceParser.

Done.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_parser.h:128: const std::string&
service_path);
On 2011/08/05 18:46:58, Steven Bennetts wrote:
> Does this need to be a static member, or could it be moved into a local
> namespace in the .cc file?

Huh.  I guess I changed things.  It used to be needed in several places, but I
guess I factored that out again without realizing it.  I'll move this into an
anonymous namespace inside the .cc.

[stevenjb, 2011-08-12 21:32:37]

I'd like to do a pass to see how many explicit friendships we can avoid, but we
do not need to do that now.
LGTM

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/native_network_constants.h (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/native_network_constants.h:11: #include
"chrome/browser/chromeos/cros/network_library.h"
On 2011/08/11 22:19:32, Greg Spencer (Chromium) wrote:
> On 2011/08/05 18:46:58, Steven Bennetts wrote:
> > We should forward declare ConnectionType, etc, instead of including
> > network_library.h here.
> 
> I would, but they are enums, so not forward-declarable.

Duh. Of course.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/native_network_parser.h (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/native_network_parser.h:66: virtual
~NativeEthernetNetworkParser();
On 2011/08/11 22:19:32, Greg Spencer (Chromium) wrote:
> On 2011/08/05 18:46:58, Steven Bennetts wrote:
> > Maybe add ParseValue() here, even if it just calls the base class parser, to
> > make that explicit?
> 
> Well, but that adds an additional virtual function call that isn't needed. 
> Maybe I can just add a comment to that effect?

Fair enough. I don't think the overhead really matters, but it's obvious enough
without the empty function.

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

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3163:
networklib->UpdateNetworkStatus(path, key, *value);
On 2011/08/11 22:19:32, Greg Spencer (Chromium) wrote:
> On 2011/08/05 18:46:58, Steven Bennetts wrote:
> > nit: Even though we do a NULL check here, we should still avoid explicit
> string
> > conversion, so add std::string(path), etc, here and elsewhere.
> 
> You mean implicit, right?
> 
> OK, I've added these in the callbacks where they're needed.

Right, yes. Cool.

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/7453051/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:54: friend class
NativeVirtualNetworkParser;
On 2011/08/11 22:19:32, Greg Spencer (Chromium) wrote:
> On 2011/08/05 18:46:58, Steven Bennetts wrote:
> > On the one hand this is convenient, on the other hand, it might be better to
> > explicitly pair friendships with the specific parser type. This could help
> catch
> > places where we are parsing a WirelessNetwork property only for WifiNetwork,
> > etc?
> 
> Yeah, I guess... The Wireless parser needs internals of the NetworkParser
> though, for instance, so it has to be in each place, and I was trying to
> simplify the management when you added a new set of parsers, and keep there
from
> being big stanzas of similar-but-not-identical friends in each class.
> 
> I'll split them out and you can decide if I should go back or not. :)

I think this is a good compromise.

http://codereview.chromium.org/7453051/diff/13002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.h:449:
NETWORK_LIBRARY_PARSER_FRIENDS;
I am curious why network parsers are modifying NetworkDevice, or accessing
private data? It seems like we should change the interface rather than having so
many friends.

http://codereview.chromium.org/7453051/diff/13002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.h:707: 
In theory, it seems like only NativeNetworkParser() should need to be a friend,
and anything that modifies a shared parameter should be in the base class. I
realize that may not be pragmatic however.

[Greg Spencer (Chromium), 2011-08-12 23:45:19]

http://codereview.chromium.org/7453051/diff/13002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/7453051/diff/13002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.h:449: friend class
NativeVirtualNetworkParser;
On 2011/08/12 21:32:37, Steven Bennetts wrote:
> I am curious why network parsers are modifying NetworkDevice, or accessing
> private data? It seems like we should change the interface rather than having
so
> many friends.

OK, turns out they're not.  I've fixed all of these classes to just use the
minimal set of friends required, and in most cases it's just one or two.

http://codereview.chromium.org/7453051/diff/13002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.h:707: friend class
NativeVirtualNetworkParser;
On 2011/08/12 21:32:37, Steven Bennetts wrote:
> In theory, it seems like only NativeNetworkParser() should need to be a
friend,
> and anything that modifies a shared parameter should be in the base class. I
> realize that may not be pragmatic however.

I tried that approach, and it leads to a lot of protected functions in the base
class that look like:

static void set_network_name(Network* network, const std::string& name) {
network->set_name(name); }

and the call sites aren't as simple either, instead of:

network->set_name(name);

you get:

NativeNetworkParser::set_network_name(network, name);

It's not horrible, but it also meant that for every attribute added to the
network class, you have to go out to all to any parser that needs to set it and
add a function like this, so I opted for something more centralized: all the
accessors are next to the variables they set in the network class.

This way, you *could* set the private var directly, but using the private setter
gets you most of the benefits of having a more public setter, while restricting
use to classes that need it, and still you have the independence of changing the
representation of the private var without changing all the call sites if needed.

I could certainly be convinced that another method was better, but at this point
I've tried making all the setters public (with no friends needed), making the
subclasses all go through the base class, and just making the subclasses friends
where necessary, and the last one appears to be the best, so that's why I picked
it.

[stevenjb, 2011-08-13 00:01:45]

LEBTM (looks even better to me)
LGTM (for the bots)

http://codereview.chromium.org/7453051/diff/13002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/7453051/diff/13002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.h:449: friend class
NativeVirtualNetworkParser;
On 2011/08/12 23:45:19, Greg Spencer (Chromium) wrote:
> On 2011/08/12 21:32:37, Steven Bennetts wrote:
> > I am curious why network parsers are modifying NetworkDevice, or accessing
> > private data? It seems like we should change the interface rather than
having
> so
> > many friends.
> 
> OK, turns out they're not.  I've fixed all of these classes to just use the
> minimal set of friends required, and in most cases it's just one or two.

Awesome, this looks much better to me.

http://codereview.chromium.org/7453051/diff/13002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.h:707: friend class
NativeVirtualNetworkParser;
On 2011/08/12 23:45:19, Greg Spencer (Chromium) wrote:
> On 2011/08/12 21:32:37, Steven Bennetts wrote:
> > In theory, it seems like only NativeNetworkParser() should need to be a
> friend,
> > and anything that modifies a shared parameter should be in the base class. I
> > realize that may not be pragmatic however.
> 
> I tried that approach, and it leads to a lot of protected functions in the
base
> class that look like:
> 
> static void set_network_name(Network* network, const std::string& name) {
> network->set_name(name); }
> 
> and the call sites aren't as simple either, instead of:
> 
> network->set_name(name);
> 
> you get:
> 
> NativeNetworkParser::set_network_name(network, name);
> 
> It's not horrible, but it also meant that for every attribute added to the
> network class, you have to go out to all to any parser that needs to set it
and
> add a function like this, so I opted for something more centralized: all the
> accessors are next to the variables they set in the network class.
> 
> This way, you *could* set the private var directly, but using the private
setter
> gets you most of the benefits of having a more public setter, while
restricting
> use to classes that need it, and still you have the independence of changing
the
> representation of the private var without changing all the call sites if
needed.
> 
> I could certainly be convinced that another method was better, but at this
point
> I've tried making all the setters public (with no friends needed), making the
> subclasses all go through the base class, and just making the subclasses
friends
> where necessary, and the last one appears to be the best, so that's why I
picked
> it.

This is fine I think now that we are down to a more minimal set.