[OmniboxEverywhere][3/5] Implement screenshare Mojo definitions, capturing, and unit tests [chromium/src : main]
Hamzah Behery (Gerrit)
Hamzah Behery added 1 comment![]( "Open in Gerrit")
Comment for future reference: patchset 12 implements the native screenpicker
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hamzah Behery (Gerrit)
Hamzah Behery voted Commit-Queue+1![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
Shadow: ari...@chromium.org; IPC: ke...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
Shadow IPC reviewer(s): ari...@chromium.org. Please conduct an IPC review and CR+1 when satisfied. Remember to add the main reviewers to the attention set if needed.
Main IPC reviewer(s): ke...@chromium.org. Please wait for the shadowed IPC reviewer to CR+1 before reviewing.
Shadowed: ari...@chromium.org
Reviewer source(s):
ari...@chromium.org, ke...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Ari Chivukula
- Foromo Daniel Soromou
- Juliet Lévesque Knighton
- Ken Buchanan
- Nihar Majmudar
- Tove Petersson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ari Chivukula
| Code-Review | +1 |
2 comments
StartScreenshare(bool entire_screen)nit: maybe note entire_screen is just a hint for the picker and not enforced. Maybe `prefer_entire_screen`
Related details
- Foromo Daniel Soromou
- Hamzah Behery
- Juliet Lévesque Knighton
- Ken Buchanan
- Nihar Majmudar
- Tove Petersson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ari Chivukula
| Code-Review | +0 |
1 comment
actually wait, is the returned token not used anywhere? I'm a little worried about exposing info that isn't used/checked in some way
Related details
- Foromo Daniel Soromou
- Hamzah Behery
- Juliet Lévesque Knighton
- Ken Buchanan
- Nihar Majmudar
- Tove Petersson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Hamzah Behery (Gerrit)
Hamzah Behery added 2 comments![]( "Open in Gerrit")
actually wait, is the returned token not used anywhere? I'm a little worried about exposing info that isn't used/checked in some way
not currently, but I intend to implement the token's use in a subsequent CL in this stack to manage the screensharing state on the frontend.
should follow a similar pattern for image uploads:
when the WebUI invokes startScreenshare(), it receives the token in the promise resolution. It uses this token synchronously to open the chat window and show a loading placeholder chip. When the screenshot processing is finished, the browser process calls addFileContext(token, file_info) asynchronously, and the WebUI uses the token to match the file info (and base64 image data) to the placeholder chip.
StartScreenshare(bool entire_screen)nit: maybe note entire_screen is just a hint for the picker and not enforced. Maybe `prefer_entire_screen`
- Ari Chivukula
- Foromo Daniel Soromou
Ari Chivukula (Gerrit)
Ari Chivukula voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ari Chivukula
| Code-Review | +1 |
1 comment
Hamzah Beheryactually wait, is the returned token not used anywhere? I'm a little worried about exposing info that isn't used/checked in some way
not currently, but I intend to implement the token's use in a subsequent CL in this stack to manage the screensharing state on the frontend.
should follow a similar pattern for image uploads:
when the WebUI invokes startScreenshare(), it receives the token in the promise resolution. It uses this token synchronously to open the chat window and show a loading placeholder chip. When the screenshot processing is finished, the browser process calls addFileContext(token, file_info) asynchronously, and the WebUI uses the token to match the file info (and base64 image data) to the placeholder chip.
okay that seems reasonable, leaving open for primary IPC reviewer
Related details
- Foromo Daniel Soromou
- Hamzah Behery
- Juliet Lévesque Knighton
- Ken Buchanan
- Nihar Majmudar
- Tove Petersson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Juliet Lévesque Knighton (Gerrit)
Juliet Lévesque Knighton added 8 comments![]( "Open in Gerrit")
void set_bypass_ui_manager_for_testing(bool bypass) {nit: is this and `bypass_ui_manager_for_testing_` needed? I don't see them used in any tests
#include "chrome/browser/media/webrtc/desktop_media_picker.h" // nogncheck
#include "chrome/browser/media/webrtc/desktop_media_picker_controller.h" // nogncheck
#include "chrome/browser/media/webrtc/desktop_media_picker_factory_impl.h" // nogncheck
#include "chrome/browser/ui/browser_window/public/profile_browser_collection.h"
#include "chrome/browser/ui/omnibox/omnibox_everywhere/omnibox_everywhere_ui_manager.h" // nogncheckWhy do we need // nogncheck here? Can these be added to the BUILD file instead?
#include "chrome/browser/ui/omnibox/omnibox_everywhere/omnibox_everywhere_ui_manager.h" // nognchecknit: needed? I don't see this used anywhere
screenshare_picker_controller_ =Is there any chance that screenshare_picker_controller_ already exists here (a screen share is already in session) and we are rewriting this field? consider adding a guard at the beginning of StartScreenshare or FallbackToChromeDefaultPicker to check if this is already active
is_capturing_ = true;similar concern here: should we check to see if we are already capturing something (check if is_capturing_ is true) before continuing this logic?
ContextualSearchboxHandler::ProcessedScreenshot result;nit: is it expected that we never populate thumbnail_data_url?
file_info_mojom->file_name = "Screenshot.png";
file_info_mojom->mime_type = "image/png";nit: consider these hard coded strings that are used in multiple places moving to a constant in the anon namespace
#include "third_party/webrtc/modules/desktop_capture/desktop_capturer.h"nit: needed? I don't see this used anywhere (same with the DEPS file)
Related details
- Foromo Daniel Soromou
- Hamzah Behery
Ken Buchanan (Gerrit)
Ken Buchanan voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ken Buchanan
| Code-Review | +1 |
3 comments
lgtm with one comment
Hamzah Beheryactually wait, is the returned token not used anywhere? I'm a little worried about exposing info that isn't used/checked in some way
Ari Chivukulanot currently, but I intend to implement the token's use in a subsequent CL in this stack to manage the screensharing state on the frontend.
should follow a similar pattern for image uploads:
when the WebUI invokes startScreenshare(), it receives the token in the promise resolution. It uses this token synchronously to open the chat window and show a loading placeholder chip. When the screenshot processing is finished, the browser process calls addFileContext(token, file_info) asynchronously, and the WebUI uses the token to match the file info (and base64 image data) to the placeholder chip.
okay that seems reasonable, leaving open for primary IPC reviewer
I agree that it's reasonable.
// Triggers screensharing (window or entire screen picker).The comment should mention the purpose of the token and when it will return null.
Related details
- Foromo Daniel Soromou
- Hamzah Behery
- Nihar Majmudar
- Tove Petersson
Hamzah Behery (Gerrit)
Hamzah Behery added 9 comments![]( "Open in Gerrit")
void set_bypass_ui_manager_for_testing(bool bypass) {nit: is this and `bypass_ui_manager_for_testing_` needed? I don't see them used in any tests
Done
#include "chrome/browser/ui/omnibox/omnibox_everywhere/omnibox_everywhere_ui_manager.h" // nognchecknit: needed? I don't see this used anywhere
removed
#include "chrome/browser/media/webrtc/desktop_media_picker.h" // nogncheck
#include "chrome/browser/media/webrtc/desktop_media_picker_controller.h" // nogncheck
#include "chrome/browser/media/webrtc/desktop_media_picker_factory_impl.h" // nogncheck
#include "chrome/browser/ui/browser_window/public/profile_browser_collection.h"
#include "chrome/browser/ui/omnibox/omnibox_everywhere/omnibox_everywhere_ui_manager.h" // nogncheckWhy do we need // nogncheck here? Can these be added to the BUILD file instead?
These were previously causing CQ to fail despite being inside the build flags, added to DEPS to fix this
Is there any chance that screenshare_picker_controller_ already exists here (a screen share is already in session) and we are rewriting this field? consider adding a guard at the beginning of StartScreenshare or FallbackToChromeDefaultPicker to check if this is already active
Done
similar concern here: should we check to see if we are already capturing something (check if is_capturing_ is true) before continuing this logic?
Done
ContextualSearchboxHandler::ProcessedScreenshot result;nit: is it expected that we never populate thumbnail_data_url?
currently yes. populating `thumbnail_data_url` is to be implemented in a subsequent CL in this stack (crrev.com/c/8232123) to keep this CL more focused. added a TODO comment to address this
file_info_mojom->file_name = "Screenshot.png";
file_info_mojom->mime_type = "image/png";nit: consider these hard coded strings that are used in multiple places moving to a constant in the anon namespace
Done
#include "third_party/webrtc/modules/desktop_capture/desktop_capturer.h"nit: needed? I don't see this used anywhere (same with the DEPS file)
unless Im mistaken, this is needed to implement `FakeDesktopCapturer` on line 3891
// Triggers screensharing (window or entire screen picker).The comment should mention the purpose of the token and when it will return null.
- Foromo Daniel Soromou
- Juliet Lévesque Knighton
- Nihar Majmudar
- Tove Petersson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Juliet Lévesque Knighton (Gerrit)
Juliet Lévesque Knighton voted and added 1 comment![]( "Open in Gerrit")
Votes added by Juliet Lévesque Knighton
| Code-Review | +1 |
- Foromo Daniel Soromou
- Hamzah Behery
- Nihar Majmudar
- Tove Petersson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou voted and added 1 comment![]( "Open in Gerrit")
Votes added by Foromo Daniel Soromou
| Code-Review | +1 |
1 comment
contextual_search::ContextualSearchSessionHandle*
GetContextualSessionHandleForTesting() {
return GetContextualSessionHandle();
}
Unsed, please remove.
Related details
- Hamzah Behery
- Nihar Majmudar
- Tove Petersson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Tove Petersson (Gerrit)
Tove Petersson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tove Petersson
| Code-Review | +1 |
1 comment
LGTM for chrome/browser/media/webrtc/fake_desktop_media_picker_factory.cc
Related details
- Hamzah Behery
- Nihar Majmudar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nihar Majmudar (Gerrit)
Nihar Majmudar added 8 comments![]( "Open in Gerrit")
raw_ptr<DesktopMediaPickerFactory> picker_factory_ = nullptr;
std::unique_ptr<content::desktop_capture::ScreenshotCaptureRequest>
active_screenshot_request_;
bool is_capturing_ = false;This can be move with the variable definition above? i.e. doesn't need separate buildflag
void FallbackToChromeDefaultPicker(bool prefer_entire_screen,
StartScreenshareCallback callback);
void OnChromeDefaultPickerResults(StartScreenshareCallback callback,
const std::string& err,
content::DesktopMediaID source);
void CaptureAndUploadScreenshot(content::DesktopMediaID source,
StartScreenshareCallback callback);
void OnScreenshotCaptured(StartScreenshareCallback callback,
const SkBitmap& bitmap);
void OnScreenshotRequestCreated(
std::unique_ptr<content::desktop_capture::ScreenshotCaptureRequest>
request);
void OnScreenshotProcessed(StartScreenshareCallback callback,
ProcessedScreenshot result);Can these be move to where the rest of the functions are defined? Not sure why there's some mixing of variable and function declaration above
#if !BUILDFLAG(IS_ANDROID)
void set_desktop_media_picker_factory_for_testing(
DesktopMediaPickerFactory* factory) {
picker_factory_ = factory;
}
#endifCan you combine with one of the other buildflags above
#include "media/base/media_switches.h"
#include "skia/ext/image_operations.h"
#include "ui/base/base_window.h"
#include "ui/base/mojom/ui_base_types.mojom-shared.h"
#include "ui/gfx/geometry/size.h"
#if !BUILDFLAG(IS_ANDROID)
#include "base/base64.h"
#include "base/compiler_specific.h"
#include "base/task/bind_post_task.h"
#include "chrome/browser/media/webrtc/desktop_media_picker.h"
#include "chrome/browser/media/webrtc/desktop_media_picker_controller.h"
#include "chrome/browser/media/webrtc/desktop_media_picker_factory_impl.h"
#include "chrome/browser/ui/browser_window/public/profile_browser_collection.h"
#include "chrome/browser/ui/omnibox/omnibox_everywhere_service.h"
#include "chrome/browser/ui/omnibox/omnibox_everywhere_service_factory.h"
#include "chrome/browser/ui/webui/top_chrome/webui_contents_wrapper.h"
#include "chrome/grit/branded_strings.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/desktop_capture.h"
#include "third_party/skia/include/core/SkCanvas.h"
#include "third_party/skia/include/core/SkImage.h"
#include "third_party/skia/include/core/SkPath.h"
#include "third_party/webrtc/modules/desktop_capture/desktop_capturer.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/gfx/codec/png_codec.h"Can you make sure all of these are needed? Doesn't look like all of them are used but not sure
constexpr char kScreenshotFileName[] = "Screenshot.png";
constexpr char kScreenshotMimeType[] = "image/png";
ContextualSearchboxHandler::ProcessedScreenshot ProcessScreenshotInBackground(
const SkBitmap& bitmap) {
ContextualSearchboxHandler::ProcessedScreenshot result;
std::optional<std::vector<uint8_t>> png_bytes =
gfx::PNGCodec::EncodeBGRASkBitmap(bitmap,
/*discard_transparency=*/false);
if (png_bytes) {
result.png_bytes = std::move(*png_bytes);
}
// TODO(crbug.com/532197177): Populate result.thumbnail_data_url with the
// optimized base64 thumbnail URL.
return result;
}Move logic in namepsace at the top of file namespace?
is_capturing_ = false;Should this be set after the screenshot is actually captured or is this safe here (i.e. a user can't trigger screenshot again in the middle of all of this)
file_info_mojom->image_data_url = result.thumbnail_data_url.value_or("");should this be std::nullopt since image_data_url is optional?
Related details
- Hamzah Behery
Hamzah Behery (Gerrit)
Hamzah Behery added 1 comment![]( "Open in Gerrit")
contextual_search::ContextualSearchSessionHandle*
GetContextualSessionHandleForTesting() {
return GetContextualSessionHandle();
}
Hamzah BeheryUnsed, please remove.
Done
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hamzah Behery (Gerrit)
Hamzah Behery added 8 comments![]( "Open in Gerrit")
raw_ptr<DesktopMediaPickerFactory> picker_factory_ = nullptr;
std::unique_ptr<content::desktop_capture::ScreenshotCaptureRequest>
active_screenshot_request_;
bool is_capturing_ = false;This can be move with the variable definition above? i.e. doesn't need separate buildflag
Done
void FallbackToChromeDefaultPicker(bool prefer_entire_screen,
StartScreenshareCallback callback);
void OnChromeDefaultPickerResults(StartScreenshareCallback callback,
const std::string& err,
content::DesktopMediaID source);
void CaptureAndUploadScreenshot(content::DesktopMediaID source,
StartScreenshareCallback callback);
void OnScreenshotCaptured(StartScreenshareCallback callback,
const SkBitmap& bitmap);
void OnScreenshotRequestCreated(
std::unique_ptr<content::desktop_capture::ScreenshotCaptureRequest>
request);
void OnScreenshotProcessed(StartScreenshareCallback callback,
ProcessedScreenshot result);Can these be move to where the rest of the functions are defined? Not sure why there's some mixing of variable and function declaration above
Done
#if !BUILDFLAG(IS_ANDROID)
void set_desktop_media_picker_factory_for_testing(
DesktopMediaPickerFactory* factory) {
picker_factory_ = factory;
}
#endifCan you combine with one of the other buildflags above
Done
#include "base/mac/mac_util.h"Hamzah Beheryneeded?
Done
Can you make sure all of these are needed? Doesn't look like all of them are used but not sure
Fixed, got left behind when this CL was split
constexpr char kScreenshotFileName[] = "Screenshot.png";
constexpr char kScreenshotMimeType[] = "image/png";
ContextualSearchboxHandler::ProcessedScreenshot ProcessScreenshotInBackground(
const SkBitmap& bitmap) {
ContextualSearchboxHandler::ProcessedScreenshot result;
std::optional<std::vector<uint8_t>> png_bytes =
gfx::PNGCodec::EncodeBGRASkBitmap(bitmap,
/*discard_transparency=*/false);
if (png_bytes) {
result.png_bytes = std::move(*png_bytes);
}
// TODO(crbug.com/532197177): Populate result.thumbnail_data_url with the
// optimized base64 thumbnail URL.
return result;
}Move logic in namepsace at the top of file namespace?
Done
is_capturing_ = false;Should this be set after the screenshot is actually captured or is this safe here (i.e. a user can't trigger screenshot again in the middle of all of this)
`is_capturing_` is meant to only guard the active OS capture session, which is complete once OnScreenshotCaptured receives the SkBitmap. The remaining flow is standard async attachment upload, so resetting it here allows subsequent captures without blocking on background PNG encoding or upload.
file_info_mojom->image_data_url = result.thumbnail_data_url.value_or("");should this be std::nullopt since image_data_url is optional?
Done
Related details
- Nihar Majmudar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nihar Majmudar (Gerrit)
Nihar Majmudar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nihar Majmudar
| Code-Review | +1 |
1 comment
Related details
- Hamzah Behery
Code-Coverage
Code-Owners
Hamzah Behery (Gerrit)
Hamzah Behery voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hamzah Behery (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
12 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[OmniboxEverywhere][3a/5] Implement screenshare Mojo definitions, capturing, and unit tests
Wires the frontend screenshare buttons to the C++ backend via Mojo IPC,
implements screenshot capturing, formatting, and adds test coverage.
This CL serves two major purposes:
1. It implements the integration and triggering of the Default Capture
Picker (utilizing WebRTC's DesktopCapturer to capture screenshots of
windows/screens).
2. It acts as the shared Mojo IPC controller and post-processor for both
the macOS Native Picker and the Default Capture Picker CLs
(crrev.com/c/8177909 and crrev.com/c/8179066), handling bitmap
formatting and attaching them to the searchbox session context.
Key implementation details:
- Declares the StartScreenshare Mojo interface method in
searchbox.mojom.
- Configures the fallback desktop media picker to present both Screen
and Window tabs, pre-selecting the appropriate tab based on the user's
initial selection (entire screen vs window).
- Implements Mojo page handler receivers to trigger screensharing and
post the task to content::desktop_capture::CaptureScreenshot (which
spawns the Default Capture Picker).
- Performs asynchronous post-processing on the captured SkBitmap (from
both pickers) to format it as PNG bytes and attaches the resulting
file context to the active OmniboxEverywhere session.
- Adds comprehensive unit tests for ContextualSearchboxHandler and
core screensharing logic.
- M chrome/browser/contextual_tasks/contextual_tasks_extension_handler.cc
- M chrome/browser/contextual_tasks/contextual_tasks_extension_handler.h
- M chrome/browser/media/webrtc/fake_desktop_media_picker_factory.cc
- M chrome/browser/ui/webui/DEPS
- M chrome/browser/ui/webui/cr_components/searchbox/BUILD.gn
- M chrome/browser/ui/webui/cr_components/searchbox/contextual_searchbox_handler.cc
- M chrome/browser/ui/webui/cr_components/searchbox/contextual_searchbox_handler.h
- M chrome/browser/ui/webui/cr_components/searchbox/searchbox_handler.h
- M chrome/browser/ui/webui/searchbox/DEPS
- M chrome/browser/ui/webui/searchbox/contextual_searchbox_handler_unittest.cc
- M chrome/test/data/webui/cr_components/searchbox/test_searchbox_browser_proxy.ts
- M chrome/test/data/webui/lens/overlay/test_searchbox_browser_proxy.ts
- M chrome/test/data/webui/omnibox_everywhere/test_searchbox_browser_proxy.ts
- M components/omnibox/browser/searchbox.mojom
Code-Review: +1 by Ari Chivukula, +1 by Foromo Daniel Soromou, +1 by Ken Buchanan, +1 by Tove Petersson, +1 by Juliet Lévesque Knighton, +1 by Nihar Majmudar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |