Adds screenshot options to context menu and commands #27083

jonathansampson · 2024-12-21T12:55:50Z

Resolves brave/brave-browser#25816
Security/Privacy Review: https://github.com/brave/reviews/issues/1831

screenshot-context-menu.mp4

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

Right-click on a page
Click Screenshot Tools from the context menu
Select an option (i.e., Selection, Viewport, Full page) from the submenu

Once a screenshot has been taken, you should see a UI element (with a preview of your screenshot) informing you that the screenshot has been copied to your device's clipboard.

jonathansampson · 2025-01-02T15:03:09Z

I've refactored this into a TabHelper pattern, with deferred attachment (until user requests a screenshot). Commits landing here in a bit.

bridiver · 2025-01-02T18:39:33Z

I've refactored this into a TabHelper pattern, with deferred attachment (until user requests a screenshot). Commits landing here in a bit.

Make sure you follow the latest updates using TabInterface and not WebContentsUserData

jonathansampson · 2025-01-02T18:40:42Z

I've refactored this into a TabHelper pattern, with deferred attachment (until user requests a screenshot). Commits landing here in a bit.

Make sure you follow the latest updates using TabInterface and not WebContentsUserData

Looking into that now. Thanks!

app/brave_generated_resources.grd

browser/brave_screenshots/screenshots_tab_feature.h

browser/brave_screenshots/screenshots_utils.cc

chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc

fallaciousreasoning

lgtm % nits

Remove general "NotifyUser" method and use explicit methods. Remove redundancy in tab feature logic, now passing a callback to the fullscreen page method so that all methods call a single OnComplete when they finish.

By using a browser reference we can avoid the reliance on global methods to look up a window from a given webcontents instance.

Removed the helper "TakeScreenshot" method which originally existed to stand up and instance of the helper. Now that the helper is a proper tab feature, we can use it as such from context menu commands and elsewhere.

If there is nothing selected, we shouldn't attempt to construct an autocomplete match, run the autocomplete classifier, etc. Furthermore, this enables us to simplify tests involving the context menu due to reduced setup.

After rebasing, there is now a failure in this PR unrelated to the contents or changes introduced by the PR itself. A fresh build from scratch will resolve this, but a component build will proceed if any change is made to the policy template file. As such, a typo has been found and fixed so as to 1) Improve the comments, and importantly 2) Fix the build 😀 See https://bravesoftware.slack.com/archives/C7VLGSR55/p1736928018175489?thread_ts=1736911378.517619&cid=C7VLGSR55 for more info.

github-actions · 2025-01-16T13:33:41Z

[puLL-Merge] - brave/brave-core@27083

Description

This PR introduces a new screenshot feature for the Brave browser. It adds functionality to capture screenshots of selected regions, the viewport, or full pages, and copy them to the clipboard. The feature is implemented as a set of new commands in the browser's context menu and is controlled by a feature flag.

Changes

Changes

app/brave_command_ids.h:
- Added new command IDs for screenshot tools.
app/brave_generated_resources.grd and app/brave_screenshots_strings.grdp:
- Added new string resources for screenshot menu items.
browser/about_flags.cc:
- Added a new flag for enabling the screenshot feature.
browser/brave_screenshots/:
- Added new files implementing the screenshot functionality:
  - features.cc/h: Defines the feature flag.
  - screenshots_tab_feature.cc/h: Implements the main screenshot functionality.
  - screenshots_utils.cc/h: Utility functions for screenshot operations.
  - strategies/: Various screenshot capture strategies (fullpage, selection, viewport).
browser/ui/brave_browser_command_controller.cc:
- Added support for new screenshot commands.
browser/ui/browser_commands.cc/h:
- Added a new TakeScreenshot function.
chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc/h:
- Modified to include the new screenshot menu items in the context menu.
chromium_src/chrome/browser/ui/tabs/public/tab_features.h and chromium_src/chrome/browser/ui/tabs/tab_features.cc:
- Added support for the new screenshot feature in tab features.
test/BUILD.gn:
- Added unit tests for the new screenshot feature.

