Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23179002:
Update the app launcher menu on profile name change. (Closed)
|
Created:
7 years, 4 months ago by calamity Modified:
7 years, 4 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@le_refactor_gigante_signin_into_model Visibility:
Public. |
DescriptionUpdate the app launcher menu on profile name change.
Adds a ProfileInfoCacheObserver to the AppListViewDelegate so it can
invalidate the app launcher menu on profile change.
BUG=262863
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218486
Patch Set 1 #
Total comments: 2
Patch Set 2 : actually register the observer #
Total comments: 12
Patch Set 3 : rework #Patch Set 4 : extension_service_->GetInstalledExtension( #Patch Set 5 : rebase #
Messages
Total messages: 13 (0 generated)
https://chromiumcodereview.appspot.com/23179002/diff/1/chrome/browser/ui/app_... File chrome/browser/ui/app_list/app_list_view_delegate.h (right): https://chromiumcodereview.appspot.com/23179002/diff/1/chrome/browser/ui/app_... chrome/browser/ui/app_list/app_list_view_delegate.h:88: // Overridden from ProfileInfoCacheObserver: Did you mean to add this as an observer somewhere? It seems these functions are never called.
https://chromiumcodereview.appspot.com/23179002/diff/1/chrome/browser/ui/app_... File chrome/browser/ui/app_list/app_list_view_delegate.h (right): https://chromiumcodereview.appspot.com/23179002/diff/1/chrome/browser/ui/app_... chrome/browser/ui/app_list/app_list_view_delegate.h:88: // Overridden from ProfileInfoCacheObserver: On 2013/08/14 09:59:17, koz wrote: > Did you mean to add this as an observer somewhere? It seems these functions are > never called. Done.
lgtm
https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... File chrome/browser/ui/app_list/app_list_view_delegate.cc (right): https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... chrome/browser/ui/app_list/app_list_view_delegate.cc:67: AppListViewDelegate::~AppListViewDelegate() { Just be aware... I don't think this destructor is called except when switching profiles, because the app list services are intentionally leaked. .. Which would mean the observers will still be on the ProfileInfoCache when it is destroyed. But I think this is normal. ~ObserverList() only DCHECKs that the observers are gone if you ask it to https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... chrome/browser/ui/app_list/app_list_view_delegate.cc:240: OnProfileChanged(); I don't think we need to do anything here https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... chrome/browser/ui/app_list/app_list_view_delegate.cc:245: OnProfileChanged(); This is currently handled in AppListServiceImpl::OnProfileWillBeRemoved() - I don't think it should do anything here. https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... chrome/browser/ui/app_list/app_list_view_delegate.cc:250: const string16& old_profile_name) { nit: base::string16 https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... File chrome/browser/ui/app_list/app_list_view_delegate.h (right): https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... chrome/browser/ui/app_list/app_list_view_delegate.h:93: const string16& old_profile_name) OVERRIDE; nit: base::string16 + #include base/strings/string16.h ?
https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... File chrome/browser/ui/app_list/app_list_view_delegate.cc (right): https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... chrome/browser/ui/app_list/app_list_view_delegate.cc:251: OnProfileChanged(); Probably also worth adding before this: if (profile_->GetPath() != profile_path) return; This makes it consistent with the filter applied to the notifications
https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... File chrome/browser/ui/app_list/app_list_view_delegate.cc (right): https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... chrome/browser/ui/app_list/app_list_view_delegate.cc:240: OnProfileChanged(); On 2013/08/15 06:34:31, tapted wrote: > I don't think we need to do anything here Done. https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... chrome/browser/ui/app_list/app_list_view_delegate.cc:245: OnProfileChanged(); On 2013/08/15 06:34:31, tapted wrote: > This is currently handled in AppListServiceImpl::OnProfileWillBeRemoved() - I > don't think it should do anything here. Done. https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... chrome/browser/ui/app_list/app_list_view_delegate.cc:250: const string16& old_profile_name) { On 2013/08/15 06:34:31, tapted wrote: > nit: base::string16 Done. https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... chrome/browser/ui/app_list/app_list_view_delegate.cc:251: OnProfileChanged(); On 2013/08/15 07:15:54, tapted wrote: > Probably also worth adding before this: > > if (profile_->GetPath() != profile_path) > return; > > This makes it consistent with the filter applied to the notifications Done. https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... File chrome/browser/ui/app_list/app_list_view_delegate.h (right): https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... chrome/browser/ui/app_list/app_list_view_delegate.h:93: const string16& old_profile_name) OVERRIDE; On 2013/08/15 06:34:31, tapted wrote: > nit: base::string16 + #include base/strings/string16.h ? Done.
https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... File chrome/browser/ui/app_list/app_list_view_delegate.h (right): https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... chrome/browser/ui/app_list/app_list_view_delegate.h:93: const string16& old_profile_name) OVERRIDE; On 2013/08/15 06:34:31, tapted wrote: > nit: base::string16 + #include base/strings/string16.h ? if this is an override, it means profile_info_cache_observer.h should be already including string16.h, so I wouldn't include it again in this header file.
lgtm On 2013/08/15 17:01:51, tfarina wrote: > https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... > File chrome/browser/ui/app_list/app_list_view_delegate.h (right): > > https://chromiumcodereview.appspot.com/23179002/diff/5001/chrome/browser/ui/a... > chrome/browser/ui/app_list/app_list_view_delegate.h:93: const string16& > old_profile_name) OVERRIDE; > On 2013/08/15 06:34:31, tapted wrote: > > nit: base::string16 + #include base/strings/string16.h ? > > if this is an override, it means profile_info_cache_observer.h should be already > including string16.h, so I wouldn't include it again in this header file. I'm down with that (i.e. OK with either). Looks like it's oft-debated (e.g. https://groups.google.com/a/chromium.org/d/msg/chromium-dev/i_5qAOxe0xY/YHPDy... ). I haven't used the internal iwyu script enough to form a strong opinion.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/23179002/9002
Failed to apply patch for chrome/browser/ui/app_list/app_list_view_delegate.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/ui/app_list/app_list_view_delegate.cc
Hunk #1 FAILED at 61.
Hunk #2 succeeded at 209 with fuzz 2 (offset -23 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/app_list/app_list_view_delegate.cc.rej
Patch: chrome/browser/ui/app_list/app_list_view_delegate.cc
Index: chrome/browser/ui/app_list/app_list_view_delegate.cc
diff --git a/chrome/browser/ui/app_list/app_list_view_delegate.cc
b/chrome/browser/ui/app_list/app_list_view_delegate.cc
index
dba22c70885eb3132f03e674ef5c488ce9a0f189..7b4d4f39ea74cd8542fc09b34ccf3b1134774680
100644
--- a/chrome/browser/ui/app_list/app_list_view_delegate.cc
+++ b/chrome/browser/ui/app_list/app_list_view_delegate.cc
@@ -61,11 +61,14 @@
AppListViewDelegate::AppListViewDelegate(AppListControllerDelegate* controller,
DCHECK(profile_);
registrar_.Add(this, chrome::NOTIFICATION_GOOGLE_SIGNED_OUT,
content::Source<Profile>(profile_));
+
g_browser_process->profile_manager()->GetProfileInfoCache().AddObserver(this);
}
AppListViewDelegate::~AppListViewDelegate() {
if (signin_delegate_)
signin_delegate_->RemoveObserver(this);
+ g_browser_process->
+ profile_manager()->GetProfileInfoCache().RemoveObserver(this);
}
void AppListViewDelegate::OnProfileChanged() {
@@ -232,3 +235,12 @@ void AppListViewDelegate::Observe(
const content::NotificationDetails& details) {
OnProfileChanged();
}
+
+void AppListViewDelegate::OnProfileNameChanged(
+ const base::FilePath& profile_path,
+ const base::string16& old_profile_name) {
+ if (profile_->GetPath() != profile_path)
+ return;
+
+ OnProfileChanged();
+}
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/23179002/3001
Message was sent while issue was closed.
Change committed as 218486 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
