Skip to content

Commit

Permalink
Reland of move deprecated extension notification from background appl…
Browse files Browse the repository at this point in the history 
…ication list model (patchset chromium#1 id:1 of https://codereview.chromium.org/2790303002/ )

Reason for revert:
Original CL is not a true culprit for crbug.com/707735.

Original issue's description:
> Revert of Remove deprecated extension notification from background application list model (patchset chromium#7 id:450001 of https://codereview.chromium.org/2724343002/ )
>
> Reason for revert:
> Suspected cause of ReleaseBlock-Dev top crasher: https://bugs.chromium.org/p/chromium/issues/detail?id=707735#c5
>
> Original issue's description:
> > Remove deprecated extension notification from background application list model
> >
> > Currently we have ExtensionRegistry[Observer] that substitutes old and deprecated
> > extension notifications. Since BackgroundApplicationListModel still uses them,
> > use ExtensionRegistryObserver instead.
> >
> > BUG=411568, 411566
> > TEST=BackgroundApplicationListModelTest.ExtensionLoadAndUnload
> >
> > Review-Url: https://codereview.chromium.org/2724343002
> > Cr-Commit-Position: refs/heads/master@{#461077}
> > Committed: https://chromium.googlesource.com/chromium/src/+/1a96e39ce2f4cd1956cba47c4e6413de95d11039
>
> TBR=rdevlin.cronin@chromium.org,atwilson@chromium.org,limasdf@gmail.com
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=411568, 411566
> NOTRY=true
>
> Review-Url: https://codereview.chromium.org/2790303002
> Cr-Commit-Position: refs/heads/master@{#461596}
> Committed: https://chromium.googlesource.com/chromium/src/+/cef092a92ecbef2465ccf990bfc16db49fbe24fb

TBR=rdevlin.cronin@chromium.org,atwilson@chromium.org,caseq@chromium.org,japhet@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=411568, 411566

Review-Url: https://codereview.chromium.org/2803713002
Cr-Commit-Position: refs/heads/master@{#462455}
  • Loading branch information
@sunglim
sunglim authored and Commit bot committed Apr 6, 2017
1 parent 17fc628 commit 8ab328b
Show file tree
Hide file tree
Showing 4 changed files with 174 additions and 78 deletions.
68 changes: 36 additions & 32 deletions chrome/browser/background/background_application_list_model.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include "extensions/common/extension_set.h"
#include "extensions/common/manifest_handlers/background_info.h"
#include "extensions/common/manifest_handlers/icons_handler.h"
#include "extensions/common/one_shot_event.h"
#include "extensions/common/permissions/permission_set.h"
#include "extensions/common/permissions/permissions_data.h"
#include "ui/base/l10n/l10n_util_collator.h"
Expand Down Expand Up @@ -151,29 +152,20 @@ BackgroundApplicationListModel::~BackgroundApplicationListModel() {

BackgroundApplicationListModel::BackgroundApplicationListModel(Profile* profile)
: profile_(profile),
ready_(false) {
ready_(false),
extension_registry_observer_(this),
weak_ptr_factory_(this) {
DCHECK(profile_);
registrar_.Add(this,
extensions::NOTIFICATION_EXTENSION_LOADED_DEPRECATED,
content::Source<Profile>(profile));
registrar_.Add(this,
extensions::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED,
content::Source<Profile>(profile));
registrar_.Add(this,
extensions::NOTIFICATION_EXTENSIONS_READY_DEPRECATED,
content::Source<Profile>(profile));
registrar_.Add(this,
extensions::NOTIFICATION_EXTENSION_PERMISSIONS_UPDATED,
content::Source<Profile>(profile));
registrar_.Add(this,
chrome::NOTIFICATION_BACKGROUND_CONTENTS_SERVICE_CHANGED,
content::Source<Profile>(profile));
ExtensionService* service = extensions::ExtensionSystem::Get(profile)->
extension_service();
if (service && service->is_ready()) {
Update();
ready_ = true;
}
extensions::ExtensionSystem::Get(profile_)->ready().Post(
FROM_HERE,
base::Bind(&BackgroundApplicationListModel::OnExtensionSystemReady,
weak_ptr_factory_.GetWeakPtr()));
}

void BackgroundApplicationListModel::AddObserver(Observer* observer) {
Expand All @@ -196,7 +188,6 @@ void BackgroundApplicationListModel::AssociateApplicationData(
base::MakeUnique<Application>(this, extension);
application = application_ptr.get();
applications_[extension->id()] = std::move(application_ptr);
Update();
application->RequestIcon(extension_misc::EXTENSION_ICON_BITTY);
}
}
Expand Down Expand Up @@ -295,24 +286,12 @@ void BackgroundApplicationListModel::Observe(
int type,
const content::NotificationSource& source,
const content::NotificationDetails& details) {
if (type == extensions::NOTIFICATION_EXTENSIONS_READY_DEPRECATED) {
Update();
ready_ = true;
return;
}
ExtensionService* service = extensions::ExtensionSystem::Get(profile_)->
extension_service();
if (!service || !service->is_ready())
return;

switch (type) {
case extensions::NOTIFICATION_EXTENSION_LOADED_DEPRECATED:
OnExtensionLoaded(content::Details<Extension>(details).ptr());
break;
case extensions::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED:
OnExtensionUnloaded(
content::Details<UnloadedExtensionInfo>(details)->extension);
break;
case extensions::NOTIFICATION_EXTENSION_PERMISSIONS_UPDATED:
OnExtensionPermissionsUpdated(
content::Details<UpdatedExtensionPermissionsInfo>(details)->extension,
Expand All @@ -335,21 +314,45 @@ void BackgroundApplicationListModel::SendApplicationDataChangedNotifications(
}

void BackgroundApplicationListModel::OnExtensionLoaded(
content::BrowserContext* browser_context,
const Extension* extension) {
// We only care about extensions that are background applications
// We only care about extensions that are background applications.
if (!IsBackgroundApp(*extension, profile_))
return;
Update();
AssociateApplicationData(extension);
}

void BackgroundApplicationListModel::OnExtensionUnloaded(
const Extension* extension) {
content::BrowserContext* browser_context,
const Extension* extension,
UnloadedExtensionInfo::Reason reason) {
if (!IsBackgroundApp(*extension, profile_))
return;
Update();
DissociateApplicationData(extension);
}

void BackgroundApplicationListModel::OnExtensionSystemReady() {
// All initial extensions will be loaded when extension system ready. So we
// can get everything here.
Update();
for (const auto& extension : extensions_)
AssociateApplicationData(extension.get());

// If we register for extension loaded notifications in the ctor, we need to
// know that this object is constructed prior to the initialization process
// for the extension system, which isn't a guarantee. Thus, register here and
// associate all initial extensions.
extension_registry_observer_.Add(ExtensionRegistry::Get(profile_));
ready_ = true;
}

void BackgroundApplicationListModel::OnShutdown(ExtensionRegistry* registry) {
DCHECK_EQ(ExtensionRegistry::Get(profile_), registry);
extension_registry_observer_.Remove(registry);
}

void BackgroundApplicationListModel::OnExtensionPermissionsUpdated(
const Extension* extension,
UpdatedExtensionPermissionsInfo::Reason reason,
Expand All @@ -358,7 +361,8 @@ void BackgroundApplicationListModel::OnExtensionPermissionsUpdated(
switch (reason) {
case UpdatedExtensionPermissionsInfo::ADDED:
DCHECK(IsBackgroundApp(*extension, profile_));
OnExtensionLoaded(extension);
Update();
AssociateApplicationData(extension);
break;
case UpdatedExtensionPermissionsInfo::REMOVED:
DCHECK(!IsBackgroundApp(*extension, profile_));
Expand Down
36 changes: 29 additions & 7 deletions chrome/browser/background/background_application_list_model.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,12 @@
#include <string>

#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "base/observer_list.h"
#include "base/scoped_observer.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "extensions/browser/extension_registry_observer.h"
#include "extensions/common/extension.h"

class Profile;
Expand All @@ -22,10 +25,16 @@ namespace gfx {
class ImageSkia;
}

namespace extensions {
class ExtensionRegistry;
}

// Model for list of Background Applications associated with a Profile (i.e.
// extensions with kBackgroundPermission set, or hosted apps with a
// BackgroundContents).
class BackgroundApplicationListModel : public content::NotificationObserver {
class BackgroundApplicationListModel
: public content::NotificationObserver,
public extensions::ExtensionRegistryObserver {
public:
// Observer is informed of changes to the model. Users of the
// BackgroundApplicationListModel should anticipate that associated data,
Expand Down Expand Up @@ -119,6 +128,18 @@ class BackgroundApplicationListModel : public content::NotificationObserver {
const content::NotificationSource& source,
const content::NotificationDetails& details) override;

// extensions::ExtensionRegistryObserver implementation.
void OnExtensionLoaded(content::BrowserContext* browser_context,
const extensions::Extension* extension) override;
void OnExtensionUnloaded(
content::BrowserContext* browser_context,
const extensions::Extension* extension,
extensions::UnloadedExtensionInfo::Reason reason) override;
void OnShutdown(extensions::ExtensionRegistry* registry) override;

// Intended to be called when extension system is ready.
void OnExtensionSystemReady();

// Notifies observers that some of the data associated with this background
// application, e. g. the Icon, has changed.
void SendApplicationDataChangedNotifications(
Expand All @@ -128,12 +149,6 @@ class BackgroundApplicationListModel : public content::NotificationObserver {
// or removed.
void SendApplicationListChangedNotifications();

// Invoked by Observe for NOTIFICATION_EXTENSION_LOADED_DEPRECATED.
void OnExtensionLoaded(const extensions::Extension* extension);

// Invoked by Observe for NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED.
void OnExtensionUnloaded(const extensions::Extension* extension);

// Invoked by Observe for NOTIFICATION_EXTENSION_PERMISSIONS_UPDATED.
void OnExtensionPermissionsUpdated(
const extensions::Extension* extension,
Expand All @@ -152,6 +167,13 @@ class BackgroundApplicationListModel : public content::NotificationObserver {
content::NotificationRegistrar registrar_;
bool ready_;

// Listens to extension load, unload notifications.
ScopedObserver<extensions::ExtensionRegistry,
extensions::ExtensionRegistryObserver>
extension_registry_observer_;

base::WeakPtrFactory<BackgroundApplicationListModel> weak_ptr_factory_;

DISALLOW_COPY_AND_ASSIGN(BackgroundApplicationListModel);
};

Expand Down
Loading

0 comments on commit 8ab328b

Please sign in to comment.