sequenceDiagram
    participant User
    participant ContextMenu
    participant BraveScreenshotsTabFeature
    participant ScreenshotStrategy
    participant WebContents
    participant Clipboard

    User->>ContextMenu: Select screenshot option
    ContextMenu->>BraveScreenshotsTabFeature: StartScreenshot
    BraveScreenshotsTabFeature->>ScreenshotStrategy: Create strategy
    ScreenshotStrategy->>WebContents: Capture screenshot
    WebContents-->>ScreenshotStrategy: Return image
    ScreenshotStrategy-->>BraveScreenshotsTabFeature: OnCaptureComplete
    BraveScreenshotsTabFeature->>Clipboard: Copy image
    BraveScreenshotsTabFeature->>User: Display screenshot bubble

Possible Issues

The feature is currently limited to desktop platforms (Windows, Mac, Linux).
There might be performance concerns for capturing very large or complex pages.
The feature may not work correctly in all scenarios, such as with certain web technologies or in specific browser states.

Security Hotspots

fullpage_strategy.cc: The use of DevTools protocol for capturing full-page screenshots could potentially expose sensitive information if not properly secured.
screenshots_utils.cc: Writing to the clipboard could potentially be a vector for data exfiltration if not properly handled.

darkdh · 2025-01-17T18:36:41Z

browser/brave_screenshots/BUILD.gn

+  ]
+
+  deps = [
+    "//chrome/browser/image_editor:image_editor",


this can be just //chrome/browser/image_editor same as "//chrome/browser/ui and //ui/base/clipboard

darkdh · 2025-01-17T18:40:31Z

browser/brave_screenshots/screenshots_tab_feature.h

+  ~BraveScreenshotsTabFeature();
+
+  void StartScreenshot(Browser* browser, ScreenshotType type);
+  void OnCaptureComplete(const image_editor::ScreenshotCaptureResult& result);


This doesn't need to be public.

darkdh · 2025-01-17T18:43:27Z

browser/brave_screenshots/screenshots_tab_feature.cc

+// Some screenshots may need to be clipped to avoid the GPU limit.
+// See https://crbug.com/1260828. When this happens, we may wish to notify the
+// user that only a portion of their page could be captured.
+void DisplayScreenshotClippedNotification(base::WeakPtr<Browser> browser) {
+  NOTIMPLEMENTED();
+}


can we create an GitHub issue as follow-up so we don't forget?

darkdh · 2025-01-17T18:48:34Z

browser/brave_screenshots/screenshots_tab_feature.cc

+  if (strategy_) {
+    strategy_.reset();
+  }


This won't be necessary, it will get destructed along with destructor.

darkdh · 2025-01-17T18:51:38Z

browser/brave_screenshots/screenshots_tab_feature.cc

+  if (strategy_) {
+    strategy_.reset();
+  }


When the new unique_ptr gets move into it in the next few lines, the old one will be destroyed so we don't need to explicitly reset it here.

darkdh · 2025-01-17T18:53:09Z

browser/brave_screenshots/screenshots_tab_feature.cc

+  if (!strategy_) {
+    OnCaptureComplete({});
+    return;
+  }


This won't happen because there is no nullptr returned in CreateStrategy

darkdh · 2025-01-17T19:02:52Z

browser/brave_screenshots/strategies/fullpage_strategy.cc

+  base::Value::Dict params;  // No parameters needed
+  SendDevToolsCommand("Page.getLayoutMetrics", std::move(params), next_id_++);


This can be inlined in function call as base::Value::Dict()

darkdh · 2025-01-17T19:15:22Z

browser/brave_screenshots/test/brave_screenshots_unittests.cc

+  content::ContextMenuParams params;
+  params.page_url = GURL("devtools://devtools");
+
+  auto context_menu = CreateContextMenu(GetWebContents(), params, true);


why is is_pwa_browser true in this case?

jonathansampson added enhancement CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS labels Dec 21, 2024

jonathansampson self-assigned this Dec 21, 2024

github-actions bot added the CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) label Dec 21, 2024

jonathansampson marked this pull request as ready for review December 23, 2024 04:48

jonathansampson requested review from a team and bridiver as code owners December 23, 2024 04:48

jonathansampson marked this pull request as draft December 26, 2024 22:48

jonathansampson force-pushed the sampson-brave-screenshots branch 2 times, most recently from 0e69a8a to 4b7451d Compare December 28, 2024 23:18

jonathansampson marked this pull request as ready for review December 29, 2024 02:58

jonathansampson requested review from fallaciousreasoning and cdesouza-chromium December 29, 2024 02:58

jonathansampson marked this pull request as draft January 2, 2025 21:17

