diff options
author | akalin@chromium.org <akalin@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-04-17 07:35:04 +0000 |
---|---|---|
committer | akalin@chromium.org <akalin@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-04-17 07:35:04 +0000 |
commit | 90310d9d9e8a7a0f598d27b2982a7a4498e14138 (patch) | |
tree | bf5779408bd47ad2c36a0c08df907a61d905575f /chrome/browser | |
parent | df37c6b66ee155d84f8c07cbd5d9e0d745799f3f (diff) | |
download | chromium_src-90310d9d9e8a7a0f598d27b2982a7a4498e14138.zip chromium_src-90310d9d9e8a7a0f598d27b2982a7a4498e14138.tar.gz chromium_src-90310d9d9e8a7a0f598d27b2982a7a4498e14138.tar.bz2 |
[Sync] Move some extension-sync-related logic to ExtensionService
Add ExtensionSyncData class to hold the data that goes between the
extension service and extension sync.
Add ProcessSyncData() method to ExtensionService (with unit tests!).
Remove now-unneeded extension sync functions.
BUG=77995
TEST=
Review URL: http://codereview.chromium.org/6852029
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@81899 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome/browser')
16 files changed, 380 insertions, 206 deletions
diff --git a/chrome/browser/background_application_list_model_unittest.cc b/chrome/browser/background_application_list_model_unittest.cc index 1ed6ae0..21d5918 100644 --- a/chrome/browser/background_application_list_model_unittest.cc +++ b/chrome/browser/background_application_list_model_unittest.cc @@ -68,7 +68,7 @@ void BackgroundApplicationListModelTest::InitializeEmptyExtensionService() { profile_.reset(profile); service_ = profile->CreateExtensionService( CommandLine::ForCurrentProcess(), - bogus_file_path()); + bogus_file_path(), false); service_->set_extensions_enabled(true); service_->set_show_extensions_prompts(false); service_->OnLoadedInstalledExtensions(); /* Sends EXTENSIONS_READY */ diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc index 2197792..3ddccd1 100644 --- a/chrome/browser/extensions/extension_service.cc +++ b/chrome/browser/extensions/extension_service.cc @@ -38,6 +38,7 @@ #include "chrome/browser/extensions/extension_process_manager.h" #include "chrome/browser/extensions/extension_processes_api.h" #include "chrome/browser/extensions/extension_special_storage_policy.h" +#include "chrome/browser/extensions/extension_sync_data.h" #include "chrome/browser/extensions/extension_updater.h" #include "chrome/browser/extensions/extension_web_ui.h" #include "chrome/browser/extensions/extension_webnavigation_api.h" @@ -1197,6 +1198,63 @@ void ExtensionService::CheckForUpdatesSoon() { } } +void ExtensionService::ProcessSyncData( + const ExtensionSyncData& extension_sync_data, + PendingExtensionInfo::ShouldAllowInstallPredicate should_allow) { + const std::string& id = extension_sync_data.id; + + // Handle uninstalls first. + if (extension_sync_data.uninstalled) { + std::string error; + if (!UninstallExtensionHelper(this, id)) { + LOG(WARNING) << "Could not uninstall extension " << id + << " for sync"; + } + return; + } + + const Extension* extension = GetExtensionByIdInternal(id, true, true); + // TODO(akalin): Figure out what to do with terminated extensions. + + // Handle already-installed extensions (just update settings). + // + // TODO(akalin): Ideally, we should be able to set prefs for an + // extension regardless of whether or not it's installed (and have + // it automatially apply on install). + if (extension) { + if (extension_sync_data.enabled) { + EnableExtension(id); + } else { + DisableExtension(id); + } + SetIsIncognitoEnabled(id, extension_sync_data.incognito_enabled); + int result = extension->version()->CompareTo(extension_sync_data.version); + if (result < 0) { + // Extension is outdated. + CheckForUpdatesSoon(); + } else if (result > 0) { + // Sync version is outdated. Do nothing for now, as sync code + // in other places will eventually update the sync data. + // + // TODO(akalin): Move that code here. + } + return; + } + + // Handle not-yet-installed extensions. + // + // TODO(akalin): Replace silent update with a list of enabled + // permissions. + pending_extension_manager()->AddFromSync( + id, + extension_sync_data.update_url, + should_allow, + true, // install_silently + extension_sync_data.enabled, + extension_sync_data.incognito_enabled); + CheckForUpdatesSoon(); +} + bool ExtensionService::IsIncognitoEnabled( const std::string& extension_id) const { // If this is an existing component extension we always allow it to diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h index 3c5b657..c63ec78 100644 --- a/chrome/browser/extensions/extension_service.h +++ b/chrome/browser/extensions/extension_service.h @@ -19,6 +19,7 @@ #include "base/task.h" #include "base/time.h" #include "base/tuple.h" +#include "base/version.h" #include "chrome/browser/extensions/apps_promo.h" #include "chrome/browser/extensions/extension_icon_manager.h" #include "chrome/browser/extensions/extension_menu_manager.h" @@ -39,6 +40,7 @@ class ExtensionBrowserEventRouter; class ExtensionPreferenceEventRouter; class ExtensionServiceBackend; +struct ExtensionSyncData; class ExtensionToolbarModel; class ExtensionUpdater; class GURL; @@ -80,9 +82,29 @@ class ExtensionServiceInterface { // Safe to call multiple times in a row. // - // TODO(akalin): Remove this method (and others) once we add - // ProcessSyncData(). + // TODO(akalin): Remove this method (and others) once we refactor + // themes sync to not use it directly. virtual void CheckForUpdatesSoon() = 0; + + // Take any actions required to make the local state of the + // extension match the state in |extension_sync_data| (including + // installing/uninstalling the extension). + // + // TODO(akalin): We'll eventually need a separate method for app + // sync. See http://crbug.com/58077 and http://crbug.com/61447. + virtual void ProcessSyncData( + const ExtensionSyncData& extension_sync_data, + PendingExtensionInfo::ShouldAllowInstallPredicate should_allow) = 0; + + // TODO(akalin): Add a method like: + // + // virtual void + // GetInitialSyncData(bool (*filter)(Extension), + // map<string, ExtensionSyncData>* out) const; + // + // which would fill |out| with sync data for the extensions that + // match |filter|. Sync would use this for the initial syncing + // step. }; // Manages installed and running Chromium extensions. @@ -342,6 +364,11 @@ class ExtensionService virtual void CheckForUpdatesSoon(); + virtual void ProcessSyncData( + const ExtensionSyncData& extension_sync_data, + PendingExtensionInfo::ShouldAllowInstallPredicate + should_allow_install); + void set_extensions_enabled(bool enabled) { extensions_enabled_ = enabled; } bool extensions_enabled() { return extensions_enabled_; } diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc index 60d8738..2b4bae1 100644 --- a/chrome/browser/extensions/extension_service_unittest.cc +++ b/chrome/browser/extensions/extension_service_unittest.cc @@ -28,10 +28,14 @@ #include "chrome/browser/extensions/extension_error_reporter.h" #include "chrome/browser/extensions/extension_service.h" #include "chrome/browser/extensions/extension_special_storage_policy.h" +#include "chrome/browser/extensions/extension_sync_data.h" +#include "chrome/browser/extensions/extension_updater.h" #include "chrome/browser/extensions/external_extension_provider_impl.h" #include "chrome/browser/extensions/external_extension_provider_interface.h" #include "chrome/browser/extensions/external_pref_extension_loader.h" #include "chrome/browser/extensions/pack_extension_job.cc" +#include "chrome/browser/extensions/pending_extension_info.h" +#include "chrome/browser/extensions/pending_extension_manager.h" #include "chrome/browser/prefs/browser_prefs.h" #include "chrome/browser/prefs/pref_service_mock_builder.h" #include "chrome/browser/prefs/scoped_user_pref_update.h" @@ -395,7 +399,8 @@ ExtensionServiceTestBase::~ExtensionServiceTestBase() { } void ExtensionServiceTestBase::InitializeExtensionService( - const FilePath& pref_file, const FilePath& extensions_install_dir) { + const FilePath& pref_file, const FilePath& extensions_install_dir, + bool autoupdate_enabled) { ExtensionTestingProfile* profile = new ExtensionTestingProfile(); // Create a PrefService that only contains user defined preference values. PrefService* prefs = @@ -408,7 +413,8 @@ void ExtensionServiceTestBase::InitializeExtensionService( service_ = profile->CreateExtensionService( CommandLine::ForCurrentProcess(), - extensions_install_dir); + extensions_install_dir, + autoupdate_enabled); service_->set_extensions_enabled(true); service_->set_show_extensions_prompts(false); profile->set_extensions_service(service_.get()); @@ -437,10 +443,20 @@ void ExtensionServiceTestBase::InitializeInstalledExtensionService( file_util::Delete(extensions_install_dir_, true); file_util::CopyDirectory(source_install_dir, extensions_install_dir_, true); - InitializeExtensionService(temp_prefs, extensions_install_dir_); + InitializeExtensionService(temp_prefs, extensions_install_dir_, false); } void ExtensionServiceTestBase::InitializeEmptyExtensionService() { + InitializeExtensionServiceHelper(false); +} + +void ExtensionServiceTestBase::InitializeExtensionServiceWithUpdater() { + InitializeExtensionServiceHelper(true); + service_->updater()->Start(); +} + +void ExtensionServiceTestBase::InitializeExtensionServiceHelper( + bool autoupdate_enabled) { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); FilePath path_ = temp_dir_.path(); path_ = path_.Append(FILE_PATH_LITERAL("TestingExtensionsPath")); @@ -452,7 +468,8 @@ void ExtensionServiceTestBase::InitializeEmptyExtensionService() { file_util::Delete(extensions_install_dir_, true); file_util::CreateDirectory(extensions_install_dir_); - InitializeExtensionService(prefs_filename, extensions_install_dir_); + InitializeExtensionService(prefs_filename, extensions_install_dir_, + autoupdate_enabled); } // static @@ -3256,7 +3273,8 @@ TEST(ExtensionServiceTestSimple, Enabledness) { // By default, we are enabled. command_line.reset(new CommandLine(CommandLine::NO_PROGRAM)); service = profile->CreateExtensionService(command_line.get(), - install_dir); + install_dir, + false); EXPECT_TRUE(service->extensions_enabled()); service->Init(); loop.RunAllPending(); @@ -3267,7 +3285,8 @@ TEST(ExtensionServiceTestSimple, Enabledness) { profile.reset(new TestingProfile()); command_line->AppendSwitch(switches::kDisableExtensions); service = profile->CreateExtensionService(command_line.get(), - install_dir); + install_dir, + false); EXPECT_FALSE(service->extensions_enabled()); service->Init(); loop.RunAllPending(); @@ -3277,7 +3296,8 @@ TEST(ExtensionServiceTestSimple, Enabledness) { profile.reset(new TestingProfile()); profile->GetPrefs()->SetBoolean(prefs::kDisableExtensions, true); service = profile->CreateExtensionService(command_line.get(), - install_dir); + install_dir, + false); EXPECT_FALSE(service->extensions_enabled()); service->Init(); loop.RunAllPending(); @@ -3288,7 +3308,8 @@ TEST(ExtensionServiceTestSimple, Enabledness) { profile->GetPrefs()->SetBoolean(prefs::kDisableExtensions, true); command_line.reset(new CommandLine(CommandLine::NO_PROGRAM)); service = profile->CreateExtensionService(command_line.get(), - install_dir); + install_dir, + false); EXPECT_FALSE(service->extensions_enabled()); service->Init(); loop.RunAllPending(); @@ -3400,6 +3421,138 @@ TEST_F(ExtensionServiceTest, ComponentExtensions) { EXPECT_EQ(extension_id, service_->extensions()->at(0)->id()); } +namespace { + +bool AlwaysInstall(const Extension& extension) { + return true; +} + +} // namespace + +TEST_F(ExtensionServiceTest, ProcessSyncDataUninstall) { + InitializeEmptyExtensionService(); + + ExtensionSyncData extension_sync_data; + extension_sync_data.id = good_crx; + extension_sync_data.uninstalled = true; + + // Should do nothing. + service_->ProcessSyncData(extension_sync_data, &AlwaysInstall); + + // Install the extension. + FilePath extension_path = data_dir_.AppendASCII("good.crx"); + InstallCrx(extension_path, true); + EXPECT_TRUE(service_->GetExtensionById(good_crx, true)); + + // Should uninstall the extension. + service_->ProcessSyncData(extension_sync_data, &AlwaysInstall); + EXPECT_FALSE(service_->GetExtensionById(good_crx, true)); + + // Should again do nothing. + service_->ProcessSyncData(extension_sync_data, &AlwaysInstall); +} + + +TEST_F(ExtensionServiceTest, ProcessSyncDataSettings) { + InitializeEmptyExtensionService(); + + FilePath extension_path = data_dir_.AppendASCII("good.crx"); + InstallCrx(extension_path, true); + EXPECT_TRUE(service_->IsExtensionEnabled(good_crx)); + EXPECT_FALSE(service_->IsIncognitoEnabled(good_crx)); + + ExtensionSyncData extension_sync_data; + extension_sync_data.id = good_crx; + extension_sync_data.version = + *(service_->GetExtensionById(good_crx, true)->version()); + + extension_sync_data.enabled = false; + service_->ProcessSyncData(extension_sync_data, &AlwaysInstall); + EXPECT_FALSE(service_->IsExtensionEnabled(good_crx)); + EXPECT_FALSE(service_->IsIncognitoEnabled(good_crx)); + + extension_sync_data.enabled = true; + extension_sync_data.incognito_enabled = true; + service_->ProcessSyncData(extension_sync_data, &AlwaysInstall); + EXPECT_TRUE(service_->IsExtensionEnabled(good_crx)); + EXPECT_TRUE(service_->IsIncognitoEnabled(good_crx)); + + extension_sync_data.enabled = false; + extension_sync_data.incognito_enabled = true; + service_->ProcessSyncData(extension_sync_data, &AlwaysInstall); + EXPECT_FALSE(service_->IsExtensionEnabled(good_crx)); + EXPECT_TRUE(service_->IsIncognitoEnabled(good_crx)); + + EXPECT_FALSE(service_->pending_extension_manager()->IsIdPending(good_crx)); +} + +TEST_F(ExtensionServiceTest, ProcessSyncDataVersionCheck) { + InitializeExtensionServiceWithUpdater(); + + // Install the extension. + FilePath extension_path = data_dir_.AppendASCII("good.crx"); + InstallCrx(extension_path, true); + EXPECT_TRUE(service_->IsExtensionEnabled(good_crx)); + EXPECT_FALSE(service_->IsIncognitoEnabled(good_crx)); + + ExtensionSyncData extension_sync_data; + extension_sync_data.id = good_crx; + extension_sync_data.enabled = true; + extension_sync_data.version = + *(service_->GetExtensionById(good_crx, true)->version()); + + // Should do nothing if extension version == sync version. + service_->ProcessSyncData(extension_sync_data, &AlwaysInstall); + EXPECT_FALSE(service_->updater()->WillCheckSoon()); + + // Should do nothing if extension version > sync version (but see + // the TODO in ProcessSyncData). + { + scoped_ptr<Version> version(Version::GetVersionFromString("0.0.0.0")); + extension_sync_data.version = *version; + service_->ProcessSyncData(extension_sync_data, &AlwaysInstall); + EXPECT_FALSE(service_->updater()->WillCheckSoon()); + } + + // Should kick off an update if extension version < sync version. + { + scoped_ptr<Version> version(Version::GetVersionFromString("9.9.9.9")); + extension_sync_data.version = *version; + service_->ProcessSyncData(extension_sync_data, &AlwaysInstall); + EXPECT_TRUE(service_->updater()->WillCheckSoon()); + } + + EXPECT_FALSE(service_->pending_extension_manager()->IsIdPending(good_crx)); +} + +TEST_F(ExtensionServiceTest, ProcessSyncDataNotInstalled) { + InitializeExtensionServiceWithUpdater(); + + ExtensionSyncData extension_sync_data; + extension_sync_data.id = good_crx; + extension_sync_data.update_url = GURL("http://www.google.com"); + extension_sync_data.enabled = true; + { + scoped_ptr<Version> version(Version::GetVersionFromString("1.2.3.4")); + extension_sync_data.version = *version; + } + + service_->ProcessSyncData(extension_sync_data, &AlwaysInstall); + EXPECT_TRUE(service_->updater()->WillCheckSoon()); + + PendingExtensionInfo info; + EXPECT_TRUE( + service_->pending_extension_manager()->GetById(good_crx, &info)); + EXPECT_EQ(extension_sync_data.update_url, info.update_url()); + EXPECT_TRUE(info.is_from_sync()); + EXPECT_TRUE(info.install_silently()); + EXPECT_EQ(extension_sync_data.enabled, info.enable_on_install()); + EXPECT_EQ(extension_sync_data.incognito_enabled, + info.enable_incognito_on_install()); + EXPECT_EQ(Extension::INTERNAL, info.install_source()); + // TODO(akalin): Figure out a way to test |info.ShouldAllowInstall()|. +} + // Test that when multiple sources try to install an extension, // we consistently choose the right one. To make tests easy to read, // methods that fake requests to install crx files in several ways diff --git a/chrome/browser/extensions/extension_service_unittest.h b/chrome/browser/extensions/extension_service_unittest.h index feba7bd..a3cf9d7 100644 --- a/chrome/browser/extensions/extension_service_unittest.h +++ b/chrome/browser/extensions/extension_service_unittest.h @@ -18,15 +18,18 @@ class ExtensionServiceTestBase : public testing::Test { public: ExtensionServiceTestBase(); - ~ExtensionServiceTestBase(); + virtual ~ExtensionServiceTestBase(); - virtual void InitializeExtensionService( - const FilePath& pref_file, const FilePath& extensions_install_dir); + void InitializeExtensionService( + const FilePath& pref_file, const FilePath& extensions_install_dir, + bool autoupdate_enabled); - virtual void InitializeInstalledExtensionService( + void InitializeInstalledExtensionService( const FilePath& prefs_file, const FilePath& source_install_dir); - virtual void InitializeEmptyExtensionService(); + void InitializeEmptyExtensionService(); + + void InitializeExtensionServiceWithUpdater(); static void SetUpTestCase(); @@ -37,6 +40,8 @@ class ExtensionServiceTestBase : public testing::Test { } protected: + void InitializeExtensionServiceHelper(bool autoupdate_enabled); + ScopedTempDir temp_dir_; scoped_ptr<Profile> profile_; FilePath extensions_install_dir_; diff --git a/chrome/browser/extensions/extension_sync_data.cc b/chrome/browser/extensions/extension_sync_data.cc new file mode 100644 index 0000000..0ef75b6 --- /dev/null +++ b/chrome/browser/extensions/extension_sync_data.cc @@ -0,0 +1,10 @@ +// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/extensions/extension_sync_data.h" + +ExtensionSyncData::ExtensionSyncData() + : uninstalled(false), enabled(false), incognito_enabled(false) {} + +ExtensionSyncData::~ExtensionSyncData() {} diff --git a/chrome/browser/extensions/extension_sync_data.h b/chrome/browser/extensions/extension_sync_data.h new file mode 100644 index 0000000..3a29d22 --- /dev/null +++ b/chrome/browser/extensions/extension_sync_data.h @@ -0,0 +1,34 @@ +// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_SYNC_DATA_H_ +#define CHROME_BROWSER_EXTENSIONS_EXTENSION_SYNC_DATA_H_ +#pragma once + +#include <string> + +#include "base/version.h" +#include "googleurl/src/gurl.h" + +// A struct that encapsulates the synced properties of an Extension. +struct ExtensionSyncData { + ExtensionSyncData(); + ~ExtensionSyncData(); + + std::string id; + + // Version-independent properties (i.e., used even when the + // version of the currently-installed extension doesn't match + // |version|). + bool uninstalled; + bool enabled; + bool incognito_enabled; + + // Version-dependent properties (i.e., should be used only when the + // version of the currenty-installed extension matches |version|). + Version version; + GURL update_url; +}; + +#endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_SYNC_DATA_H_ diff --git a/chrome/browser/extensions/extension_updater_unittest.cc b/chrome/browser/extensions/extension_updater_unittest.cc index 3479d11..277329c 100644 --- a/chrome/browser/extensions/extension_updater_unittest.cc +++ b/chrome/browser/extensions/extension_updater_unittest.cc @@ -132,6 +132,13 @@ class MockService : public ExtensionServiceInterface { return &pending_extension_manager_; } + virtual void ProcessSyncData( + const ExtensionSyncData& extension_sync_data, + PendingExtensionInfo::ShouldAllowInstallPredicate + should_allow_install) { + FAIL(); + } + Profile* profile() { return &profile_; } ExtensionPrefs* extension_prefs() { return prefs_.prefs(); } diff --git a/chrome/browser/extensions/mock_extension_service.h b/chrome/browser/extensions/mock_extension_service.h index 7a6c4b5..68325a5 100644 --- a/chrome/browser/extensions/mock_extension_service.h +++ b/chrome/browser/extensions/mock_extension_service.h @@ -10,6 +10,8 @@ #include <vector> #include "chrome/browser/extensions/extension_service.h" +// Needed to keep gmock happy. +#include "chrome/browser/extensions/extension_sync_data.h" #include "testing/gmock/include/gmock/gmock.h" class MockExtensionService : public ExtensionServiceInterface { @@ -38,6 +40,10 @@ class MockExtensionService : public ExtensionServiceInterface { MOCK_CONST_METHOD1(IsIncognitoEnabled, bool(const std::string&)); MOCK_METHOD2(SetIsIncognitoEnabled, void(const std::string&, bool)); MOCK_METHOD0(CheckForUpdatesSoon, void()); + MOCK_METHOD2(ProcessSyncData, + void(const ExtensionSyncData&, + PendingExtensionInfo::ShouldAllowInstallPredicate + should_allow_install)); }; #endif // CHROME_BROWSER_EXTENSIONS_MOCK_EXTENSION_SERVICE_H_ diff --git a/chrome/browser/sync/glue/extension_change_processor.cc b/chrome/browser/sync/glue/extension_change_processor.cc index 8a1bf72..dd0da34 100644 --- a/chrome/browser/sync/glue/extension_change_processor.cc +++ b/chrome/browser/sync/glue/extension_change_processor.cc @@ -10,6 +10,7 @@ #include "base/logging.h" #include "base/stl_util-inl.h" #include "chrome/browser/extensions/extension_service.h" +#include "chrome/browser/extensions/extension_sync_data.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/sync/glue/extension_sync.h" #include "chrome/browser/sync/glue/extension_util.h" @@ -101,6 +102,7 @@ void ExtensionChangeProcessor::ApplyChangesFromSyncModel( } for (int i = 0; i < change_count; ++i) { const sync_api::SyncManager::ChangeRecord& change = changes[i]; + sync_pb::ExtensionSpecifics specifics; switch (change.action) { case sync_api::SyncManager::ChangeRecord::ACTION_ADD: case sync_api::SyncManager::ChangeRecord::ACTION_UPDATE: { @@ -113,30 +115,14 @@ void ExtensionChangeProcessor::ApplyChangesFromSyncModel( return; } DCHECK_EQ(node.GetModelType(), traits_.model_type); - const sync_pb::ExtensionSpecifics& specifics = - (*traits_.extension_specifics_getter)(node); - if (!IsExtensionSpecificsValid(specifics)) { - std::string error = - std::string("Invalid server specifics: ") + - ExtensionSpecificsToString(specifics); - error_handler()->OnUnrecoverableError(FROM_HERE, error); - return; - } - StopObserving(); - UpdateClient(traits_, specifics, extension_service_); - StartObserving(); + specifics = (*traits_.extension_specifics_getter)(node); break; } case sync_api::SyncManager::ChangeRecord::ACTION_DELETE: { - sync_pb::ExtensionSpecifics specifics; - if ((*traits_.extension_specifics_entity_getter)( + if (!(*traits_.extension_specifics_entity_getter)( change.specifics, &specifics)) { - StopObserving(); - RemoveFromClient(traits_, specifics.id(), extension_service_); - StartObserving(); - } else { std::stringstream error; - error << "Could not get extension ID for deleted node " + error << "Could not get extension specifics from deleted node " << change.id; error_handler()->OnUnrecoverableError(FROM_HERE, error.str()); LOG(DFATAL) << error.str(); @@ -144,6 +130,21 @@ void ExtensionChangeProcessor::ApplyChangesFromSyncModel( break; } } + ExtensionSyncData sync_data; + if (!GetExtensionSyncData(specifics, &sync_data)) { + // TODO(akalin): Should probably recover or drop. + std::string error = + std::string("Invalid server specifics: ") + + ExtensionSpecificsToString(specifics); + error_handler()->OnUnrecoverableError(FROM_HERE, error); + return; + } + sync_data.uninstalled = + (change.action == sync_api::SyncManager::ChangeRecord::ACTION_DELETE); + StopObserving(); + extension_service_->ProcessSyncData(sync_data, + traits_.is_valid_and_syncable); + StartObserving(); } } diff --git a/chrome/browser/sync/glue/extension_sync.cc b/chrome/browser/sync/glue/extension_sync.cc index b6693ad..1aedc4a 100644 --- a/chrome/browser/sync/glue/extension_sync.cc +++ b/chrome/browser/sync/glue/extension_sync.cc @@ -9,6 +9,7 @@ #include "base/logging.h" #include "chrome/browser/extensions/extension_updater.h" #include "chrome/browser/extensions/extension_service.h" +#include "chrome/browser/extensions/extension_sync_data.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/sync/engine/syncapi.h" #include "chrome/browser/sync/glue/extension_data.h" @@ -234,75 +235,6 @@ bool UpdateServer( return true; } -// Tries to update the client data from the given extension data. -// extension_data->ServerNeedsUpdate() must not hold and -// extension_data->ClientNeedsUpdate() must hold before this function -// is called. If the update was successful, -// extension_data->ClientNeedsUpdate() will be false after this -// function is called. Otherwise, the extension needs updating to a -// new version. -void TryUpdateClient( - IsValidAndSyncablePredicate is_valid_and_syncable, - ExtensionServiceInterface* extensions_service, - ExtensionData* extension_data) { - DCHECK(!extension_data->NeedsUpdate(ExtensionData::SERVER)); - DCHECK(extension_data->NeedsUpdate(ExtensionData::CLIENT)); - const sync_pb::ExtensionSpecifics& specifics = - extension_data->merged_data(); - DcheckIsExtensionSpecificsValid(specifics); - const std::string& id = specifics.id(); - const Extension* extension = extensions_service->GetExtensionById(id, true); - if (extension) { - if (!is_valid_and_syncable(*extension)) { - LOG(DFATAL) << "TryUpdateClient() called for non-syncable extension " - << id; - return; - } - if (specifics.name() != extension->name()) { - LOG(WARNING) << "specifics for extension " << id - << "has a different name than the extension: " - << specifics.name() << " vs. " << extension->name(); - } - GURL update_url(specifics.update_url()); - if (update_url != extension->update_url()) { - LOG(WARNING) << "specifics for extension " << id - << "has a different update URL than the extension: " - << update_url.spec() << " vs. " << extension->update_url(); - } - if (specifics.enabled()) { - extensions_service->EnableExtension(id); - } else { - extensions_service->DisableExtension(id); - } - extensions_service->SetIsIncognitoEnabled(id, - specifics.incognito_enabled()); - { - sync_pb::ExtensionSpecifics extension_specifics; - GetExtensionSpecifics(*extension, *extensions_service, - &extension_specifics); - DCHECK(AreExtensionSpecificsUserPropertiesEqual( - specifics, extension_specifics)) - << ExtensionSpecificsToString(specifics) << ", " - << ExtensionSpecificsToString(extension_specifics); - } - if (!IsExtensionOutdated(*extension, specifics)) { - extension_data->ResolveData(ExtensionData::CLIENT); - DCHECK(!extension_data->NeedsUpdate(ExtensionData::CLIENT)); - } - } else { - GURL update_url(specifics.update_url()); - // TODO(akalin): Replace silent update with a list of enabled - // permissions. - extensions_service->pending_extension_manager()->AddFromSync( - id, update_url, - is_valid_and_syncable, - true, // install_silently - specifics.enabled(), - specifics.incognito_enabled()); - } - DCHECK(!extension_data->NeedsUpdate(ExtensionData::SERVER)); -} - } // namespace bool FlushExtensionData(const ExtensionSyncTraits& traits, @@ -329,14 +261,14 @@ bool FlushExtensionData(const ExtensionSyncTraits& traits, } } DCHECK(!extension_data.NeedsUpdate(ExtensionData::SERVER)); - if (extension_data.NeedsUpdate(ExtensionData::CLIENT)) { - TryUpdateClient(traits.is_valid_and_syncable, - extensions_service, &extension_data); - if (extension_data.NeedsUpdate(ExtensionData::CLIENT)) { - extensions_service->CheckForUpdatesSoon(); - } + ExtensionSyncData sync_data; + if (!GetExtensionSyncData(extension_data.merged_data(), &sync_data)) { + // TODO(akalin): Should probably recover or drop. + NOTREACHED(); + return false; } - DCHECK(!extension_data.NeedsUpdate(ExtensionData::SERVER)); + extensions_service->ProcessSyncData(sync_data, + traits.is_valid_and_syncable); } return true; } @@ -410,53 +342,4 @@ void RemoveServerData(const ExtensionSyncTraits& traits, } } -void UpdateClient(const ExtensionSyncTraits& traits, - const sync_pb::ExtensionSpecifics& server_data, - ExtensionServiceInterface* extensions_service) { - DcheckIsExtensionSpecificsValid(server_data); - ExtensionData extension_data = - ExtensionData::FromData(ExtensionData::SERVER, server_data); - const Extension* extension = - extensions_service->GetExtensionById(server_data.id(), true); - if (extension) { - if (!traits.is_valid_and_syncable(*extension)) { - LOG(WARNING) << "Ignoring server data for invalid or " - << "non-syncable extension " << extension->id(); - return; - } - sync_pb::ExtensionSpecifics client_data; - GetExtensionSpecifics(*extension, *extensions_service, - &client_data); - DcheckIsExtensionSpecificsValid(client_data); - extension_data = - ExtensionData::FromData(ExtensionData::CLIENT, client_data); - extension_data.SetData(ExtensionData::SERVER, true, server_data); - } - DCHECK(!extension_data.NeedsUpdate(ExtensionData::SERVER)); - if (extension_data.NeedsUpdate(ExtensionData::CLIENT)) { - TryUpdateClient(traits.is_valid_and_syncable, - extensions_service, &extension_data); - if (extension_data.NeedsUpdate(ExtensionData::CLIENT)) { - extensions_service->CheckForUpdatesSoon(); - } - } - DCHECK(!extension_data.NeedsUpdate(ExtensionData::SERVER)); -} - -void RemoveFromClient(const ExtensionSyncTraits& traits, - const std::string& id, - ExtensionServiceInterface* extensions_service) { - const Extension* extension = extensions_service->GetExtensionById(id, true); - if (extension) { - if (traits.is_valid_and_syncable(*extension)) { - extensions_service->UninstallExtension(id, false, NULL); - } else { - LOG(WARNING) << "Ignoring server data for invalid or " - << "non-syncable extension " << extension->id(); - } - } else { - LOG(ERROR) << "Trying to uninstall nonexistent extension " << id; - } -} - } // namespace browser_sync diff --git a/chrome/browser/sync/glue/extension_sync.h b/chrome/browser/sync/glue/extension_sync.h index 494c02d..e1f3f15 100644 --- a/chrome/browser/sync/glue/extension_sync.h +++ b/chrome/browser/sync/glue/extension_sync.h @@ -76,16 +76,6 @@ void RemoveServerData(const ExtensionSyncTraits& traits, const std::string& id, sync_api::UserShare* user_share); -// Starts updating the client data from the given server data. -void UpdateClient(const ExtensionSyncTraits& traits, - const sync_pb::ExtensionSpecifics& server_data, - ExtensionServiceInterface* extensions_service); - -// Removes existing client data for the given extension. -void RemoveFromClient(const ExtensionSyncTraits& traits, - const std::string& id, - ExtensionServiceInterface* extensions_service); - } // namespace browser_sync #endif // CHROME_BROWSER_SYNC_GLUE_EXTENSION_SYNC_H_ diff --git a/chrome/browser/sync/glue/extension_util.cc b/chrome/browser/sync/glue/extension_util.cc index 4d24a43..5578f89 100644 --- a/chrome/browser/sync/glue/extension_util.cc +++ b/chrome/browser/sync/glue/extension_util.cc @@ -12,6 +12,7 @@ #include "base/version.h" #include "chrome/browser/extensions/extension_prefs.h" #include "chrome/browser/extensions/extension_service.h" +#include "chrome/browser/extensions/extension_sync_data.h" #include "chrome/browser/sync/protocol/extension_specifics.pb.h" #include "chrome/common/extensions/extension.h" #include "chrome/common/extensions/extension_constants.h" @@ -157,19 +158,6 @@ void GetExtensionSpecifics(const Extension& extension, DcheckIsExtensionSpecificsValid(*specifics); } -bool IsExtensionOutdated(const Extension& extension, - const sync_pb::ExtensionSpecifics& specifics) { - DCHECK(IsExtensionValid(extension)); - DcheckIsExtensionSpecificsValid(specifics); - scoped_ptr<Version> specifics_version( - Version::GetVersionFromString(specifics.version())); - if (!specifics_version.get()) { - // If version is invalid, assume we're up-to-date. - return false; - } - return extension.version()->CompareTo(*specifics_version) < 0; -} - void MergeExtensionSpecifics( const sync_pb::ExtensionSpecifics& specifics, bool merge_user_properties, @@ -194,4 +182,31 @@ void MergeExtensionSpecifics( } } +bool GetExtensionSyncData( + const sync_pb::ExtensionSpecifics& specifics, + ExtensionSyncData* sync_data) { + if (!Extension::IdIsValid(specifics.id())) { + return false; + } + + scoped_ptr<Version> version( + Version::GetVersionFromString(specifics.version())); + if (!version.get()) { + return false; + } + + // The update URL must be either empty or valid. + GURL update_url(specifics.update_url()); + if (!update_url.is_empty() && !update_url.is_valid()) { + return false; + } + + sync_data->id = specifics.id(); + sync_data->update_url = update_url; + sync_data->version = *version; + sync_data->enabled = specifics.enabled(); + sync_data->incognito_enabled = specifics.incognito_enabled(); + return true; +} + } // namespace browser_sync diff --git a/chrome/browser/sync/glue/extension_util.h b/chrome/browser/sync/glue/extension_util.h index 6433dfd..d914a75 100644 --- a/chrome/browser/sync/glue/extension_util.h +++ b/chrome/browser/sync/glue/extension_util.h @@ -14,6 +14,7 @@ class Extension; class ExtensionPrefs; class ExtensionServiceInterface; +struct ExtensionSyncData; struct UninstalledExtensionInfo; namespace sync_pb { @@ -84,12 +85,6 @@ void GetExtensionSpecifics(const Extension& extension, const ExtensionServiceInterface& extension_service, sync_pb::ExtensionSpecifics* specifics); -// Returns whether or not the extension should be updated according to -// the specifics. |extension| must be syncable and |specifics| must -// be valid. -bool IsExtensionOutdated(const Extension& extension, - const sync_pb::ExtensionSpecifics& specifics); - // Merge |specifics| into |merged_specifics|. Both must be valid and // have the same ID. The merge policy is currently to copy the // non-user properties of |specifics| into |merged_specifics| (and the @@ -100,6 +95,12 @@ void MergeExtensionSpecifics( bool merge_user_properties, sync_pb::ExtensionSpecifics* merged_specifics); +// Fills |sync_data| with the data from |specifics|. Returns true iff +// succesful. +bool GetExtensionSyncData( + const sync_pb::ExtensionSpecifics& specifics, + ExtensionSyncData* sync_data); + } // namespace browser_sync #endif // CHROME_BROWSER_SYNC_GLUE_EXTENSION_UTIL_H_ diff --git a/chrome/browser/sync/glue/extension_util_unittest.cc b/chrome/browser/sync/glue/extension_util_unittest.cc index 4df21b9..35a4065 100644 --- a/chrome/browser/sync/glue/extension_util_unittest.cc +++ b/chrome/browser/sync/glue/extension_util_unittest.cc @@ -420,22 +420,6 @@ TEST_F(ExtensionUtilTest, GetExtensionSpecifics) { EXPECT_EQ(kName, specifics.name()); } -TEST_F(ExtensionUtilTest, IsExtensionOutdated) { - FilePath file_path(kExtensionFilePath); - scoped_refptr<Extension> extension( - MakeSyncableExtension(kVersion2, kValidUpdateUrl1, kName, file_path)); - sync_pb::ExtensionSpecifics specifics; - specifics.set_id(kValidId); - specifics.set_update_url(kValidUpdateUrl1); - - specifics.set_version(kVersion1); - EXPECT_FALSE(IsExtensionOutdated(*extension, specifics)); - specifics.set_version(kVersion2); - EXPECT_FALSE(IsExtensionOutdated(*extension, specifics)); - specifics.set_version(kVersion3); - EXPECT_TRUE(IsExtensionOutdated(*extension, specifics)); -} - // TODO(akalin): Make ExtensionService/ExtensionUpdater testable // enough to be able to write a unittest for SetExtensionProperties(). diff --git a/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm b/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm index 7636cef..50c0b12 100644 --- a/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm +++ b/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm @@ -271,7 +271,7 @@ class BookmarkBarControllerTestBase : public CocoaTest { BookmarkBarControllerTestBase() { FilePath extension_dir; helper_.profile()->CreateExtensionService(CommandLine::ForCurrentProcess(), - extension_dir); + extension_dir, false); resizeDelegate_.reset([[ViewResizerPong alloc] init]); NSRect parent_frame = NSMakeRect(0, 0, 800, 50); parent_view_.reset([[NSView alloc] initWithFrame:parent_frame]); |