Only show the open link as user context menu if at least one of the profiles is open

chrome/browser/renderer_context_menu/render_view_context_menu.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/renderer_context_menu/render_view_context_menu.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <set>
@@ skipping 987 matching lines @@
         }
         UMA_HISTOGRAM_ENUMERATION(
             "RenderViewContextMenu.OpenLinkAsUserProfilesState", profiles_state,
             OPEN_LINK_AS_USER_PROFILES_STATE_LAST_ENUM_ID);
 
         UMA_HISTOGRAM_ENUMERATION("RenderViewContextMenu.OpenLinkAsUserShown",
                                   target_profiles_entries.size(),
                                   kOpenLinkAsUserMaxProfilesReported);
       }
 
-      if (target_profiles_entries.size() == 1u) {
-        int menu_index = static_cast<int>(profile_link_paths_.size());
-        ProfileAttributesEntry* entry = target_profiles_entries.front();
-        profile_link_paths_.push_back(entry->GetPath());
-        menu_model_.AddItem(
-            IDC_OPEN_LINK_IN_PROFILE_FIRST + menu_index,
-            l10n_util::GetStringFUTF16(IDS_CONTENT_CONTEXT_OPENLINKINPROFILE,
-                                       entry->GetName()));
-        AddAvatarToLastMenuItem(entry->GetAvatarIcon(), &menu_model_);
-      } else if (target_profiles_entries.size() > 1u) {
-        for (ProfileAttributesEntry* entry : target_profiles_entries) {
+      if (multiple_profiles_open_) {

[Peter Kasting, 2016/06/22 22:53:21]

Nit: If you change this to "if (multiple_profiles_open_ &&
!target_profiles_entries.empty())", you can omit the conditional on the "else"
below (and the code reads a little clearer to me).

[jochen (gone - plz use gerrit), 2016/06/23 09:21:07]

the idea is that you need to have at least one additional profile open - if not,
we don't show anything.

So even if you have many additional profiles, but only your main profile is
open, you won't get the context menu.

I guess that also explains the confusion about the CL description?

[Peter Kasting, 2016/06/23 09:26:13]

I don't understand how this reply has anything to do with my suggestion, which
doesn't modify the functionality of your CL at all, it just makes the same thing
(IMO) clearer.

[Peter Kasting, 2016/06/23 09:39:02]

Please reply on the comment thread in question next time and not with the
general "mail comments" box, so that the whole discussion context stays in one
place, where the code is visible simultaneously...

You're misunderstanding my suggestion.  I'm asking for this:

  if (multiple_profiles_open_ && !target_profiles_entries.empty()) {
    if (target_profiles_entries.size() == 1u) {
      ...
    } else {  // No conditional here anymore, it got moved
      ...
    }
  }

This makes it clear right at the beginning of the loop that we only care about
cases when multiple profiles are open and we have at least one target profile.

[jochen (gone - plz use gerrit), 2016/06/23 09:43:33]

ah, thanks for clarifying. updated the CL.

+        if (target_profiles_entries.size() == 1u) {

[Peter Kasting, 2016/06/22 22:53:21]

Nit: Remove 'u' suffix (2 places)

           int menu_index = static_cast<int>(profile_link_paths_.size());
-          // In extreme cases, we might have more profiles than available
-          // command ids. In that case, just stop creating new entries - the
-          // menu is probably useless at this point already.
-          if (IDC_OPEN_LINK_IN_PROFILE_FIRST + menu_index >
-              IDC_OPEN_LINK_IN_PROFILE_LAST) {
-            break;
+          ProfileAttributesEntry* entry = target_profiles_entries.front();
+          profile_link_paths_.push_back(entry->GetPath());
+          menu_model_.AddItem(
+              IDC_OPEN_LINK_IN_PROFILE_FIRST + menu_index,
+              l10n_util::GetStringFUTF16(IDS_CONTENT_CONTEXT_OPENLINKINPROFILE,
+                                         entry->GetName()));
+          AddAvatarToLastMenuItem(entry->GetAvatarIcon(), &menu_model_);
+        } else if (target_profiles_entries.size() > 1u) {
+          for (ProfileAttributesEntry* entry : target_profiles_entries) {
+            int menu_index = static_cast<int>(profile_link_paths_.size());
+            // In extreme cases, we might have more profiles than available
+            // command ids. In that case, just stop creating new entries - the
+            // menu is probably useless at this point already.
+            if (IDC_OPEN_LINK_IN_PROFILE_FIRST + menu_index >
+                IDC_OPEN_LINK_IN_PROFILE_LAST) {
+              break;
+            }
+            profile_link_paths_.push_back(entry->GetPath());
+            profile_link_submenu_model_.AddItem(
+                IDC_OPEN_LINK_IN_PROFILE_FIRST + menu_index, entry->GetName());
+            AddAvatarToLastMenuItem(entry->GetAvatarIcon(),
+                                    &profile_link_submenu_model_);
           }
-          profile_link_paths_.push_back(entry->GetPath());
-          profile_link_submenu_model_.AddItem(
-              IDC_OPEN_LINK_IN_PROFILE_FIRST + menu_index, entry->GetName());
-          AddAvatarToLastMenuItem(entry->GetAvatarIcon(),
-                                  &profile_link_submenu_model_);
+          menu_model_.AddSubMenuWithStringId(
+              IDC_CONTENT_CONTEXT_OPENLINKINPROFILE,
+              IDS_CONTENT_CONTEXT_OPENLINKINPROFILES,
+              &profile_link_submenu_model_);
         }
-        menu_model_.AddSubMenuWithStringId(
-            IDC_CONTENT_CONTEXT_OPENLINKINPROFILE,
-            IDS_CONTENT_CONTEXT_OPENLINKINPROFILES,
-            &profile_link_submenu_model_);
       }
     }
 #endif  // !defined(OS_CHROMEOS)
     menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
     menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_SAVELINKAS,
                                     IDS_CONTENT_CONTEXT_SAVELINKAS);
   }
 
   menu_model_.AddItemWithStringId(
       IDC_CONTENT_CONTEXT_COPYLINKLOCATION,
@@ skipping 1343 matching lines @@
   source_web_contents_->GetRenderViewHost()->
       ExecuteMediaPlayerActionAtLocation(location, action);
 }
 
 void RenderViewContextMenu::PluginActionAt(
     const gfx::Point& location,
     const WebPluginAction& action) {
   source_web_contents_->GetRenderViewHost()->
       ExecutePluginActionAtLocation(location, action);
 }