Adding new extension sync integration tests. Also modified the way that profile

Adding new apps/extension sync integration tests.  Also modified the way that profile
extensions states are compared for equality.  Previously, the allow in incognito mode
setting was not accounted for in checking for equality.

New tests:
TwoClientLiveExtensionsSyncTest.Merge
TwoClientLiveExtensionsSyncTest.UpdateEnableDisableExtension
TwoClientLiveExtensionsSyncTest.UpdateIncognitoEnableDisable
TwoClientLiveAppsSyncTest.Merge
TwoClientLiveAppsSyncTest.UpdateEnableDisableApp
TwoClientLiveAppsSyncTest.UpdateIncognitoEnableDisable

BUG=85482
TEST=


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

[akalin, 2011-06-09 03:21:59]

http://codereview.chromium.org/7104072/diff/5002/chrome/test/live_sync/live_e...
File chrome/test/live_sync/live_extensions_sync_test.h (left):

http://codereview.chromium.org/7104072/diff/5002/chrome/test/live_sync/live_e...
chrome/test/live_sync/live_extensions_sync_test.h:43: 
You'll want to make similar changes to live_apps_sync_test.{h,cc}.

http://codereview.chromium.org/7104072/diff/5002/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_extension_helper.h (right):

http://codereview.chromium.org/7104072/diff/5002/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:23: enum ExtensionState {
DISABLED, PENDING, ENABLED };
It's probably better to have something like this:

struct ExtensionState {
  enum EnabledState { DISABLED, PENDING, ENABLED };
  bool incognito_enabled;
};
typedef std::map<std::string, ExtensionState> ExtensionStateMap;

http://codereview.chromium.org/7104072/diff/5002/chrome/test/live_sync/two_cl...
File chrome/test/live_sync/two_client_live_extensions_sync_test.cc (right):

http://codereview.chromium.org/7104072/diff/5002/chrome/test/live_sync/two_cl...
chrome/test/live_sync/two_client_live_extensions_sync_test.cc:150: // TCM ID -
3635304.
similar tests for two_client_live_apps_sync_test.cc.

[braffert, 2011-06-09 18:20:41]

I've added similar tests for apps, and also changed ExtensionState to a struct
as suggested.

http://codereview.chromium.org/7104072/diff/5002/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_extension_helper.h (right):

http://codereview.chromium.org/7104072/diff/5002/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:23: enum ExtensionState {
DISABLED, PENDING, ENABLED };
On 2011/06/09 03:21:59, akalin wrote:
> It's probably better to have something like this:
> 
> struct ExtensionState {
>   enum EnabledState { DISABLED, PENDING, ENABLED };
>   bool incognito_enabled;
> };
> typedef std::map<std::string, ExtensionState> ExtensionStateMap;

