Add chromeos::FlimflamManagerClient

Add chromeos::FlimflamManagerClient

Add chromeos::FlimflamManagerClient
Add FlimflamManagerClientTest
Add FlimflamClientHelper::AppendDataAsVariant

BUG=chromium-os:16557
TEST=unit_tests --gtest_filter="FlimflamManagerClientTest.*"

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=131731

[hashimoto, 2012-04-09 02:26:06]

Rebased onto ToT
PTAL

[stevenjb, 2012-04-09 19:00:55]

Yay for tests.

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
File chromeos/dbus/flimflam_manager_client.cc (right):

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
chromeos/dbus/flimflam_manager_client.cc:23: const base::DictionaryValue&
dictionary) {
This looks like something that should be in the helper class?

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
chromeos/dbus/flimflam_manager_client.cc:103: virtual void
ConfigureService(const base::DictionaryValue& properties,
flimflam requires the dictionary to contain either:
kGuidProperty
or
kTypeProperty, kSecurityProperty (if wifi), and kSSIDProperty

We should check for those here; it will make debugging much easier since the
error from flimflam if one of those is missing is unlikely to be very
informative.

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
chromeos/dbus/flimflam_manager_client.cc:113: const ObjectPathCallback&
callback) OVERRIDE {
Same here. Probably worth putting the checks in a common function.

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
File chromeos/dbus/flimflam_manager_client_unittest.cc (right):

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
chromeos/dbus/flimflam_manager_client_unittest.cc:213:
base::Value::CreateStringValue(""));
You will need to set the GUID (or type/(security if wifi)/ssid) properties once
ConfigureService requires it. Maybe move the code that populates the dictionary
below into a common helper.

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
chromeos/dbus/flimflam_manager_client_unittest.cc:253: 
We should also test ConfigureService + GetService. We should update the stub to
actually store and retrieve configured services.

[hashimoto, 2012-04-10 04:37:22]

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
File chromeos/dbus/flimflam_manager_client.cc (right):

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
chromeos/dbus/flimflam_manager_client.cc:23: const base::DictionaryValue&
dictionary) {
On 2012/04/09 19:00:57, stevenjb (chromium) wrote:
> This looks like something that should be in the helper class?

Let me describe the reason why I put this function here.

Flimflam uses two types of dictionaries, string-to-string and string-to-variant.
A string-to-string dictionary is used as one property which is passed as a
variant argument to methods like SetProperty.
A string-to-variant dictionary is used to store all properties (basic type
values and string-to-string dictionary).  It is passed as a dictionary argument
to methods like ConfigureService.

Manager is the only interface which needs to append a string-to-variant
dictionary to a method call because Manager and Agent are the only interfaces
which takes a dictionary as a method's argument, and chromeos_network.cc has
nothing to do with Agent interface.  (You can see it by grepping in flimflam/doc
directory. http://goo.gl/P1O3s)
I put this function here, not in FlimflamClientHelper, because only Manager
needs to append a string-to-variant dictionary to a method call.
Does it make sense?

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
chromeos/dbus/flimflam_manager_client.cc:103: virtual void
ConfigureService(const base::DictionaryValue& properties,
On 2012/04/09 19:00:57, stevenjb (chromium) wrote:
> flimflam requires the dictionary to contain either:
> kGuidProperty
> or
> kTypeProperty, kSecurityProperty (if wifi), and kSSIDProperty
> 
> We should check for those here; it will make debugging much easier since the
> error from flimflam if one of those is missing is unlikely to be very
> informative.

Added IsPropertyValid

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
chromeos/dbus/flimflam_manager_client.cc:113: const ObjectPathCallback&
callback) OVERRIDE {
On 2012/04/09 19:00:57, stevenjb (chromium) wrote:
> Same here. Probably worth putting the checks in a common function.

Done.

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
File chromeos/dbus/flimflam_manager_client_unittest.cc (right):

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
chromeos/dbus/flimflam_manager_client_unittest.cc:213:
base::Value::CreateStringValue(""));
On 2012/04/09 19:00:57, stevenjb (chromium) wrote:
> You will need to set the GUID (or type/(security if wifi)/ssid) properties
once
> ConfigureService requires it. Maybe move the code that populates the
dictionary
> below into a common helper.

Added GUID and moved the dictionary creation code to CreateExampleProperties.

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
chromeos/dbus/flimflam_manager_client_unittest.cc:253: 
On 2012/04/09 19:00:57, stevenjb (chromium) wrote:
> We should also test ConfigureService + GetService. 

What you want to test is the integrity between ConfigureService and GetService,
right?
I think it must be in the test of Flimflam itself, not in a client's test.

This test's scope is to ensure that each method of FlimflamManagerClient makes a
correct D-Bus method call and pops the result correctly.
All FlimflamManagerClient's methods are independent of each other and there is
no merit to test integrity between multiple methods.

> We should update the stub to
> actually store and retrieve configured services. 
Since we are testing with the real(non-stub) implementation here and we have
NetworkLibraryImplStub, we have no code path which uses
FlimflamManagerClientStubImpl now.
Can we do it later when we really need the stub feature?

[stevenjb, 2012-04-10 22:08:37]

lgtm with nits

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
File chromeos/dbus/flimflam_manager_client.cc (right):

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
chromeos/dbus/flimflam_manager_client.cc:23: const base::DictionaryValue&
dictionary) {
On 2012/04/10 04:37:22, hashimoto wrote:
> On 2012/04/09 19:00:57, stevenjb (chromium) wrote:
> > This looks like something that should be in the helper class?
> 
> Let me describe the reason why I put this function here.
> 
> Flimflam uses two types of dictionaries, string-to-string and
string-to-variant.
> A string-to-string dictionary is used as one property which is passed as a
> variant argument to methods like SetProperty.
> A string-to-variant dictionary is used to store all properties (basic type
> values and string-to-string dictionary).  It is passed as a dictionary
argument
> to methods like ConfigureService.
> 
> Manager is the only interface which needs to append a string-to-variant
> dictionary to a method call because Manager and Agent are the only interfaces
> which takes a dictionary as a method's argument, and chromeos_network.cc has
> nothing to do with Agent interface.  (You can see it by grepping in
flimflam/doc
> directory. http://goo.gl/P1O3s)
> I put this function here, not in FlimflamClientHelper, because only Manager
> needs to append a string-to-variant dictionary to a method call.
> Does it make sense?

That's fine, I don't have any problem with this being here, it just feels like a
generic "AppendDictionaryValue" function should in the Helper class, even if
this is the only code that uses it. If we keep it here, maybe rename it
AppendServiceDictionaryValue?

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
File chromeos/dbus/flimflam_manager_client_unittest.cc (right):

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
chromeos/dbus/flimflam_manager_client_unittest.cc:253: 
Yeah, you're right, since this is a unit test we'll never run it against an
actual flimflam instance, so that really wouldn't test anything.

That said, I expect we may want a more functional stub for testing on Linux at
some point (instead of NetworkLibraryStubImpl), but that can wait.

http://codereview.chromium.org/9950001/diff/11017/chromeos/dbus/flimflam_mana...
File chromeos/dbus/flimflam_manager_client.cc (right):

http://codereview.chromium.org/9950001/diff/11017/chromeos/dbus/flimflam_mana...
chromeos/dbus/flimflam_manager_client.cc:22: bool IsPropertyValid(const
base::DictionaryValue& properties) {
nit: Name this something more specific like IsServiceDictionaryValid or
AreServicePropertiesValid.

[hashimoto, 2012-04-11 03:34:33]

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
File chromeos/dbus/flimflam_manager_client.cc (right):

http://codereview.chromium.org/9950001/diff/23001/chromeos/dbus/flimflam_mana...
chromeos/dbus/flimflam_manager_client.cc:23: const base::DictionaryValue&
dictionary) {
On 2012/04/10 22:08:38, stevenjb (chromium) wrote:
> On 2012/04/10 04:37:22, hashimoto wrote:
> > On 2012/04/09 19:00:57, stevenjb (chromium) wrote:
> > > This looks like something that should be in the helper class?
> > 
> > Let me describe the reason why I put this function here.
> > 
> > Flimflam uses two types of dictionaries, string-to-string and
> string-to-variant.
> > A string-to-string dictionary is used as one property which is passed as a
> > variant argument to methods like SetProperty.
> > A string-to-variant dictionary is used to store all properties (basic type
> > values and string-to-string dictionary).  It is passed as a dictionary
> argument
> > to methods like ConfigureService.
> > 
> > Manager is the only interface which needs to append a string-to-variant
> > dictionary to a method call because Manager and Agent are the only
interfaces
> > which takes a dictionary as a method's argument, and chromeos_network.cc has
> > nothing to do with Agent interface.  (You can see it by grepping in
> flimflam/doc
> > directory. http://goo.gl/P1O3s)
> > I put this function here, not in FlimflamClientHelper, because only Manager
> > needs to append a string-to-variant dictionary to a method call.
> > Does it make sense?
> 
> That's fine, I don't have any problem with this being here, it just feels like
a
> generic "AppendDictionaryValue" function should in the Helper class, even if
> this is the only code that uses it. If we keep it here, maybe rename it
> AppendServiceDictionaryValue?

Renamed to AppendServicePropertiesDictionary

http://codereview.chromium.org/9950001/diff/11017/chromeos/dbus/flimflam_mana...
File chromeos/dbus/flimflam_manager_client.cc (right):

http://codereview.chromium.org/9950001/diff/11017/chromeos/dbus/flimflam_mana...
chromeos/dbus/flimflam_manager_client.cc:22: bool IsPropertyValid(const
base::DictionaryValue& properties) {
On 2012/04/10 22:08:38, stevenjb (chromium) wrote:
> nit: Name this something more specific like IsServiceDictionaryValid or
> AreServicePropertiesValid.

Done.