Skip to content

Commit

Permalink
Stop notifying users for BG app upgrades.
Browse files Browse the repository at this point in the history 
The extension subsystem doesn't reliably differentiate between app installs
and upgrades in all cases, so now BackgroundModeManager tracks which apps it
has seen and only notifies the user when it sees a new app.

BUG=361637
TBR=benwells

Review URL: https://codereview.chromium.org/368813005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@283071 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
atwilson@chromium.org committed Jul 14, 2014
1 parent 62cf1d7 commit bb0ef38
Show file tree
Hide file tree
Showing 7 changed files with 396 additions and 339 deletions.
8 changes: 6 additions & 2 deletions chrome/browser/background/background_application_list_model.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,8 @@ BackgroundApplicationListModel::~BackgroundApplicationListModel() {
}

BackgroundApplicationListModel::BackgroundApplicationListModel(Profile* profile)
: profile_(profile) {
: profile_(profile),
ready_(false) {
DCHECK(profile_);
registrar_.Add(this,
chrome::NOTIFICATION_EXTENSION_LOADED_DEPRECATED,
Expand All @@ -193,8 +194,10 @@ BackgroundApplicationListModel::BackgroundApplicationListModel(Profile* profile)
content::Source<Profile>(profile));
ExtensionService* service = extensions::ExtensionSystem::Get(profile)->
extension_service();
if (service && service->is_ready())
if (service && service->is_ready()) {
Update();
ready_ = true;
}
}

void BackgroundApplicationListModel::AddObserver(Observer* observer) {
Expand Down Expand Up @@ -346,6 +349,7 @@ void BackgroundApplicationListModel::Observe(
const content::NotificationDetails& details) {
if (type == chrome::NOTIFICATION_EXTENSIONS_READY) {
Update();
ready_ = true;
return;
}
ExtensionService* service = extensions::ExtensionSystem::Get(profile_)->
Expand Down
6 changes: 6 additions & 0 deletions chrome/browser/background/background_application_list_model.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ class BackgroundApplicationListModel : public content::NotificationObserver {
return extensions_.size();
}

// Returns true if all startup notifications have already been issued.
bool is_ready() const {
return ready_;
}

private:
// Contains data associated with a background application that is not
// represented by the Extension class.
Expand Down Expand Up @@ -151,6 +156,7 @@ class BackgroundApplicationListModel : public content::NotificationObserver {
ObserverList<Observer, true> observers_;
Profile* profile_;
content::NotificationRegistrar registrar_;
bool ready_;

DISALLOW_COPY_AND_ASSIGN(BackgroundApplicationListModel);
};
Expand Down
92 changes: 32 additions & 60 deletions chrome/browser/background/background_mode_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,29 @@ base::string16 BackgroundModeManager::BackgroundModeData::name() {
return name_;
}

std::set<const extensions::Extension*>
BackgroundModeManager::BackgroundModeData::GetNewBackgroundApps() {
std::set<const extensions::Extension*> new_apps;

// Copy all current extensions into our list of |current_extensions_|.
for (extensions::ExtensionList::const_iterator it = applications_->begin();
it != applications_->end(); ++it) {
const extensions::ExtensionId& id = (*it)->id();
if (current_extensions_.count(id) == 0) {
// Not found in our set yet - add it and maybe return as a previously
// unseen extension.
current_extensions_.insert(id);
// If this application has been newly loaded after the initial startup,
// notify the user.
if (applications_->is_ready()) {
const extensions::Extension* extension = (*it).get();
new_apps.insert(extension);
}
}
}
return new_apps;
}

// static
bool BackgroundModeManager::BackgroundModeData::BackgroundModeDataCompare(
const BackgroundModeData* bmd1,
Expand Down Expand Up @@ -273,16 +296,6 @@ void BackgroundModeManager::RegisterProfile(Profile* profile) {
name = profile_cache_->GetNameOfProfileAtIndex(index);
bmd->SetName(name);

// Listen for when extensions are loaded or add the background permission so
// we can display a "background app installed" notification and enter
// "launch on login" mode on the Mac.
registrar_.Add(this,
chrome::NOTIFICATION_EXTENSION_LOADED_DEPRECATED,
content::Source<Profile>(profile));
registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_PERMISSIONS_UPDATED,
content::Source<Profile>(profile));


// Check for the presence of background apps after all extensions have been
// loaded, to handle the case where an extension has been manually removed
// while Chrome was not running.
Expand Down Expand Up @@ -324,37 +337,6 @@ void BackgroundModeManager::Observe(
// process alive any more when running in no-startup-window mode.
DecrementKeepAliveCountForStartup();
break;

case chrome::NOTIFICATION_EXTENSION_LOADED_DEPRECATED: {
Extension* extension = content::Details<Extension>(details).ptr();
Profile* profile = content::Source<Profile>(source).ptr();
if (BackgroundApplicationListModel::IsBackgroundApp(
*extension, profile)) {
// Extensions loaded after the ExtensionsService is ready should be
// treated as new installs.
if (extensions::ExtensionSystem::Get(profile)->extension_service()->
is_ready()) {
bool is_being_reloaded = false;
CheckReloadStatus(extension, &is_being_reloaded);
// No need to show the notification if we showed to the user
// previously for this app.
if (!is_being_reloaded)
OnBackgroundAppInstalled(extension);
}
}
}
break;
case chrome::NOTIFICATION_EXTENSION_PERMISSIONS_UPDATED: {
UpdatedExtensionPermissionsInfo* info =
content::Details<UpdatedExtensionPermissionsInfo>(details).ptr();
if (info->permissions->HasAPIPermission(
extensions::APIPermission::kBackground) &&
info->reason == UpdatedExtensionPermissionsInfo::ADDED) {
// Turned on background permission, so treat this as a new install.
OnBackgroundAppInstalled(info->extension);
}
}
break;
case chrome::NOTIFICATION_APP_TERMINATING:
// Make sure we aren't still keeping the app alive (only happens if we
// don't receive an EXTENSIONS_READY notification for some reason).
Expand Down Expand Up @@ -424,6 +406,15 @@ void BackgroundModeManager::OnApplicationListChanged(Profile* profile) {
}
// List of applications changed so update the UI.
UpdateStatusTrayIconContextMenu();

// Notify the user about any new applications.
BackgroundModeData* bmd = GetBackgroundModeData(profile);
std::set<const extensions::Extension*> new_apps =
bmd->GetNewBackgroundApps();
for (std::set<const extensions::Extension*>::const_iterator it =
new_apps.begin(); it != new_apps.end(); ++it) {
OnBackgroundAppInstalled(*it);
}
}
}

Expand Down Expand Up @@ -662,25 +653,6 @@ void BackgroundModeManager::OnBackgroundAppInstalled(
}
}

void BackgroundModeManager::CheckReloadStatus(
const Extension* extension,
bool* is_being_reloaded) {
// Walk the BackgroundModeData for all profiles to see if one of their
// extensions is being reloaded.
for (BackgroundModeInfoMap::const_iterator it =
background_mode_data_.begin();
it != background_mode_data_.end();
++it) {
ExtensionService* service =
extensions::ExtensionSystem::Get(it->first)->extension_service();
// If the extension is being reloaded, no need to show a notification.
if (service->IsBeingReloaded(extension->id())) {
*is_being_reloaded = true;
return;
}
}
}

void BackgroundModeManager::CreateStatusTrayIcon() {
// Only need status icons on windows/linux. ChromeOS doesn't allow exiting
// Chrome and Mac can use the dock icon instead.
Expand Down
23 changes: 17 additions & 6 deletions chrome/browser/background/background_mode_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "components/keyed_service/core/keyed_service.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "extensions/common/extension.h"

class Browser;
class PrefRegistrySimple;
Expand All @@ -30,10 +31,6 @@ namespace base {
class CommandLine;
}

namespace extensions {
class Extension;
}

typedef std::vector<int> CommandIdExtensionVector;

// BackgroundModeManager is responsible for switching Chrome into and out of
Expand Down Expand Up @@ -86,6 +83,7 @@ class BackgroundModeManager
private:
friend class AppBackgroundPageApiTest;
friend class BackgroundModeManagerTest;
friend class BackgroundModeManagerWithExtensionsTest;
friend class TestBackgroundModeManager;
FRIEND_TEST_ALL_PREFIXES(BackgroundModeManagerTest,
BackgroundAppLoadUnload);
Expand All @@ -107,10 +105,12 @@ class BackgroundModeManager
ProfileInfoCacheObserver);
FRIEND_TEST_ALL_PREFIXES(BackgroundModeManagerTest,
DeleteBackgroundProfile);
FRIEND_TEST_ALL_PREFIXES(BackgroundModeManagerTest,
FRIEND_TEST_ALL_PREFIXES(BackgroundModeManagerWithExtensionsTest,
BackgroundMenuGeneration);
FRIEND_TEST_ALL_PREFIXES(BackgroundModeManagerTest,
FRIEND_TEST_ALL_PREFIXES(BackgroundModeManagerWithExtensionsTest,
BackgroundMenuGenerationMultipleProfile);
FRIEND_TEST_ALL_PREFIXES(BackgroundModeManagerWithExtensionsTest,
BalloonDisplay);
FRIEND_TEST_ALL_PREFIXES(BackgroundAppBrowserTest,
ReloadBackgroundApp);

Expand Down Expand Up @@ -154,6 +154,10 @@ class BackgroundModeManager
static bool BackgroundModeDataCompare(const BackgroundModeData* bmd1,
const BackgroundModeData* bmd2);

// Returns the set of new background apps (apps that have been loaded since
// the last call to GetNewBackgroundApps()).
std::set<const extensions::Extension*> GetNewBackgroundApps();

private:
// Name associated with this profile which is used to label its submenu.
base::string16 name_;
Expand All @@ -166,6 +170,13 @@ class BackgroundModeManager
// extension indices. A value of -1 indicates no extension is associated
// with the index.
CommandIdExtensionVector* command_id_extension_vector_;

// The list of notified extensions for this profile. We track this to ensure
// that we never notify the user about the same extension twice in a single
// browsing session - this is done because the extension subsystem is not
// good about tracking changes to the background permission around
// extension reloads, and will sometimes report spurious permission changes.
std::set<extensions::ExtensionId> current_extensions_;
};

// Ideally we would want our BackgroundModeData to be scoped_ptrs,
Expand Down
Loading

0 comments on commit bb0ef38

Please sign in to comment.