jonathansampson force-pushed the sampson-brave-screenshots branch 2 times, most recently from 22868e1 to fdd6104 Compare January 4, 2025 17:34

github-actions bot added the CI/run-network-audit Run network-audit label Jan 4, 2025

fallaciousreasoning reviewed Jan 5, 2025

View reviewed changes

app/brave_generated_resources.grd Outdated Show resolved Hide resolved

fallaciousreasoning reviewed Jan 5, 2025

View reviewed changes

browser/brave_screenshots/screenshots_tab_feature.h Outdated Show resolved Hide resolved

fallaciousreasoning reviewed Jan 5, 2025

View reviewed changes

browser/brave_screenshots/screenshots_utils.cc Outdated Show resolved Hide resolved

fallaciousreasoning reviewed Jan 5, 2025

View reviewed changes

chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc Outdated Show resolved Hide resolved

fallaciousreasoning approved these changes Jan 5, 2025

View reviewed changes

jonathansampson marked this pull request as ready for review January 7, 2025 14:04

jonathansampson added 25 commits January 15, 2025 09:17

conditionally enable the tab feature

fadc22d

more accurate conditional method name

fdeaefb

resolves omission of parameter names

6e77f33

improves screenshot utilities

4b5e0a4

Remove general "NotifyUser" method and use explicit methods. Remove redundancy in tab feature logic, now passing a callback to the fullscreen page method so that all methods call a single OnComplete when they finish.

refactors to use browser reference

2198da7

By using a browser reference we can avoid the reliance on global methods to look up a window from a given webcontents instance.

more accurate feature description

5b5682c

various simplifications

8197ef8

Removed the helper "TakeScreenshot" method which originally existed to stand up and instance of the helper. Now that the helper is a proper tab feature, we can use it as such from context menu commands and elsewhere.

moves screenshot strings to own file

1b71d0e

Don't show the submenu within devtools

6bf8a54

refactors devtools code into distinct helper

50798f7

pesubmit fixes

0f78167

avoid forward declarations per style guide

c86dabb

Skip autocomplete classifier on empty selection

a2887ec

If there is nothing selected, we shouldn't attempt to construct an autocomplete match, run the autocomplete classifier, etc. Furthermore, this enables us to simplify tests involving the context menu due to reduced setup.

adds context-menu related unittests for screenshots

92f82e3

cleanup and simplify unit tests

317462b

dependency cleanup

748c3c6

moves to strategy-based screenshotting

fd5d3c3

Simplifies clip-indicating member

bba8861

Update screenshots_tab_feature.cc

f76198e

adds some comments around screenshot limitations

32d4f72

minor note changes

194be01

removes unused includes

ee4b82e

clean up build.gn deps

9ebdb08

uses explicit/intentional targets

01c9de6

uses debug logs

6d39b1e

jonathansampson force-pushed the sampson-brave-screenshots branch from f8c8d02 to 6d39b1e Compare January 15, 2025 17:02

jonathansampson requested a review from darkdh January 16, 2025 13:09

darkdh reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds screenshot options to context menu and commands #27083

Adds screenshot options to context menu and commands #27083

jonathansampson commented Dec 21, 2024 •

edited

Loading

jonathansampson commented Jan 2, 2025

bridiver commented Jan 2, 2025

jonathansampson commented Jan 2, 2025

fallaciousreasoning left a comment

github-actions bot commented Jan 16, 2025

Changes

darkdh Jan 17, 2025

darkdh Jan 17, 2025

darkdh Jan 17, 2025

darkdh Jan 17, 2025

darkdh Jan 17, 2025

darkdh Jan 17, 2025

darkdh Jan 17, 2025

darkdh Jan 17, 2025

		base::Value::Dict params; // No parameters needed
		SendDevToolsCommand("Page.getLayoutMetrics", std::move(params), next_id_++);

Adds screenshot options to context menu and commands #27083

Are you sure you want to change the base?

Adds screenshot options to context menu and commands #27083

Conversation

jonathansampson commented Dec 21, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

jonathansampson commented Jan 2, 2025

bridiver commented Jan 2, 2025

jonathansampson commented Jan 2, 2025

fallaciousreasoning left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 16, 2025

Description

Changes

Possible Issues

Security Hotspots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathansampson commented Dec 21, 2024 •

edited

Loading