-
Notifications
You must be signed in to change notification settings - Fork 896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds screenshot options to context menu and commands #27083
Open
jonathansampson
wants to merge
51
commits into
master
Choose a base branch
from
sampson-brave-screenshots
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,107
−6
Open
Changes from all commits
Commits
Show all changes
51 commits
Select commit
Hold shift + click to select a range
7decc5d
adds screenshot feature and flag
jonathansampson 43206cd
adds context menu screenshot options
jonathansampson be49547
addresses flaky assumption in unit tests
jonathansampson 0af8697
exposes functions to commander
jonathansampson 0366f68
adds unit tests for context-menu/feature-flag
jonathansampson 9033fcd
refactored into TabHelper pattern
jonathansampson eeed506
minor presubmit fixes
jonathansampson 378d40a
removes unnecessary /tabs/ subdirectory
jonathansampson cfa83b7
removes unnecessary /ui/views/ subdirectory
jonathansampson 19fc390
removes unnecessary /components/ directory
jonathansampson 3477637
fixes dates and header guards
jonathansampson 33b8bac
presubmit fixes
jonathansampson acf6d38
revert remnant changes to brave_ui_* files
jonathansampson 9b43670
cleanup "command" remnants
jonathansampson 7aa360c
more consistent naming
jonathansampson ac4b114
adopting more consistently-used pattern
jonathansampson ee798bf
provides an enabled-state method
jonathansampson c73fd66
presubmit cleanup
jonathansampson 4a4767e
cleanup
jonathansampson e3a53fa
drop reliance on browser_finder
jonathansampson 1d4e4ba
refactors into a tab feature
jonathansampson ec8b216
updates macro for feature inclusion
jonathansampson cbe723d
removes complex multi-line /* ... */ comment
jonathansampson fadc22d
conditionally enable the tab feature
jonathansampson fdeaefb
more accurate conditional method name
jonathansampson 6e77f33
resolves omission of parameter names
jonathansampson 4b5e0a4
improves screenshot utilities
jonathansampson 2198da7
refactors to use browser reference
jonathansampson 5b5682c
more accurate feature description
jonathansampson 8197ef8
various simplifications
jonathansampson 1b71d0e
moves screenshot strings to own file
jonathansampson 6bf8a54
Don't show the submenu within devtools
jonathansampson 50798f7
refactors devtools code into distinct helper
jonathansampson 0f78167
pesubmit fixes
jonathansampson c86dabb
avoid forward declarations per style guide
jonathansampson a2887ec
Skip autocomplete classifier on empty selection
jonathansampson 92f82e3
adds context-menu related unittests for screenshots
jonathansampson 317462b
cleanup and simplify unit tests
jonathansampson 748c3c6
dependency cleanup
jonathansampson fd5d3c3
moves to strategy-based screenshotting
jonathansampson bba8861
Simplifies clip-indicating member
jonathansampson f76198e
Update screenshots_tab_feature.cc
jonathansampson 32d4f72
adds some comments around screenshot limitations
jonathansampson 194be01
minor note changes
jonathansampson ee4b82e
removes unused includes
jonathansampson 9ebdb08
clean up build.gn deps
jonathansampson 01c9de6
uses explicit/intentional targets
jonathansampson 6d39b1e
uses debug logs
jonathansampson 9599a59
typo-correction to resolve build
jonathansampson 3f4ba33
addressing minor feedback items
jonathansampson aecc9d0
adds GitHub Issue reference
jonathansampson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
<?xml version='1.0' encoding='UTF-8'?> | ||
<grit-part> | ||
<if expr="use_titlecase"> | ||
<message name="IDS_IDC_BRAVE_SCREENSHOT_TOOLS" desc="Title for the screenshot submenu"> | ||
Screenshot Tools | ||
</message> | ||
<message name="IDS_IDC_BRAVE_SCREENSHOTS_START_SELECTION_TO_CLIPBOARD" desc="Title for option to screenshot a region and copy to clipboard"> | ||
Screenshot Selection to Clipboard | ||
</message> | ||
<message name="IDS_IDC_BRAVE_SCREENSHOTS_START_VIEWPORT_TO_CLIPBOARD" desc="Title for option to screenshot the viewport and copy to clipboard"> | ||
Screenshot Viewport to Clipboard | ||
</message> | ||
<message name="IDS_IDC_BRAVE_SCREENSHOTS_START_FULLPAGE_TO_CLIPBOARD" desc="Title for option to screenshot the full page and copy to clipboard"> | ||
Screenshot Full Page to Clipboard | ||
</message> | ||
</if> | ||
<if expr="not use_titlecase"> | ||
<message name="IDS_IDC_BRAVE_SCREENSHOT_TOOLS" desc="Title for the screenshot submenu"> | ||
Screenshot tools | ||
</message> | ||
<message name="IDS_IDC_BRAVE_SCREENSHOTS_START_SELECTION_TO_CLIPBOARD" desc="Title for option to screenshot a region and copy to clipboard"> | ||
Screenshot selection to clipboard | ||
</message> | ||
<message name="IDS_IDC_BRAVE_SCREENSHOTS_START_VIEWPORT_TO_CLIPBOARD" desc="Title for option to screenshot the viewport and copy to clipboard"> | ||
Screenshot viewport to clipboard | ||
</message> | ||
<message name="IDS_IDC_BRAVE_SCREENSHOTS_START_FULLPAGE_TO_CLIPBOARD" desc="Title for option to screenshot the full page and copy to clipboard"> | ||
Screenshot full page to clipboard | ||
</message> | ||
</if> | ||
</grit-part> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
# Copyright (c) 2025 The Brave Authors. All rights reserved. | ||
# This Source Code Form is subject to the terms of the Mozilla Public | ||
# License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
# You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
||
import("//brave/browser/brave_screenshots/buildflags/buildflags.gni") | ||
|
||
assert(enable_brave_screenshots) | ||
|
||
source_set("utils") { | ||
sources = [ | ||
"screenshots_utils.cc", | ||
"screenshots_utils.h", | ||
] | ||
|
||
deps = [ | ||
"//chrome/browser/image_editor:image_editor", | ||
"//chrome/browser/ui:ui", | ||
"//chrome/browser/ui/browser_window:browser_window", | ||
"//ui/base/clipboard:clipboard", | ||
] | ||
} | ||
|
||
source_set("strategies") { | ||
sources = [ | ||
"strategies/fullpage_strategy.cc", | ||
"strategies/fullpage_strategy.h", | ||
"strategies/screenshot_strategy.h", | ||
"strategies/selection_strategy.cc", | ||
"strategies/selection_strategy.h", | ||
"strategies/viewport_strategy.cc", | ||
"strategies/viewport_strategy.h", | ||
] | ||
|
||
deps = [ | ||
"//chrome/browser/image_editor:image_editor", | ||
"//chrome/browser/ui:ui", | ||
] | ||
} | ||
|
||
source_set("tabs") { | ||
sources = [ | ||
"screenshots_tab_feature.cc", | ||
"screenshots_tab_feature.h", | ||
] | ||
|
||
deps = [ | ||
":strategies", | ||
":utils", | ||
"//chrome/browser/image_editor:image_editor", | ||
"//chrome/browser/ui:ui", | ||
] | ||
} | ||
|
||
source_set("features") { | ||
sources = [ | ||
"features.cc", | ||
"features.h", | ||
] | ||
|
||
deps = [ "//base:base" ] | ||
} | ||
|
||
source_set("unit_tests") { | ||
testonly = true | ||
sources = [ "test/brave_screenshots_unittests.cc" ] | ||
deps = [ | ||
":features", | ||
"//base:base", | ||
"//brave/app:command_ids", | ||
"//chrome/test:test_support", | ||
"//testing/gtest:gtest", | ||
] | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
# Copyright (c) 2025 The Brave Authors. All rights reserved. | ||
# This Source Code Form is subject to the terms of the Mozilla Public | ||
# License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
# You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
||
declare_args() { | ||
enable_brave_screenshots = is_win || is_mac || is_linux | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
// Copyright (c) 2025 The Brave Authors. All rights reserved. | ||
// This Source Code Form is subject to the terms of the Mozilla Public | ||
// License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
// You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
||
#include "brave/browser/brave_screenshots/features.h" | ||
|
||
namespace brave_screenshots::features { | ||
|
||
BASE_FEATURE(kBraveScreenshots, | ||
"BraveScreenshots", | ||
base::FEATURE_ENABLED_BY_DEFAULT); | ||
|
||
bool IsBraveScreenshotsEnabled() { | ||
return base::FeatureList::IsEnabled(kBraveScreenshots); | ||
} | ||
} // namespace brave_screenshots::features |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
// Copyright (c) 2025 The Brave Authors. All rights reserved. | ||
// This Source Code Form is subject to the terms of the Mozilla Public | ||
// License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
// You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
||
#ifndef BRAVE_BROWSER_BRAVE_SCREENSHOTS_FEATURES_H_ | ||
#define BRAVE_BROWSER_BRAVE_SCREENSHOTS_FEATURES_H_ | ||
|
||
#include "base/feature_list.h" | ||
|
||
namespace brave_screenshots::features { | ||
BASE_DECLARE_FEATURE(kBraveScreenshots); | ||
|
||
bool IsBraveScreenshotsEnabled(); | ||
} // namespace brave_screenshots::features | ||
|
||
#endif // BRAVE_BROWSER_BRAVE_SCREENSHOTS_FEATURES_H_ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
// Copyright (c) 2025 The Brave Authors. All rights reserved. | ||
// This Source Code Form is subject to the terms of the Mozilla Public | ||
// License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
// You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
||
#include "brave/browser/brave_screenshots/screenshots_tab_feature.h" | ||
|
||
#include "base/notreached.h" | ||
#include "brave/browser/brave_screenshots/screenshots_utils.h" | ||
#include "brave/browser/brave_screenshots/strategies/fullpage_strategy.h" | ||
#include "brave/browser/brave_screenshots/strategies/selection_strategy.h" | ||
#include "brave/browser/brave_screenshots/strategies/viewport_strategy.h" | ||
#include "chrome/browser/image_editor/screenshot_flow.h" | ||
#include "chrome/browser/ui/browser.h" | ||
#include "content/public/browser/web_contents.h" | ||
|
||
namespace { | ||
|
||
// 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) { | ||
// Issue: https://github.com/brave/brave-browser/issues/43369 | ||
NOTIMPLEMENTED(); | ||
} | ||
|
||
} // namespace | ||
namespace brave_screenshots { | ||
|
||
BraveScreenshotsTabFeature::BraveScreenshotsTabFeature() { | ||
DVLOG(1) << "BraveScreenshotsTabFeature created"; | ||
} | ||
|
||
BraveScreenshotsTabFeature::~BraveScreenshotsTabFeature() { | ||
DVLOG(1) << "BraveScreenshotsTabFeature destroyed"; | ||
} | ||
|
||
void BraveScreenshotsTabFeature::StartScreenshot(Browser* browser, | ||
ScreenshotType type) { | ||
DVLOG(1) << "Called StartScreenshot"; | ||
CHECK(browser); | ||
|
||
browser_ = browser->AsWeakPtr(); | ||
web_contents_ = | ||
browser_->tab_strip_model()->GetActiveWebContents()->GetWeakPtr(); | ||
|
||
// We've determined the appropriate strategy to use | ||
strategy_ = CreateStrategy(type); | ||
|
||
DVLOG(2) << "Starting capture"; | ||
|
||
strategy_->Capture( | ||
web_contents_.get(), | ||
base::BindOnce(&BraveScreenshotsTabFeature::OnCaptureComplete, | ||
weak_factory_.GetWeakPtr())); | ||
} | ||
|
||
std::unique_ptr<BraveScreenshotStrategy> | ||
BraveScreenshotsTabFeature::CreateStrategy(ScreenshotType type) { | ||
switch (type) { | ||
case ScreenshotType::kFullPage: | ||
DVLOG(3) << "Creating FullPageStrategy"; | ||
return std::make_unique<FullPageStrategy>(); | ||
case ScreenshotType::kSelection: | ||
// Based on image_editor::ScreenshotFlow, which requires a WebContents | ||
DVLOG(3) << "Creating SelectionStrategy"; | ||
return std::make_unique<SelectionStrategy>(web_contents_.get()); | ||
case ScreenshotType::kViewport: | ||
// Based on image_editor::ScreenshotFlow, which requires a WebContents | ||
DVLOG(3) << "Creating ViewportStrategy"; | ||
return std::make_unique<ViewportStrategy>(web_contents_.get()); | ||
default: | ||
NOTREACHED(); | ||
} | ||
} | ||
|
||
void BraveScreenshotsTabFeature::OnCaptureComplete( | ||
const image_editor::ScreenshotCaptureResult& result) { | ||
DVLOG(2) << __func__; | ||
|
||
if (result.image.IsEmpty()) { | ||
DVLOG(2) << "Screenshot capture failed"; | ||
return; | ||
} | ||
|
||
if (strategy_->DidClipScreenshot()) { | ||
DisplayScreenshotClippedNotification(browser_); | ||
} | ||
|
||
if (browser_) { | ||
utils::CopyImageToClipboard(result); | ||
utils::DisplayScreenshotBubble(result, browser_); | ||
} | ||
} | ||
|
||
} // namespace brave_screenshots |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
// Copyright (c) 2025 The Brave Authors. All rights reserved. | ||
// This Source Code Form is subject to the terms of the Mozilla Public | ||
// License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
// You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
||
#ifndef BRAVE_BROWSER_BRAVE_SCREENSHOTS_SCREENSHOTS_TAB_FEATURE_H_ | ||
#define BRAVE_BROWSER_BRAVE_SCREENSHOTS_SCREENSHOTS_TAB_FEATURE_H_ | ||
|
||
#include <memory> | ||
|
||
#include "base/memory/weak_ptr.h" | ||
#include "brave/browser/brave_screenshots/strategies/screenshot_strategy.h" | ||
#include "chrome/browser/image_editor/screenshot_flow.h" | ||
#include "chrome/browser/ui/browser.h" | ||
#include "content/public/browser/web_contents.h" | ||
|
||
namespace brave_screenshots { | ||
|
||
enum ScreenshotType { | ||
kSelection, | ||
kViewport, | ||
kFullPage, | ||
}; | ||
|
||
class BraveScreenshotsTabFeature { | ||
public: | ||
BraveScreenshotsTabFeature(); | ||
BraveScreenshotsTabFeature(const BraveScreenshotsTabFeature&) = delete; | ||
BraveScreenshotsTabFeature& operator=(const BraveScreenshotsTabFeature&) = | ||
delete; | ||
~BraveScreenshotsTabFeature(); | ||
|
||
void StartScreenshot(Browser* browser, ScreenshotType type); | ||
|
||
private: | ||
void OnCaptureComplete(const image_editor::ScreenshotCaptureResult& result); | ||
std::unique_ptr<BraveScreenshotStrategy> CreateStrategy(ScreenshotType type); | ||
base::WeakPtr<Browser> browser_ = nullptr; | ||
std::unique_ptr<BraveScreenshotStrategy> strategy_ = nullptr; | ||
base::WeakPtr<content::WebContents> web_contents_ = nullptr; | ||
base::WeakPtrFactory<BraveScreenshotsTabFeature> weak_factory_{this}; | ||
|
||
}; // class BraveScreenshotsTabFeature | ||
|
||
} // namespace brave_screenshots | ||
|
||
#endif // BRAVE_BROWSER_BRAVE_SCREENSHOTS_SCREENSHOTS_TAB_FEATURE_H_ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
// Copyright (c) 2025 The Brave Authors. All rights reserved. | ||
// This Source Code Form is subject to the terms of the Mozilla Public | ||
// License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
// You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
||
#include "brave/browser/brave_screenshots/screenshots_utils.h" | ||
|
||
#include "chrome/browser/image_editor/screenshot_flow.h" | ||
#include "chrome/browser/ui/browser.h" | ||
#include "chrome/browser/ui/browser_window.h" | ||
#include "ui/base/clipboard/scoped_clipboard_writer.h" | ||
|
||
namespace brave_screenshots::utils { | ||
|
||
using image_editor::ScreenshotCaptureResult; | ||
|
||
void CopyImageToClipboard(const ScreenshotCaptureResult& result) { | ||
DVLOG(2) << __func__; | ||
if (result.image.IsEmpty()) { | ||
DVLOG(2) << "Image is empty"; | ||
return; | ||
} | ||
|
||
DVLOG(2) << "Writing image to clipboard"; | ||
// Copy the image to the user's clipboard | ||
ui::ScopedClipboardWriter(ui::ClipboardBuffer::kCopyPaste) | ||
.WriteImage(*result.image.ToSkBitmap()); | ||
} | ||
|
||
void DisplayScreenshotBubble(const ScreenshotCaptureResult& result, | ||
base::WeakPtr<Browser> browser) { | ||
DVLOG(2) << __func__; | ||
if (!browser || result.image.IsEmpty()) { | ||
DVLOG(2) << "Browser is null or image is empty"; | ||
return; | ||
} | ||
|
||
DVLOG(2) << "Displaying screenshot bubble"; | ||
// Leverage the screenshot bubble to show the user the screenshot | ||
browser->window()->ShowScreenshotCapturedBubble( | ||
browser->tab_strip_model()->GetActiveWebContents(), result.image); | ||
} | ||
|
||
} // namespace brave_screenshots::utils |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be just
//chrome/browser/image_editor
same as"//chrome/browser/ui
and//ui/base/clipboard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I purposefully provide the target names, just to more clearly communicate intent. If it's okay, I'd prefer to keep them explicit.