Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(74)

Unified Diff: chrome/browser/themes/theme_service.cc

Issue 2893693002: Remove NOTIFICATION_EXTENSION_ENABLED. (Closed)
Patch Set: address comments + fix theme update path for skipping already set theme Created 3 years, 7 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: chrome/browser/themes/theme_service.cc
diff --git a/chrome/browser/themes/theme_service.cc b/chrome/browser/themes/theme_service.cc
index 6a9d0e1e40a102972fba8dd3e20467802ff9fe0a..d511a4cd9102ddd4ea4f8e48376dc2e794ff03c8 100644
--- a/chrome/browser/themes/theme_service.cc
+++ b/chrome/browser/themes/theme_service.cc
@@ -49,6 +49,7 @@
#include "ui/native_theme/native_theme.h"
#if BUILDFLAG(ENABLE_EXTENSIONS)
+#include "base/scoped_observer.h"
#include "extensions/browser/extension_registry_observer.h"
#endif
@@ -160,35 +161,46 @@ base::RefCountedMemory* ThemeService::BrowserThemeProvider::GetRawData(
class ThemeService::ThemeObserver
: public extensions::ExtensionRegistryObserver {
public:
- explicit ThemeObserver(ThemeService* service) : theme_service_(service) {
- extensions::ExtensionRegistry::Get(theme_service_->profile_)
- ->AddObserver(this);
+ explicit ThemeObserver(ThemeService* service)
+ : theme_service_(service), extension_registry_observer_(this) {
+ extension_registry_observer_.Add(
+ extensions::ExtensionRegistry::Get(theme_service_->profile_));
}
~ThemeObserver() override {
- extensions::ExtensionRegistry::Get(theme_service_->profile_)
- ->RemoveObserver(this);
}
private:
+ // extensions::ExtensionRegistryObserver:
void OnExtensionWillBeInstalled(content::BrowserContext* browser_context,
const extensions::Extension* extension,
bool is_update,
const std::string& old_name) override {
if (extension->is_theme()) {
- // The theme may be initially disabled. Wait till it is loaded (if ever).
+ // Remember ID of the newly installed theme.
theme_service_->installed_pending_load_id_ = extension->id();
}
}
void OnExtensionLoaded(content::BrowserContext* browser_context,
const extensions::Extension* extension) override {
- if (extension->is_theme() &&
+ if (!extension->is_theme())
+ return;
+
+ // If this theme is a new version, we will use it.
+ bool is_new_version =
theme_service_->installed_pending_load_id_ != kDefaultThemeID &&
- theme_service_->installed_pending_load_id_ == extension->id()) {
- theme_service_->SetTheme(extension);
- }
+ theme_service_->installed_pending_load_id_ == extension->id();
theme_service_->installed_pending_load_id_ = kDefaultThemeID;
+
+ // Do not load existing theme.
pkotwicz 2017/05/31 21:54:06 Perhaps clarify in the comment that SetTheme() sho
lazyboy 2017/05/31 23:31:56 I've removed the comment on line 190, added commen
+ if (!is_new_version && extension->id() == theme_service_->GetThemeID())
+ return;
+
+ // Set the new theme during extension load.
+ // Even if the theme was an update of a disabled extension, it shouldn't be
+ // loaded.
+ theme_service_->SetTheme(extension);
}
void OnExtensionUnloaded(
@@ -204,6 +216,12 @@ class ThemeService::ThemeObserver
}
ThemeService* theme_service_;
+
+ ScopedObserver<extensions::ExtensionRegistry,
+ extensions::ExtensionRegistryObserver>
+ extension_registry_observer_;
+
+ DISALLOW_COPY_AND_ASSIGN(ThemeObserver);
};
#endif // BUILDFLAG(ENABLE_EXTENSIONS)
@@ -259,12 +277,6 @@ void ThemeService::Observe(int type,
content::Source<Profile>(profile_));
OnExtensionServiceReady();
break;
- case extensions::NOTIFICATION_EXTENSION_ENABLED: {
- const Extension* extension = Details<const Extension>(details).ptr();
- if (extension->is_theme())
- SetTheme(extension);
- break;
- }
default:
NOTREACHED();
}
@@ -792,13 +804,9 @@ void ThemeService::OnExtensionServiceReady() {
}
#if BUILDFLAG(ENABLE_EXTENSIONS)
- theme_observer_.reset(new ThemeObserver(this));
+ theme_observer_ = base::MakeUnique<ThemeObserver>(this);
#endif
- registrar_.Add(this,
- extensions::NOTIFICATION_EXTENSION_ENABLED,
- content::Source<Profile>(profile_));
-
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
FROM_HERE, base::Bind(&ThemeService::RemoveUnusedThemes,
weak_ptr_factory_.GetWeakPtr(), false),

Powered by Google App Engine
This is Rietveld 408576698