Infrastructure for accessing and manipulating network UI data.

Infrastructure for accessing and manipulating network UI data.

BUG=chromium-os:23124
TEST=compiles and passes tests, no observable changes.


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

[Mattias Nissler (ping if slow), 2011-11-29 14:47:38]

Here's the new ui data handling CL that moves all the ui data details to a
separate file.

[stevenjb, 2011-11-29 17:35:15]

Thanks for separating these out, I think this will be a lot easier to maintain.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_ui_data.cc (right):

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.cc:93: ONC_SOURCE_NONE);
Use CR_DEFINE_STATIC_LOCAL to make this static so it doesn't have to be
constructed on every call (see NetworkLibrary usage).

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.cc:109: :
controller_(CONTROLLER_USER) {}
nit: } on a separate line is easier to read outside of class declarations.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.cc:149:
default_value_.reset(default_value);
Do we always want to reset default_value, even if it isn't present in ui_data?

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.cc:162: std::string
NetworkPropertyUIData::EncodeController(Controller controller) {
Are we likely to need these outside of this class? If not, or not sure, we
should just make them file local functions unless we need them.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_ui_data.h (right):

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:23: // up your data members, and
call BuildDictionary().
This is a bit confusing - BuildDictionary is a member of NetworkPropertyUIData,
not NetworkUIData. Perhaps an example of how these two classes are used would
help?

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:38: void set_onc_source(ONCSource
source) { source_ = source; }
nit: should be set_source, or data member should be onc_source_

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:41: void SetProperty(const char*
property_key,
Maybe rename this SetPropertyUIData?

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:45: // values.
nit: Make it clear in the comment that |dict| is what is getting filled. Should
also mention that dict[kKeyONCSource] and dict[kKeyProperties] are the keys that
are getting filled.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:116: // Initializes the object
from the network-level UI data dictionary.
s/Initializes/Updates/ (otherwise looks like this is another constructor)

[stevenjb, 2011-11-29 17:45:48]

One more note:

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_ui_data.h (right):

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:128: bool editable() const {
return controller() != CONTROLLER_POLICY; }
I think we should probably either just expose Controller and use that
everywhere, or make that enum private and have a managed() accessor, so that we
are consistent in the implementation code.

[stevenjb, 2011-11-29 17:49:04]

OK, thinking this through more, we're not going to want to store a NetworkUIData
structure when we just need to know whether or not something is controlled, so I
suggest we eliminate the editable() accessor and use the enum everywhere.

[Mattias Nissler (ping if slow), 2011-11-29 20:48:47]

Hey Steven, I've just uploaded a new version that also adds unit tests. Please
take another look!

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_ui_data.cc (right):

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.cc:93: ONC_SOURCE_NONE);
On 2011/11/29 17:35:16, Steven Bennetts wrote:
> Use CR_DEFINE_STATIC_LOCAL to make this static so it doesn't have to be
> constructed on every call (see NetworkLibrary usage).

Done.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.cc:109: :
controller_(CONTROLLER_USER) {}
On 2011/11/29 17:35:16, Steven Bennetts wrote:
> nit: } on a separate line is easier to read outside of class declarations.

Some people prefer one style, some people the other. I really don't care :)
Done.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.cc:149:
default_value_.reset(default_value);
On 2011/11/29 17:35:16, Steven Bennetts wrote:
> Do we always want to reset default_value, even if it isn't present in ui_data?

