Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2086333002:
Only show the open link as user context menu if at least one of the profiles is open (Closed)
|
Created:
4 years, 6 months ago by jochen (gone - plz use gerrit) Modified:
4 years, 5 months ago Reviewers:
Peter Kasting CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnly show the open link as user context menu if at least one of the profiles is open
R=pkasting@chromium.org
BUG=623592
Committed: https://crrev.com/1a028fbffb2a321037ff2213b03418447998756d
Cr-Commit-Position: refs/heads/master@{#402201}
Patch Set 1 #
Total comments: 7
Patch Set 2 : updates #
Messages
Total messages: 16 (4 generated)
ptal according to UMA, the click through rate when at least one other profile is open exceeds our goal (equal or more to incognito)
Is there a BUG= for this? The CL description seems to say something different than the code -- I think it should read "at least one" instead of "more than one"? LGTM https://codereview.chromium.org/2086333002/diff/1/chrome/browser/renderer_con... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2086333002/diff/1/chrome/browser/renderer_con... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1008: if (multiple_profiles_open_) { 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). https://codereview.chromium.org/2086333002/diff/1/chrome/browser/renderer_con... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1009: if (target_profiles_entries.size() == 1u) { Nit: Remove 'u' suffix (2 places) https://codereview.chromium.org/2086333002/diff/1/chrome/browser/renderer_con... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/2086333002/diff/1/chrome/browser/renderer_con... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:422: Profile* profile1 = CreateSecondaryProfile(1); Nit: There's no |profile2|, so just call this |profile|.
I'll file a bug and reference it from the CL description https://codereview.chromium.org/2086333002/diff/1/chrome/browser/renderer_con... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2086333002/diff/1/chrome/browser/renderer_con... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1008: if (multiple_profiles_open_) { On 2016/06/22 at 22:53:21, Peter Kasting (OOO) wrote: > 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). 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?
https://codereview.chromium.org/2086333002/diff/1/chrome/browser/renderer_con... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2086333002/diff/1/chrome/browser/renderer_con... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1008: if (multiple_profiles_open_) { On 2016/06/23 09:21:07, jochen wrote: > On 2016/06/22 at 22:53:21, Peter Kasting (OOO) wrote: > > 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). > > 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 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.
On 2016/06/23 at 09:26:13, pkasting wrote: > https://codereview.chromium.org/2086333002/diff/1/chrome/browser/renderer_con... > File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): > > https://codereview.chromium.org/2086333002/diff/1/chrome/browser/renderer_con... > chrome/browser/renderer_context_menu/render_view_context_menu.cc:1008: if (multiple_profiles_open_) { > On 2016/06/23 09:21:07, jochen wrote: > > On 2016/06/22 at 22:53:21, Peter Kasting (OOO) wrote: > > > 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). > > > > 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 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. I'd have to repeat the if (multiple_profiles_open_ && ...) in the else branch as well, as having more than one target profile doesn't imply that multiple profiles are actually open.
https://codereview.chromium.org/2086333002/diff/1/chrome/browser/renderer_con... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2086333002/diff/1/chrome/browser/renderer_con... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1008: if (multiple_profiles_open_) { On 2016/06/23 09:26:13, Peter Kasting (OOO) wrote: > On 2016/06/23 09:21:07, jochen wrote: > > On 2016/06/22 at 22:53:21, Peter Kasting (OOO) wrote: > > > 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). > > > > 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 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. 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.
updated, ptal https://codereview.chromium.org/2086333002/diff/1/chrome/browser/renderer_con... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2086333002/diff/1/chrome/browser/renderer_con... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1008: if (multiple_profiles_open_) { ah, thanks for clarifying. updated the CL.
LGTM, sorry it took so long for such a trivial modification :)
Description was changed from ========== Only show the open link as user context menu if more than one profile is open R=pkasting@chromium.org BUG= ========== to ========== Only show the open link as user context menu if more than one profile is open R=pkasting@chromium.org BUG=623592 ==========
Description was changed from ========== Only show the open link as user context menu if more than one profile is open R=pkasting@chromium.org BUG=623592 ========== to ========== Only show the open link as user context menu if at least one of the profiles is open R=pkasting@chromium.org BUG=623592 ==========
The CQ bit was checked by jochen@chromium.org
updated CL description as proposed and linked to bug
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Only show the open link as user context menu if at least one of the profiles is open R=pkasting@chromium.org BUG=623592 ========== to ========== Only show the open link as user context menu if at least one of the profiles is open R=pkasting@chromium.org BUG=623592 Committed: https://crrev.com/1a028fbffb2a321037ff2213b03418447998756d Cr-Commit-Position: refs/heads/master@{#402201} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1a028fbffb2a321037ff2213b03418447998756d Cr-Commit-Position: refs/heads/master@{#402201} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
