[Win] Add a fast profile switcher to the Windows taskbar item.

[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}

[noms (inactive), 2014-11-03 19:49:10]

Patchset #1 (id:1) has been deleted

[noms (inactive), 2014-11-03 19:49:18]

Patchset #1 (id:20001) has been deleted

[noms (inactive), 2014-11-03 20:08:12]

noms@chromium.org changed reviewers:
+ tapted@chromium.org

[noms (inactive), 2014-11-03 20:08:13]

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!

[tapted, 2014-11-03 23:57:06]

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 ?

[tapted, 2014-11-03 23:58:29]

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" :)

[noms (inactive), 2014-11-04 20:03:09]

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!

[tapted, 2014-11-04 23:10:48]

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.

[noms (inactive), 2014-11-10 21:23:21]

Patchset #5 (id:120001) has been deleted

[noms (inactive), 2014-11-10 21:46:08]

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 (inactive), 2014-11-10 22:05:12]

noms@chromium.org changed reviewers:
+ cpu@chromium.org, jwd@chromium.org, rogerta@chromium.org, sky@chromium.org,
xiyuan@chromium.org

[noms (inactive), 2014-11-10 22:05:13]

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!

[sky, 2014-11-10 23:28:25]

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?

[tapted, 2014-11-10 23:47:42]

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)

[noms (inactive), 2014-11-12 19:04:11]

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.

[cpu_(ooo_6.6-7.5), 2014-11-12 20:56:28]

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?

[cpu_(ooo_6.6-7.5), 2014-11-12 20:57:41]

and a request. can you add a screenshot in the bug please?

[noms (inactive), 2014-11-12 20:58:21]

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 :)

[noms (inactive), 2014-11-12 21:45:24]

> 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.

[jwd, 2014-11-12 21:55:42]

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?

[cpu_(ooo_6.6-7.5), 2014-11-12 23:04:58]

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.

[tapted, 2014-11-12 23:32:49]

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)

[sky, 2014-11-13 00:17:11]

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?

[xiyuan, 2014-11-13 00:37:03]

c/b/chromeos/* LGTM

[Roger Tawa OOO till Jul 10th, 2014-11-13 14:54:19]

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?

[Roger Tawa OOO till Jul 10th, 2014-11-13 15:02:58]

lgtm for avatar_menu refactor

[noms (inactive), 2014-11-13 16:59:55]

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.

[sky, 2014-11-13 17:17:18]

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.

[noms (inactive), 2014-11-13 17:45:56]

Patchset #12 (id:280001) has been deleted

[noms (inactive), 2014-11-13 17:46:38]

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!

[jwd, 2014-11-13 18:31:53]

lgtm

[noms (inactive), 2014-11-13 19:25:28]

Yay, thanks everyone for playing "Chromium's worst CL"! Sending this to the CQ
:)

[noms (inactive), 2014-11-13 19:25:55]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-11-13 19:27:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/660813002/260001

[commit-bot: I haz the power, 2014-11-13 19:39:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-13 19:39:30]

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_...)

[noms (inactive), 2014-11-13 19:57:28]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-11-13 19:58:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/660813002/300001

[commit-bot: I haz the power, 2014-11-13 20:58:04]

Committed patchset #12 (id:300001)

[commit-bot: I haz the power, 2014-11-13 20:58:46]

Patchset 12 (id:??) landed as
https://crrev.com/f9b8e2af097ad14a094214087eda989bd8fc8f3a
Cr-Commit-Position: refs/heads/master@{#304078}