Commit b5f7fba2 authored by Dominic Mazzoni's avatar Dominic Mazzoni Committed by Commit Bot

Revert "Main frame's context menu should exclude frame-specific commands."

This reverts commit cd9df201.

Reason for revert: Failing on Mac, see bug

Original change's description:
> Main frame's context menu should exclude frame-specific commands.
> 
> If content::ContextMenuParams includes a non-empty |frame_url|, then the
> context menu (the menu shown after right clicking in a frame) includes
> frame-specific commands like "View frame source" and/or "Reload frame".
> Before r749036 / 82.0.4084.0, context menu shown for the main frame
> would exclude such frame-specific items.  This behavior has regressed
> in r749036, because it failed to preserve the following logic from
> //third_party/blink/renderer/core/page/context_menu_controller.cc:
> 
>   if (selected_frame != page_->MainFrame())
>     data.frame_url = WebURL(UrlFromFrame(selected_frame));
> 
> and instead started to unconditionally populate the |frame_url| field in
> RenderFrameHostImpl::OnContextMenu.  This CL fixes the regression.
> 
> Bug: 1085040
> Change-Id: I149447654bee858037a4230828e2c4a7b3c7c0bf
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2212995
> Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
> Reviewed-by: Nasko Oskov <nasko@chromium.org>
> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#774333}

TBR=nasko@chromium.org,lazyboy@chromium.org,lukasza@chromium.org,lazyboy@google.com

