Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2850153002:
Disable Open in Incognito context item for urls disallowed in Incognito (Closed)
DescriptionDisable Open in Incognito menu item for urls disallowed in Incognito
Some URLs are not permitted to load in Incognito mode, instead opening back in the regular browser window, making the "Open Link in Incognito Window" context menu link misleading.
To address this, disable the menu command for links that are disallowed in Incognito mode.
BUG=682163
Review-Url: https://codereview.chromium.org/2850153002
Cr-Commit-Position: refs/heads/master@{#468958}
Committed: https://chromium.googlesource.com/chromium/src/+/b60fcce6235c4ff9a4a2fbdd18e0d36f29879d4f
Patch Set 1 : Use IsURLAllowedInIncognito function to control link #
Total comments: 4
Patch Set 2 : Address feedback #
Messages
Total messages: 21 (16 generated)
The CQ bit was checked by elawrence@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Disable Open Link in Incognito Window context menu link for chrome urls Chrome URLs often do not load in Incognito mode, instead opening back in the regular browser window, making the "Open Link in Incognito Window" context menu link misleading. To address this, disable the menu command for links that target the chrome:// url scheme. BUG=682163 ========== to ========== Disable Open in Incognito menu item for urls disallowed in Incognito Some URLs are not permitted to load in Incognito mode, instead opening back in the regular browser window, making the "Open Link in Incognito Window" context menu link misleading. To address this, disable the menu command for links that are disallowed in Incognito mode. BUG=682163 ==========
elawrence@chromium.org changed reviewers: + lazyboy@chromium.org
PTAL?
The CQ bit was checked by elawrence@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2850153002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2850153002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:2158: if (!chrome::IsURLAllowedInIncognito(params_.link_url, browser_context_)) { nit: drop {} https://codereview.chromium.org/2850153002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc (right): https://codereview.chromium.org/2850153002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc:398: return ::CreateContextMenuOnChromeLink(web_contents(), registry_.get()); Is there a reason not to put the implementation right here and extract it to a separate function in line 101 above? If not then we should just create the menu instance right here.
https://codereview.chromium.org/2850153002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2850153002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:2158: if (!chrome::IsURLAllowedInIncognito(params_.link_url, browser_context_)) { On 2017/05/02 23:44:55, lazyboy wrote: > nit: drop {} Done. https://codereview.chromium.org/2850153002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc (right): https://codereview.chromium.org/2850153002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc:398: return ::CreateContextMenuOnChromeLink(web_contents(), registry_.get()); On 2017/05/02 23:44:55, lazyboy wrote: > Is there a reason not to put the implementation right here and extract it to a > separate function in line 101 above? > If not then we should just create the menu instance right here. Done. I was mimic'ing the pattern of CreateContextMenu(w,r) above, but I see now that the existing code was written that way because it's used in two different test fixtures.
The CQ bit was checked by elawrence@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2850153002/#ps60001 (title: "Address feedback")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493816672950530,
"parent_rev": "d23690e9d836c08d1122bd1b3b022429843ae3d9", "commit_rev":
"b60fcce6235c4ff9a4a2fbdd18e0d36f29879d4f"}
Message was sent while issue was closed.
Description was changed from ========== Disable Open in Incognito menu item for urls disallowed in Incognito Some URLs are not permitted to load in Incognito mode, instead opening back in the regular browser window, making the "Open Link in Incognito Window" context menu link misleading. To address this, disable the menu command for links that are disallowed in Incognito mode. BUG=682163 ========== to ========== Disable Open in Incognito menu item for urls disallowed in Incognito Some URLs are not permitted to load in Incognito mode, instead opening back in the regular browser window, making the "Open Link in Incognito Window" context menu link misleading. To address this, disable the menu command for links that are disallowed in Incognito mode. BUG=682163 Review-Url: https://codereview.chromium.org/2850153002 Cr-Commit-Position: refs/heads/master@{#468958} Committed: https://chromium.googlesource.com/chromium/src/+/b60fcce6235c4ff9a4a2fbdd18e0... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b60fcce6235c4ff9a4a2fbdd18e0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
