Skip to content

Commit

Permalink
[Merge M57] Fix import legacy, pref based pins.
Browse files Browse the repository at this point in the history 
This CL contains 2 fixes for importing legacy pref based pins.
1. Import may happen at the moment when profile is not synced yet and
in last case it does not actually contain information from previous
bulds.
2. Importing ignores apps that currently are not installed. This
leads to situation that we loose pin information for such apps
and in case apps are installed later they do not appear pinned.
Solution is to import all existings apps in prefs.

BUG=680821
TEST=Clearn app sync server data. Login on M52 and do some pinning.
     Next login on M55 and pin additional app. Next login to ToT
     and observe that pin is restored for existing apps and
     pins appear automatically once apps are installed later.
TBR=stevenjb@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2646973003
Cr-Commit-Position: refs/heads/master@{#445398}
(cherry picked from commit d8cc9d2)

Review-Url: https://codereview.chromium.org/2655553002
Cr-Commit-Position: refs/branch-heads/2987@{#51}
Cr-Branched-From: ad51088-refs/heads/master@{#444943}
  • Loading branch information
khmel authored and Commit bot committed Jan 24, 2017
1 parent 1995096 commit 9329198
Show file tree
Hide file tree
Showing 4 changed files with 135 additions and 32 deletions.
55 changes: 35 additions & 20 deletions chrome/browser/ui/ash/chrome_launcher_prefs.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,15 +237,19 @@ class AppTracker {
}

void MaybeAddApp(const AppLauncherId& app_launcher_id,
const LauncherControllerHelper* helper) {
const LauncherControllerHelper* helper,
bool check_for_valid_app) {
DCHECK_NE(kPinnedAppsPlaceholder, app_launcher_id.ToString());
if (!helper->IsValidIDForCurrentUser(app_launcher_id.ToString()))
if (check_for_valid_app &&
!helper->IsValidIDForCurrentUser(app_launcher_id.ToString())) {
return;
}
AddApp(app_launcher_id);
}

void MaybeAddAppFromPref(const base::DictionaryValue* app_pref,
const LauncherControllerHelper* helper) {
const LauncherControllerHelper* helper,
bool check_for_valid_app) {
std::string app_id;
if (!app_pref->GetString(kPinnedAppsPrefAppIDPath, &app_id)) {
LOG(ERROR) << "Cannot get app id from app pref entry.";
Expand All @@ -262,7 +266,7 @@ class AppTracker {
return;
}

MaybeAddApp(AppLauncherId(app_id), helper);
MaybeAddApp(AppLauncherId(app_id), helper, check_for_valid_app);
}

const std::vector<AppLauncherId>& app_list() const { return app_list_; }
Expand Down Expand Up @@ -399,6 +403,7 @@ void SetShelfAlignmentPref(PrefService* prefs,
// Helper that extracts app list from policy preferences.
void GetAppsPinnedByPolicy(const PrefService* prefs,
const LauncherControllerHelper* helper,
bool check_for_valid_app,
AppTracker* apps) {
DCHECK(apps);
DCHECK(apps->app_list().empty());
Expand Down Expand Up @@ -433,17 +438,19 @@ void GetAppsPinnedByPolicy(const PrefService* prefs,
for (const auto& activity : activities) {
const std::string arc_app_id =
ArcAppListPrefs::GetAppId(arc_package, activity);
apps->MaybeAddApp(AppLauncherId(arc_app_id), helper);
apps->MaybeAddApp(AppLauncherId(arc_app_id), helper,
check_for_valid_app);
}
} else {
apps->MaybeAddApp(AppLauncherId(app_id), helper);
apps->MaybeAddApp(AppLauncherId(app_id), helper, check_for_valid_app);
}
}
}

std::vector<AppLauncherId> GetPinnedAppsFromPrefsLegacy(
const PrefService* prefs,
const LauncherControllerHelper* helper) {
const LauncherControllerHelper* helper,
bool check_for_valid_app) {
// Adding the app list item to the list of items requires that the ID is not
// a valid and known ID for the extension system. The ID was constructed that
// way - but just to make sure...
Expand All @@ -461,7 +468,7 @@ std::vector<AppLauncherId> GetPinnedAppsFromPrefsLegacy(
CreateAppDict(AppLauncherId(extension_misc::kChromeAppId)));

AppTracker apps;
GetAppsPinnedByPolicy(prefs, helper, &apps);
GetAppsPinnedByPolicy(prefs, helper, check_for_valid_app, &apps);

std::string app_id;
for (size_t i = 0; i < pinned_apps->GetSize(); ++i) {
Expand All @@ -474,7 +481,7 @@ std::vector<AppLauncherId> GetPinnedAppsFromPrefsLegacy(
LOG(ERROR) << "There is no dictionary for app entry.";
continue;
}
apps.MaybeAddAppFromPref(app_pref, helper);
apps.MaybeAddAppFromPref(app_pref, helper, check_for_valid_app);
}

// If not added yet, the chrome item will be the last item in the list.
Expand Down Expand Up @@ -559,22 +566,24 @@ syncer::StringOrdinal GetLastPinPosition(Profile* profile) {

std::vector<AppLauncherId> ImportLegacyPinnedApps(
const PrefService* prefs,
LauncherControllerHelper* helper,
const AppTracker& policy_apps) {
std::vector<AppLauncherId> legacy_pins =
GetPinnedAppsFromPrefsLegacy(prefs, helper);
DCHECK(!legacy_pins.empty());
LauncherControllerHelper* helper) {
std::vector<AppLauncherId> legacy_pins_all =
GetPinnedAppsFromPrefsLegacy(prefs, helper, false);
DCHECK(!legacy_pins_all.empty());

app_list::AppListSyncableService* app_service =
app_list::AppListSyncableServiceFactory::GetForProfile(helper->profile());

std::vector<AppLauncherId> legacy_pins_valid;
syncer::StringOrdinal last_position =
syncer::StringOrdinal::CreateInitialOrdinal();
// Convert to sync item record.
for (const auto& app_launcher_id : legacy_pins) {
for (const auto& app_launcher_id : legacy_pins_all) {
DCHECK_NE(kPinnedAppsPlaceholder, app_launcher_id.ToString());
app_service->SetPinPosition(app_launcher_id.ToString(), last_position);
last_position = last_position.CreateAfter();
if (helper->IsValidIDForCurrentUser(app_launcher_id.ToString()))
legacy_pins_valid.push_back(app_launcher_id);
}

// Now process default apps.
Expand All @@ -590,7 +599,7 @@ std::vector<AppLauncherId> ImportLegacyPinnedApps(
last_position = last_position.CreateAfter();
}

return legacy_pins;
return legacy_pins_valid;
}

std::vector<AppLauncherId> GetPinnedAppsFromPrefs(
Expand All @@ -604,9 +613,6 @@ std::vector<AppLauncherId> GetPinnedAppsFromPrefs(

std::vector<PinInfo> pin_infos;

AppTracker policy_apps;
GetAppsPinnedByPolicy(prefs, helper, &policy_apps);

// Empty pins indicates that sync based pin model is used for the first
// time. In normal workflow we have at least Chrome browser pin info.
bool first_run = true;
Expand All @@ -627,14 +633,23 @@ std::vector<AppLauncherId> GetPinnedAppsFromPrefs(
}

if (first_run) {
// Return default apps in case profile is not synced yet.
sync_preferences::PrefServiceSyncable* const pref_service_syncable =
PrefServiceSyncableFromProfile(helper->profile());
if (!pref_service_syncable->IsSyncing())
return GetPinnedAppsFromPrefsLegacy(prefs, helper, true);

// We need to import legacy pins model and convert it to sync based
// model.
return ImportLegacyPinnedApps(prefs, helper, policy_apps);
return ImportLegacyPinnedApps(prefs, helper);
}

// Sort pins according their ordinals.
std::sort(pin_infos.begin(), pin_infos.end(), ComparePinInfo());

AppTracker policy_apps;
GetAppsPinnedByPolicy(prefs, helper, true, &policy_apps);

// Pinned by policy apps appear first, if they were not shown before.
syncer::StringOrdinal front_position = GetFirstPinPosition(helper->profile());
std::vector<AppLauncherId>::const_reverse_iterator it;
Expand Down
10 changes: 10 additions & 0 deletions chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include "chrome/browser/defaults.h"
#include "chrome/browser/extensions/extension_util.h"
#include "chrome/browser/prefs/incognito_mode_prefs.h"
#include "chrome/browser/prefs/pref_service_syncable_util.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/browser/ui/app_list/app_list_syncable_service_factory.h"
Expand Down Expand Up @@ -75,6 +76,7 @@
#include "components/prefs/scoped_user_pref_update.h"
#include "components/signin/core/account_id/account_id.h"
#include "components/strings/grit/components_strings.h"
#include "components/sync_preferences/pref_service_syncable.h"
#include "components/user_manager/user_manager.h"
#include "content/public/browser/navigation_entry.h"
#include "content/public/browser/web_contents.h"
Expand Down Expand Up @@ -826,6 +828,8 @@ void ChromeLauncherControllerImpl::AttachProfile(Profile* profile_to_attach) {
app_list::AppListSyncableServiceFactory::GetForProfile(profile());
if (app_service)
app_service->AddObserverAndStart(this);

PrefServiceSyncableFromProfile(profile())->AddObserver(this);
}

///////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -1114,6 +1118,10 @@ void ChromeLauncherControllerImpl::OnSyncModelUpdated() {
UpdateAppLaunchersFromPref();
}

void ChromeLauncherControllerImpl::OnIsSyncingChanged() {
UpdateAppLaunchersFromPref();
}

void ChromeLauncherControllerImpl::ScheduleUpdateAppLaunchersFromPref() {
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
Expand Down Expand Up @@ -1389,6 +1397,8 @@ void ChromeLauncherControllerImpl::ReleaseProfile() {
app_list::AppListSyncableServiceFactory::GetForProfile(profile());
if (app_service)
app_service->RemoveObserver(this);

PrefServiceSyncableFromProfile(profile())->RemoveObserver(this);
}

///////////////////////////////////////////////////////////////////////////////
Expand Down
7 changes: 6 additions & 1 deletion chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
#include "chrome/browser/ui/ash/launcher/launcher_app_updater.h"
#include "components/prefs/pref_change_registrar.h"
#include "components/sync_preferences/pref_service_syncable_observer.h"
#include "ui/aura/window_observer.h"

class AppSyncUIState;
Expand Down Expand Up @@ -53,7 +54,8 @@ class ChromeLauncherControllerImpl
private ash::ShelfModelObserver,
private ash::WindowTreeHostManager::Observer,
private AppSyncUIStateObserver,
private app_list::AppListSyncableService::Observer {
private app_list::AppListSyncableService::Observer,
private sync_preferences::PrefServiceSyncableObserver {
public:
ChromeLauncherControllerImpl(Profile* profile, ash::ShelfModel* model);
~ChromeLauncherControllerImpl() override;
Expand Down Expand Up @@ -265,6 +267,9 @@ class ChromeLauncherControllerImpl
// app_list::AppListSyncableService::Observer:
void OnSyncModelUpdated() override;

// sync_preferences::PrefServiceSyncableObserver:
void OnIsSyncingChanged() override;

// Unpins shelf item and optionally updates pin prefs when |update_prefs| is
// set to true.
void UnpinAndUpdatePrefs(ash::ShelfID id, bool update_prefs);
Expand Down
Loading

0 comments on commit 9329198

Please sign in to comment.