Change-Id: I39ca90cca707409aede5a46f27f300528b4d5d57
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1085040, 1090564
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2225246Reviewed-by: default avatarDominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774387}
parent 05e34dde
......@@ -80,8 +80,6 @@
#include "net/test/embedded_test_server/http_request.h"
#include "net/test/embedded_test_server/http_response.h"
#include "services/service_manager/public/cpp/interface_provider.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
#include "third_party/blink/public/common/context_menu_data/media_type.h"
#include "third_party/blink/public/common/input/web_input_event.h"
......@@ -894,85 +892,6 @@ IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, SuggestedFileName) {
ASSERT_EQ(kSuggestedFilename, base::UTF16ToUTF8(suggested_filename).c_str());
}
// Check which commands are present after opening the context menu for the main
// frame. This is a regression test for https://crbug.com/1085040.
IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest,
MenuContentsVerification_MainFrame) {
ASSERT_TRUE(embedded_test_server()->Start());
GURL url(embedded_test_server()->GetURL("/iframe.html"));
ui_test_utils::NavigateToURL(browser(), url);
// Open a context menu.
ContextMenuWaiter menu_observer;
blink::WebMouseEvent mouse_event(
blink::WebInputEvent::Type::kMouseDown,
blink::WebInputEvent::kNoModifiers,
blink::WebInputEvent::GetStaticTimeStampForTests());
mouse_event.button = blink::WebMouseEvent::Button::kRight;
mouse_event.SetPositionInWidget(2, 2); // This is over the main frame.
content::WebContents* tab =
browser()->tab_strip_model()->GetActiveWebContents();
tab->GetRenderViewHost()->GetWidget()->ForwardMouseEvent(mouse_event);
mouse_event.SetType(blink::WebInputEvent::Type::kMouseUp);
tab->GetRenderViewHost()->GetWidget()->ForwardMouseEvent(mouse_event);
// Wait for context menu to be visible.
menu_observer.WaitForMenuOpenAndClose();
// Verify that the expected context menu items are present. This also
// verifies that unwanted commands are missing (e.g. that
// IDC_CONTENT_CONTEXT_VIEWFRAMESOURCE is not present when clicking in the
// main frame - see https://crbug.com/1085040).
EXPECT_THAT(menu_observer.GetCapturedCommandIds(),
testing::ElementsAre(IDC_BACK, IDC_FORWARD, IDC_RELOAD,
-1, // separator
IDC_SAVE_PAGE, IDC_PRINT,
IDC_ROUTE_MEDIA, // aka "Cast"
IDC_CONTENT_CONTEXT_TRANSLATE,
-1, // separator
IDC_VIEW_SOURCE,
IDC_CONTENT_CONTEXT_INSPECTELEMENT));
}
// Check which commands are present after opening the context menu for a
// subframe.
IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest,
MenuContentsVerification_Subframe) {
ASSERT_TRUE(embedded_test_server()->Start());
GURL url(embedded_test_server()->GetURL("/iframe.html"));
ui_test_utils::NavigateToURL(browser(), url);
// Open a context menu.
ContextMenuWaiter menu_observer;
blink::WebMouseEvent mouse_event(
blink::WebInputEvent::Type::kMouseDown,
blink::WebInputEvent::kNoModifiers,
blink::WebInputEvent::GetStaticTimeStampForTests());
mouse_event.button = blink::WebMouseEvent::Button::kRight;
mouse_event.SetPositionInWidget(25, 25); // This is over the subframe.
content::WebContents* tab =
browser()->tab_strip_model()->GetActiveWebContents();
tab->GetRenderViewHost()->GetWidget()->ForwardMouseEvent(mouse_event);
mouse_event.SetType(blink::WebInputEvent::Type::kMouseUp);
tab->GetRenderViewHost()->GetWidget()->ForwardMouseEvent(mouse_event);
// Wait for context menu to be visible.
menu_observer.WaitForMenuOpenAndClose();
// Verify that only the expected context menu items are present.
EXPECT_THAT(
menu_observer.GetCapturedCommandIds(),
testing::ElementsAre(IDC_BACK, IDC_FORWARD, IDC_RELOAD,
-1, // separator
IDC_SAVE_PAGE, IDC_PRINT,
IDC_ROUTE_MEDIA, // aka "Cast"
IDC_CONTENT_CONTEXT_TRANSLATE,
-1, // separator
IDC_VIEW_SOURCE, IDC_CONTENT_CONTEXT_VIEWFRAMESOURCE,
IDC_CONTENT_CONTEXT_RELOADFRAME,
IDC_CONTENT_CONTEXT_INSPECTELEMENT));
}
// Check filename on clicking "Save Link As" is ignored for cross origin.
IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, SuggestedFileNameCrossOrigin) {
// Register observer.
......
......@@ -64,18 +64,8 @@ content::ContextMenuParams& ContextMenuWaiter::params() {
return params_;
}
const std::vector<int>& ContextMenuWaiter::GetCapturedCommandIds() const {
return captured_command_ids_;
}
void ContextMenuWaiter::Cancel(RenderViewContextMenu* context_menu) {
params_ = context_menu->params();
const ui::SimpleMenuModel& menu_model = context_menu->menu_model();
captured_command_ids_.reserve(menu_model.GetItemCount());
for (int i = 0; i < menu_model.GetItemCount(); ++i)
captured_command_ids_.push_back(menu_model.GetCommandIdAt(i));
if (maybe_command_to_execute_)
context_menu->ExecuteCommand(*maybe_command_to_execute_, 0);
context_menu->Cancel();
......
......@@ -36,7 +36,6 @@ class ContextMenuWaiter {
~ContextMenuWaiter();
content::ContextMenuParams& params();
const std::vector<int>& GetCapturedCommandIds() const;
// Wait until the context menu is opened and closed.
void WaitForMenuOpenAndClose();
......@@ -47,8 +46,6 @@ class ContextMenuWaiter {
void Cancel(RenderViewContextMenu* context_menu);
content::ContextMenuParams params_;
std::vector<int> captured_command_ids_;
base::RunLoop run_loop_;
base::Optional<int> maybe_command_to_execute_;
......
......@@ -3152,9 +3152,8 @@ void RenderFrameHostImpl::OnContextMenu(
// Validate the URLs in |params|. If the renderer can't request the URLs
// directly, don't show them in the context menu.
ContextMenuParams validated_params(params);
validated_params.frame_url = GetLastCommittedURL();
validated_params.page_url = GetMainFrame()->GetLastCommittedURL();
if (GetParent()) // Only populate |frame_url| for subframes.
validated_params.frame_url = GetLastCommittedURL();
// We don't validate |unfiltered_link_url| so that this field can be used
// when users want to copy the original link URL.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment