Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 2 months ago by noms (inactive) Modified:
6 years, 1 month ago Reviewers:
tapted, Roger Tawa OOO till Jul 10th, sky, xiyuan, cpu_(ooo_6.6-7.5), jwd CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Win] Add a fast profile switcher to the Windows taskbar item.
Mac and Linux now have a fast profile switcher in the OS menu bar, so for the
sake of feature parity, the Windows taskbar should have one as well. Since we
can only have 10 items in total in the Jumplist, we are going to run an
experiment and see if the hypothesis that nobody uses the Most Visited items
in the Jumplist is true (and that we can replace them with a list of Profiles)
I've also added a new histogram, WinJumplist.Action to track what got clicked in
the Jumplisy. Because the Jumplist is just a list of shortcuts, this means adding
a command line switch, that gets checked when the chrome process starts up.
Also includes a small refactor of AvatarMenu::GetImageForMenuButton so that it
can be used without a Profile* (also, it was completely unused on CrOS)
Screenshot: https://drive.google.com/folderview?id=0B1B1Up4p2NRMNzVLMVpfSkJTbmc&usp=sharing
BUG=421490
TEST=Start Chrome. Right click on the Windows taskbar item. A list of profiles
should appear above the Tasks list. Clicking on a profile name should either
open a new browser window for the profile, or activate an existing one.
Committed: https://crrev.com/f9b8e2af097ad14a094214087eda989bd8fc8f3a
Cr-Commit-Position: refs/heads/master@{#304078}
Patch Set 1 : #Patch Set 2 : fix compile #
Total comments: 18
Patch Set 3 : locking! browser test! nits! #
Total comments: 10
Patch Set 4 : only display switcher for >1 profiles #Patch Set 5 : metrics! finches! #Patch Set 6 : fix compile #
Total comments: 18
Patch Set 7 : sky + tapted review comments #
Total comments: 2
Patch Set 8 : rebase + added renamed jumplist files #
Total comments: 14
Patch Set 9 : moar review comments! #Patch Set 10 : ifdef code correctly #
Total comments: 2
Patch Set 11 : sort list #Patch Set 12 : commas. commas are important #
Created: 6 years, 1 month ago
Messages
Total messages: 40 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
noms@chromium.org changed reviewers: + tapted@chromium.org
Hiya Trent! Here is the CL that we talked about, that adds a list of Profiles to the Windows JumpList. Would you mind giving it a review, before I send it over to Scott? Thanks!
main comment below about thread safety. I think it would be nice (and keep owners happy :) if there was a browser_test for the command line flags - i.e. to verify that new windows aren't opened - probably something in startup_browser_creator_browsertest . For the jumplist itself, tests would be nice... but I don't think there's a way to add an integration test without exploding the bots, and a unit test harness would be a special challenge with the threading :/. I guess the jumplist is currently a source of regressions, so maybe that's ok.. Also I'd make sure it behaves nicely when Chrome is pinned to the start menu, and not currently running. This should give `Chrome` the same set of profiles via a submenu (which I think is a super-cool bonus feature :). Maybe include a screenshot for that - you might want to run it past ui review. https://codereview.chromium.org/660813002/diff/60001/chrome/browser/jumplist_... File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/660813002/diff/60001/chrome/browser/jumplist_... chrome/browser/jumplist_win.cc:144: link->set_title(base::UTF16ToWide(item.name)); is the UTF16ToWide needed? (pretty sure std::wstring is utf16 on windows, so it's just a typedef, and there's a lingering TODO to convert wstring to string16) https://codereview.chromium.org/660813002/diff/60001/chrome/browser/jumplist_... chrome/browser/jumplist_win.cc:159: base::UTF16ToWide( remove UTF16ToWide? https://codereview.chromium.org/660813002/diff/60001/chrome/browser/jumplist_... chrome/browser/jumplist_win.cc:555: avatar_menu_.get(), profile_avatars_, icon_dir_); RunUpdate runs on the FILE thread, so accessing |avatar_menu_| and |profile_avatars_| here isn't threadsafe. I think you'll want to more closely follow the pattern used for most_visited_pages_ and recently_closed_pages_ above. E.g. a data member like `ShellLinkItemList profile_switcher_;` I'd probably also start by renaming RunUpdate to RunUpdateOnFileThread so this is clearer. https://codereview.chromium.org/660813002/diff/60001/chrome/browser/jumplist_... File chrome/browser/jumplist_win.h (right): https://codereview.chromium.org/660813002/diff/60001/chrome/browser/jumplist_... chrome/browser/jumplist_win.h:78: virtual void OnAvatarMenuChanged(AvatarMenu* avatar_menu) override; nit: I think this will eventually lose the `virtual`, so I'd leave that off in case the cleanup CL wins the race.. The overrides above are also missing any `override` since msvc doesn't check, but a cleanup CL should get those. https://codereview.chromium.org/660813002/diff/60001/chrome/browser/jumplist_... chrome/browser/jumplist_win.h:187: scoped_ptr<AvatarMenu> avatar_menu_; nit: comment? (for consistency - it looks lonely) https://codereview.chromium.org/660813002/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/660813002/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_avatar_icon_util.cc:294: void GetTransparentBackgroundProfileAvatar(const base::FilePath& profile_path, nit: move to end of file? (to be consistent with declaration order) https://codereview.chromium.org/660813002/diff/60001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/660813002/diff/60001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:626: if (command_line.HasSwitch(switches::kReuseExistingProfileBrowser)) { an OWNER here might have a different opinion, but WDYT about putting this in `ProcessCommandLineAlreadyRunning`? A browser can only be found if the profile is loaded, so the asynchronous path doesn't need to be handled, and putting it there puts the switch check nearer the GetProfileByPath call which feels kinda nice. https://codereview.chromium.org/660813002/diff/60001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:627: Browser* browser = chrome::FindTabbedBrowser(last_used_profile, false, nit: one arg per line. https://codereview.chromium.org/660813002/diff/60001/chrome/common/chrome_swi... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/660813002/diff/60001/chrome/common/chrome_swi... chrome/common/chrome_switches.cc:1039: // If set, Chrome will reuse any existing browsers for a specific profile. Used nit: "reuse" suggests to me that it will open a new tab in that browser, rather than simply activate it. Maybe kActivateExistingProfileBrowser ?
On 2014/11/03 23:57:06, tapted wrote: > I guess the jumplist is currently a source of regressions uhhh. that should say "isn't" :)
Woohoo! Thank you so much! I think things make more sense now. I've added a browser test for the new flag, and hooooopefully cleaned up the threading problems. Also, tested pinning Chrome to the taskbar, and it works great (screenshot added). Let me know what you think! https://codereview.chromium.org/660813002/diff/60001/chrome/browser/jumplist_... File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/660813002/diff/60001/chrome/browser/jumplist_... chrome/browser/jumplist_win.cc:144: link->set_title(base::UTF16ToWide(item.name)); On 2014/11/03 23:57:06, tapted wrote: > is the UTF16ToWide needed? (pretty sure std::wstring is utf16 on windows, so > it's just a typedef, and there's a lingering TODO to convert wstring to > string16) Done. https://codereview.chromium.org/660813002/diff/60001/chrome/browser/jumplist_... chrome/browser/jumplist_win.cc:159: base::UTF16ToWide( On 2014/11/03 23:57:05, tapted wrote: > remove UTF16ToWide? Done. https://codereview.chromium.org/660813002/diff/60001/chrome/browser/jumplist_... chrome/browser/jumplist_win.cc:555: avatar_menu_.get(), profile_avatars_, icon_dir_); On 2014/11/03 23:57:05, tapted wrote: > RunUpdate runs on the FILE thread, so accessing |avatar_menu_| and > |profile_avatars_| here isn't threadsafe. I think you'll want to more closely > follow the pattern used for most_visited_pages_ and recently_closed_pages_ > above. E.g. a data member like `ShellLinkItemList profile_switcher_;` > > I'd probably also start by renaming RunUpdate to RunUpdateOnFileThread so this > is clearer. Done. https://codereview.chromium.org/660813002/diff/60001/chrome/browser/jumplist_... File chrome/browser/jumplist_win.h (right): https://codereview.chromium.org/660813002/diff/60001/chrome/browser/jumplist_... chrome/browser/jumplist_win.h:78: virtual void OnAvatarMenuChanged(AvatarMenu* avatar_menu) override; On 2014/11/03 23:57:06, tapted wrote: > nit: I think this will eventually lose the `virtual`, so I'd leave that off in > case the cleanup CL wins the race.. The overrides above are also missing any > `override` since msvc doesn't check, but a cleanup CL should get those. Done. Cleaned up the other ones too. https://codereview.chromium.org/660813002/diff/60001/chrome/browser/jumplist_... chrome/browser/jumplist_win.h:187: scoped_ptr<AvatarMenu> avatar_menu_; On 2014/11/03 23:57:06, tapted wrote: > nit: comment? (for consistency - it looks lonely) Done. https://codereview.chromium.org/660813002/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/profile_avatar_icon_util.cc (right): https://codereview.chromium.org/660813002/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_avatar_icon_util.cc:294: void GetTransparentBackgroundProfileAvatar(const base::FilePath& profile_path, On 2014/11/03 23:57:06, tapted wrote: > nit: move to end of file? (to be consistent with declaration order) Turns out everything was in a whack order, so I reordered all the things. https://codereview.chromium.org/660813002/diff/60001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/660813002/diff/60001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:626: if (command_line.HasSwitch(switches::kReuseExistingProfileBrowser)) { On 2014/11/03 23:57:06, tapted wrote: > an OWNER here might have a different opinion, but WDYT about putting this in > `ProcessCommandLineAlreadyRunning`? A browser can only be found if the profile > is loaded, so the asynchronous path doesn't need to be handled, and putting it > there puts the switch check nearer the GetProfileByPath call which feels kinda > nice. Done. https://codereview.chromium.org/660813002/diff/60001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:627: Browser* browser = chrome::FindTabbedBrowser(last_used_profile, false, On 2014/11/03 23:57:06, tapted wrote: > nit: one arg per line. Done. https://codereview.chromium.org/660813002/diff/60001/chrome/common/chrome_swi... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/660813002/diff/60001/chrome/common/chrome_swi... chrome/common/chrome_switches.cc:1039: // If set, Chrome will reuse any existing browsers for a specific profile. Used On 2014/11/03 23:57:06, tapted wrote: > nit: "reuse" suggests to me that it will open a new tab in that browser, rather > than simply activate it. Maybe kActivateExistingProfileBrowser ? Oh, much better. I had a lot of trouble coming up with a name. Thanks!
looking good! I think it's ready for some OWNERS. If you think so too, I'd do that before waiting for my lg so you avoid some timezone lag. https://codereview.chromium.org/660813002/diff/80001/chrome/browser/jumplist_... File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/660813002/diff/80001/chrome/browser/jumplist_... chrome/browser/jumplist_win.cc:154: MulDiv(jumplist_updater.user_max_items(), kMostVisited, kTotal); I think you'll need to update these values - maxing out the most visted/recently closed will probably omit profiles (and tasks) below when trying to add them as well. And.. it might get a bit tricky. Probably something like // If a user has many profiles, don't clobber all the sites, and if user_max_items has been set to be very small, don't underflow. const int kTotalSites = kMostVisited + kRecentlyClosed; const int kMaxProfiles = 5; size_t shown_profile_count = std::min(jumplist_updater.user_max_items(), std::min(kMaxProfiles, profile_switcher.size())); size_t available_slots = jumplist_updater.user_max_items() - shown_profile_count; size_t most_visited_items = MulDiv(available_slots, kMostVisited, kTotalSites); ... size_t recently_closed_items = available_slots - most_visited_items; It's a tricky algorithm - maybe check with a PM what a good distribution is. (note: apparently the default limit is 10, but it's user-configurable - http://msdn.microsoft.com/en-us/library/windows/desktop/dd378398(v=vs.85).aspx, there's also a limit of 4 section headers, which I think we've hit now) https://codereview.chromium.org/660813002/diff/80001/chrome/browser/jumplist_... chrome/browser/jumplist_win.cc:180: // Update the "Profiles" category of the JumpList. nit: Profiles -> People? (and super-nit: should the comment in generated_resources.grd be updated? It's currently "The name of the sync group in the options dialog when using the new Profiles UI." but I guess the section on chrome://settings is now the old Profile UI, and it should probably say something about being used for OS Menu taskbar switchers.) https://codereview.chromium.org/660813002/diff/80001/chrome/browser/jumplist_... chrome/browser/jumplist_win.cc:563: } This is a bit nitty, but it's typically good to avoid calling "unknown" code (i.e. calling out to functions in other interfaces) while holding a lock. I'd probably do something like ShellLinkItemList new_profile_switcher; for (size_t ... ) { .. } { base::AutoLock auto_lock(list_lock_); new_profile_switcher.swap(profile_switcher_); } (doing .swap also means you don't call out to malloc/free while holding the lock) https://codereview.chromium.org/660813002/diff/80001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/660813002/diff/80001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:749: if (command_line.HasSwitch(switches::kActivateExistingProfileBrowser)) { nit: perhaps comment here, something like "If the profile is loaded, it might also have an existing browser to activate when using the taskbar-activated profile switcher. In this case, skip regular command line processing, since that will open a new tab." https://codereview.chromium.org/660813002/diff/80001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/660813002/diff/80001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:302: EXPECT_TRUE(browser()->window()->IsActive()); Calls to IsActive need to be in interactive_ui_tests since another test might steal focus, but it would be good to avoid moving to an interactive test. I think repeating the test after closing all windows for the profile would make a compelling test. (but there's a trap: closing all the windows will hit the keepalive and start a shutdown procedure, so to avoid that you can probably call Increment/DecrementKeepAliveCount() in application_lifetime.h. Another option would be to create a second profile and open a browser there. That would be more authentic, but it has to be done asynchronously since the profile isn't loaded (and it takes a while in debug builds so you might get flaky timeouts). So, comment to say the increment/decrement calls are being used to simulate this.
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/660813002/diff/80001/chrome/browser/jumplist_... File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/660813002/diff/80001/chrome/browser/jumplist_... chrome/browser/jumplist_win.cc:154: MulDiv(jumplist_updater.user_max_items(), kMostVisited, kTotal); On 2014/11/04 23:10:48, tapted wrote: > I think you'll need to update these values - maxing out the most visted/recently > closed will probably omit profiles (and tasks) below when trying to add them as > well. And.. it might get a bit tricky. Probably something like > > // If a user has many profiles, don't clobber all the sites, and if > user_max_items has been set to be very small, don't underflow. > const int kTotalSites = kMostVisited + kRecentlyClosed; > const int kMaxProfiles = 5; > size_t shown_profile_count = std::min(jumplist_updater.user_max_items(), > std::min(kMaxProfiles, profile_switcher.size())); > size_t available_slots = jumplist_updater.user_max_items() - > shown_profile_count; > size_t most_visited_items = MulDiv(available_slots, kMostVisited, kTotalSites); > ... > size_t recently_closed_items = available_slots - most_visited_items; > > It's a tricky algorithm - maybe check with a PM what a good distribution is. > > (note: apparently the default limit is 10, but it's user-configurable - > http://msdn.microsoft.com/en-us/library/windows/desktop/dd378398(v=vs.85).aspx, > there's also a limit of 4 section headers, which I think we've hit now) Annnnnd this went down a crazy alley and now there's a Finch experiment to see if we should replace Most Visited with Profiles (PMs and UI are in favour of this) https://codereview.chromium.org/660813002/diff/80001/chrome/browser/jumplist_... chrome/browser/jumplist_win.cc:180: // Update the "Profiles" category of the JumpList. On 2014/11/04 23:10:48, tapted wrote: > nit: Profiles -> People? > > (and super-nit: should the comment in generated_resources.grd be updated? It's > currently "The name of the sync group in the options dialog when using the new > Profiles UI." but I guess the section on chrome://settings is now the old > Profile UI, and it should probably say something about being used for OS Menu > taskbar switchers.) Done re: the comment. Chrome://settings is still part of the "new" profiles UI. By "old" UI we mean the old avatar button/bubble, and using "Users" instead of "People". There will be a giant cleanup bug when this thing actually lands for more than a release.... https://codereview.chromium.org/660813002/diff/80001/chrome/browser/jumplist_... chrome/browser/jumplist_win.cc:563: } On 2014/11/04 23:10:48, tapted wrote: > This is a bit nitty, but it's typically good to avoid calling "unknown" code > (i.e. calling out to functions in other interfaces) while holding a lock. I'd > probably do something like > > ShellLinkItemList new_profile_switcher; > for (size_t ... ) { > .. > } > { > base::AutoLock auto_lock(list_lock_); > new_profile_switcher.swap(profile_switcher_); > } > > > (doing .swap also means you don't call out to malloc/free while holding the > lock) Done. https://codereview.chromium.org/660813002/diff/80001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/660813002/diff/80001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:749: if (command_line.HasSwitch(switches::kActivateExistingProfileBrowser)) { On 2014/11/04 23:10:48, tapted wrote: > nit: perhaps comment here, something like "If the profile is loaded, it might > also have an existing browser to activate when using the taskbar-activated > profile switcher. In this case, skip regular command line processing, since that > will open a new tab." Done. https://codereview.chromium.org/660813002/diff/80001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/660813002/diff/80001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:302: EXPECT_TRUE(browser()->window()->IsActive()); On 2014/11/04 23:10:48, tapted wrote: > Calls to IsActive need to be in interactive_ui_tests since another test might > steal focus, but it would be good to avoid moving to an interactive test. I > think repeating the test after closing all windows for the profile would make a > compelling test. (but there's a trap: closing all the windows will hit the > keepalive and start a shutdown procedure, so to avoid that you can probably call > Increment/DecrementKeepAliveCount() in application_lifetime.h. > > Another option would be to create a second profile and open a browser there. > That would be more authentic, but it has to be done asynchronously since the > profile isn't loaded (and it takes a while in debug builds so you might get > flaky timeouts). So, comment to say the increment/decrement calls are being used > to simulate this. I've just removed the check to active, and checking that no new browser got opened (which is partly correct). This CL is growing up to be a giant, so I didn't want to block the review on the test. (which I'll work on once I send it to the owners)
noms@chromium.org changed reviewers: + cpu@chromium.org, jwd@chromium.org, rogerta@chromium.org, sky@chromium.org, xiyuan@chromium.org
Hiya, We'd like to run a Finch experiment to see whether a) anyone uses the items in the Windows Jumplist and b) it is safe to replace the Most Visited items with a list of Profiles. This CL does (or at least try to do) all of this. Adding Jumplist related metrics is a little messy, because the items are essentially just shortcuts, so I've added a new command-line parameter that is checked and logged on chrome startup. Please take a look! + jwd: tools/metrics/histograms (and if you don’t mind, double check the histogram-related code in c/b/jumplist_metrics.cc and profile_management_switches.cc just to make sure i’m not doing it wrong). + rogerta: components/signin/core/* and chrome/browser/profiles/* + sky: c/b/ui/views/profiles and c/b/ui/startup + cpu: c/b/jumplist*, c/b/chrome_browser_main.cc + xiyuan: c/b/chromeos/* Thanks!
https://codereview.chromium.org/660813002/diff/160001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/660813002/diff/160001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:753: if (command_line.HasSwitch(switches::kActivateExistingProfileBrowser)) { How do you know there isn't anything else interesting on the command line that needs processing?
https://codereview.chromium.org/660813002/diff/160001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/660813002/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:463: command_line.GetSwitchValueASCII(switches::kWinJumplistAction)); If you want to catch metrics recording for the cases Chrome isn't running, I think this will need to go somewhere else. I'm thinking mainly about the pinned-to-start-menu case (as opposed to pinned-to-taskbar). i.e. like https://drive.google.com/a/google.com/file/d/0Bw8WCbRaSxWbYTlaTVRfNXRWM00/vie... [corp] You probably want this in StartupBrowserCreator::ProcessCmdLineImpl(). Maybe the not-running case is different enough to want to have a separate UMA (or different enum values). Otherwise (if you stay with just one UMA axis), you'll also want to move the activate-existing-browser part back into ProcessCmdLineImpl (from ProcessCommandLineAlreadyRunning), otherwise the logic will bail out before recording UMA. edit: after seeing sky's comment - perhaps moving it back into ProcessCmdLineImpl is better anyway :) https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... File chrome/browser/jumplist_metrics.cc (right): https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... chrome/browser/jumplist_metrics.cc:5: #include "chrome/browser/jumplist_metrics.h" Name this .cc file (and header) jumplist_metrics_win? This will just stop it getting compiled/linked on non-Windows (but the linker should cull it regardless) https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... chrome/browser/jumplist_metrics.cc:9: // static nit: usually this just goes before static member function definitions (since data member definitions are always static). https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... File chrome/browser/jumplist_metrics.h (right): https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... chrome/browser/jumplist_metrics.h:5: #ifndef CHROME_BROWSER_JUMPLIST_METRICS_H_ this might be better in a subfolder of chrome/browser, but I'm not sure what that would be. I see why you didn't immediately go for c/b/profiles, but I'm leaning towards that as marginally better anyway. Other options could be c/b/metrics or c/b/ui/startup or a new folder (c/b/shell_ingegration?), I think I like c/b/profiles the best, but an OWNER might have a better idea. https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... chrome/browser/jumplist_metrics.h:10: class JumplistMetrics { nit: needs a comment, or (probably better) drop the class, since it's just bundling statics which the style guide doesn't like. (although a class is consistent with c/b/profiles/profile_metrics.h, but not with others like c/b/diagnostics/diagnostics_metrics.h) https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... chrome/browser/jumplist_win.cc:150: bool use_profiles_category = switches::HasProfilesJumplistExperiment(); This might hurt.. but I don't think it's safe to call HasProfilesJumplistExperiment() here. it calls FieldTrialList which doesn't have locking and a comment at the top of field_trial.h says "All code is called exclusively on the UI thread currently." https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... chrome/browser/jumplist_win.cc:162: jumplist_updater.user_max_items() - profiles_or_most_visited_items; this could underflow if the user had set the limit to be smaller than the default - something like `profiles_or_most_visited_item = std::min(profiles_or_most_visited_item, jumplist_updater.user_max_items());` beforehand (or merging that into the initial assignment) should fix it https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... chrome/browser/jumplist_win.cc:595: profile_switcher_.clear(); nit: this clear isn't needed (these will get freed when the function returns and `new_profile_switcher` goes out of scope)
https://codereview.chromium.org/660813002/diff/160001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/660813002/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:463: command_line.GetSwitchValueASCII(switches::kWinJumplistAction)); On 2014/11/10 23:47:41, tapted wrote: > If you want to catch metrics recording for the cases Chrome isn't running, I > think this will need to go somewhere else. I'm thinking mainly about the > pinned-to-start-menu case (as opposed to pinned-to-taskbar). i.e. like > https://drive.google.com/a/google.com/file/d/0Bw8WCbRaSxWbYTlaTVRfNXRWM00/vie... > [corp] > > You probably want this in StartupBrowserCreator::ProcessCmdLineImpl(). Maybe the > not-running case is different enough to want to have a separate UMA (or > different enum values). Otherwise (if you stay with just one UMA axis), you'll > also want to move the activate-existing-browser part back into > ProcessCmdLineImpl (from ProcessCommandLineAlreadyRunning), otherwise the logic > will bail out before recording UMA. > > edit: after seeing sky's comment - perhaps moving it back into > ProcessCmdLineImpl is better anyway :) Ahh, I misunderstood when this was called. I was under the impression it was called every time you open a new browser, but I don't think that's true. Done! https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... File chrome/browser/jumplist_metrics.cc (right): https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... chrome/browser/jumplist_metrics.cc:5: #include "chrome/browser/jumplist_metrics.h" On 2014/11/10 23:47:41, tapted wrote: > Name this .cc file (and header) jumplist_metrics_win? This will just stop it > getting compiled/linked on non-Windows (but the linker should cull it > regardless) Done. https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... chrome/browser/jumplist_metrics.cc:9: // static On 2014/11/10 23:47:41, tapted wrote: > nit: usually this just goes before static member function definitions (since > data member definitions are always static). Done. https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... File chrome/browser/jumplist_metrics.h (right): https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... chrome/browser/jumplist_metrics.h:5: #ifndef CHROME_BROWSER_JUMPLIST_METRICS_H_ On 2014/11/10 23:47:41, tapted wrote: > this might be better in a subfolder of chrome/browser, but I'm not sure what > that would be. I see why you didn't immediately go for c/b/profiles, but I'm > leaning towards that as marginally better anyway. > > Other options could be c/b/metrics or c/b/ui/startup or a new folder > (c/b/shell_ingegration?), I think I like c/b/profiles the best, but an OWNER > might have a better idea. I'm an owner of profiles :) The reason why I didn't put it in profiles is that it isn't really profile related. I've added logging that is useful even if the Finch experiment doesn't pan out. I think I like c/b/metrics the best, so I did that. https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... chrome/browser/jumplist_metrics.h:10: class JumplistMetrics { On 2014/11/10 23:47:41, tapted wrote: > nit: needs a comment, or (probably better) drop the class, since it's just > bundling statics which the style guide doesn't like. (although a class is > consistent with c/b/profiles/profile_metrics.h, but not with others like > c/b/diagnostics/diagnostics_metrics.h) Done. I've added a namespace. I hope that's not worse :/ https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... chrome/browser/jumplist_win.cc:150: bool use_profiles_category = switches::HasProfilesJumplistExperiment(); On 2014/11/10 23:47:41, tapted wrote: > This might hurt.. but I don't think it's safe to call > HasProfilesJumplistExperiment() here. it calls FieldTrialList which doesn't have > locking and a comment at the top of field_trial.h says "All code is called > exclusively on the UI thread currently." Done. Moved it to the constructor (it only needs to be checked once anyway). https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... chrome/browser/jumplist_win.cc:162: jumplist_updater.user_max_items() - profiles_or_most_visited_items; On 2014/11/10 23:47:41, tapted wrote: > this could underflow if the user had set the limit to be smaller than the > default - something like `profiles_or_most_visited_item = > std::min(profiles_or_most_visited_item, jumplist_updater.user_max_items());` > beforehand (or merging that into the initial assignment) should fix it Done. https://codereview.chromium.org/660813002/diff/160001/chrome/browser/jumplist... chrome/browser/jumplist_win.cc:595: profile_switcher_.clear(); On 2014/11/10 23:47:41, tapted wrote: > nit: this clear isn't needed (these will get freed when the function returns and > `new_profile_switcher` goes out of scope) Done. https://codereview.chromium.org/660813002/diff/160001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/660813002/diff/160001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:753: if (command_line.HasSwitch(switches::kActivateExistingProfileBrowser)) { On 2014/11/10 23:28:25, sky wrote: > How do you know there isn't anything else interesting on the command line that > needs processing? This was originally in ProcessCmdLineImpl, but tapted@ suggested i moved it here. Moved it back to ProcessCmdLineImpl.
some questions. This is a finch experiment, so how do we measure success? The users are expecting something in the jumplist and we are showing them something else. what do we measure to decide this new thing is better or they are better off with the recently closed or whathave you? second one is what happens if chrome is set to start in metro mode and you use the jumplist way to start in a new profile?
and a request. can you add a screenshot in the bug please?
On 2014/11/12 20:57:41, cpu wrote: > and a request. can you add a screenshot in the bug please? There's a screenshot in the CL description, but I'll add it to the bug too :)
> This is a finch experiment, so how do we measure success? The > users are expecting something in the jumplist and we are showing them something > else. what do we measure to decide this new thing is better or they are better > off with the recently closed or whathave you? At the moment I couldn't find anyone to know or care that "most visited" is even in the jumplist (including the most likely team). The theory behind being ok with replacing "most visited" with profiles (spoke to John and Glen, I'm not cowboy-ing this one) is that "most visited" is one click away on the NTP, whereas profiles and "recently closed" are two+ clicks away. In any case, I have added histograms to check how often users actually click on items in the jumplist, and we're going to roll out the experiment 50/50 in dev. One of three things can happen: a) we will be wrong, and we'll notice that the people actually click on "most visited" urls (and will probably file an angry bug), b) people will start clicking on the profile switcher c) nobody knows about the jumplist and nobody is clicking on anything I think b) and c) are success scenarios in this experiment. In any case, we're not going to roll out to beta/stable if this rings any alarm bells. > second one is what happens if chrome is set to start in metro mode and you use > the jumplist way to start in a new profile? I've tested this in Win 7 immersive mode, and it's not good (it crashes, "Unspecified error"). This also happens for "most visited"/"recently closed" URLs, though, so this CL doesn't make anything worse. In Win 8 metro mode, the jumplist doesn't have "most visited"/"recently closed" (it only has the Tasks category), so I don't think that would change.
https://codereview.chromium.org/660813002/diff/200001/chrome/browser/metrics/... File chrome/browser/metrics/jumplist_metrics_win.h (right): https://codereview.chromium.org/660813002/diff/200001/chrome/browser/metrics/... chrome/browser/metrics/jumplist_metrics_win.h:12: // Enum for counting which category was clicked. Add a comment that these are used with a histogram and should be futzed with. Something like https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics... https://codereview.chromium.org/660813002/diff/200001/chrome/browser/metrics/... chrome/browser/metrics/jumplist_metrics_win.h:26: void LogJumplistActionFromSwitchValue(const std::string& value); Add a comment please, mention histograms. https://codereview.chromium.org/660813002/diff/200001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/660813002/diff/200001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56116: + <int value="0" label="Recently Closed URL"/> Is it a url or does it open the tab? https://codereview.chromium.org/660813002/diff/200001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56117: + <int value="1" label="Most Visited URL"/> Maybe most visited site?
jumplist code lgtm https://codereview.chromium.org/660813002/diff/200001/chrome/browser/jumplist... File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/660813002/diff/200001/chrome/browser/jumplist... chrome/browser/jumplist_win.cc:266: DCHECK(profile_manager); move this to CHECK() or don't check at all. Note that in line 268 it will crash in release anyways if it is zero.
general review lgtm . As discussed, I'm pretty sure the code in browser_frame_ashwin.cc has you covered for the corner case of chrome-in-metro-with-browser-open user-on-desktop-right-clicks-pinned-taskbar-shortcut (chrome-should-switch-to-metro), but it's something to check. It might be different whether or not a browser with the target profile is active already in Metro mode. I think it's orthogonal enough to do in a follow-up in any case, since there might be related issues it touches. https://codereview.chromium.org/660813002/diff/180001/chrome/browser/jumplist... File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/660813002/diff/180001/chrome/browser/jumplist... chrome/browser/jumplist_win.cc:226: weak_ptr_factory_(this) { nit: initialize use_profiles_category_ . (it's set below but things like this are hard to spot in later refactoring)
https://codereview.chromium.org/660813002/diff/200001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/660813002/diff/200001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:639: command_line.GetSwitchValueASCII(switches::kWinJumplistAction)); Should the command line switch get removed so that if we relaunch (say about:flags or there is an update and you choose restart) doesn't result in counting the switch again?
c/b/chromeos/* LGTM
lgtm profile_management_switches.h|cc https://codereview.chromium.org/660813002/diff/200001/components/signin/core/... File components/signin/core/common/profile_management_switches.cc (right): https://codereview.chromium.org/660813002/diff/200001/components/signin/core/... components/signin/core/common/profile_management_switches.cc:198: return group_name == "Use-Profiles"; Maybe remove '-' from trial and group names?
lgtm for avatar_menu refactor
https://codereview.chromium.org/660813002/diff/180001/chrome/browser/jumplist... File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/660813002/diff/180001/chrome/browser/jumplist... chrome/browser/jumplist_win.cc:226: weak_ptr_factory_(this) { On 2014/11/12 23:32:49, tapted wrote: > nit: initialize use_profiles_category_ . (it's set below but things like this > are hard to spot in later refactoring) Done. https://codereview.chromium.org/660813002/diff/200001/chrome/browser/jumplist... File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/660813002/diff/200001/chrome/browser/jumplist... chrome/browser/jumplist_win.cc:266: DCHECK(profile_manager); On 2014/11/12 23:04:57, cpu wrote: > move this to CHECK() or don't check at all. Note that in line 268 it will crash > in release anyways if it is zero. Done. https://codereview.chromium.org/660813002/diff/200001/chrome/browser/metrics/... File chrome/browser/metrics/jumplist_metrics_win.h (right): https://codereview.chromium.org/660813002/diff/200001/chrome/browser/metrics/... chrome/browser/metrics/jumplist_metrics_win.h:12: // Enum for counting which category was clicked. On 2014/11/12 21:55:41, Jesse Doherty wrote: > Add a comment that these are used with a histogram and should be futzed with. > Something like > https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics... Done. https://codereview.chromium.org/660813002/diff/200001/chrome/browser/metrics/... chrome/browser/metrics/jumplist_metrics_win.h:26: void LogJumplistActionFromSwitchValue(const std::string& value); On 2014/11/12 21:55:41, Jesse Doherty wrote: > Add a comment please, mention histograms. Done. https://codereview.chromium.org/660813002/diff/200001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/660813002/diff/200001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:639: command_line.GetSwitchValueASCII(switches::kWinJumplistAction)); On 2014/11/13 00:17:11, sky wrote: > Should the command line switch get removed so that if we relaunch (say > about:flags or there is an update and you choose restart) doesn't result in > counting the switch again? Ah. I found where to do it correctly -- there's a switches blacklist in switch_utils, so that it never gets copied on a restart. Added both of the new flags there. Thanks! https://codereview.chromium.org/660813002/diff/200001/components/signin/core/... File components/signin/core/common/profile_management_switches.cc (right): https://codereview.chromium.org/660813002/diff/200001/components/signin/core/... components/signin/core/common/profile_management_switches.cc:198: return group_name == "Use-Profiles"; On 2014/11/13 14:54:19, Roger Tawa wrote: > Maybe remove '-' from trial and group names? Done. Also moved it to jumplist_win.cc because it's the only one using it, and it didn't really belong in the switches namespace. https://codereview.chromium.org/660813002/diff/200001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/660813002/diff/200001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56116: + <int value="0" label="Recently Closed URL"/> On 2014/11/12 21:55:41, Jesse Doherty wrote: > Is it a url or does it open the tab? It's a url that opens a tab. https://codereview.chromium.org/660813002/diff/200001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56117: + <int value="1" label="Most Visited URL"/> On 2014/11/12 21:55:42, Jesse Doherty wrote: > Maybe most visited site? Done. Renamed them to actually match the category names in the jumplist so that it's less confusing.
LGTM https://codereview.chromium.org/660813002/diff/240001/chrome/common/switch_ut... File chrome/common/switch_utils.cc (right): https://codereview.chromium.org/660813002/diff/240001/chrome/common/switch_ut... chrome/common/switch_utils.cc:24: switches::kWinJumplistAction, nit: sort this list.
Patchset #12 (id:280001) has been deleted
Yay! Thanks Scott! https://codereview.chromium.org/660813002/diff/240001/chrome/common/switch_ut... File chrome/common/switch_utils.cc (right): https://codereview.chromium.org/660813002/diff/240001/chrome/common/switch_ut... chrome/common/switch_utils.cc:24: switches::kWinJumplistAction, On 2014/11/13 17:17:18, sky wrote: > nit: sort this list. Eeeek. Done!
lgtm
Yay, thanks everyone for playing "Chromium's worst CL"! Sending this to the CQ :)
The CQ bit was checked by noms@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/660813002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by noms@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/660813002/300001
Message was sent while issue was closed.
Committed patchset #12 (id:300001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/f9b8e2af097ad14a094214087eda989bd8fc8f3a Cr-Commit-Position: refs/heads/master@{#304078} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
