components: add wifi_sync component

Add a new component for WiFi credential sync. Populate the
new component with a stub SyncableService, and its
corresponding factory. This SyncableService is plumbed
into sync iff "--enable-wifi-credential-sync" is present
in the command-line.

Also:
- add sync integration tests (single client, and two client)
- add WifiSecurityClass: an enum to provide an internal
  representation of WiFi security classes (e.g. WEP, PSK).
- add WifiCredential: an abstraction of the parameters
  required to find and connect to a WiFi network
- add conversions between ModelType and NotificationType,
  for WIFI_CREDENTIALS

BUG=chromium:426693
BUG=chromium:426696
BUG=chromium:427595
TEST=sync_integration_tests --gtest_filter="*Wifi*"
TEST=components_unittests --gtest_filter="Wifi*"

Committed: https://crrev.com/271c6cca48a6cef32c0f3add3b17b700707deec5
Cr-Commit-Position: refs/heads/master@{#309333}

[mukesh agrawal, 2014-11-08 00:39:11]

This is patch 2 of 7, in a series of CLs to implement the sync side of
WifiCredentialSyncableService.

  http://crrev.com/675993005 add wifi_sync component
* http://crrev.com/709683004 add a stub SyncableService
  http://crrev.com/713583002 add utility to check if a network allows passwords
  http://crrev.com/706233004 implement ACTION_ADD for
WifiCredentialSyncableService
  http://crrev.com/687803012 add password caching to
WifiCredentialSyncableService
  http://crrev.com/708223002 implement ACTION_UPDATE for
WifiCredentialSyncableService
  http://crrev.com/706423002 implement ACTION_DELETE for
WifiCredentialSyncableService

* Denotes this CL.

[mukesh agrawal, 2014-11-08 00:54:56]

quiche@chromium.org changed reviewers:
+ erikwright@chromium.org, pavely@chromium.org, pstew@chromium.org,
stevenjb@chromium.org

[mukesh agrawal, 2014-11-08 00:54:56]

quiche@chromium.org changed required reviewers:
+ erikwright@chromium.org, pavely@chromium.org, stevenjb@chromium.org

[mukesh agrawal, 2014-11-08 01:14:13]

Sorry about the size of this CL. I couldn't find a way to break this into
smaller pieces, while still keeping it buildable, and including tests.

[stevenjb, 2014-11-12 21:17:41]

I'd actually like to see this merged with
https://codereview.chromium.org/675993005/, I don't think that CL has much value
on its own, and it shouldn't make this CL any more difficult to review.

[stevenjb, 2014-11-12 21:30:44]

https://codereview.chromium.org/709683004/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/wifi_credentials_helper.h (right):

https://codereview.chromium.org/709683004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/wifi_credentials_helper.h:10: class
Profile;
nit: WS

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
File components/wifi_sync/wifi_credential_syncable_service.h (right):

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service.h:33: // managed by the
ChromeOS connection manager ("shill").
nit: Shill

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service.h:37: // Construct a
service for the given |context|. Local changes will
Constructs

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service.h:49: void
StopSyncing(syncer::ModelType type) override;
WS

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service.h:59:
std::set<std::pair<std::vector<char>, WifiSecurityClass>>
Maybe typedef this (with 'using')?

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
File components/wifi_sync/wifi_credential_syncable_service_factory.cc (right):

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service_factory.cc:38: //
crbug.com/430681.
Hmm, we probably need to add a helper method to src/chromeos/login to return the
"primary" browser context so that we can compare that to |context| and assert or
return nullptr for a non-primary context.

[mukesh agrawal, 2014-11-13 16:19:23]

On 2014/11/12 21:17:41, stevenjb wrote:
> I'd actually like to see this merged with
> https://codereview.chromium.org/675993005/, I don't think that CL has much
value
> on its own, and it shouldn't make this CL any more difficult to review.

Done. Note that I've dropped the functions for converting the enum to/from shill
strings, as those functions aren't used by this CL. I'll add them back (all with
their unit tests) when I have code that calls them.

[mukesh agrawal, 2014-11-13 16:24:59]

https://codereview.chromium.org/709683004/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/wifi_credentials_helper.h (right):

https://codereview.chromium.org/709683004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/wifi_credentials_helper.h:10: class
Profile;
On 2014/11/12 21:30:44, stevenjb wrote:
> nit: WS

Done.

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
File components/wifi_sync/wifi_credential_syncable_service.h (right):

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service.h:33: // managed by the
ChromeOS connection manager ("shill").
On 2014/11/12 21:30:44, stevenjb wrote:
> nit: Shill

Done.

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service.h:37: // Construct a
service for the given |context|. Local changes will
On 2014/11/12 21:30:44, stevenjb wrote:
> Constructs

Done.

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service.h:49: void
StopSyncing(syncer::ModelType type) override;
On 2014/11/12 21:30:44, stevenjb wrote:
> WS

Sorry, I don't understand. What's normal practice for spacing here?

Is the problem that there's no blank line before the comment on line 50? If so,
the reason I didn't add one is that GetAllSyncData and ProcessSyncChanges still
fall under the comment at line 43. (They're part of the SyncableService
interface.)

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service.h:59:
std::set<std::pair<std::vector<char>, WifiSecurityClass>>
On 2014/11/12 21:30:44, stevenjb wrote:
> Maybe typedef this (with 'using')?

Can do. Do we prefer something abstract (e.g. "WifiNetworks"), or something more
explicit (e.g. "SetOfSsidAndSecurityClassPairs")?

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
File components/wifi_sync/wifi_credential_syncable_service_factory.cc (right):

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service_factory.cc:38: //
crbug.com/430681.
On 2014/11/12 21:30:44, stevenjb wrote:
> Hmm, we probably need to add a helper method to src/chromeos/login to return
the
> "primary" browser context so that we can compare that to |context| and assert
or
> return nullptr for a non-primary context.

I'd guess that asserting is too strong, because its probably normal for sync to
run for the secondary profiles in a multi-profile session. (Incognito windows
would be a different story -- sync simply shouldn't be running for incognito.)
But returning nullptr seems reasonable.

For determining the primary browser context, could we use
chromeos::ProfileHelper::IsPrimaryProfile()?

[mukesh agrawal, 2014-11-13 16:25:27]

Thanks, Steven. PTAL.

[stevenjb, 2014-11-13 22:55:36]

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
File components/wifi_sync/wifi_credential_syncable_service.h (right):

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service.h:49: void
StopSyncing(syncer::ModelType type) override;
On 2014/11/13 16:24:58, mukesh agrawal wrote:
> On 2014/11/12 21:30:44, stevenjb wrote:
> > WS
> 
> Sorry, I don't understand. What's normal practice for spacing here?
> 
> Is the problem that there's no blank line before the comment on line 50? If
so,
> the reason I didn't add one is that GetAllSyncData and ProcessSyncChanges
still
> fall under the comment at line 43. (They're part of the SyncableService
> interface.)
> 

So, normally overridden methods are not commented, and commented method are
spaced with a blank line. I would either:
* Move this comment to the base class header and/or the implementation
* Space all three methods and add a blank line after // syncer::SyncableService
* Leave it as-is, it's a little odd looking but we can live with it.

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service.h:59:
std::set<std::pair<std::vector<char>, WifiSecurityClass>>
On 2014/11/13 16:24:58, mukesh agrawal wrote:
> On 2014/11/12 21:30:44, stevenjb wrote:
> > Maybe typedef this (with 'using')?
> 
> Can do. Do we prefer something abstract (e.g. "WifiNetworks"), or something
more
> explicit (e.g. "SetOfSsidAndSecurityClassPairs")?

I'd split the difference, e.g. WifiSsidSecuritySet. You might want to also
typedef std::vector<char> as something like HexSsid.

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
File components/wifi_sync/wifi_credential_syncable_service_factory.cc (right):

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service_factory.cc:38: //
crbug.com/430681.
On 2014/11/13 16:24:59, mukesh agrawal wrote:
> On 2014/11/12 21:30:44, stevenjb wrote:
> > Hmm, we probably need to add a helper method to src/chromeos/login to return
> the
> > "primary" browser context so that we can compare that to |context| and
assert
> or
> > return nullptr for a non-primary context.
> 
> I'd guess that asserting is too strong, because its probably normal for sync
to
> run for the secondary profiles in a multi-profile session. (Incognito windows
> would be a different story -- sync simply shouldn't be running for incognito.)
> But returning nullptr seems reasonable.
> 
> For determining the primary browser context, could we use
> chromeos::ProfileHelper::IsPrimaryProfile()? 

Unfortunately ProfileHelper is in src/chrome so we can't use it. I will talk to
skhune@ and oshima@ about whether something already exists or if we need to add
it.

https://codereview.chromium.org/709683004/diff/60001/chrome/browser/sync/test...
File
chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc
(right):

https://codereview.chromium.org/709683004/diff/60001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:13:
virtual ~SingleClientWifiCredentialsSyncTest() {}
override

https://codereview.chromium.org/709683004/diff/60001/chrome/browser/sync/test...
File
chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc
(right):

https://codereview.chromium.org/709683004/diff/60001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc:16:
virtual ~TwoClientWifiCredentialsSyncTest() {}
override

https://codereview.chromium.org/709683004/diff/60001/components/wifi_sync/wif...
File components/wifi_sync/wifi_credential_syncable_service.h (right):

https://codereview.chromium.org/709683004/diff/60001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:39: // |context|.
We should note that on Chrome OS this should only be instantiated for the
primary user profile since that is the profile that Shill is associated.

[mukesh agrawal, 2014-11-14 02:14:54]

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
File components/wifi_sync/wifi_credential_syncable_service.h (right):

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service.h:49: void
StopSyncing(syncer::ModelType type) override;
On 2014/11/13 22:55:35, stevenjb wrote:
> So, normally overridden methods are not commented, and commented method are
> spaced with a blank line. I would either:
> * Move this comment to the base class header and/or the implementation
> * Space all three methods and add a blank line after //
syncer::SyncableService
> * Leave it as-is, it's a little odd looking but we can live with it.

Done.

Since the base class has many subclasses, I thought it'd be noisy to call out
one specific subclass there. So I've gone with option 2.

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service.h:59:
std::set<std::pair<std::vector<char>, WifiSecurityClass>>
On 2014/11/13 22:55:35, stevenjb wrote:
> On 2014/11/13 16:24:58, mukesh agrawal wrote:
> > On 2014/11/12 21:30:44, stevenjb wrote:
> > > Maybe typedef this (with 'using')?
> > 
> > Can do. Do we prefer something abstract (e.g. "WifiNetworks"), or something
> more
> > explicit (e.g. "SetOfSsidAndSecurityClassPairs")?
> 
> I'd split the difference, e.g. WifiSsidSecuritySet. You might want to also
> typedef std::vector<char> as something like HexSsid.
> 

Done.

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
File components/wifi_sync/wifi_credential_syncable_service_factory.cc (right):

https://codereview.chromium.org/709683004/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service_factory.cc:38: //
crbug.com/430681.
On 2014/11/13 22:55:35, stevenjb wrote:
> Unfortunately ProfileHelper is in src/chrome so we can't use it. I will talk
to
> skhune@ and oshima@ about whether something already exists or if we need to
add
> it.

I take it the idea is that components shouldn't depend on Chrome?

Based on that, I've removed the include of chrome/browser/profiles/profile.h,
and updated this factory's interface accordingly.

https://codereview.chromium.org/709683004/diff/60001/chrome/browser/sync/test...
File
chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc
(right):

https://codereview.chromium.org/709683004/diff/60001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:13:
virtual ~SingleClientWifiCredentialsSyncTest() {}
On 2014/11/13 22:55:36, stevenjb wrote:
> override

Done.

https://codereview.chromium.org/709683004/diff/60001/chrome/browser/sync/test...
File
chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc
(right):

https://codereview.chromium.org/709683004/diff/60001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc:16:
virtual ~TwoClientWifiCredentialsSyncTest() {}
On 2014/11/13 22:55:36, stevenjb wrote:
> override

Done.

https://codereview.chromium.org/709683004/diff/60001/components/wifi_sync/wif...
File components/wifi_sync/wifi_credential_syncable_service.h (right):

https://codereview.chromium.org/709683004/diff/60001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:39: // |context|.
On 2014/11/13 22:55:36, stevenjb wrote:
> We should note that on Chrome OS this should only be instantiated for the
> primary user profile since that is the profile that Shill is associated.

Done.

[mukesh agrawal, 2014-11-14 02:15:42]

Thanks, and PTAL. :)

[stevenjb, 2014-11-14 23:56:25]

LGTM!

[pavely, 2014-11-17 19:23:44]

sync related files: lgtm

[erikwright (departed), 2014-11-17 20:18:58]

https://codereview.chromium.org/709683004/diff/80001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/wifi_credentials_helper.cc (right):

https://codereview.chromium.org/709683004/diff/80001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/wifi_credentials_helper.cc:36: const
WifiSsidSecuritySet a_networks = service_a.GetWifiNetworksForTest();
Instead of querying the services, shouldn't you be querying whatever it is they
sync _to_? i.e., I have two SyncableService instances, but they are presumably
backed by databases or other services. I should assert that _those_ end up
consistent.

It's a little hard to be more precise at this point in the development, because
I can't tell what the SyncableService will ultimately do. But any useful
implementation should be permit data consistency testing without a "ForTest"
method.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
File components/wifi_sync/wifi_credential_syncable_service.cc (right):

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.cc:10: const enum
syncer::ModelType WifiCredentialSyncableService::kModelType =
I'm not used to seeing 'enum' somewhere like this. I would remove it unless you
see examples or have reasons to the contrary.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.cc:16: thread_checker_() {
not required

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.cc:38: return
syncer::SyncMergeResult(type);
IWYU

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.cc:53: return
syncer::SyncDataList();
IWYU

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.cc:60: NOTIMPLEMENTED();
base/logging.h

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.cc:61: return
syncer::SyncError();
this, on the other hand, will require a full type and (assuming you remove it
from the .h) an include

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
File components/wifi_sync/wifi_credential_syncable_service.h (right):

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:11: #include <string>
not used

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:12: #include <utility> 
// for std::pair
This comment is unnecessary.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:20: #include
"sync/api/sync_error.h"
Not required (you only use it as part of declaring an override; we are
guaranteed that the base class declaration already had sufficient definition of
SyncError for this purpose).

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:43: // Constructs a
service for the given |context|. Local changes will
either "for |context|" or "for the given BrowserContext."

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:52: 
remove the blank lines 52,58,60,65

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:61: // The
implementation of GetAllSyncData may not actually return the
There should not be a reason to add comments to an override declaration. The
caller of this method is presumably using a base class and reading the base
class's documentation. They might never read this comment. Which means that
nothing important should be written here.

Of course, nothing _unimportant_ should be written here, either, so nothing
should be written here at all.

If clients of the derived class (i.e., the instantiator) need this information
it should go in the class comments.

If this comment is meant to be a warning to clients about the potential variety
of derived implementations it should go in the base class.

If this comment is targeted at future developers of
WifiCredentialSyncableService it should potentially go in the implementation
(.cc file).

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:76: static const enum
syncer::ModelType kModelType;
I think you are using this include transitively. You should introduce a direct
include of this type.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:76: static const enum
syncer::ModelType kModelType;
I'm not used to seeing 'enum' somewhere like this. I would remove it unless you
see examples or have reasons to the contrary.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:78: // Const members.
I don't think the section headings here are required. The declarations already
include the 'const' attribute.

On the other hand, the Google C++ style guide expects comments on each member
declaration.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:83:
scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
include base/memory/scoped_ptr.h

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:83:
scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
Requires an include?

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
File components/wifi_sync/wifi_credential_syncable_service_factory.h (right):

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service_factory.h:8: #include
"base/basictypes.h"
replace with base/macros.h

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
File components/wifi_sync/wifi_credential_syncable_service_unittest.cc (right):

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service_unittest.cc:44: return
syncable_service_->sync_processor_.get();
Why access this via invasive 'friend' declaration when it's _you_ that
instantiated and supplied this dependency. You already keep a reference to it in
sync_change_processor_. Use that.

Ditto for all the other accesses to private members. Remove them, and then
remove the friend declaration.

Friend declarations should be a last resort. In fact, I don't personally believe
they are _ever_ required.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service_unittest.cc:70:
EXPECT_EQ(sync_change_processor(), GetSyncChangeProcessor());
You are asserting that the class correctly stored a parameter as a member
variable? This is not necessary.

You should be asserting that the class correctly did whatever it is
contractually obligated to do to the supplied dependency. Did it send the right
changes to be processed? Does it use the error factory appropriately (whatever
is appropriate)?

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
File components/wifi_sync/wifi_security_class.h (right):

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_security_class.h:8: #include <string>
not required.

[mukesh agrawal, 2014-11-18 17:17:44]

You raise a good point about testing the data store, rather than testing
internal data of the WifiCredentialSyncableService. I'm still thinking about how
to do that.

For the rest of the comments, please see replies inline. Feel free to look them
over now, or to wait until I've addressed the integration test issue.

Thanks for the thorough code review!

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
File components/wifi_sync/wifi_credential_syncable_service.cc (right):

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.cc:10: const enum
syncer::ModelType WifiCredentialSyncableService::kModelType =
On 2014/11/17 20:18:57, erikwright wrote:
> I'm not used to seeing 'enum' somewhere like this. I would remove it unless
you
> see examples or have reasons to the contrary.

Done.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.cc:16: thread_checker_() {
On 2014/11/17 20:18:57, erikwright wrote:
> not required

Without the explicit initialization of thread_checker_, building fails with the
following error:

../../components/wifi_sync/wifi_credential_syncable_service.cc:13:32: error:
constructor for 'wifi_sync::WifiCredentialSyncableService' must explicitly
initialize the const member 'thread_checker_'
WifiCredentialSyncableService::WifiCredentialSyncableService(
                               ^
../../components/wifi_sync/wifi_credential_syncable_service.h:80:29: note:
'thread_checker_' declared here
  const base::ThreadChecker thread_checker_;

This happens when compiling using clang++ and goma.

Apparently, this is because "If a program calls for the default initialization
of an object of a const-qualified type T, T shall be a class type with a
user-provided default constructor.", and base::ThreadChecker does not have a
user-provided default constructor. (http://stackoverflow.com/a/24943751)

Alternatively, I could declare thread_checker_ non-const, or I could add a
user-provided default constructor to base::ThreadChecker.

Which solution would you recommend? (Leave code as-is, declare member non-const,
add ctor to base::ThreadChecker, or something else.)

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.cc:38: return
syncer::SyncMergeResult(type);
On 2014/11/17 20:18:57, erikwright wrote:
> IWYU

Done.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.cc:53: return
syncer::SyncDataList();
On 2014/11/17 20:18:57, erikwright wrote:
> IWYU

Done.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.cc:60: NOTIMPLEMENTED();
On 2014/11/17 20:18:57, erikwright wrote:
> base/logging.h

Done.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.cc:61: return
syncer::SyncError();
On 2014/11/17 20:18:57, erikwright wrote:
> this, on the other hand, will require a full type and (assuming you remove it
> from the .h) an include

Done.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
File components/wifi_sync/wifi_credential_syncable_service.h (right):

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:11: #include <string>
On 2014/11/17 20:18:58, erikwright wrote:
> not used

Done.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:12: #include <utility> 
// for std::pair
On 2014/11/17 20:18:58, erikwright wrote:
> This comment is unnecessary.

Done.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:20: #include
"sync/api/sync_error.h"
On 2014/11/17 20:18:57, erikwright wrote:
> Not required (you only use it as part of declaring an override; we are
> guaranteed that the base class declaration already had sufficient definition
of
> SyncError for this purpose).

Done. (Also removed sync_data, for the same reason.)

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:43: // Constructs a
service for the given |context|. Local changes will
On 2014/11/17 20:18:57, erikwright wrote:
> either "for |context|" or "for the given BrowserContext."

Done.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:52: 
On 2014/11/17 20:18:57, erikwright wrote:
> remove the blank lines 52,58,60,65

Done.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:61: // The
implementation of GetAllSyncData may not actually return the
On 2014/11/17 20:18:57, erikwright wrote:
> There should not be a reason to add comments to an override declaration. The
> caller of this method is presumably using a base class and reading the base
> class's documentation. They might never read this comment. Which means that
> nothing important should be written here.
> 
> Of course, nothing _unimportant_ should be written here, either, so nothing
> should be written here at all.
> 
> If clients of the derived class (i.e., the instantiator) need this information
> it should go in the class comments.
> 
> If this comment is meant to be a warning to clients about the potential
variety
> of derived implementations it should go in the base class.
> 
> If this comment is targeted at future developers of
> WifiCredentialSyncableService it should potentially go in the implementation
> (.cc file).

Done. (Removed, as I couldn't find a reason to keep it. I may it back to the
implementation file in a later CL.)

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:76: static const enum
syncer::ModelType kModelType;
On 2014/11/17 20:18:58, erikwright wrote:
> I'm not used to seeing 'enum' somewhere like this. I would remove it unless
you
> see examples or have reasons to the contrary.

Done.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:76: static const enum
syncer::ModelType kModelType;
On 2014/11/17 20:18:57, erikwright wrote:
> I think you are using this include transitively. You should introduce a direct
> include of this type.

You are correct that this is included transitively. I believe this is the way
the since headers are intended to be used. In particular, the direct include
would require sync/internal_api/public/base/model_type.h. From the path
("internal_api"), I inferred that the file should not be directly used outside
of sync.

A search on the existing source seems to confirm that idea. In particular,
outside of the sync code, only one file includes model_type.h directly [1].
Nonetheless, if you still think model_type.h should be included directly, I can
make that change.

[1]
https://code.google.com/p/chromium/codesearch#search/&q=include.%2Bmodel_type...

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:78: // Const members.
On 2014/11/17 20:18:58, erikwright wrote:
> I don't think the section headings here are required. The declarations already
> include the 'const' attribute.
> 
> On the other hand, the Google C++ style guide expects comments on each member
> declaration.

Done.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:83:
scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
On 2014/11/17 20:18:57, erikwright wrote:
> include base/memory/scoped_ptr.h

Done.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service.h:83:
scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
On 2014/11/17 20:18:57, erikwright wrote:
> Requires an include?

Done.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
File components/wifi_sync/wifi_credential_syncable_service_factory.h (right):

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service_factory.h:8: #include
"base/basictypes.h"
On 2014/11/17 20:18:58, erikwright wrote:
> replace with base/macros.h

Done.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
File components/wifi_sync/wifi_credential_syncable_service_unittest.cc (right):

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service_unittest.cc:44: return
syncable_service_->sync_processor_.get();
On 2014/11/17 20:18:58, erikwright wrote:
> Why access this via invasive 'friend' declaration when it's _you_ that
> instantiated and supplied this dependency. You already keep a reference to it
in
> sync_change_processor_. Use that.
> 
> Ditto for all the other accesses to private members. Remove them, and then
> remove the friend declaration.
> 
> Friend declarations should be a last resort. In fact, I don't personally
believe
> they are _ever_ required.

Done.

Removed this getter. Also removed StopSyncingClearsNonConstFields, since all it
was doing was checking private members of WifiCredentialSyncableService. And
since the SyncableService doesn't (yet) do anything other than set its internal
state, I've removed the unit tests file.

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service_unittest.cc:70:
EXPECT_EQ(sync_change_processor(), GetSyncChangeProcessor());
On 2014/11/17 20:18:58, erikwright wrote:
> You are asserting that the class correctly stored a parameter as a member
> variable? This is not necessary.
> 
> You should be asserting that the class correctly did whatever it is
> contractually obligated to do to the supplied dependency. Did it send the
right
> changes to be processed? Does it use the error factory appropriately (whatever
> is appropriate)?

Done. (Removed this test, since the class doesn't yet do anything with its
dependencies. Testing that the class DTRT with the dependencies will come when I
add the ability to modify the data, in later CLs.)

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
File components/wifi_sync/wifi_security_class.h (right):

https://codereview.chromium.org/709683004/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_security_class.h:8: #include <string>
On 2014/11/17 20:18:58, erikwright wrote:
> not required.

Done.

[mukesh agrawal, 2014-12-03 01:44:43]

https://codereview.chromium.org/709683004/diff/120001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/wifi_credentials_helper.h (right):

https://codereview.chromium.org/709683004/diff/120001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/wifi_credentials_helper.h:18: // Used to
access the WiFi credentials within a particular sync profile.
Removed this, since the test cases no longer use the SyncableService to verify
changes.

https://codereview.chromium.org/709683004/diff/120001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/wifi_credentials_helper.h:22: // Used to
access the WiFi credentials within the verifier sync profile.
Same as above.

https://codereview.chromium.org/709683004/diff/120001/components/wifi_sync/wi...
File components/wifi_sync/wifi_credential_syncable_service.h (right):

https://codereview.chromium.org/709683004/diff/120001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service.h:38: using SsidBytes =
std::vector<uint8_t>;
No longer needed, after removal of GetWifiNetworksForTest.

https://codereview.chromium.org/709683004/diff/120001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service.h:39: using
WifiSsidSecuritySet =
As above.

https://codereview.chromium.org/709683004/diff/120001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service.h:47: explicit
WifiCredentialSyncableService(content::BrowserContext *context);
Removed |context| argument, as it wasn't actually used.

https://codereview.chromium.org/709683004/diff/120001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service.h:62: // Test support.
Removed in favor of testing integration with chromeos::NetworkStateHandler.

https://codereview.chromium.org/709683004/diff/120001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service.h:70: // The
BrowserContext that this SyncableService processes and
Removed since it wasn't actually used.

https://codereview.chromium.org/709683004/diff/120001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service.h:80: // Our
SyncErrorFactory instance. Used to report an error condition
Removed since it wasn't actually used.

https://codereview.chromium.org/709683004/diff/140001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/709683004/diff/140001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_components_factory_impl.cc:423: #if
defined(OS_CHROMEOS)
This change is to prevent UI for WiFi sync from appearing on other platforms
(since the initial release will only support ChromeOS).

https://codereview.chromium.org/709683004/diff/140001/components/wifi_sync/BU...
File components/wifi_sync/BUILD.gn (right):

https://codereview.chromium.org/709683004/diff/140001/components/wifi_sync/BU...
components/wifi_sync/BUILD.gn:35: "//components/user_manager",
Stray dependency. Will be removed in PS8.

[mukesh agrawal, 2014-12-03 01:47:04]

https://codereview.chromium.org/709683004/diff/140001/components/wifi_sync/BU...
File components/wifi_sync/BUILD.gn (right):

https://codereview.chromium.org/709683004/diff/140001/components/wifi_sync/BU...
components/wifi_sync/BUILD.gn:35: "//components/user_manager",
On 2014/12/03 01:44:43, mukesh agrawal wrote:
> Stray dependency. Will be removed in PS8.

Er, make that PS9.

[mukesh agrawal, 2014-12-03 01:53:23]

The most significant change in PS8 is that we now check the state of the
ChromeOS networking configuration back-end (via chromeos::NetworkStateHandler),
rather than the internal state of WifiCredentialSyncableService. Code-wise, the
relevant files are wifi_credentials_helper.cc,
wifi_credentials_helper_chromeos.cc, and network_state_helper_chromeos.cc.

Other significant changes:
- addition of WifiCredential class
- addition of wifi_security_class_chromeos (needed by
network_state_helper_chromeos)

I've also added some comments to PS7, and PS8, to explain some other changes
that might not be obvious.

[mukesh agrawal, 2014-12-04 18:55:01]

https://codereview.chromium.org/709683004/diff/80001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/wifi_credentials_helper.cc (right):

https://codereview.chromium.org/709683004/diff/80001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/wifi_credentials_helper.cc:36: const
WifiSsidSecuritySet a_networks = service_a.GetWifiNetworksForTest();
Agreed. This is fixed in PS8.

[stevenjb, 2014-12-05 20:28:17]

Re-reviewed all of this since a lot has changed.

https://codereview.chromium.org/709683004/diff/160001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc
(right):

https://codereview.chromium.org/709683004/diff/160001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:38:
DCHECK(chromeos::NetworkHandler::Get()->network_state_handler());
Get() asserts if not initialized. You can use NetworkHandler::IsInitialized() to
test whether or not it was initialized if that is a valid use case, but here you
don't need either check.

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
File components/wifi_sync/network_state_helper_chromeos.cc (right):

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:19: LOG(ERROR) <<
"network_state_handler is null";
Should this ever happen? Maybe just DCHECK?

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:26: true,  /*
configured_only */
nit: Either use /* */ inside the comma, or // outside the comma (and align). I
think the former is preferred.

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:33: DCHECK(network);
Unnecessary.

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:37: }
nit: no {}

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:40: // a security_class()
field in NetworkState.
Is there any reason why NetworkState should know about and track "security
class"?

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:44: // GetProperties
method.
I don't think that Shill provides the passphrase via GetProperties. I thought
that the plan was to only sync newly entered passphrases (available via
NetworkConfigurationObserver::OnConfigurationCreated or OnPropertiesSet)?

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/wi...
File components/wifi_sync/wifi_credential.h (right):

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential.h:32: const std::string& passphrase);
nit: blank line between methods

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential.h:38: const std::string& passphrase);
We try to avoid multiple constructors. Do we really need the first version?

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential.h:50: static bool IsLessThan(const
WifiCredential& a, const WifiCredential& b);
WS

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/wi...
File components/wifi_sync/wifi_security_class.h (right):

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/wi...
components/wifi_sync/wifi_security_class.h:27: WifiSecurityClass
WifiSecurityClassFromShillString(
nit: FromShillSecurity or just FromShill

[pavely, 2014-12-05 22:48:26]

sync related changes: lgtm

[mukesh agrawal, 2014-12-05 23:48:12]

Thanks, Steven. PTAL.

https://codereview.chromium.org/709683004/diff/160001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc
(right):

https://codereview.chromium.org/709683004/diff/160001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:38:
DCHECK(chromeos::NetworkHandler::Get()->network_state_handler());
On 2014/12/05 20:28:17, stevenjb wrote:
> Get() asserts if not initialized. You can use NetworkHandler::IsInitialized()
to
> test whether or not it was initialized if that is a valid use case, but here
you
> don't need either check.
> 

Done.

I've taken "don't need either check" to mean the DCHECK on Get(), and the
IsInitialized() call. Let me know if I've misunderstood.(I.e. let me know if I
should remove the DCHECK of the network_state_handler() result.)

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
File components/wifi_sync/network_state_helper_chromeos.cc (right):

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:19: LOG(ERROR) <<
"network_state_handler is null";
On 2014/12/05 20:28:17, stevenjb wrote:
> Should this ever happen? Maybe just DCHECK?

Done.

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:26: true,  /*
configured_only */
On 2014/12/05 20:28:17, stevenjb wrote:
> nit: Either use /* */ inside the comma, or // outside the comma (and align). I
> think the former is preferred.
> 

Done.

I wasn't sure if alignment applied to /**/ or not. I've followed the pattern
used in network_state_handler.cc.

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:33: DCHECK(network);
On 2014/12/05 20:28:17, stevenjb wrote:
> Unnecessary.

Done.

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:37: }
On 2014/12/05 20:28:17, stevenjb wrote:
> nit: no {}

Done.

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:40: // a security_class()
field in NetworkState.
On 2014/12/05 20:28:17, stevenjb wrote:
> Is there any reason why NetworkState should know about and track "security
> class"?

Good question. When we start adding networks, we want kSecurityWpa and
kSecurityRsn networks to be interoperable. shill's SecurityClass property could
solve this problem by collapsing these two security types into just
kSecurityPsk.

Alternatively, I could incorporate this logic into
WifiSecurityClassFromShillSecurity. Let me know which you prefer.

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:44: // GetProperties
method.
On 2014/12/05 20:28:17, stevenjb wrote:
> I don't think that Shill provides the passphrase via GetProperties. I thought
> that the plan was to only sync newly entered passphrases (available via
> NetworkConfigurationObserver::OnConfigurationCreated or OnPropertiesSet)?

Correct on both counts.

The plan is still to only sync newly entered passphrases. This helper function
is intended to be called by tests, to verify that local network state has been
updated correctly. Part of that is checking that the passphrase has been set
correctly.

I believe FakeShillServiceClient retains all the properties set on it. My plan
was to take advantage of that, to allow tests to verify that the passphrase is
correct.

(Side note: In this CL, the helper function is only called from one place. That
might argue for putting this code in wifi_credentials_helper_chromeos.cc. But in
the next CL, I use the same code from the WifiConfigDelegate tests.)

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/wi...
File components/wifi_sync/wifi_credential.h (right):

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential.h:32: const std::string& passphrase);
On 2014/12/05 20:28:17, stevenjb wrote:
> nit: blank line between methods

Acknowledged.

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential.h:38: const std::string& passphrase);
On 2014/12/05 20:28:17, stevenjb wrote:
> We try to avoid multiple constructors. Do we really need the first version?