I agree - done.

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
File chrome/test/live_sync/two_client_live_apps_sync_test.cc (right):

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
chrome/test/live_sync/two_client_live_apps_sync_test.cc:171:
IN_PROC_BROWSER_TEST_F(TwoClientLiveAppsSyncTest, UpdateEnableDisableApp) {
Couldn't find TCM ids for these two cases.

[akalin, 2011-06-09 20:35:36]

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_extension_helper.cc (right):

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.cc:130: if
(extension_service->IsIncognitoEnabled((*it)->id())) {
lotta duplicated code.  Perhaps decomp into a separate function,
SetIncognitoState(ExtensionState*, const ExtensionService&)?

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.cc:131:
extension_state_map[(*it)->id()].incognito_enabled = true;
simpler to do

extension_state_map[...].incognito_enabled =
extension_service->IsIncognitoEnabled(...);
VLOG(2) << "Incognito enabled: " << extension_state_map[...].incognito_enabled

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_extension_helper.h (right):

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:25: bool operator==(const
ExtensionState &other) const;
Do you need this?  If so, operator overloading is generally against
Google/Chrome style.  Rename to Equals() should suffice.

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:26: EnabledState
enabled_state;
please define a default constructor to set these vars to some default value.

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
File chrome/test/live_sync/two_client_live_apps_sync_test.cc (right):

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
chrome/test/live_sync/two_client_live_apps_sync_test.cc:171:
IN_PROC_BROWSER_TEST_F(TwoClientLiveAppsSyncTest, UpdateEnableDisableApp) {
On 2011/06/09 18:20:42, braffert wrote:
> Couldn't find TCM ids for these two cases.

Not sure if they're needed -- ask Anna?

[anna, 2011-06-09 21:37:58]

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
File chrome/test/live_sync/two_client_live_apps_sync_test.cc (right):

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
chrome/test/live_sync/two_client_live_apps_sync_test.cc:151:
IN_PROC_BROWSER_TEST_F(TwoClientLiveAppsSyncTest, Merge) {
It would be great to test the merge of a delete as well.

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
chrome/test/live_sync/two_client_live_apps_sync_test.cc:171:
IN_PROC_BROWSER_TEST_F(TwoClientLiveAppsSyncTest, UpdateEnableDisableApp) {
TCM ID: 7723126

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
chrome/test/live_sync/two_client_live_apps_sync_test.cc:199:
UpdateIncognitoEnableDisable) {
TCM ID: 7706637

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
File chrome/test/live_sync/two_client_live_extensions_sync_test.cc (right):

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
chrome/test/live_sync/two_client_live_extensions_sync_test.cc:151:
IN_PROC_BROWSER_TEST_F(TwoClientLiveExtensionsSyncTest, Merge) {
It would be great to test the merge of a delete as well.

[braffert, 2011-06-09 23:09:35]

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_extension_helper.cc (right):

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.cc:130: if
(extension_service->IsIncognitoEnabled((*it)->id())) {
On 2011/06/09 20:35:37, akalin wrote:
> lotta duplicated code.  Perhaps decomp into a separate function,
> SetIncognitoState(ExtensionState*, const ExtensionService&)?

I'm not sure if this is a good idea as such a function would require a second
pass through all three extension lists (enabled, disabled, pending).  I've
cleaned up the code so that there is less repetition.  Also, the extra VLOG
statements shouldn't have made it into the last CL - they were added while
debugging.

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.cc:131:
extension_state_map[(*it)->id()].incognito_enabled = true;
On 2011/06/09 20:35:37, akalin wrote:
> simpler to do
> 
> extension_state_map[...].incognito_enabled =
> extension_service->IsIncognitoEnabled(...);
> VLOG(2) << "Incognito enabled: " << extension_state_map[...].incognito_enabled

Done.

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_extension_helper.h (right):

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:25: bool operator==(const
ExtensionState &other) const;
On 2011/06/09 20:35:37, akalin wrote:
> Do you need this?  If so, operator overloading is generally against
> Google/Chrome style.  Rename to Equals() should suffice.

Done.  This required some additional changes, because we can no longer use the
== operator to compare ExtensionStateMaps.

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:26: EnabledState
enabled_state;
On 2011/06/09 20:35:37, akalin wrote:
> please define a default constructor to set these vars to some default value.

Done.

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
File chrome/test/live_sync/two_client_live_apps_sync_test.cc (right):

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
chrome/test/live_sync/two_client_live_apps_sync_test.cc:151:
IN_PROC_BROWSER_TEST_F(TwoClientLiveAppsSyncTest, Merge) {
On 2011/06/09 21:37:58, anna wrote:
> It would be great to test the merge of a delete as well.

Done.

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
chrome/test/live_sync/two_client_live_apps_sync_test.cc:171:
IN_PROC_BROWSER_TEST_F(TwoClientLiveAppsSyncTest, UpdateEnableDisableApp) {
On 2011/06/09 21:37:58, anna wrote:
> TCM ID: 7723126

Done.

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
chrome/test/live_sync/two_client_live_apps_sync_test.cc:199:
UpdateIncognitoEnableDisable) {
On 2011/06/09 21:37:58, anna wrote:
> TCM ID: 7706637

Done.

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
File chrome/test/live_sync/two_client_live_extensions_sync_test.cc (right):

http://codereview.chromium.org/7104072/diff/5003/chrome/test/live_sync/two_cl...
chrome/test/live_sync/two_client_live_extensions_sync_test.cc:151:
IN_PROC_BROWSER_TEST_F(TwoClientLiveExtensionsSyncTest, Merge) {
On 2011/06/09 21:37:58, anna wrote:
> It would be great to test the merge of a delete as well.

Done.

[anna, 2011-06-09 23:25:58]

LGTM: tests.

[akalin, 2011-06-09 23:44:39]

http://codereview.chromium.org/7104072/diff/4026/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_extension_helper.cc (right):

http://codereview.chromium.org/7104072/diff/4026/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.cc:176: if
(!it->second.Equals(state_map2[it->first])) {
this isn't quite correct.  state_map2[...] auto-creates a value if it's not
there, so if it happens to match the default values, this won't return false.

You want something like:

const ExtensionStateMap& state_map1 = ...;
const ExtensionStateMap& state_map2 = ...;
if (state_map1.size() != state_map2.size()) {
  VLOG(1) << ...;
  return false;
}
return std::equal(state_map1.begin(), state_map1.end(), state_map2.begin(),
ExtensionStateEqual());

You'll have to define ExtensionStateEqual to forward to ExtensionState::Equal
(and add some logging if they don't match).  Don't forget to #include
<algorithm> for std::equal.

Alternatively, you could replace the call to std::equal with a loop over both
maps.  That might be more straightforward.

http://codereview.chromium.org/7104072/diff/4026/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_extension_helper.h (right):

http://codereview.chromium.org/7104072/diff/4026/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:23: struct ExtensionState {
Move this to private section

http://codereview.chromium.org/7104072/diff/4026/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:71: // Returns a map from
|profile|'s installed extensions to their
Move this to private section

http://codereview.chromium.org/7104072/diff/4026/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:76: bool
ExtensionStateMapsMatch(Profile* profile1, Profile* profile2);
Name this something like ExtensionStatesMatch().  Remove reference to extension
state maps, i.e. say that it returns true iff profile1 and profile2 have the
same extensions/apps and they all are in the same state.  Make it const.

[braffert, 2011-06-10 01:23:57]

Hopefully this will take care of everything.  Thanks for the thorough reviews.

http://codereview.chromium.org/7104072/diff/4026/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_extension_helper.cc (right):

http://codereview.chromium.org/7104072/diff/4026/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.cc:176: if
(!it->second.Equals(state_map2[it->first])) {
On 2011/06/09 23:44:39, akalin wrote:
> this isn't quite correct.  state_map2[...] auto-creates a value if it's not
> there, so if it happens to match the default values, this won't return false.
> 
> You want something like:
> 
> const ExtensionStateMap& state_map1 = ...;
> const ExtensionStateMap& state_map2 = ...;
> if (state_map1.size() != state_map2.size()) {
>   VLOG(1) << ...;
>   return false;
> }
> return std::equal(state_map1.begin(), state_map1.end(), state_map2.begin(),
> ExtensionStateEqual());
> 
> You'll have to define ExtensionStateEqual to forward to ExtensionState::Equal
> (and add some logging if they don't match).  Don't forget to #include
> <algorithm> for std::equal.
> 
> Alternatively, you could replace the call to std::equal with a loop over both
> maps.  That might be more straightforward.

Done.

http://codereview.chromium.org/7104072/diff/4026/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_extension_helper.h (right):

http://codereview.chromium.org/7104072/diff/4026/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:23: struct ExtensionState {
On 2011/06/09 23:44:39, akalin wrote:
> Move this to private section

Done.

http://codereview.chromium.org/7104072/diff/4026/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:71: // Returns a map from
|profile|'s installed extensions to their
On 2011/06/09 23:44:39, akalin wrote:
> Move this to private section

Done.

http://codereview.chromium.org/7104072/diff/4026/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:76: bool
ExtensionStateMapsMatch(Profile* profile1, Profile* profile2);
On 2011/06/09 23:44:39, akalin wrote:
> Name this something like ExtensionStatesMatch().  Remove reference to
extension
> state maps, i.e. say that it returns true iff profile1 and profile2 have the
> same extensions/apps and they all are in the same state.  Make it const.

Done.

[akalin, 2011-06-10 01:56:44]

LGTM, after the below nits

Since commit-bot doesn't run the sync integration tests, after you make your
final changes, run those locally (probably just need the Extension/Apps only) to
make sure they still pass.  Then check the "Commit" box, but make sure you're
around until after it's committed to make sure it doesn't break on the
waterfall.

Good luck!

http://codereview.chromium.org/7104072/diff/1013/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_extension_helper.cc (right):

http://codereview.chromium.org/7104072/diff/1013/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.cc:187: } else if
(!(it1->second.Equals(it2->second))) {
usual Google/Chrome style is to not have parentheses inside !, so:

else if (!it->second.Equals(it2->second)) {

http://codereview.chromium.org/7104072/diff/1013/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_extension_helper.h (right):

http://codereview.chromium.org/7104072/diff/1013/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:63: bool
ExtensionStatesMatch(Profile* profile1, Profile* profile2) const;
I think this can be static

http://codereview.chromium.org/7104072/diff/1013/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:69: enum EnabledState {
DISABLED, PENDING, ENABLED };
Put types first, then blank line, then functions, then blank line, then
variables.  So:

struct ExtensionState {
  enum EnabledState ...;

  ExtensionState();
  ~ExtensionState();

  EnabledState enabled_state;
  bool incognito_enabled;
};

http://codereview.chromium.org/7104072/diff/1013/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:80: ExtensionStateMap
GetExtensionStates(Profile* profile) const;
I think this can be static

[braffert, 2011-06-10 16:49:17]

The last round of suggestions have all been handled - submitting to the commit
bot.  Thanks again.

http://codereview.chromium.org/7104072/diff/1013/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_extension_helper.cc (right):

http://codereview.chromium.org/7104072/diff/1013/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.cc:188: VLOG(1) << "Extension
states for profile " << profile1->GetDebugName()
Done.  I had thought that without the parentheses it may have been ambiguous
exactly what was being inverted.

http://codereview.chromium.org/7104072/diff/1013/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_extension_helper.h (right):

http://codereview.chromium.org/7104072/diff/1013/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:63: bool
ExtensionStatesMatch(Profile* profile1, Profile* profile2) const;
On 2011/06/10 01:56:44, akalin wrote:
> I think this can be static

Done.  This required making the function non-const, and the same for
ExtensionStateMap(Profile*).

http://codereview.chromium.org/7104072/diff/1013/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:69: enum EnabledState {
DISABLED, PENDING, ENABLED };
On 2011/06/10 01:56:44, akalin wrote:
> Put types first, then blank line, then functions, then blank line, then
> variables.  So:
> 
> struct ExtensionState {
>   enum EnabledState ...;
> 
>   ExtensionState();
>   ~ExtensionState();
> 
>   EnabledState enabled_state;
>   bool incognito_enabled;
> };

Done.

http://codereview.chromium.org/7104072/diff/1013/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_extension_helper.h:80: ExtensionStateMap
GetExtensionStates(Profile* profile) const;
On 2011/06/10 01:56:44, akalin wrote:
> I think this can be static

Done.

[commit-bot: I haz the power, 2011-06-10 17:38:51]

Change committed as 88681