Done.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.cc:162: std::string
NetworkPropertyUIData::EncodeController(Controller controller) {
On 2011/11/29 17:35:16, Steven Bennetts wrote:
> Are we likely to need these outside of this class? If not, or not sure, we
> should just make them file local functions unless we need them.

Done.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_ui_data.h (right):

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:23: // up your data members, and
call BuildDictionary().
On 2011/11/29 17:35:16, Steven Bennetts wrote:
> This is a bit confusing - BuildDictionary is a member of
NetworkPropertyUIData,
> not NetworkUIData. Perhaps an example of how these two classes are used would
> help?

Ah, so the FillDictionary was originally also called BuildDictionary, so this is
a bit outdated. Also added an example.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:38: void set_onc_source(ONCSource
source) { source_ = source; }
On 2011/11/29 17:35:16, Steven Bennetts wrote:
> nit: should be set_source, or data member should be onc_source_

Done.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:41: void SetProperty(const char*
property_key,
On 2011/11/29 17:35:16, Steven Bennetts wrote:
> Maybe rename this SetPropertyUIData?

Since this is all UI data, I feel mentioning again would be redundant.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:45: // values.
On 2011/11/29 17:35:16, Steven Bennetts wrote:
> nit: Make it clear in the comment that |dict| is what is getting filled.
Should
> also mention that dict[kKeyONCSource] and dict[kKeyProperties] are the keys
that
> are getting filled.

Done.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:116: // Initializes the object
from the network-level UI data dictionary.
On 2011/11/29 17:35:16, Steven Bennetts wrote:
> s/Initializes/Updates/ (otherwise looks like this is another constructor)

Done.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:128: bool editable() const {
return controller() != CONTROLLER_POLICY; }
On 2011/11/29 17:45:48, Steven Bennetts wrote:
> I think we should probably either just expose Controller and use that
> everywhere, or make that enum private and have a managed() accessor, so that
we
> are consistent in the implementation code.

So, I switched it to accessors only. Still kept the enum public so we can use it
for the constructor. I think that's the best interface we can give people, hope
you're OK with it.

[stevenjb, 2011-11-29 23:39:05]

Yay for tests!
LGTM with comments

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_ui_data.cc (right):

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.cc:162: std::string
NetworkPropertyUIData::EncodeController(Controller controller) {
On 2011/11/29 20:48:47, Mattias Nissler wrote:
> On 2011/11/29 17:35:16, Steven Bennetts wrote:
> > Are we likely to need these outside of this class? If not, or not sure, we
> > should just make them file local functions unless we need them.
> 
> Done.

What I meant to suggest was that, rather than expose these in the header as
private statics in the class, they could be defined in an anonymous namespace
just in the .cc file. It's fine either way, I just like to keep stuff out of the
header where possible if it is not part of the API.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_ui_data.h (right):

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:41: void SetProperty(const char*
property_key,
On 2011/11/29 20:48:47, Mattias Nissler wrote:
> On 2011/11/29 17:35:16, Steven Bennetts wrote:
> > Maybe rename this SetPropertyUIData?
> 
> Since this is all UI data, I feel mentioning again would be redundant.
Fair enough.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:128: bool editable() const {
return controller() != CONTROLLER_POLICY; }
On 2011/11/29 20:48:47, Mattias Nissler wrote:
> On 2011/11/29 17:45:48, Steven Bennetts wrote:
> > I think we should probably either just expose Controller and use that
> > everywhere, or make that enum private and have a managed() accessor, so that
> we
> > are consistent in the implementation code.
> 
> So, I switched it to accessors only. Still kept the enum public so we can use
it
> for the constructor. I think that's the best interface we can give people,
hope
> you're OK with it.

Looks good. I think that technically since these aren't direct accessors, they
should be something like IsManaged(), etc, but I don't feel strongly about that.

[Mattias Nissler (ping if slow), 2011-11-30 10:16:47]

Thanks for the review! Now waiting for Charlie's CL to reland before I can
submit this.

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_ui_data.cc (right):

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.cc:162: std::string
NetworkPropertyUIData::EncodeController(Controller controller) {
On 2011/11/29 23:39:05, Steven Bennetts wrote:
> On 2011/11/29 20:48:47, Mattias Nissler wrote:
> > On 2011/11/29 17:35:16, Steven Bennetts wrote:
> > > Are we likely to need these outside of this class? If not, or not sure, we
> > > should just make them file local functions unless we need them.
> > 
> > Done.
> 
> What I meant to suggest was that, rather than expose these in the header as
> private statics in the class, they could be defined in an anonymous namespace
> just in the .cc file. It's fine either way, I just like to keep stuff out of
the
> header where possible if it is not part of the API.

Ah, the reason I decided to leave as is is that private statics allow the enum
to be private (which it was for a short while till I realized I need it for the
constructor).

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_ui_data.h (right):

http://codereview.chromium.org/8728030/diff/1004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:128: bool editable() const {
return controller() != CONTROLLER_POLICY; }
On 2011/11/29 23:39:05, Steven Bennetts wrote:
> On 2011/11/29 20:48:47, Mattias Nissler wrote:
> > On 2011/11/29 17:45:48, Steven Bennetts wrote:
> > > I think we should probably either just expose Controller and use that
> > > everywhere, or make that enum private and have a managed() accessor, so
that
> > we
> > > are consistent in the implementation code.
> > 
> > So, I switched it to accessors only. Still kept the enum public so we can
use
> it
> > for the constructor. I think that's the best interface we can give people,
> hope
> > you're OK with it.
> 
> Looks good. I think that technically since these aren't direct accessors, they
> should be something like IsManaged(), etc, but I don't feel strongly about
that.

Our style rules actually suggest to use unix_hacker_style for "member functions
that do so little work they're effectively free". So I guess I'm good :)

[commit-bot: I haz the power, 2011-12-01 14:11:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/8728030/11008

[commit-bot: I haz the power, 2011-12-01 16:20:05]

Change committed as 112472