[Extensions] Sync the 'allowed scripting on all urls' preference

chrome/browser/extensions/extension_sync_data_unittest.cc

Index: chrome/browser/extensions/extension_sync_data_unittest.cc
diff --git a/chrome/browser/extensions/extension_sync_data_unittest.cc b/chrome/browser/extensions/extension_sync_data_unittest.cc
index 46b35493c1546fe4a4bb908dfda63496111ace5d..a25d89efef1234f293b87221333d528b7201e82c 100644
--- a/chrome/browser/extensions/extension_sync_data_unittest.cc
+++ b/chrome/browser/extensions/extension_sync_data_unittest.cc
@@ -12,6 +12,8 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
+namespace extensions {
+
 namespace {
 
 const char kValidId[] = "abcdefghijklmnopabcdefghijklmnop";
@@ -20,6 +22,12 @@ const char kValidUpdateUrl[] =
     "https://clients2.google.com/service/update2/crx";
 const char kName[] = "MyExtension";
 
+const ExtensionSyncData::PrefValue kPrefTrue = ExtensionSyncData::PREF_TRUE;
+const ExtensionSyncData::PrefValue kPrefFalse = ExtensionSyncData::PREF_FALSE;
+const ExtensionSyncData::PrefValue kPrefUnset = ExtensionSyncData::PREF_UNSET;
+
+}  // namespace
+
 class ExtensionSyncDataTest : public testing::Test {
 };
 
@@ -30,12 +38,13 @@ TEST_F(ExtensionSyncDataTest, SyncDataToExtensionSyncDataForExtension) {
   extension_specifics->set_update_url(kValidUpdateUrl);
   extension_specifics->set_enabled(false);
   extension_specifics->set_incognito_enabled(true);
+  extension_specifics->set_all_urls_enabled(true);
   extension_specifics->set_version(kVersion);
   extension_specifics->set_name(kName);
   syncer::SyncData sync_data =
       syncer::SyncData::CreateLocalData("sync_tag", "non_unique_title", entity);
 
-  extensions::ExtensionSyncData extension_sync_data(sync_data);
+  ExtensionSyncData extension_sync_data(sync_data);
   EXPECT_EQ(extension_specifics->id(), extension_sync_data.id());
   EXPECT_EQ(extension_specifics->version(),
             extension_sync_data.version().GetString());
@@ -44,8 +53,16 @@ TEST_F(ExtensionSyncDataTest, SyncDataToExtensionSyncDataForExtension) {
   EXPECT_EQ(extension_specifics->enabled(), extension_sync_data.enabled());
   EXPECT_EQ(extension_specifics->incognito_enabled(),
             extension_sync_data.incognito_enabled());
+  EXPECT_EQ(extension_specifics->all_urls_enabled(),
+            extension_sync_data.all_urls_enabled() == kPrefTrue);

[not at google - send to devlin, 2015/02/11 01:34:51]

This looks convoluted.

We know that extension_specifics->all_urls_enabled() is true.

Therefore this condition is equivalent to
EXPECT_EQ(true, extension_sync_data.all_urls_enabled() == kPrefTrue)

which is only true when extension_sync_data.all_urls_enabled() == kPrefTrue

Therefore this condition is equivalent to
EXPECT_EQ(kPrefTrue, extension_sync_data.all_urls_enabled())

which is a lot simpler.

However, I see that it's a symptom of a larger problem. These tests seem to be
basically testing that if you serialize ExtensionSyncData into
ExtensionSpecifics, then back again, you get the same result. They just
overcomplicate it. Nevertheless it was a good idea, and now would be a good time
to implement it properly.

Proposal: have 2 methods. One asserts that serializing a SyncData and then
deserializing it equals the old value. The other, the opposite.

Then call that 3 times each, mutating some data if you really want, but most
importantly test all 3 configurations of all urls enabled (not set, true,
false).

Note that the protobuf objects implement an operator= method. That might come in
handy.

[Devlin, 2015/02/11 17:52:21]

Yeah, the current test leaves a little to be wanted.  New test: check
serialization/deserialization in both directions, as well as correctness.  A few
more lines of code, but probably worth it.

   EXPECT_EQ(extension_specifics->name(), extension_sync_data.name());
   EXPECT_FALSE(extension_sync_data.uninstalled());
+
+  extension_specifics->clear_all_urls_enabled();
+  sync_data = syncer::SyncData::CreateLocalData(
+      "sync_tag", "non_unique_title", entity);
+  extension_sync_data = ExtensionSyncData(sync_data);
+  EXPECT_EQ(kPrefUnset, extension_sync_data.all_urls_enabled());
 }
 
 TEST_F(ExtensionSyncDataTest, ExtensionSyncDataToSyncDataForExtension) {
@@ -55,11 +72,12 @@ TEST_F(ExtensionSyncDataTest, ExtensionSyncDataToSyncDataForExtension) {
   input_extension->set_update_url(kValidUpdateUrl);
   input_extension->set_enabled(true);
   input_extension->set_incognito_enabled(false);
+  input_extension->set_all_urls_enabled(false);
   input_extension->set_version(kVersion);
   input_extension->set_name(kName);
   syncer::SyncData sync_data =
       syncer::SyncData::CreateLocalData("sync_tag", "non_unique_title", entity);
-  extensions::ExtensionSyncData extension_sync_data(sync_data);
+  ExtensionSyncData extension_sync_data(sync_data);
 
   syncer::SyncData output_sync_data = extension_sync_data.GetSyncData();
   const sync_pb::ExtensionSpecifics& output_specifics =
@@ -70,9 +88,20 @@ TEST_F(ExtensionSyncDataTest, ExtensionSyncDataToSyncDataForExtension) {
   EXPECT_EQ(extension_sync_data.enabled(), output_specifics.enabled());
   EXPECT_EQ(extension_sync_data.incognito_enabled(),
             output_specifics.incognito_enabled());
+  EXPECT_EQ(kPrefFalse, extension_sync_data.all_urls_enabled());
+  EXPECT_FALSE(output_specifics.all_urls_enabled());
   EXPECT_EQ(extension_sync_data.version().GetString(),
             output_specifics.version());
   EXPECT_EQ(extension_sync_data.name(), output_specifics.name());
+
+  input_extension->clear_all_urls_enabled();
+  sync_data = syncer::SyncData::CreateLocalData(
+      "sync_tag", "non_unique_title", entity);
+  extension_sync_data = ExtensionSyncData(sync_data);
+  output_sync_data = extension_sync_data.GetSyncData();
+  EXPECT_EQ(kPrefUnset, extension_sync_data.all_urls_enabled());
+  EXPECT_FALSE(
+      output_sync_data.GetSpecifics().extension().has_all_urls_enabled());
 }
 
-}  // namespace
+}  // namespace extensions