Skip to content

Commit

Permalink
Add support for suppressing custom cursors across navigations.
Browse files Browse the repository at this point in the history 
This CL adds observations of the WebContents that have been active
while custom cursor suppression is on. If the RenderFrameHost of the
primary main frame changes, the new RenderFrameHost will also have
custom cursors suppressed.

Bug: 1478613
Change-Id: I2ebe50d8caaf2f52c3ba7c957391595d34d9ac50
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4898211
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: Jan Keitel <jkeitel@google.com>
Cr-Commit-Position: refs/heads/main@{#1206271}
  • Loading branch information
Jan Keitel authored and Chromium LUCI CQ committed Oct 6, 2023
1 parent 3b06ad0 commit b3471ed
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 18 deletions.
51 changes: 42 additions & 9 deletions chrome/browser/ui/views/autofill/popup/custom_cursor_suppressor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,37 @@

#include "chrome/browser/ui/views/autofill/popup/custom_cursor_suppressor.h"

#include <map>
#include <memory>
#include <utility>
#include <vector>

#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_list.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "content/public/browser/global_routing_id.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/render_widget_host.h"
#include "content/public/browser/web_contents.h"
#include "content/public/browser/web_contents_observer.h"

CustomCursorSuppressor::NavigationObserver::NavigationObserver(
content::WebContents* web_contents,
Callback callback)
: content::WebContentsObserver(web_contents),
callback_(std::move(callback)) {
CHECK(callback_);
}

CustomCursorSuppressor::NavigationObserver::~NavigationObserver() = default;

void CustomCursorSuppressor::NavigationObserver::RenderFrameHostChanged(
content::RenderFrameHost* old_host,
content::RenderFrameHost* new_host) {
if (new_host && new_host->IsInPrimaryMainFrame()) {
callback_.Run(*web_contents());
}
}

CustomCursorSuppressor::CustomCursorSuppressor() = default;

Expand All @@ -28,6 +51,7 @@ void CustomCursorSuppressor::Start(int max_dimension_dips) {
browser->tab_strip_model()->AddObserver(this);
if (content::WebContents* active_contents =
browser->tab_strip_model()->GetActiveWebContents()) {
MaybeObserveNavigationsInWebContents(*active_contents);
SuppressForWebContents(*active_contents);
}
}
Expand All @@ -39,8 +63,10 @@ void CustomCursorSuppressor::Stop() {
browser_list_observation_.Reset();
}

bool CustomCursorSuppressor::IsSuppressing() const {
return browser_list_observation_.IsObserving();
bool CustomCursorSuppressor::IsSuppressing(
content::WebContents& web_contents) const {
return disallow_custom_cursor_scopes_.contains(
web_contents.GetPrimaryMainFrame()->GetGlobalId());
}

std::vector<content::GlobalRenderFrameHostId>
Expand All @@ -54,17 +80,23 @@ CustomCursorSuppressor::SuppressedRenderFrameHostIdsForTesting() const {

void CustomCursorSuppressor::SuppressForWebContents(
content::WebContents& web_contents) {
content::GlobalRenderFrameHostId main_frame_id =
web_contents.GetPrimaryMainFrame()->GetGlobalId();
if (disallow_custom_cursor_scopes_.contains(main_frame_id)) {
if (IsSuppressing(web_contents)) {
return;
}
disallow_custom_cursor_scopes_.insert(
{main_frame_id,
{web_contents.GetPrimaryMainFrame()->GetGlobalId(),
web_contents.CreateDisallowCustomCursorScope(max_dimension_dips_)});
// TODO(crbug.com/1478613): Start observing the WebContents at this point to
// guard against navigations in it that cause a change in the RFH of the
// primary main frame.
}

void CustomCursorSuppressor::MaybeObserveNavigationsInWebContents(
content::WebContents& web_contents) {
if (IsSuppressing(web_contents)) {
return;
}
navigation_observers_.push_back(std::make_unique<NavigationObserver>(
&web_contents,
base::BindRepeating(&CustomCursorSuppressor::SuppressForWebContents,
base::Unretained(this))));
}

void CustomCursorSuppressor::OnBrowserAdded(Browser* browser) {
Expand All @@ -76,6 +108,7 @@ void CustomCursorSuppressor::OnTabStripModelChanged(
const TabStripModelChange& change,
const TabStripSelectionChange& selection) {
if (selection.active_tab_changed() && selection.new_contents) {
MaybeObserveNavigationsInWebContents(*selection.new_contents);
SuppressForWebContents(*selection.new_contents);
}
}
45 changes: 42 additions & 3 deletions chrome/browser/ui/views/autofill/popup/custom_cursor_suppressor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#define CHROME_BROWSER_UI_VIEWS_AUTOFILL_POPUP_CUSTOM_CURSOR_SUPPRESSOR_H_

#include <map>
#include <memory>
#include <vector>

#include "base/functional/callback_helpers.h"
Expand All @@ -14,13 +15,22 @@
#include "chrome/browser/ui/browser_list_observer.h"
#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
#include "content/public/browser/global_routing_id.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/web_contents_observer.h"

namespace content {
class WebContents;
} // namespace content

// While active, it suppresses custom cursors exceeding a given size limit on
// all the active `WebContents` or all `Browser`s of the current profile.
// While active, this class suppresses custom cursors exceeding a given size
// limit on all the active `WebContents` or all `Browser`s of the current
// profile.
// Note that `CustomCursorSuppressor` is expected to have a short lifetime, e.g.
// while an Autofill popup is showing - therefore it does not clean up (safe,
// but stale) observation entries related to `RenderFrameHost`s of previous
// navigations or of `WebContents` that are no longer active.
// Should the class become used in a wider context, additional logic to remove
// such stale entries should be added.
class CustomCursorSuppressor : public BrowserListObserver,
public TabStripModelObserver {
public:
Expand All @@ -36,7 +46,9 @@ class CustomCursorSuppressor : public BrowserListObserver,
void Start(int max_dimension_dips = 0);
// Stops suppressing custom cursors.
void Stop();
bool IsSuppressing() const;

// Returns whether custom cursors are disallowed on `web_contents`.
bool IsSuppressing(content::WebContents& web_contents) const;

// Returns the ids of `RenderFrameHost`s on which custom cursors are
// suppressed. Note that not every id needs to correspond to an active
Expand All @@ -50,6 +62,11 @@ class CustomCursorSuppressor : public BrowserListObserver,
// no-op.
void SuppressForWebContents(content::WebContents& web_contents);

// Observes navigations in `web_contents` that lead to changes in the
// `RenderFrameHost` of the primary main frame iff custom cursors are not
// already disallowed on `web_contents`.
void MaybeObserveNavigationsInWebContents(content::WebContents& web_contents);

// BrowserListObserver:
// Starts observing the tab strip model of `browser`. Note that there is
// no corresponding `OnBrowserRemoved`, since `TabStripModelObserver`
Expand All @@ -62,13 +79,35 @@ class CustomCursorSuppressor : public BrowserListObserver,
const TabStripModelChange& change,
const TabStripSelectionChange& selection) override;

// A helper to filter and forward `RenderFrameHostChanged` events of a single
// `WebContents`. Used to allow `CustomCursorSuppressor` to effectively
// observe multiple `WebContents`.
class NavigationObserver : public content::WebContentsObserver {
public:
// A callback that is called whenever the `RenderFrameHost` of the primary
// main frame of the observed `WebContents` has changed.
using Callback = base::RepeatingCallback<void(content::WebContents&)>;

NavigationObserver(content::WebContents* web_contents, Callback callback);
~NavigationObserver() override;

private:
// content::WebContentsObserver:
void RenderFrameHostChanged(content::RenderFrameHost* old_host,
content::RenderFrameHost* new_host) override;

Callback callback_;
};

base::ScopedObservation<BrowserList, BrowserListObserver>
browser_list_observation_{this};

int max_dimension_dips_ = 0;

std::map<content::GlobalRenderFrameHostId, base::ScopedClosureRunner>
disallow_custom_cursor_scopes_;

std::vector<std::unique_ptr<NavigationObserver>> navigation_observers_;
};

#endif // CHROME_BROWSER_UI_VIEWS_AUTOFILL_POPUP_CUSTOM_CURSOR_SUPPRESSOR_H_
38 changes: 32 additions & 6 deletions chrome/browser/ui/views/autofill/popup/custom_cursor_suppressor_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "chrome/browser/ui/browser_navigator_params.h"
#include "chrome/test/base/browser_with_test_window_test.h"
#include "content/public/browser/global_routing_id.h"
#include "content/public/browser/web_contents.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/base/ui_base_types.h"
Expand Down Expand Up @@ -57,15 +58,43 @@ TEST_F(CustomCursorSuppressorTest, SingleBrowserSingleTab) {
AddTab(browser(), GURL(kUrl1));

CustomCursorSuppressor suppressor;
EXPECT_FALSE(suppressor.IsSuppressing());
EXPECT_FALSE(suppressor.IsSuppressing(
*browser()->tab_strip_model()->GetActiveWebContents()));

suppressor.Start();
EXPECT_TRUE(suppressor.IsSuppressing());
EXPECT_TRUE(suppressor.IsSuppressing(
*browser()->tab_strip_model()->GetActiveWebContents()));
EXPECT_THAT(suppressor.SuppressedRenderFrameHostIdsForTesting(),
UnorderedElementsAre(GetRfhIdOfActiveWebContents(*browser())));

suppressor.Stop();
EXPECT_FALSE(suppressor.IsSuppressing());
EXPECT_FALSE(suppressor.IsSuppressing(
*browser()->tab_strip_model()->GetActiveWebContents()));
}

// Tests that a navigation that results in a different `RenderFrameHost` for the
// still maintains a suppressed custom cursor.
TEST_F(CustomCursorSuppressorTest,
SingleBrowserSingleTabWithNavigationToDifferentOrigin) {
AddTab(browser(), GURL(kUrl1));

CustomCursorSuppressor suppressor;
suppressor.Start();

EXPECT_TRUE(suppressor.IsSuppressing(
*browser()->tab_strip_model()->GetActiveWebContents()));
std::vector<GlobalRenderFrameHostId> expected_suppressed_ids = {
GetRfhIdOfActiveWebContents(*browser())};

// Simulate a navigation to a different origin.
NavigateAndCommitActiveTab(GURL(kUrl2));
EXPECT_NE(GetRfhIdOfActiveWebContents(*browser()),
expected_suppressed_ids.front());
expected_suppressed_ids.push_back(GetRfhIdOfActiveWebContents(*browser()));
EXPECT_TRUE(suppressor.IsSuppressing(
*browser()->tab_strip_model()->GetActiveWebContents()));
EXPECT_THAT(suppressor.SuppressedRenderFrameHostIdsForTesting(),
UnorderedElementsAreArray(expected_suppressed_ids));
}

// Tests that custom cursor suppression reacts to active tab changes in a single
Expand Down Expand Up @@ -170,7 +199,6 @@ TEST_F(CustomCursorSuppressorTest, MultipleBrowsers) {

CustomCursorSuppressor suppressor;
suppressor.Start();
EXPECT_TRUE(suppressor.IsSuppressing());
EXPECT_THAT(suppressor.SuppressedRenderFrameHostIdsForTesting(),
UnorderedElementsAre(GetRfhIdOfActiveWebContents(*browser()),
GetRfhIdOfActiveWebContents(*browser2)));
Expand All @@ -188,7 +216,6 @@ TEST_F(CustomCursorSuppressorTest, BrowserAddition) {

CustomCursorSuppressor suppressor;
suppressor.Start();
EXPECT_TRUE(suppressor.IsSuppressing());
EXPECT_THAT(suppressor.SuppressedRenderFrameHostIdsForTesting(),
UnorderedElementsAre(GetRfhIdOfActiveWebContents(*browser())));

Expand All @@ -204,7 +231,6 @@ TEST_F(CustomCursorSuppressorTest, BrowserAddition) {
GetRfhIdOfActiveWebContents(*browser2)));

suppressor.Stop();
EXPECT_FALSE(suppressor.IsSuppressing());

// All tabs must be closed prior to browser destruction.
browser2->tab_strip_model()->CloseAllTabs();
Expand Down

0 comments on commit b3471ed

Please sign in to comment.