Done.

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential.h:50: static bool IsLessThan(const
WifiCredential& a, const WifiCredential& b);
On 2014/12/05 20:28:17, stevenjb wrote:
> WS

Done.

Following the pattern of chromeos/network/certificate_pattern.h, I left the
getters alone. (Packed one-after-the-other, without whitespace.)

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/wi...
File components/wifi_sync/wifi_security_class.h (right):

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/wi...
components/wifi_sync/wifi_security_class.h:27: WifiSecurityClass
WifiSecurityClassFromShillString(
On 2014/12/05 20:28:17, stevenjb wrote:
> nit: FromShillSecurity or just FromShill

Done.

[stevenjb, 2014-12-08 17:54:26]

lgtm

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
File components/wifi_sync/network_state_helper_chromeos.cc (right):

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:40: // a security_class()
field in NetworkState.
On 2014/12/05 23:48:11, mukesh agrawal wrote:
> On 2014/12/05 20:28:17, stevenjb wrote:
> > Is there any reason why NetworkState should know about and track "security
> > class"?
> 
> Good question. When we start adding networks, we want kSecurityWpa and
> kSecurityRsn networks to be interoperable. shill's SecurityClass property
could
> solve this problem by collapsing these two security types into just
> kSecurityPsk.
> 
> Alternatively, I could incorporate this logic into
> WifiSecurityClassFromShillSecurity. Let me know which you prefer.

I looked through the Chrome code. We already have a function,
shill_property_util::GetSecurityClass that maps Wpa and Rsn to Psk. This code
could use that function for now, and Shill could stop differentiating (i.e. map
Rns and Wpa to Security or a new SecurityClass which NetworkState could switch
to) at any time.

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:44: // GetProperties
method.
On 2014/12/05 23:48:11, mukesh agrawal wrote:
> On 2014/12/05 20:28:17, stevenjb wrote:
> > I don't think that Shill provides the passphrase via GetProperties. I
thought
> > that the plan was to only sync newly entered passphrases (available via
> > NetworkConfigurationObserver::OnConfigurationCreated or OnPropertiesSet)?
> 
> Correct on both counts.
> 
> The plan is still to only sync newly entered passphrases. This helper function
> is intended to be called by tests, to verify that local network state has been
> updated correctly. Part of that is checking that the passphrase has been set
> correctly.
> 
> I believe FakeShillServiceClient retains all the properties set on it. My plan
> was to take advantage of that, to allow tests to verify that the passphrase is
> correct.
> 
> (Side note: In this CL, the helper function is only called from one place.
That
> might argue for putting this code in wifi_credentials_helper_chromeos.cc. But
in
> the next CL, I use the same code from the WifiConfigDelegate tests.)

Anything that depends on Fake behavior should clearly be designated as test only
code, either in the name of the method (e.g. append ForTest) or by putting the
behavior in test only code. This is fine I guess for now, I'll look more closely
in the follow up CLs.

[erikwright (departed), 2014-12-08 21:21:37]

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/ne...
File components/wifi_sync/network_state_helper_chromeos.cc (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:10: #include
"components/wifi_sync/wifi_credential.h"
not needed (in header)

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:22:
chromeos::NetworkTypePattern::WiFi(),
#include chromeos/network/network_type_pattern.h

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/ne...
File components/wifi_sync/network_state_helper_chromeos.h (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.h:16: // Functions which deal
with ChromeOS-specific network state.
This comment is confusing (1 since it is on a namespace and 2 since there is
only one function).

Since the file is named .._helper_chromeos.h I think it's superfluous and would
remove it.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
File components/wifi_sync/wifi_credential.cc (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential.cc:26: // Nothing special to do.
remove comment. The reasons for an empty destructor definition are well-known.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential.cc:30: DCHECK(ssid_.size() <=
std::numeric_limits<size_t>::max() / 2);
Is there not some more reasonable upper limit on an SSID?

Note that I can come up with some SSIDs that would pass this test but still
cause the generated string to exceed size_t chars (because of the boiler-plate
in the string)...

Also, this doesn't actually protect against exploit attempts (if the SSID could
actually be made this long by an attacker) since it's a DCHECK.

I would just remove it, or, if it is meant to be a protection, rethink it.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
File components/wifi_sync/wifi_credential.h (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential.h:39: // Returns a string representation of
the credential. The string
Document the intended usage of the string. Debugging?

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
File components/wifi_sync/wifi_credential_syncable_service.h (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service.h:11: #include <utility>
double-check all the includes here.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service.h:55: const
base::ThreadChecker thread_checker_;
Is this a common convention for "SyncableService" implementations? Unless I'm
mistaken, all keyed services are expected to be used exclusively on the UI
thread, and I don't think we typically go to the trouble of asserting this.

Basically, we wouldn't add a ThreadChecker to every class in Chrome (even though
most are single-threaded) so I want to understand what source of risk warrants
adding one here.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
File components/wifi_sync/wifi_credential_syncable_service_factory.h (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service_factory.h:26:
content::BrowserContext* browser_context);
forward-decl BrowserContext.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
File components/wifi_sync/wifi_security_class.h (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_security_class.h:28: const std::string& security_in);
It seems uncommon in Chromium to suffix with _in

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
File components/wifi_sync/wifi_security_class_chromeos.cc (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_security_class_chromeos.cc:7: #include <string>
not needed (in header)

[mukesh agrawal, 2014-12-09 01:23:50]

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
File components/wifi_sync/network_state_helper_chromeos.cc (right):

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:40: // a security_class()
field in NetworkState.
On 2014/12/08 17:54:26, stevenjb wrote:
> I looked through the Chrome code. We already have a function,
> shill_property_util::GetSecurityClass that maps Wpa and Rsn to Psk. This code
> could use that function for now, and Shill could stop differentiating (i.e.
map
> Rns and Wpa to Security or a new SecurityClass which NetworkState could switch
> to) at any time.

Acknowledged. Migration to SecurityClass is underway in crbug.com/440032.

https://codereview.chromium.org/709683004/diff/160001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:44: // GetProperties
method.
On 2014/12/08 17:54:26, stevenjb wrote:
> Anything that depends on Fake behavior should clearly be designated as test
only
> code, either in the name of the method (e.g. append ForTest) or by putting the
> behavior in test only code. This is fine I guess for now, I'll look more
closely
> in the follow up CLs.

Acknowledged. Opened crbug.com/440206 for renaming the function.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
File components/wifi_sync/wifi_credential_syncable_service_factory.h (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service_factory.h:26:
content::BrowserContext* browser_context);
On 2014/12/08 21:21:36, erikwright wrote:
> forward-decl BrowserContext.

content::BrowserContext must have been declared, because BuildServiceInstanceFor
uses it, and BuildServiceInstanceFor is a method declared in
BrowserContextKeyedServiceFactory. Should I forward-declare anyway?

(On the one hand, I see that the forward declaration would make the declaration
of the new function independent of the overridden function. On the other hand,
the forward declaration might be considered redundant. I'm happy to go either
way on it; I just want to make sure I learn the right way to think about it.)

[mukesh agrawal, 2014-12-09 01:41:11]

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/ne...
File components/wifi_sync/network_state_helper_chromeos.cc (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:10: #include
"components/wifi_sync/wifi_credential.h"
On 2014/12/08 21:21:36, erikwright wrote:
> not needed (in header)

Done.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.cc:22:
chromeos::NetworkTypePattern::WiFi(),
On 2014/12/08 21:21:36, erikwright wrote:
> #include chromeos/network/network_type_pattern.h

Done.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
File components/wifi_sync/wifi_credential.cc (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential.cc:26: // Nothing special to do.
On 2014/12/08 21:21:36, erikwright wrote:
> remove comment. The reasons for an empty destructor definition are well-known.

Done.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential.cc:30: DCHECK(ssid_.size() <=
std::numeric_limits<size_t>::max() / 2);
On 2014/12/08 21:21:36, erikwright wrote:
> Is there not some more reasonable upper limit on an SSID?
> 
> Note that I can come up with some SSIDs that would pass this test but still
> cause the generated string to exceed size_t chars (because of the boiler-plate
> in the string)...
> 
> Also, this doesn't actually protect against exploit attempts (if the SSID
could
> actually be made this long by an attacker) since it's a DCHECK.
> 
> I would just remove it, or, if it is meant to be a protection, rethink it.

Done.

You're quite right on all three counts. After some thought, I've concluded that
the length checking should be done by whoever creates the WifiCredential. E.g.,
when we read data from sync, or the local network configuration back-end.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
File components/wifi_sync/wifi_credential.h (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential.h:39: // Returns a string representation of
the credential. The string
On 2014/12/08 21:21:36, erikwright wrote:
> Document the intended usage of the string. Debugging?

Done.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
File components/wifi_sync/wifi_credential_syncable_service.h (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service.h:11: #include <utility>
On 2014/12/08 21:21:36, erikwright wrote:
> double-check all the includes here.

Done.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service.h:55: const
base::ThreadChecker thread_checker_;
On 2014/12/08 21:21:36, erikwright wrote:
> Is this a common convention for "SyncableService" implementations? Unless I'm
> mistaken, all keyed services are expected to be used exclusively on the UI
> thread, and I don't think we typically go to the trouble of asserting this.
> 
> Basically, we wouldn't add a ThreadChecker to every class in Chrome (even
though
> most are single-threaded) so I want to understand what source of risk warrants
> adding one here.

After checking the existing code, I see that, of the 21 SyncableService
subclasses, only HistoryService and ThemeSyncableService have a ThreadChecker.
HistoryService probably needs the ThreadChecker because HistoryService creates
its own thread.

Since ThreadChecker isn't a common requirement of SyncableServices, I've removed
the ThreadChecker from WifiCredentialSyncableService.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
File components/wifi_sync/wifi_security_class.h (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_security_class.h:28: const std::string& security_in);
On 2014/12/08 21:21:36, erikwright wrote:
> It seems uncommon in Chromium to suffix with _in

Done.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
File components/wifi_sync/wifi_security_class_chromeos.cc (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_security_class_chromeos.cc:7: #include <string>
On 2014/12/08 21:21:36, erikwright wrote:
> not needed (in header)

Done.

[mukesh agrawal, 2014-12-09 01:45:27]

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/ne...
File components/wifi_sync/network_state_helper_chromeos.h (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/ne...
components/wifi_sync/network_state_helper_chromeos.h:16: // Functions which deal
with ChromeOS-specific network state.
On 2014/12/08 21:21:36, erikwright wrote:
> This comment is confusing (1 since it is on a namespace and 2 since there is
> only one function).
> 
> Since the file is named .._helper_chromeos.h I think it's superfluous and
would
> remove it.

Done.

[mukesh agrawal, 2014-12-09 01:47:48]

erikwright: please take another look
pavely: can you confirm/refute that ThreadChecker is not generally necessary for
SyncableServices?
stevenjb: I've filed crbug.com/440032, and crbug.com/440206, for the last two
issues you mentioned in PS10.

Thanks, everyone!

[erikwright (departed), 2014-12-09 12:42:03]

LGTM with one final request (my response to your IWYU question).

If there is a follow-up CL that I should now be reviewing please explicitly flag
it for me.

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
File components/wifi_sync/wifi_credential_syncable_service_factory.h (right):

https://codereview.chromium.org/709683004/diff/180001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service_factory.h:26:
content::BrowserContext* browser_context);
On 2014/12/09 01:23:50, mukesh agrawal wrote:
> On 2014/12/08 21:21:36, erikwright wrote:
> > forward-decl BrowserContext.
> 
> content::BrowserContext must have been declared, because
BuildServiceInstanceFor
> uses it, and BuildServiceInstanceFor is a method declared in
> BrowserContextKeyedServiceFactory. Should I forward-declare anyway?
> 
> (On the one hand, I see that the forward declaration would make the
declaration
> of the new function independent of the overridden function. On the other hand,
> the forward declaration might be considered redundant. I'm happy to go either
> way on it; I just want to make sure I learn the right way to think about it.)

You are correct that the principle is that the include of an implemented
interface should only be relied upon to make the declaration of the overridden
methods valid.

In some cases you might later remove an implemented interface but still use some
of the types referenced. Or a method from that interface might be removed, and
thus the transitive include lost, and then many implementors will suddenly be
missing includes.

In this case that seems unlikely, but simple IWYU rules save trouble in
rationalizing individual cases.

[mukesh agrawal, 2014-12-09 20:56:17]

pstew: can you approve DEPS for
third_party/cros_system_api/dbus/service_constants.h?

[Paul Stewart, 2014-12-09 21:36:09]

On 2014/12/09 20:56:17, mukesh agrawal wrote:
> pstew: can you approve DEPS for
> third_party/cros_system_api/dbus/service_constants.h?

LGTM for deps on service_constants.h

[mukesh agrawal, 2014-12-09 22:33:25]

Conflicts were just textual (non-semantic), in a couple of gyp files.
Reformatted wifi_sync/BUILD.gn per pre-submit warning (using "gn format
--in-place components/wifi_sync/BUILD.gn").

[mukesh agrawal, 2014-12-09 22:33:37]

The CQ bit was checked by quiche@chromium.org

[commit-bot: I haz the power, 2014-12-09 22:34:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/709683004/260001

[commit-bot: I haz the power, 2014-12-09 22:44:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-09 22:44:43]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[mukesh agrawal, 2014-12-09 22:54:16]

On 2014/12/09 22:44:43, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_aosp on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

android_aosp failure looks unrelated to this change.

gyp:
/b/build/slave/android_aosp/build/android-src/external/chromium_org/third_party/android_tools/android_tools.gyp
not found (cwd:
/b/build/slave/android_aosp/build/android-src/external/chromium_org)

[mukesh agrawal, 2014-12-09 22:54:25]

The CQ bit was checked by quiche@chromium.org

[commit-bot: I haz the power, 2014-12-09 22:54:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/709683004/260001

[commit-bot: I haz the power, 2014-12-09 23:03:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-09 23:03:55]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[mukesh agrawal, 2014-12-09 23:18:03]

android_dbg_tests_recipe failure was due to the use of std::vector<>::data().
This method was added in c++11. (Before C++11, std::string had a data() method,
but std::vector did not.)

Fixed in PS15.

[mukesh agrawal, 2014-12-11 00:52:16]

PS16 improves the build configuration, as follows:

1. Fixes non-chromeos, non-gyp builds, by only building wifi_sync when
|enable_wifi_sync| is set. The value of enable_wifi_sync default to 0, except on
chromeos.

2. Removes some unnecessary checks for chromeos, by taking advantage of the fact
that GYP and GN understand platform-specific suffixes. (AIUI, files ending in
<platform>.{cc,h} are automagically excluded if we're not building for
<platform>.)

3. Separates out code which should only be built when wifi sync is enabled
(protected by ENABLE_WIFI_SYNC), from wifi_sync code which should only be built
for chromeos (protected by OS_CHROMEOS).

PS16 was tested by building
{chrome,sync_integration_tests,components_unittests}x{chromeos,linux}x{gyp,gn}.

[mukesh agrawal, 2014-12-11 01:02:01]

I forgot to note: the sync_integration_tests,chromeos,gn build fails. But it
fails in the same way as on ToT.

[mukesh agrawal, 2014-12-11 01:08:28]

quiche@chromium.org changed reviewers:
+ brettw@chromium.org

[brettw, 2014-12-15 22:46:02]

Do we really need a global GYP variable and a global define set for every single
file in the build for this? CAn we just put this behind the chromeos flag? It
seems that this will not be used outside of ChromeOS.

[mukesh agrawal, 2014-12-16 00:26:11]

On 2014/12/15 22:46:02, brettw wrote:
> Do we really need a global GYP variable and a global define set for every
single
> file in the build for this? CAn we just put this behind the chromeos flag? It
> seems that this will not be used outside of ChromeOS.

My reason for proposing a separate variable is that WiFi sync isn't inherently
ChromeOS-only. While WiFi sync is ChromeOS-only for now, we might well want
to implement WiFi sync on other platforms in the future.

Granted, it's possible to over-think what we might need, and over-engineer
things to be "future-proof". But my (non-expert) belief was that a GYP/GN
variable and a preprocessor define were are a reasonable price to pay.

Let me know your judgment on this, and I'll make changes accordingly.
(If the possibility of supporting this feature on other platforms in the
future justifies the global variable, then I'll leave the CL unchanged.
Otherwise, I'll remove enable_wifi_sync, and use chromeos==1/is_chromeos
instead.)

Thanks.

[brettw, 2014-12-16 03:34:22]

I've been pushing back on new global defines. We've got way too many and it's
growing at an unsustainable rate. These global ones get added to the command
line of every single file compiled in the project.

I was basing my ChromeOS define suggestion on my understanding of this feature.
I don't think Chrome is in charge of logging on to your wifi on any system other
than ChromeOS, so it seems pretty unlikely that we would need wifi credentials
on any other system. If my understanding is correct, I think this is the best
approach.

For other cases, I'd recommend trying to limit the scope of the define so it
only applies to the specific target that needs it.

[mukesh agrawal, 2014-12-16 17:57:57]

Thanks for elaborating. I think I understand your concerns now.

Since the ENABLE_WIFI_SYNC define is only used in one file, it does seem
silly to add a command-line argument to the compilation of every file in
the build. I'll update the code in profile_sync_components_factory_impl.cc
to use OS_CHROMEOS instead of needing ENABLE_WIFI_SYNC.

Two more questions, if I may. Inside the build files, I think the code
will be clearer if I abstract away wifi sync as a feature, from the
platform it's built on. For example, if I use the chromeos variable
directly, the components/components_tests.gyp code would look like this:

            ['chromeos == 1', {
              'sources': [
                'wifi_sync/wifi_security_class_chromeos_unittest.cc',
              ],
              'dependencies': [
                # Dependencies of wifi_sync
                 'components.gyp:wifi_sync',
              ],
            }]

Having the sources clause conditioned on the chromeos variable seems
redundant, since a file named *chromeos_unittest.cc will automatically
be filtered out on other platforms. So my questions are:

1) Is avoiding the apparent redundancy a good reason to have a separate
   enable_wifi_sync variable in the GYP and GN files? Or is it preferable
   to have code like the above?
2) If it is better to have the enable_wifi_sync variable, should that
   variable be global in the build config? Or should the variable only
   be present in the files that actually reference it?

Thanks!

[brettw, 2014-12-17 18:52:29]

> Two more questions, if I may. Inside the build files, I think the code
> will be clearer if I abstract away wifi sync as a feature, from the
> platform it's built on.

This is a fuzzy judgement call and might go different ways depending on the
feature. If nobody ever runs with the flag in the non-default configuration
(otherwise it will bit rot -- we have a lot of bitrotted build flags!) and we
don't expect the meaning of the feature to be changed then just using the same
flag seems better.

After going through a log of GYP files in doing the GN build, I've encountered a
lot of flags and it's not at all clear when that corresponding code is compiled.
You have to grep through common.gypi to see that this thing is enabled on
Android (or whatever). So while there is some clarity advantage to keeping a
feature self-contained in some cases, most of the time sombody reading your GYP
code just wants to know when the give condition is true, and using a separate
flag makes it harder to discern.

Last, if ChromeOS is always built with the flag and nothing else is, over time
the flag's separateness will bitrot and chromeos can't compile with the flag
turned off, and nothing else will compile with the flag turned on. The result
will be a flag that can only have the exact meaning of "chromeos" and can't be
toggled.

I think this applies to you, so in this case just using "chromeos" seems best to
me. But my assumptions about this feature might be incorrect.


> 1) Is avoiding the apparent redundancy a good reason to have a separate
>    enable_wifi_sync variable in the GYP and GN files? Or is it preferable
>    to have code like the above?

This redundancy doesn't bother me a bit. The automatic filtering is a bit
mysterious and in many cases it's more clear if this is explicit.


> 2) If it is better to have the enable_wifi_sync variable, should that
>    variable be global in the build config? Or should the variable only
>    be present in the files that actually reference it?

I can't remember the GYP scoping rules enough to be confident in giving you good
advice. GN supports more modular flags. If only one BUILD file needs it in GN,
you should put the flag in that file. If there are a couple, you can put it in a
.gni file to share the value without making it global.

[mukesh agrawal, 2014-12-18 18:00:40]

Thanks for taking the time to explain the trade-offs. It's super-helpful,
as I hadn't considered some of the issues you mentioned.

In particular, I hadn't thought about bit rot. And your intuition is
dead-on. We won't have any ChromeOS builds that disable the feature, and
there are no immediate plans to support it on other platforms. So there's
a high risk of bit rot.

Given that, I've switched over to using the chromeos and is_chromeos
variables for GYP and GN respectively. Could you take another look at the
chrome/test/BUILD.gn? (None of the other current reviewers can sign off
on that file.)

Thanks again. :)

[brettw, 2014-12-19 18:23:31]

*.gn LGTM. Thanks!

[mukesh agrawal, 2014-12-19 18:44:37]

The CQ bit was checked by quiche@chromium.org

[commit-bot: I haz the power, 2014-12-19 18:45:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/709683004/360001

[commit-bot: I haz the power, 2014-12-19 18:52:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-19 18:52:34]

Try jobs failed on following builders:
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mukesh agrawal, 2014-12-19 22:16:38]

The CQ bit was checked by quiche@chromium.org

[commit-bot: I haz the power, 2014-12-19 22:17:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/709683004/380001

[commit-bot: I haz the power, 2014-12-19 22:34:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-19 22:34:50]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[mukesh agrawal, 2014-12-20 03:07:55]

The CQ bit was checked by quiche@chromium.org

[commit-bot: I haz the power, 2014-12-20 03:08:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/709683004/400001

[commit-bot: I haz the power, 2014-12-20 03:58:25]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2014-12-20 03:59:19]

Patchset 21 (id:??) landed as
https://crrev.com/271c6cca48a6cef32c0f3add3b17b700707deec5
Cr-Commit-Position: refs/heads/master@{#309333}