Implement "open link as user" context menu entry

Implement "open link as user" context menu entry

The menu entry is only present if multiple profiles are defined, and
only displays those that currently have open windows.

If only a single other profile has an open window, no sub-menu is
created

BUG=103073
R=pkasting@chromium.org
TEST=ContextMenuBrowserTest.OpenLinkInProfile*

Committed: https://crrev.com/3917ff9e77c4da3d2d56398cebdc52312b2b63d3
Cr-Commit-Position: refs/heads/master@{#356023}

[Peter Kasting, 2015-06-15 23:45:15]

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:746: //
g_browser_process->profile_manager() is NULL during unit tests.
Is null, or may be null?  (i.e. Is it ever non-null?)

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:750:
ProfileInfoCache& profile_info_cache =
I believe that if you mark ProfileInfoCache::GetAvatarIconOfProfileAtIndex() as
a const function, this can become a const ref.  (I think that function is, in
fact, const, so this should be possible.)

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:753:
std::vector<Profile*> target_profiles;
Nit: Maybe above this block say something like "Find all regular profiles other
than the current one which have at least one open window."

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:756:
chrome::FindBrowserWithWebContents(source_web_contents_)
See null-check question I have about this in ExecuteCommand() below.

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:759: it !=
all_profiles.end(); ++it) {
Nit: Use range-based for

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:760: if
((*it)->GetProfileType() != Profile::REGULAR_PROFILE)
Nit: Shorter:

        if ((profile->GetProfileType() == Profile::REGULAR_PROFILE) &&
            (profile != GetProfile() &&
            chrome::FindLastActiveWithProfile(profile, desktop_type))
          target_profiles.push_back(profile);

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:781: } else if
(target_profiles.size() > 1) {
Can this conditional ever fail?

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:783: it !=
target_profiles.end(); ++it) {
Nit: Use range-based for

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1509:
chrome::FindBrowserWithWebContents(source_web_contents_)
The code below for IDC_CONTENT_CONTEXT_OPENLINKNEWTAB null-checks the result of
this call.  Does this code need to do that?  It seems like these need to be the
same, though I'm not precisely sure how this could return null in either case?

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:411:
// With two profiles, we don't show a submenu.
This comment is the same as the one below, but the code in the two places is
different.

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu_test_util.cc
(right):

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_test_util.cc:47:
DCHECK_LT(command_id_first, command_id_last);
Nit: This should probably be _LE, since it's sane (if dumb) to specify equal IDs
for the two args.

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h
(right):

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h:44: //
Returns true if command specified by |command_id| is present
Nit: While here: if command -> if the command

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h:46: //
List of command ids can be found in chrome/app/chrome_command_ids.h.
Nit: While here: List -> A list

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h:49: //
Returns true if command specified by any command id between
Nit: command -> a command

[jochen (gone - plz use gerrit), 2015-06-16 08:00:14]

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:746: //
g_browser_process->profile_manager() is NULL during unit tests.
On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> Is null, or may be null?  (i.e. Is it ever non-null?)

it's always null during the context menu unit tests.

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:750:
ProfileInfoCache& profile_info_cache =
On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> I believe that if you mark ProfileInfoCache::GetAvatarIconOfProfileAtIndex()
as a const function, this can become a const ref.  (I think that function is, in
fact, const, so this should be possible.)

done

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:753:
std::vector<Profile*> target_profiles;
On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> Nit: Maybe above this block say something like "Find all regular profiles
other than the current one which have at least one open window."

done

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:756:
chrome::FindBrowserWithWebContents(source_web_contents_)
On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> See null-check question I have about this in ExecuteCommand() below.

done

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:759: it !=
all_profiles.end(); ++it) {
On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> Nit: Use range-based for

done

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:760: if
((*it)->GetProfileType() != Profile::REGULAR_PROFILE)
On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> Nit: Shorter:
> 
>         if ((profile->GetProfileType() == Profile::REGULAR_PROFILE) &&
>             (profile != GetProfile() &&
>             chrome::FindLastActiveWithProfile(profile, desktop_type))
>           target_profiles.push_back(profile);

done

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:781: } else if
(target_profiles.size() > 1) {
On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> Can this conditional ever fail?

there could be no target_profile (since target_profile does not include the
current profile)

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:783: it !=
target_profiles.end(); ++it) {
On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> Nit: Use range-based for

done

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1509:
chrome::FindBrowserWithWebContents(source_web_contents_)
On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> The code below for IDC_CONTENT_CONTEXT_OPENLINKNEWTAB null-checks the result
of this call.  Does this code need to do that?  It seems like these need to be
the same, though I'm not precisely sure how this could return null in either
case?

fair point. I'm getting the desktop_top from the native view then

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:411:
// With two profiles, we don't show a submenu.
On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> This comment is the same as the one below, but the code in the two places is
different.

done

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu_test_util.cc
(right):

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_test_util.cc:47:
DCHECK_LT(command_id_first, command_id_last);
On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> Nit: This should probably be _LE, since it's sane (if dumb) to specify equal
IDs for the two args.

done

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h
(right):

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h:46: //
List of command ids can be found in chrome/app/chrome_command_ids.h.
On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> Nit: While here: List -> A list

done

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h:49: //
Returns true if command specified by any command id between
On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> Nit: command -> a command

done

[Peter Kasting, 2015-06-16 08:20:24]

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:746: //
g_browser_process->profile_manager() is NULL during unit tests.
On 2015/06/16 08:00:14, jochen wrote:
> On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> > Is null, or may be null?  (i.e. Is it ever non-null?)
> 
> it's always null during the context menu unit tests.

OK.  (Prefer "null" to "NULL", though.)

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:781: } else if
(target_profiles.size() > 1) {
On 2015/06/16 08:00:13, jochen wrote:
> On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> > Can this conditional ever fail?
> 
> there could be no target_profile (since target_profile does not include the
> current profile)

OK; how about "} else if (!target_profiles.empty()) {" then.

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:411:
// With two profiles, we don't show a submenu.
On 2015/06/16 08:00:14, jochen wrote:
> On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> > This comment is the same as the one below, but the code in the two places is
> different.
> 
> done

I'm still confused.  Now the two comments both mention "three profiles" but
describe doing different things.

Honestly, it looks as if the tests are backwards.  Shouldn't the test here, when
there are only main and "profile 1" windows open, ASSERT_TRUE on the
OPENLINKINPROFILE item and false on the range; then the test below, once we've
opened a "profile 2" window, be the reverse?

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h
(right):

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h:44: //
Returns true if command specified by |command_id| is present
On 2015/06/15 23:45:15, Peter Kasting wrote:
> Nit: While here: if command -> if the command

This is still outstanding.

https://codereview.chromium.org/1191453002/diff/60001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1191453002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:763:
chrome::FindLastActiveWithProfile(profile, desktop_type)) {
Nit: {} not necessary

https://codereview.chromium.org/1191453002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:778: for
(Profile* profile: target_profiles) {
Nit: space before colon

[jochen (gone - plz use gerrit), 2015-06-16 09:01:50]

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:746: //
g_browser_process->profile_manager() is NULL during unit tests.
On 2015/06/16 at 08:20:24, Peter Kasting wrote:
> On 2015/06/16 08:00:14, jochen wrote:
> > On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> > > Is null, or may be null?  (i.e. Is it ever non-null?)
> > 
> > it's always null during the context menu unit tests.
> 
> OK.  (Prefer "null" to "NULL", though.)

done

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:781: } else if
(target_profiles.size() > 1) {
On 2015/06/16 at 08:20:24, Peter Kasting wrote:
> On 2015/06/16 08:00:13, jochen wrote:
> > On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> > > Can this conditional ever fail?
> > 
> > there could be no target_profile (since target_profile does not include the
> > current profile)
> 
> OK; how about "} else if (!target_profiles.empty()) {" then.

done

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:411:
// With two profiles, we don't show a submenu.
On 2015/06/16 at 08:20:24, Peter Kasting wrote:
> On 2015/06/16 08:00:14, jochen wrote:
> > On 2015/06/15 at 23:45:15, Peter Kasting wrote:
> > > This comment is the same as the one below, but the code in the two places
is
> > different.
> > 
> > done
> 
> I'm still confused.  Now the two comments both mention "three profiles" but
describe doing different things.
> 
> Honestly, it looks as if the tests are backwards.  Shouldn't the test here,
when there are only main and "profile 1" windows open, ASSERT_TRUE on the
OPENLINKINPROFILE item and false on the range; then the test below, once we've
opened a "profile 2" window, be the reverse?

I shouldn't try to write comments in the early morning...

Tried to clarify what happens.

The test creates three profiles (default + two additional ones) and then
verifies that no open link in profile is created.

Then it opens one window in the second profile, now the inline item should be
displayed.

Then it also opens one window in the third profile, no a submenu should be
displayed.

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h
(right):

https://codereview.chromium.org/1191453002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h:44: //
Returns true if command specified by |command_id| is present
On 2015/06/16 at 08:20:24, Peter Kasting wrote:
> On 2015/06/15 23:45:15, Peter Kasting wrote:
> > Nit: While here: if command -> if the command
> 
> This is still outstanding.

oops, done

https://codereview.chromium.org/1191453002/diff/60001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1191453002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:763:
chrome::FindLastActiveWithProfile(profile, desktop_type)) {
On 2015/06/16 at 08:20:24, Peter Kasting wrote:
> Nit: {} not necessary

done

https://codereview.chromium.org/1191453002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:778: for
(Profile* profile: target_profiles) {
On 2015/06/16 at 08:20:24, Peter Kasting wrote:
> Nit: space before colon

done

[Peter Kasting, 2015-06-16 22:01:48]

LGTM on implementation, obviously UI leads will have to approve on principle

https://codereview.chromium.org/1191453002/diff/80001/chrome/browser/renderer...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/1191453002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:386:
// Create two additional profiles, but do not yep open windows in them.
Nit: yep -> yet

https://codereview.chromium.org/1191453002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:396:
// We only show the menu if the profiles actually have an active browser.
Nit: Maybe: "With only the current profile open, we don't add any items to the
context menu for opening links in other profiles."

[jochen (gone - plz use gerrit), 2015-06-17 07:11:31]

On 2015/06/16 at 22:01:48, pkasting wrote:
> LGTM on implementation, obviously UI leads will have to approve on principle

thx for the review... waiting to hear back from them

> 
>
https://codereview.chromium.org/1191453002/diff/80001/chrome/browser/renderer...
> File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):
> 
>
https://codereview.chromium.org/1191453002/diff/80001/chrome/browser/renderer...
>
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:386:
// Create two additional profiles, but do not yep open windows in them.
> Nit: yep -> yet

done

> 
>
https://codereview.chromium.org/1191453002/diff/80001/chrome/browser/renderer...
>
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:396:
// We only show the menu if the profiles actually have an active browser.
> Nit: Maybe: "With only the current profile open, we don't add any items to the
context menu for opening links in other profiles."

done

[jochen (gone - plz use gerrit), 2015-10-13 17:52:16]

jochen@chromium.org changed reviewers:
+ isherman@chromium.org

[jochen (gone - plz use gerrit), 2015-10-13 17:52:17]

+isherman for histograms.xml

Peter, the CL changed quite a bit since you've reviewed it, ptal

[Peter Kasting, 2015-10-13 18:59:00]

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_metrics.h (right):

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_metrics.h:72: SWITCH_PROFILE_UNLOCK,    // User
switches to lockd profile via User Manager
Nit: While here: locked

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_metrics.h:74: CONTEXT_MENU,             // User
switches profiles from context menu
Nit: For parallel structure should this be SWITCH_PROFILE_CONTEXT_MENU?

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:872:
(target_profiles.size() == 2 && all_target_profiles.size() == 2)) {
This doesn't seem to match the slides.  Unless I'm confused, this implements:

  * If only one profile active, hide menu items.
  * If two profiles active, show top-level menu item.
  * If three profiles active and there are only three total profiles, show
top-level menu item.
  * Otherwise, show submenu.

I don't know why the third bullet is there.

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:873: for
(size_t profile_index : target_profiles) {
In extreme edge cases, this might add more profiles than we have command IDs
reserved.

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:884: for
(size_t profile_index : all_target_profiles) {
I don't think we should do what this implements.

This basically says that at some point we should show targets for all profiles,
even ones that aren't open.  I don't think we should ever do that.  If we should
do that, we should do it all the time.  I really don't think we should switch to
this behavior once we reach some threshold number of open profiles.

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1592:
profile_path, desktop_type, false /* always_create */,
Nit: Omit the param comment here; it actually misled me into thinking you were
doing the opposite of what you're doing, and there aren't multiple booleans here
so there's no need to distinguish them.

[jochen (gone - plz use gerrit), 2015-10-13 19:07:30]

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:872:
(target_profiles.size() == 2 && all_target_profiles.size() == 2)) {
On 2015/10/13 at 18:59:00, Peter Kasting wrote:
> This doesn't seem to match the slides.  Unless I'm confused, this implements:
> 
>   * If only one profile active, hide menu items.
>   * If two profiles active, show top-level menu item.
>   * If three profiles active and there are only three total profiles, show
top-level menu item.

that's step 2 b i) in the slides

>   * Otherwise, show submenu.
> 
> I don't know why the third bullet is there.

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:884: for
(size_t profile_index : all_target_profiles) {
On 2015/10/13 at 18:59:00, Peter Kasting wrote:
> I don't think we should do what this implements.
> 
> This basically says that at some point we should show targets for all
profiles, even ones that aren't open.  I don't think we should ever do that.  If
we should do that, we should do it all the time.  I really don't think we should
switch to this behavior once we reach some threshold number of open profiles.

Alex' argument is that this matches what the user switcher menu will show. If
you only have one additional profile, the desire to not show too many entries
trumps being consistent with the user switcher menu.

[Ilya Sherman, 2015-10-13 19:07:54]

histograms.xml lgtm

[Peter Kasting, 2015-10-13 19:16:39]

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:872:
(target_profiles.size() == 2 && all_target_profiles.size() == 2)) {
On 2015/10/13 19:07:30, jochen wrote:
> On 2015/10/13 at 18:59:00, Peter Kasting wrote:
> > This doesn't seem to match the slides.  Unless I'm confused, this
implements:
> > 
> >   * If only one profile active, hide menu items.
> >   * If two profiles active, show top-level menu item.
> >   * If three profiles active and there are only three total profiles, show
> top-level menu item.
> 
> that's step 2 b i) in the slides
> 
> >   * Otherwise, show submenu.
> > 
> > I don't know why the third bullet is there.

No, it's not 2bi.  2bi is the second bullet I wrote:

  if (target_profiles.size() == 1) ...

The problem is the size 2 case, which deals with having _3_ profiles.  That's
supposed to go in a submenu.

Unless the slides are incredibly confusing and "2 profiles active" and "only 2
profiles" are counting different things (active versus total profiles), in which
case (a) the slides need to be reworded, and (b) this code is wrong anyway
because you don't show both other profiles in the (target_profiles.size() == 1
&& all_target_profiles.size() == 2) case.

I think the rules on this are too complicated.  You should do the algorithm you
originally set out to do:
* Only show profiles from active windows.
* If one such profile, use a top-level slot.  Otherwise use a context menu.

This is simpler to implement, simpler to reason about as a user, and won't spam
you with tons of profile links you don't care about.  I don't find Alex' user
switcher argument compelling unless we want to show all other profiles all the
time.  The inconsistency of sometimes showing them all and sometimes not is a
much bigger disadvantage than the consistency with the switcher in some cases is
an advantage.  If you want to show all other profiles every time there's more
than one total profile currently open, I will support that.

[jochen (gone - plz use gerrit), 2015-10-13 19:31:35]

On 2015/10/13 at 19:16:39, pkasting wrote:
>
https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/rendere...
> File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):
> 
>
https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/rendere...
> chrome/browser/renderer_context_menu/render_view_context_menu.cc:872:
(target_profiles.size() == 2 && all_target_profiles.size() == 2)) {
> On 2015/10/13 19:07:30, jochen wrote:
> > On 2015/10/13 at 18:59:00, Peter Kasting wrote:
> > > This doesn't seem to match the slides.  Unless I'm confused, this
implements:
> > > 
> > >   * If only one profile active, hide menu items.
> > >   * If two profiles active, show top-level menu item.
> > >   * If three profiles active and there are only three total profiles, show
> > top-level menu item.
> > 
> > that's step 2 b i) in the slides
> > 
> > >   * Otherwise, show submenu.
> > > 
> > > I don't know why the third bullet is there.
> 
> No, it's not 2bi.  2bi is the second bullet I wrote:
> 
>   if (target_profiles.size() == 1) ...
> 
> The problem is the size 2 case, which deals with having _3_ profiles.  That's
supposed to go in a submenu.
> 
> Unless the slides are incredibly confusing and "2 profiles active" and "only 2
profiles" are counting different things (active versus total profiles), in which
case (a) the slides need to be reworded, and (b) this code is wrong anyway
because you don't show both other profiles in the (target_profiles.size() == 1
&& all_target_profiles.size() == 2) case.

ah, you're right.
 
> I think the rules on this are too complicated.  You should do the algorithm
you originally set out to do:
> * Only show profiles from active windows.
> * If one such profile, use a top-level slot.  Otherwise use a context menu.
> 
> This is simpler to implement, simpler to reason about as a user, and won't
spam you with tons of profile links you don't care about.  I don't find Alex'
user switcher argument compelling unless we want to show all other profiles all
the time.  The inconsistency of sometimes showing them all and sometimes not is
a much bigger disadvantage than the consistency with the switcher in some cases
is an advantage.  If you want to show all other profiles every time there's more
than one total profile currently open, I will support that.

fair enough, let's wait for the ui discussion

[jochen (gone - plz use gerrit), 2015-10-13 19:50:23]

meanwhile, I addressed all your other comments

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_metrics.h (right):

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_metrics.h:72: SWITCH_PROFILE_UNLOCK,    // User
switches to lockd profile via User Manager
On 2015/10/13 at 18:59:00, Peter Kasting wrote:
> Nit: While here: locked

done

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_metrics.h:74: CONTEXT_MENU,             // User
switches profiles from context menu
On 2015/10/13 at 18:59:00, Peter Kasting wrote:
> Nit: For parallel structure should this be SWITCH_PROFILE_CONTEXT_MENU?

done

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:873: for
(size_t profile_index : target_profiles) {
On 2015/10/13 at 18:59:00, Peter Kasting wrote:
> In extreme edge cases, this might add more profiles than we have command IDs
reserved.

added a bail-out

https://codereview.chromium.org/1191453002/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1592:
profile_path, desktop_type, false /* always_create */,
On 2015/10/13 at 18:59:00, Peter Kasting wrote:
> Nit: Omit the param comment here; it actually misled me into thinking you were
doing the opposite of what you're doing, and there aren't multiple booleans here
so there's no need to distinguish them.

done

[jochen (gone - plz use gerrit), 2015-10-13 19:53:08]

actually, after reading the slides again, I think you're right that it means to
always show all profiles if more than one is active

[jochen (gone - plz use gerrit), 2015-10-13 20:03:13]

updated to show all profiles as soon as more than one profile is active. ptal

[Peter Kasting, 2015-10-13 20:23:54]

LGTM

https://codereview.chromium.org/1191453002/diff/180001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1191453002/diff/180001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:873: size_t
profile_index = target_profiles.at(0);
Nit: target_profiles[0]

YOu can inline this into the next statement too.

https://codereview.chromium.org/1191453002/diff/180001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:882: } else if
(!target_profiles.empty()) {
This condition can never fail.

The best thing to do is probably to replace this with "} else if
(more_than_one_profile_open) {" and remove the outer conditional here entirely.

[jochen (gone - plz use gerrit), 2015-10-20 14:07:23]

https://codereview.chromium.org/1191453002/diff/180001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1191453002/diff/180001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:873: size_t
profile_index = target_profiles.at(0);
On 2015/10/13 at 20:23:53, Peter Kasting wrote:
> Nit: target_profiles[0]
> 
> YOu can inline this into the next statement too.

done

https://codereview.chromium.org/1191453002/diff/180001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:882: } else if
(!target_profiles.empty()) {
On 2015/10/13 at 20:23:53, Peter Kasting wrote:
> This condition can never fail.
> 
> The best thing to do is probably to replace this with "} else if
(more_than_one_profile_open) {" and remove the outer conditional here entirely.

I replaced it with } else { - even if target_profiles.size() == 1, we only want
to show an entry if more than one profile is currently open.

[jochen (gone - plz use gerrit), 2015-10-26 08:41:05]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-10-26 08:41:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1191453002/#ps300001
(title: "updates")

[commit-bot: I haz the power, 2015-10-26 08:41:21]

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

[commit-bot: I haz the power, 2015-10-26 11:19:38]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-10-26 11:20:31]

Patchset 16 (id:??) landed as
https://crrev.com/3917ff9e77c4da3d2d56398cebdc52312b2b63d3
Cr-Commit-Position: refs/heads/master@{#356023}

[marc.herbert, 2017-07-05 18:38:49]

We can't access BUG=103073, is this on purpose?

[jochen (gone - plz use gerrit), 2017-07-05 18:40:35]

On 2017/07/05 at 18:38:49, marc.herbert wrote:
> We can't access BUG=103073, is this on purpose?

yes, it's a launch tracking bug, they're all only viewable by the launch owners.
There's no information in the issue except for that it was approved for launch.

do you need some information?

[marc.herbert, 2017-07-05 18:49:43]

On 2017/07/05 18:40:35, jochen wrote:
> do you need some information?

This feature is awesome and saves us a lot of time. However it's... not enough
:-) I'm just looking for a place to suggest/discuss the "next step": the ability
to *re-open* a tab as another account after it's been opened with the wrong
account. This happens all the time when opening from another application than
the browser itself. Should I just go and open a new BUG or is there already one
maybe? Or did I just not find the existing feature?

[jochen (gone - plz use gerrit), 2017-07-05 18:50:59]

On 2017/07/05 at 18:49:43, marc.herbert wrote:
> On 2017/07/05 18:40:35, jochen wrote:
> > do you need some information?
> 
> This feature is awesome and saves us a lot of time. However it's... not enough
:-) I'm just looking for a place to suggest/discuss the "next step": the ability
to *re-open* a tab as another account after it's been opened with the wrong
account. This happens all the time when opening from another application than
the browser itself. Should I just go and open a new BUG or is there already one
maybe? Or did I just not find the existing feature?

please file a new issue, i'm not aware of an existing feature for this.

[Peter Kasting, 2017-07-05 19:17:56]

On 2017/07/05 18:49:43, marc.herbert wrote:
> On 2017/07/05 18:40:35, jochen wrote:
> > do you need some information?
> 
> This feature is awesome and saves us a lot of time. However it's... not enough
> :-) I'm just looking for a place to suggest/discuss the "next step": the
ability
> to *re-open* a tab as another account after it's been opened with the wrong
> account. This happens all the time when opening from another application than
> the browser itself. Should I just go and open a new BUG or is there already
one
> maybe? Or did I just not find the existing feature?

If you do file, discuss on the bug why it wouldn't be sufficient for your use
case to simply drag the address bar text from one profile window to the other.

[marc.herbert, 2017-07-05 23:07:32]

Filed as https://bugs.chromium.org/p/chromium/issues/detail?id=739546