Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1701563002:
Refactor ProfileInfoCache in c/b/ui/views (Closed)
|
Created:
4 years, 10 months ago by lwchkg Modified:
4 years, 10 months ago CC:
chromium-reviews, tfarina, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor ProfileInfoCache in c/b/ui/views
ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://codereview.chromium.org/1599013002/ for details.
BUG=305720
Committed: https://crrev.com/01cc8b10bc3cd1b53965a7fab5b055dc6564caed
Cr-Commit-Position: refs/heads/master@{#376513}
Patch Set 1 #
Total comments: 21
Patch Set 2 : Respond to comments #
Total comments: 1
Patch Set 3 : Respond to comments #
Total comments: 15
Patch Set 4 : Respond to most of the comments #Patch Set 5 : Respond to comments #Messages
Total messages: 43 (12 generated)
lwchkg@gmail.com changed reviewers: + anthonyvd@chromium.org, mlerman@chromium.org, pkasting@chromium.org
Dear all, PTAL: Here are the files you own: Peter: c/b/ui/views Anthony, Mike: c/b/profiles (please also check the comment at https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra...) Regards, WC Leung. https://codereview.chromium.org/1701563002/diff/1/chrome/browser/profiles/ava... File chrome/browser/profiles/avatar_menu.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/profiles/ava... chrome/browser/profiles/avatar_menu.cc:43: AvatarMenu::AvatarMenu(ProfileAttributesStorage* /*profile_storage*/, Currently only the interface of AvatarMenu refactored, and that's why this variable is unused. The actual refactoring is too large to be included in this CL, and I'll work on that later. https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:648: GetProfileAttributesWithPath(browser_->profile()->GetPath(), &entry)) { anthonyvd@ and mlerman@: AFAIK profiles either have an entry in ProfileAttributesStorage, or is off the record (i.e. being incognito and/or being guest/system). Am I correct? If yes the entire check can be removed. https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:197: !(*storage.GetAllProfilesAttributes().begin())->IsAuthenticated()); begin() will be changed to cbegin() in the next patch.
https://codereview.chromium.org/1701563002/diff/1/chrome/browser/profiles/ava... File chrome/browser/profiles/avatar_menu.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/profiles/ava... chrome/browser/profiles/avatar_menu.cc:43: AvatarMenu::AvatarMenu(ProfileAttributesStorage* /*profile_storage*/, I've found another 5 Mac-only files calling this constructor. However, uploading these files requires adding more reviewers. If the try-bots pass (which should) without modifying these 5 files, I'll refactor these files in other CLs.
LGTM https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:195: bool use_generic_button = (!browser_->profile()->IsGuestSession() && Wrapping/indenting here is now strange https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:197: !(*storage.GetAllProfilesAttributes().begin())->IsAuthenticated()); On 2016/02/14 17:07:18, lwchkg wrote: > begin() will be changed to cbegin() in the next patch. Nit: Just use front() instead: !storage.GetAllProfilesAttributes().front()->IsAuthenticated()); https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:73: const char* signed_in_email = "me@google.com"; Nit: Compile-time constants like this should be named kConstant style and use const char[] https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:74: Profile* supervised = CreateTestingProfile("supervised"); Nit: Declare variables as close to first use as possible. Here's a way of rewriting this function that tries to break this up into blocks based on what locals need to be used in each block, so as to achieve the above. This also takes into account my other comments here, and re-wraps some of the lines a bit based on how I think we normally do things. const char kSignedInEmail[] = "me@google.com"; // Sign in the |signed_in| profile. ProfileAttributesStorage* storage = g_browser_process->profile_manager()->GetProfileAttributesStorage(); ProfileAttributesEntry* entry_signed_in; ASSERT_TRUE(storage.GetProfileAttributesWithPath(signed_in->GetPath(), &entry_signed_in)); entry_signed_in->SetAuthInfo("12345", base::UTF8ToUTF16(kSignedInEmail)); signed_in->GetPrefs()->SetString(prefs::kGoogleServicesHostedDomain, "google.com"); // Create a supervised user controlled by |signed_in|. Profile* supervised = CreateTestingProfile("supervised"); ProfileAttributesEntry* entry_supervised; ASSERT_TRUE(storage.GetProfileAttributesWithPath(supervised->GetPath(), &entry_supervised)); entry_supervised->SetSupervisedUserId(signed_in_email); // |signed_in| should now be lockable. EXPECT_TRUE(profiles::IsLockAvailable(signed_in)); https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:77: g_browser_process->profile_manager()->GetProfileAttributesStorage(); Nit: Prefer pointers to non-const refs
The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1701563002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1701563002/20001
Description was changed from ========== Refactor ProfileInfoCache in c/b/ui/views ProfileInfoCache is being refactored into ProfileAttributesStorage and ProfileAttributesEntry, which use profile paths instead of numerical indices in the interface. See https://codereview.chromium.org/1599013002/ for details. BUG=305720 ========== to ========== Refactor ProfileInfoCache in c/b/ui/views ProfileInfoCache is being refactored into ProfileAttributesStorage and ProfileAttributesEntry, which use profile paths instead of numerical indices in the interface. See https://codereview.chromium.org/1599013002/ for details. BUG=305720 ==========
lwchkg@gmail.com changed reviewers: + achuith@chromium.org
PK: Thanks for a long review. A revised patch is uploaded. Anthony: Resend invitation to review on c/b/profiles. Thanks! achiuth@: Just added you to this CL. PTAL on c/b/chromeos (there is only 1 file in that path). Thanks! https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:195: bool use_generic_button = (!browser_->profile()->IsGuestSession() && On 2016/02/16 20:49:54, Peter Kasting wrote: > Wrapping/indenting here is now strange I see. Let's see if the new version (the ! is wrapped to the next line) is more readable. https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:74: Profile* supervised = CreateTestingProfile("supervised"); Thanks a lot! On 2016/02/16 20:49:54, Peter Kasting wrote: > Nit: Declare variables as close to first use as possible. > > Here's a way of rewriting this function that tries to break this up into blocks > based on what locals need to be used in each block, so as to achieve the above. > This also takes into account my other comments here, and re-wraps some of the > lines a bit based on how I think we normally do things. > > const char kSignedInEmail[] = mailto:"me@google.com"; > > // Sign in the |signed_in| profile. > ProfileAttributesStorage* storage = > g_browser_process->profile_manager()->GetProfileAttributesStorage(); > ProfileAttributesEntry* entry_signed_in; > ASSERT_TRUE(storage.GetProfileAttributesWithPath(signed_in->GetPath(), > &entry_signed_in)); > entry_signed_in->SetAuthInfo("12345", base::UTF8ToUTF16(kSignedInEmail)); > signed_in->GetPrefs()->SetString(prefs::kGoogleServicesHostedDomain, > "google.com"); > > // Create a supervised user controlled by |signed_in|. > Profile* supervised = CreateTestingProfile("supervised"); > ProfileAttributesEntry* entry_supervised; > ASSERT_TRUE(storage.GetProfileAttributesWithPath(supervised->GetPath(), > &entry_supervised)); > entry_supervised->SetSupervisedUserId(signed_in_email); > > // |signed_in| should now be lockable. > EXPECT_TRUE(profiles::IsLockAvailable(signed_in)); https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:77: g_browser_process->profile_manager()->GetProfileAttributesStorage(); On 2016/02/16 20:49:54, Peter Kasting wrote: > Nit: Prefer pointers to non-const refs Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.cc:195: bool use_generic_button = (!browser_->profile()->IsGuestSession() && On 2016/02/17 17:09:31, lwchkg wrote: > On 2016/02/16 20:49:54, Peter Kasting wrote: > > Wrapping/indenting here is now strange > > I see. Let's see if the new version (the ! is wrapped to the next line) is more > readable. Yeah, looks much better. https://codereview.chromium.org/1701563002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/1701563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:201: profiles::GetAvatarButtonTextForProfile(browser_->profile())); This wrapping is weird too, but since you're not touching this line you only need fix it if you really want.
lgtm https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/profiles/profile_list_chromeos_unittest.cc (right): https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/profiles/profile_list_chromeos_unittest.cc:94: mock_observer_.get(), NULL)); nit: nullptr https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right): https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:37: avatar_button_(nullptr) { nit: initialize this in header https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:195: bool use_generic_button = const
achuith: Looks like the nit is not trivial to fix. Please see my comments below. https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right): https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:37: avatar_button_(nullptr) { On 2016/02/19 11:55:22, achuithb wrote: > nit: initialize this in header Normally your suggestion is the better solution. But the #if section is a some corner cases in styling, which Google C++ style fails to do well. Anyway here is my suggestion of the indents. WDYT? BrowserNonClientFrameView::BrowserNonClientFrameView(BrowserFrame* frame, BrowserView* browser_view) : frame_(frame) , browser_view_(browser_view) #if defined(FRAME_AVATAR_BUTTON) , profile_switcher_(this) #endif { P.S. I'll put this question the mailing list, so more people can give feedbacks.
https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right): https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:37: avatar_button_(nullptr) { On 2016/02/19 15:00:10, lwchkg wrote: > On 2016/02/19 11:55:22, achuithb wrote: > > nit: initialize this in header > > Normally your suggestion is the better solution. But the #if section is a some > corner cases in styling, which Google C++ style fails to do well. Anyway here is > my suggestion of the indents. WDYT? > > BrowserNonClientFrameView::BrowserNonClientFrameView(BrowserFrame* frame, > BrowserView* browser_view) > : frame_(frame) > , browser_view_(browser_view) > #if defined(FRAME_AVATAR_BUTTON) > , profile_switcher_(this) > #endif > { > > P.S. I'll put this question the mailing list, so more people can give feedbacks. Personally I'm fine with this provided you leave a comment, but maybe someone has a better idea. Maybe you can see what git cl format does with this?
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/avatar_menu.cc (right): https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/avatar_menu.cc:47: &g_browser_process->profile_manager()->GetProfileInfoCache())), Would it be possible to move ProfileList into using the ProfileAttributesStorage at this time? Or at least "soon" in an upcoming CL? https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/avatar_menu.h (right): https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/avatar_menu.h:94: AvatarMenu(ProfileAttributesStorage* /*profile_storage*/, Wait... why aren't you giving a name to the first variable? Even if you're not using it, you can still name it.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1701563002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1701563002/40001
c/b/profiles/* lgtm, thanks! https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/avatar_menu.cc (right): https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/avatar_menu.cc:43: AvatarMenu::AvatarMenu(ProfileAttributesStorage* /*profile_storage*/, Can you file a bug about this follow up refactor so we don't forget about it? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
Replied some of the comments. Will address the others after my rebase and recompile is complete. https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/avatar_menu.cc (right): https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/avatar_menu.cc:43: AvatarMenu::AvatarMenu(ProfileAttributesStorage* /*profile_storage*/, On 2016/02/19 15:09:07, anthonyvd wrote: > Can you file a bug about this follow up refactor so we don't forget about it? > Thanks! As long as there is the word "ProfileInfoCache" in the source file we cannot forget about it. All files with the word "ProfileInfoCache" are revealed by code search, meaning the file is not properly refactored. https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/avatar_menu.cc:47: &g_browser_process->profile_manager()->GetProfileInfoCache())), On 2016/02/19 15:08:12, Mike Lerman wrote: > Would it be possible to move ProfileList into using the ProfileAttributesStorage > at this time? Or at least "soon" in an upcoming CL? Definitely will be soon. Anyway, I don't update the code now because this file is not in the directory I'm targeting. (If it do this it looks like I need to add an extra 10-20 files in the CL, and that make things complicated.) https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/avatar_menu.h (right): https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/avatar_menu.h:94: AvatarMenu(ProfileAttributesStorage* /*profile_storage*/, On 2016/02/19 15:08:12, Mike Lerman wrote: > Wait... why aren't you giving a name to the first variable? Even if you're not > using it, you can still name it. AFAIK this results in a compiler warning (which is treated as errors.) Anyway, I forgot which compiler generates the warning. Will try and see after rebasing and recompiling, which should take me a couple of hours. https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right): https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:37: avatar_button_(nullptr) { > Personally I'm fine with this provided you leave a comment, but maybe someone > has a better idea. Maybe you can see what git cl format does with this? Appears that someone has patched away the #if block. So I don't need to take care of the punctuation now... (P.S. Anyway... rebase time.)
The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1701563002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1701563002/60001
The CQ bit was checked by lwchkg@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, anthonyvd@chromium.org, achuith@chromium.org Link to the patchset: https://codereview.chromium.org/1701563002/#ps80001 (title: "Respond to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1701563002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1701563002/80001
Message was sent while issue was closed.
Description was changed from ========== Refactor ProfileInfoCache in c/b/ui/views ProfileInfoCache is being refactored into ProfileAttributesStorage and ProfileAttributesEntry, which use profile paths instead of numerical indices in the interface. See https://codereview.chromium.org/1599013002/ for details. BUG=305720 ========== to ========== Refactor ProfileInfoCache in c/b/ui/views ProfileInfoCache is being refactored into ProfileAttributesStorage and ProfileAttributesEntry, which use profile paths instead of numerical indices in the interface. See https://codereview.chromium.org/1599013002/ for details. BUG=305720 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Refactor ProfileInfoCache in c/b/ui/views ProfileInfoCache is being refactored into ProfileAttributesStorage and ProfileAttributesEntry, which use profile paths instead of numerical indices in the interface. See https://codereview.chromium.org/1599013002/ for details. BUG=305720 ========== to ========== Refactor ProfileInfoCache in c/b/ui/views ProfileInfoCache is being refactored into ProfileAttributesStorage and ProfileAttributesEntry, which use profile paths instead of numerical indices in the interface. See https://codereview.chromium.org/1599013002/ for details. BUG=305720 Committed: https://crrev.com/01cc8b10bc3cd1b53965a7fab5b055dc6564caed Cr-Commit-Position: refs/heads/master@{#376513} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/01cc8b10bc3cd1b53965a7fab5b055dc6564caed Cr-Commit-Position: refs/heads/master@{#376513}
Message was sent while issue was closed.
https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/profiles/profile_list_chromeos_unittest.cc (right): https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/profiles/profile_list_chromeos_unittest.cc:94: mock_observer_.get(), NULL)); On 2016/02/19 11:55:22, achuithb wrote: > nit: nullptr Done. https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/avatar_menu.h (right): https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/avatar_menu.h:94: AvatarMenu(ProfileAttributesStorage* /*profile_storage*/, On 2016/02/19 16:46:04, lwchkg wrote: > On 2016/02/19 15:08:12, Mike Lerman wrote: > > Wait... why aren't you giving a name to the first variable? Even if you're not > > using it, you can still name it. > > AFAIK this results in a compiler warning (which is treated as errors.) Anyway, I > forgot which compiler generates the warning. Will try and see after rebasing and > recompiling, which should take me a couple of hours. No compile failures. I was too paranoid. https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/1701563002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:195: bool use_generic_button = On 2016/02/19 11:55:22, achuithb wrote: > const Done.
Message was sent while issue was closed.
https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:648: GetProfileAttributesWithPath(browser_->profile()->GetPath(), &entry)) { On 2016/02/14 17:07:18, lwchkg wrote: > anthonyvd@ and mlerman@: AFAIK profiles either have an entry in > ProfileAttributesStorage, or is off the record (i.e. being incognito and/or > being guest/system). Am I correct? If yes the entire check can be removed. Forgot to follow-up my own comment. Oh no! Anyway, anthonyvd@ and mlerman@: WDYT?
Message was sent while issue was closed.
https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:648: GetProfileAttributesWithPath(browser_->profile()->GetPath(), &entry)) { On 2016/02/24 18:07:27, lwchkg wrote: > On 2016/02/14 17:07:18, lwchkg wrote: > > anthonyvd@ and mlerman@: AFAIK profiles either have an entry in > > ProfileAttributesStorage, or is off the record (i.e. being incognito and/or > > being guest/system). Am I correct? If yes the entire check can be removed. > > Forgot to follow-up my own comment. Oh no! > Anyway, anthonyvd@ and mlerman@: WDYT? Well, Guest and System end up coming down to this line, since line 629 explicitly doesn't include Guest (or System, ideally) https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:648: GetProfileAttributesWithPath(browser_->profile()->GetPath(), &entry)) { On 2016/02/24 18:07:27, lwchkg wrote: > On 2016/02/14 17:07:18, lwchkg wrote: > > anthonyvd@ and mlerman@: AFAIK profiles either have an entry in > > ProfileAttributesStorage, or is off the record (i.e. being incognito and/or > > being guest/system). Am I correct? If yes the entire check can be removed. > > Forgot to follow-up my own comment. Oh no! > Anyway, anthonyvd@ and mlerman@: WDYT? In line 629 we explicitly don't skip out GuestProfile, so we need a check here that handles Guest and System.
Message was sent while issue was closed.
https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:648: GetProfileAttributesWithPath(browser_->profile()->GetPath(), &entry)) { > > Well, Guest and System end up coming down to this line, since line 629 > explicitly doesn't include Guest (or System, ideally) Well... lines 626-637 is for ChromeOS, and lines 639-652 handles other OSes (should be Win/Linux, and we're discussing the equivalent MacOS code in the other review). AFAIK guest and system profiles are be handled at line 641, because guest and system profiles are both off the record. Maybe we can add explicitly the check of guest and system profiles at the same line, to make the code easier to read.
Message was sent while issue was closed.
https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:648: GetProfileAttributesWithPath(browser_->profile()->GetPath(), &entry)) { On 2016/02/24 18:43:18, lwchkg wrote: > > > > Well, Guest and System end up coming down to this line, since line 629 > > explicitly doesn't include Guest (or System, ideally) > > Well... lines 626-637 is for ChromeOS, and lines 639-652 handles other OSes > (should be Win/Linux, and we're discussing the equivalent MacOS code in the > other review). > > AFAIK guest and system profiles are be handled at line 641, because guest and > system profiles are both off the record. Maybe we can add explicitly the check > of guest and system profiles at the same line, to make the code easier to read. I think the current condition is exactly what we need. It accomplishes: "if the profileStorage, the place where we have avatars, doesn't have this profile, then don't display an avatar". I think writing a condition that reads "if we're, among other things, the system profile, then always display an avatar". It's logically equivalent but doesn't convey the right semantics.
Message was sent while issue was closed.
https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:648: GetProfileAttributesWithPath(browser_->profile()->GetPath(), &entry)) { On 2016/02/24 20:44:22, Mike Lerman wrote: > I think the current condition is exactly what we need. It accomplishes: "if the > profileStorage, the place where we have avatars, doesn't have this profile, then > don't display an avatar". I think writing a condition that reads "if we're, > among other things, the system profile, then always display an avatar". It's > logically equivalent but doesn't convey the right semantics. Finally I've found why the code was there: return false for profile in the process being asynchronously deleted. I've missed the whole point. (Fortunately we have git blames and diffs, otherwise I will never know it.) Here is the diff: https://chromium.googlesource.com/chromium/src/+/9bbc8241cae7b253ce22864fbe0f... BTW, do we have a more readable way to check for this "profile being deleted" status?
Message was sent while issue was closed.
https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:648: GetProfileAttributesWithPath(browser_->profile()->GetPath(), &entry)) { On 2016/02/26 15:27:30, lwchkg wrote: > On 2016/02/24 20:44:22, Mike Lerman wrote: > > I think the current condition is exactly what we need. It accomplishes: "if > the > > profileStorage, the place where we have avatars, doesn't have this profile, > then > > don't display an avatar". I think writing a condition that reads "if we're, > > among other things, the system profile, then always display an avatar". It's > > logically equivalent but doesn't convey the right semantics. > > Finally I've found why the code was there: return false for profile in the > process being asynchronously deleted. I've missed the whole point. (Fortunately > we have git blames and diffs, otherwise I will never know it.) > > Here is the diff: > https://chromium.googlesource.com/chromium/src/+/9bbc8241cae7b253ce22864fbe0f... > > BTW, do we have a more readable way to check for this "profile being deleted" > status? There isn't one currently. It would be easy to implement a public static method on ProfileDestroyer that checks to see if there's an entry in pending_destroyers_ and returns true/false as appropriate.
Message was sent while issue was closed.
https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:648: GetProfileAttributesWithPath(browser_->profile()->GetPath(), &entry)) { On 2016/02/26 15:47:49, Mike Lerman wrote: > > BTW, do we have a more readable way to check for this "profile being deleted" > > status? > > There isn't one currently. It would be easy to implement a public static method > on ProfileDestroyer that checks to see if there's an entry in > pending_destroyers_ and returns true/false as appropriate. Found it. There's a IsProfileMarkedForDeletion() in the anonymous namespace in profile_manager.cc. Just making it a public method in ProfileManager should do.
Message was sent while issue was closed.
On 2016/02/26 16:47:33, lwchkg wrote: > https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... > File chrome/browser/ui/views/frame/browser_view.cc (right): > > https://codereview.chromium.org/1701563002/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/browser_view.cc:648: > GetProfileAttributesWithPath(browser_->profile()->GetPath(), &entry)) { > On 2016/02/26 15:47:49, Mike Lerman wrote: > > > BTW, do we have a more readable way to check for this "profile being > deleted" > > > status? > > > > There isn't one currently. It would be easy to implement a public static > method > > on ProfileDestroyer that checks to see if there's an entry in > > pending_destroyers_ and returns true/false as appropriate. > > Found it. There's a IsProfileMarkedForDeletion() in the anonymous namespace in > profile_manager.cc. Just making it a public method in ProfileManager should do. Well, those are a bit different. The ProfileDestroyer I mentioned tracks deletion of the Profile* from memory, which happens 'relatively soon' after someone indicates they want to delete a Profile. The list and function you found tracks deletion of the Profile Directory from the user_data_dir. This gets queued up once the Profile object deletion is mostly cleaned up JUST before we remove the profile from the ProfileInfoCache (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...) and only finishes deleting itself on Browser Shutdown. So there can be a long period of time after the profile's been deleted where: it's not in the ProfileInfoCache, there's no profileDestroyer for it, but its directory is hanging around in ProfilesToDelete().
Message was sent while issue was closed.
> > Found it. There's a IsProfileMarkedForDeletion() in the anonymous namespace in > > profile_manager.cc. Just making it a public method in ProfileManager should > do. > > Well, those are a bit different. > > The ProfileDestroyer I mentioned tracks deletion of the Profile* from memory, > which happens 'relatively soon' after someone indicates they want to delete a > Profile. > The list and function you found tracks deletion of the Profile Directory from > the user_data_dir. This gets queued up once the Profile object deletion is > mostly cleaned up JUST before we remove the profile from the ProfileInfoCache > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...) > and only finishes deleting itself on Browser Shutdown. > > So there can be a long period of time after the profile's been deleted where: > it's not in the ProfileInfoCache, there's no profileDestroyer for it, but its > directory is hanging around in ProfilesToDelete(). Isn't ProfileDestroyer supposed to work with incognito profiles only? For normal profiles I do only see calls to ScheduleProfileForDeletion, e.g. the code below in user_manager_screen_handler.cc (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...).
Message was sent while issue was closed.
On 2016/02/26 17:38:20, lwchkg wrote: > > > Found it. There's a IsProfileMarkedForDeletion() in the anonymous namespace > in > > > profile_manager.cc. Just making it a public method in ProfileManager should > > do. > > > > Well, those are a bit different. > > > > The ProfileDestroyer I mentioned tracks deletion of the Profile* from memory, > > which happens 'relatively soon' after someone indicates they want to delete a > > Profile. > > The list and function you found tracks deletion of the Profile Directory from > > the user_data_dir. This gets queued up once the Profile object deletion is > > mostly cleaned up JUST before we remove the profile from the ProfileInfoCache > > > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...) > > and only finishes deleting itself on Browser Shutdown. > > > > So there can be a long period of time after the profile's been deleted where: > > it's not in the ProfileInfoCache, there's no profileDestroyer for it, but its > > directory is hanging around in ProfilesToDelete(). > > Isn't ProfileDestroyer supposed to work with incognito profiles only? For normal > profiles I do only see calls to ScheduleProfileForDeletion, e.g. the code below > in user_manager_screen_handler.cc > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...). Nope, it also deletes real profiles. DestroyProfileWhenAppropriate, such as: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
Message was sent while issue was closed.
We should move this to an email or something, or a bug, but off this already-landed CL. |
