[Indigo] Anchor toolbar by tracking image bounds [chromium/src : main]
Jeremy Roman (Gerrit)
Jeremy Roman added 1 comment![]( "Open in Gerrit")
This doesn't deal with headers that occlude the content (the next CL in the chain does -- it needs additional cleanup but I've ran the basic idea by Khushal). But it does do most of the indigo toolbar changes to make it start to track the element.
Related details
- Adithya Srinivasan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adithya Srinivasan (Gerrit)
Adithya Srinivasan voted and added 10 comments![]( "Open in Gerrit")
Votes added by Adithya Srinivasan
| Code-Review | +1 |
10 comments
Just have some requests for test cases, and questions about resetting
}Could we update this test/add a new test to scroll the page (well make the page scrollable first) and check that the toolbar bounds are updated?
const std::optional<base::Token>& tracked_element_id);nit: this can just be `const base::Token&` right?
#include "components/viz/common/surfaces/tracked_element_rects.h"nit: is this include still needed or can it just be forward declared?
}should we also be resetting tracked_bounds_ here?
I guess the toolbar is being hidden so it doesn't strictly matter; but I'm not sure what happens when we call ShowToolbar the next time (will a new value already be sent before that or will it be anchored to the previous primary's last known bounds?)
current_host_ = host;should we reset tracked_bounds_ here? (might not need to if we do so in Reset())
gfx::Point toolbar_top_right = top_right + *corner_offset;checking my understanding for some edge cases here:
if the image is partially in the viewport (lets say the top half is not in the viewport), the toolbar will be offset from the top right of visible half?
if we scroll down partially from the previous case (so more of the image is out of the viewport, but not all of it), the toolbar will not move?
if we scroll down all the way and the image is completely out of the viewport, the toolbar will disappear?
(would be nice to have tests for these cases too 😊)
view->SetVisible(!rect.IsEmpty());do we also need to initialize the toolbar with `SetVisible(false)`?
edit: I saw that your test checks that we still see the toolbar (using initial bounds), so maybe this is WAI
TEST_F(IndigoToolbarTest, ToolbarBounds) {Could we also test that the toolbar is hidden when the (updated) tracked position is empty?
int32 tracked_element_feature_id);Should we make this field optional (as a way of indicating that if we want to track the bounds)? We don't really need to track non-primary images right now.
Related details
- Jeremy Roman
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. |
Jeremy Roman (Gerrit)
Jeremy Roman added 10 comments![]( "Open in Gerrit")
Could we update this test/add a new test to scroll the page (well make the page scrollable first) and check that the toolbar bounds are updated?
Added such a test, including partially and completely off screen.
const std::optional<base::Token>& tracked_element_id);nit: this can just be `const base::Token&` right?
not anymore now that (as you suggested elsewhere) non-primary replacements don't track
#include "components/viz/common/surfaces/tracked_element_rects.h"nit: is this include still needed or can it just be forward declared?
Can't be forward-declared, as it's a type alias. (We could forward-declare the underlying type and then declare the type alias as well, but then it would be harder to change.)
should we also be resetting tracked_bounds_ here?
I guess the toolbar is being hidden so it doesn't strictly matter; but I'm not sure what happens when we call ShowToolbar the next time (will a new value already be sent before that or will it be anchored to the previous primary's last known bounds?)
Done, it's now reset.
should we reset tracked_bounds_ here? (might not need to if we do so in Reset())
ClearTrackedBoundsAndHideToolbar is now called when we might have lost track of the tracked element.
should we reset tracked_bounds_ here? (might not need to if we do so in Reset())
Done, we now call ClearTrackedBoundsAndHideToolbar when we stop being able to track the element.
gfx::Point toolbar_top_right = top_right + *corner_offset;checking my understanding for some edge cases here:
if the image is partially in the viewport (lets say the top half is not in the viewport), the toolbar will be offset from the top right of visible half?
if we scroll down partially from the previous case (so more of the image is out of the viewport, but not all of it), the toolbar will not move?
if we scroll down all the way and the image is completely out of the viewport, the toolbar will disappear?
(would be nice to have tests for these cases too 😊)
Yeah, it is offset from the visible half. This wasn't what was obvious but it was easiest with the way tracked elements already work, and feels pretty reasonable. So your understanding is correct on all of those, and there are now tests as well.
do we also need to initialize the toolbar with `SetVisible(false)`?
edit: I saw that your test checks that we still see the toolbar (using initial bounds), so maybe this is WAI
Changed to eliminate --force-indigo-toolbar which keeps this a bit simpler, we now only show it in response to tracked element rects. With that change, we can indeed make it initially invisible.
Could we also test that the toolbar is hidden when the (updated) tracked position is empty?
Done
Should we make this field optional (as a way of indicating that if we want to track the bounds)? We don't really need to track non-primary images right now.
- Adithya Srinivasan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeremy Roman (Gerrit)
Jeremy Roman added 1 comment![]( "Open in Gerrit")
vmpstr for components/page_content_annotations/ and components/viz/
chrome-ipc-reviews for *.mojom
Related details
- Adithya Srinivasan
- Chromium IPC Reviews
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeremy Roman (Gerrit)
Jeremy Roman added 1 comment![]( "Open in Gerrit")
arg, vmpstr doesn't own components/page_content_annotations/
+averge for components/page_content_annotations/
Related details
- Adithya Srinivasan
- Andrew Verge
- Chromium IPC Reviews
- Vladimir Levin
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: joenot...@google.com
📎 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): joenot...@google.com. Please wait for the shadowed IPC reviewer to CR+1 before reviewing.
Shadowed: ari...@chromium.org
Reviewer source(s):
ari...@chromium.org, joenot...@google.com is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Adithya Srinivasan
- Andrew Verge
- Ari Chivukula
- Joe Mason
- Vladimir Levin
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 added 3 comments![]( "Open in Gerrit")
(shadow) IPC LGTM
// `tracked_element_feature_id`: The ID of the tracked element feature to use`The ID of the tracked element feature to use for tracking.` feels redundant, maybe but the first 'tracked'?
// `tracked_element_id`: The ID of the tracked element associated with thesame here
Related details
- Adithya Srinivasan
- Andrew Verge
- Jeremy Roman
- Joe Mason
- Vladimir Levin
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 Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Adithya Srinivasan
- Andrew Verge
- Jeremy Roman
- Joe Mason
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Andrew Verge (Gerrit)
Andrew Verge voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Adithya Srinivasan
Adithya Srinivasan (Gerrit)
Adithya Srinivasan voted and added 3 comments![]( "Open in Gerrit")
Votes added by Adithya Srinivasan
| Code-Review | +1 |
3 comments
remote->StartReplacement(std::move(host_remote), feature_id);Shouldn't this be std::nullopt if !is_primary?
if (!found) {Is it guaranteed that every notification will have an entry for all tracked elements? I'm wondering if it just sends an update for an unrelated tracked element, and the element we care about hasn't moved (and is still visible).
void IndigoPageActionController::ClearTrackedBoundsAndHideToolbar() {nit: might be worth renaming the pre-existing HideToolbar method to ResetToolbar or DestroyToolbar
Related details
Ari Chivukula (Gerrit)
Ari Chivukula voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ari Chivukula
| Code-Review | +0 |
1 comment
mojo_base.mojom.Token? tracked_element_id);this should be using mojo_base.mojom.UnguessableToken to ensure future callsites aren't forgetting the required randomness (this blocks deterministic token constructors outside of tests).
Joe Mason (Gerrit)
Joe Mason voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joe Mason
| Code-Review | +1 |
1 comment
mojo_base.mojom.Token? tracked_element_id);this should be using mojo_base.mojom.UnguessableToken to ensure future callsites aren't forgetting the required randomness (this blocks deterministic token constructors outside of tests).
I agree, but the same `tracked_element_id` is also used with viz structs in services/viz/public/mojom/compositing/tracked_element_rects.mojom, so I think switching to UnguessableToken is out of scope for this patch. I think it's safe as-is, for now, but the TrackedElementRects system should be improved as a followup:
1. Use UnguessableToken, not Token, to make sure no user of the system can accidentally create tokens that can be spoofed by another renderer.
2. viz::TrackedElementRect should contain a renderer id (which is set on the browser side based on the renderer the token's received from), and comparisons should always include both the `tracked_element_id` token and the renderer id. That way even if a renderer gets ahold of another renderer's token, and presents it to the browser, the browser won't consider it the "same" token. (TrackedElementRect already contains an optional LocalFrameToken field - keying by tracked_element_id & LocalFrame is also possible, since it would be even stricter than tracked_element_id and renderer id, but possibly too strict. It would require LocalFrame to be non-optional.)
The security issue this is preventing is for a renderer to spoof some UI by associating its data with another renderer's element rect. For instance in this patch, if a compromised renderer sent `ReplacementFrameAttached(..., some_other_renderers_tracked_element_id)`, them when `IndigoPageActionController::OnTrackedElementRectsChanged` is called the toolbar would be moved to the other renderer's element.
Related details
- Jeremy Roman
- Vladimir Levin
Joe Mason (Gerrit)
Joe Mason added 1 comment![]( "Open in Gerrit")
mojo_base.mojom.Token? tracked_element_id);Joe Masonthis should be using mojo_base.mojom.UnguessableToken to ensure future callsites aren't forgetting the required randomness (this blocks deterministic token constructors outside of tests).
I agree, but the same `tracked_element_id` is also used with viz structs in services/viz/public/mojom/compositing/tracked_element_rects.mojom, so I think switching to UnguessableToken is out of scope for this patch. I think it's safe as-is, for now, but the TrackedElementRects system should be improved as a followup:
1. Use UnguessableToken, not Token, to make sure no user of the system can accidentally create tokens that can be spoofed by another renderer.
2. viz::TrackedElementRect should contain a renderer id (which is set on the browser side based on the renderer the token's received from), and comparisons should always include both the `tracked_element_id` token and the renderer id. That way even if a renderer gets ahold of another renderer's token, and presents it to the browser, the browser won't consider it the "same" token. (TrackedElementRect already contains an optional LocalFrameToken field - keying by tracked_element_id & LocalFrame is also possible, since it would be even stricter than tracked_element_id and renderer id, but possibly too strict. It would require LocalFrame to be non-optional.)
The security issue this is preventing is for a renderer to spoof some UI by associating its data with another renderer's element rect. For instance in this patch, if a compromised renderer sent `ReplacementFrameAttached(..., some_other_renderers_tracked_element_id)`, them when `IndigoPageActionController::OnTrackedElementRectsChanged` is called the toolbar would be moved to the other renderer's element.
Actually I think the best way to implement #2 is for `viz::TrackedElementId` to be a `pair<UnguessableToken, GlobalRenderFrameHostToken>`. In mojo the `tracked_element_id` will only be an UnguessableToken, and the code that reads it in the browser fills in the RenderFrameHost token when assigning it to a TrackedElementId. That way any code that compares two `TrackedElementId` values automatically includes the originating renderer.
Jeremy Roman (Gerrit)
Jeremy Roman added 6 comments![]( "Open in Gerrit")
remote->StartReplacement(std::move(host_remote), feature_id);Shouldn't this be std::nullopt if !is_primary?
Done
if (!found) {Is it guaranteed that every notification will have an entry for all tracked elements? I'm wondering if it just sends an update for an unrelated tracked element, and the element we care about hasn't moved (and is still visible).
Yes, it's my understanding that we get a complete update with every RenderFrameMetadata.
void IndigoPageActionController::ClearTrackedBoundsAndHideToolbar() {nit: might be worth renaming the pre-existing HideToolbar method to ResetToolbar or DestroyToolbar
Done (DestroyToolbar).
// `tracked_element_feature_id`: The ID of the tracked element feature to use`The ID of the tracked element feature to use for tracking.` feels redundant, maybe but the first 'tracked'?
Tried to clear it up.
// `tracked_element_id`: The ID of the tracked element associated with thesame here
Similarly added some clarification.
mojo_base.mojom.Token? tracked_element_id);Joe Masonthis should be using mojo_base.mojom.UnguessableToken to ensure future callsites aren't forgetting the required randomness (this blocks deterministic token constructors outside of tests).
Joe MasonI agree, but the same `tracked_element_id` is also used with viz structs in services/viz/public/mojom/compositing/tracked_element_rects.mojom, so I think switching to UnguessableToken is out of scope for this patch. I think it's safe as-is, for now, but the TrackedElementRects system should be improved as a followup:
1. Use UnguessableToken, not Token, to make sure no user of the system can accidentally create tokens that can be spoofed by another renderer.
2. viz::TrackedElementRect should contain a renderer id (which is set on the browser side based on the renderer the token's received from), and comparisons should always include both the `tracked_element_id` token and the renderer id. That way even if a renderer gets ahold of another renderer's token, and presents it to the browser, the browser won't consider it the "same" token. (TrackedElementRect already contains an optional LocalFrameToken field - keying by tracked_element_id & LocalFrame is also possible, since it would be even stricter than tracked_element_id and renderer id, but possibly too strict. It would require LocalFrame to be non-optional.)
The security issue this is preventing is for a renderer to spoof some UI by associating its data with another renderer's element rect. For instance in this patch, if a compromised renderer sent `ReplacementFrameAttached(..., some_other_renderers_tracked_element_id)`, them when `IndigoPageActionController::OnTrackedElementRectsChanged` is called the toolbar would be moved to the other renderer's element.
Actually I think the best way to implement #2 is for `viz::TrackedElementId` to be a `pair<UnguessableToken, GlobalRenderFrameHostToken>`. In mojo the `tracked_element_id` will only be an UnguessableToken, and the code that reads it in the browser fills in the RenderFrameHost token when assigning it to a TrackedElementId. That way any code that compares two `TrackedElementId` values automatically includes the originating renderer.
I don't think the risk here is huge, as the browser has to choose to look for these tokens. In the case of this particular feature, the objects which receive these IDs don't even exist unless the user has activated the feature on the tab, and the possibility for a malicious renderer to lie about the generated image's position on the page is pretty unavoidable and consistent with the threat model that a main page renderer controls its own content.
If you'd like to explore improvements to use UnguessableToken or something else, would you mind filing that as a separate bug against viz/cc, as I wasn't involved in the original choice to use base::Token and don't know what other factors might have motivated the current choice.
Related details
- Ari Chivukula
- Joe Mason
- Vladimir Levin
Ari Chivukula (Gerrit)
Ari Chivukula voted Code-Review+1![]( "Open in Gerrit")
- Jeremy Roman
- Joe Mason
- Vladimir Levin
Vladimir Levin (Gerrit)
Vladimir Levin voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Jeremy Roman
- Joe Mason
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. |
Jeremy Roman (Gerrit)
Jeremy Roman voted and added 3 comments![]( "Open in Gerrit")
Votes added by Jeremy Roman
| Commit-Queue | +2 |
3 comments
// `tracked_element_feature_id`: The ID of the tracked element feature to useJeremy Roman`The ID of the tracked element feature to use for tracking.` feels redundant, maybe but the first 'tracked'?
Tried to clear it up.
Resolving.
// `tracked_element_id`: The ID of the tracked element associated with theJeremy Romansame here
Similarly added some clarification.
Resolving.
Joe Masonthis should be using mojo_base.mojom.UnguessableToken to ensure future callsites aren't forgetting the required randomness (this blocks deterministic token constructors outside of tests).
Joe MasonI agree, but the same `tracked_element_id` is also used with viz structs in services/viz/public/mojom/compositing/tracked_element_rects.mojom, so I think switching to UnguessableToken is out of scope for this patch. I think it's safe as-is, for now, but the TrackedElementRects system should be improved as a followup:
1. Use UnguessableToken, not Token, to make sure no user of the system can accidentally create tokens that can be spoofed by another renderer.
2. viz::TrackedElementRect should contain a renderer id (which is set on the browser side based on the renderer the token's received from), and comparisons should always include both the `tracked_element_id` token and the renderer id. That way even if a renderer gets ahold of another renderer's token, and presents it to the browser, the browser won't consider it the "same" token. (TrackedElementRect already contains an optional LocalFrameToken field - keying by tracked_element_id & LocalFrame is also possible, since it would be even stricter than tracked_element_id and renderer id, but possibly too strict. It would require LocalFrame to be non-optional.)
The security issue this is preventing is for a renderer to spoof some UI by associating its data with another renderer's element rect. For instance in this patch, if a compromised renderer sent `ReplacementFrameAttached(..., some_other_renderers_tracked_element_id)`, them when `IndigoPageActionController::OnTrackedElementRectsChanged` is called the toolbar would be moved to the other renderer's element.
Jeremy RomanActually I think the best way to implement #2 is for `viz::TrackedElementId` to be a `pair<UnguessableToken, GlobalRenderFrameHostToken>`. In mojo the `tracked_element_id` will only be an UnguessableToken, and the code that reads it in the browser fills in the RenderFrameHost token when assigning it to a TrackedElementId. That way any code that compares two `TrackedElementId` values automatically includes the originating renderer.
I don't think the risk here is huge, as the browser has to choose to look for these tokens. In the case of this particular feature, the objects which receive these IDs don't even exist unless the user has activated the feature on the tab, and the possibility for a malicious renderer to lie about the generated image's position on the page is pretty unavoidable and consistent with the threat model that a main page renderer controls its own content.
If you'd like to explore improvements to use UnguessableToken or something else, would you mind filing that as a separate bug against viz/cc, as I wasn't involved in the original choice to use base::Token and don't know what other factors might have motivated the current choice.
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Indigo] Anchor toolbar by tracking image bounds
This implements relative positioning for the Indigo page action toolbar,
anchoring it to the top-right corner of the tracked image element.
Detailed changes:
- Blink: Register the replaced image element with the compositor as a
tracked element, and pass the tracking token.
- Browser UI: Update IndigoToolbar's layout manager to position the
toolbar relative to the tracked rect bounds.
- Page Action: Observe tracked element bounds updates from the widget
host, scale them to DIPs, and forward them to the toolbar.
- FakeApi: Add StartAcceptingConnectionsAutomatic() to automatically
answer API requests in tests, keeping UI tests clean.
- Tests: Add integration tests verifying relative positioning.
All changes are fully gated by the Indigo feature flag.
TAG=agy
CONV=63829320-975e-444d-bc37-e8f15924464c
- M chrome/browser/indigo/fake_api.cc
- M chrome/browser/indigo/fake_api.h
- M chrome/browser/indigo/indigo_browsertest.cc
- M chrome/browser/indigo/indigo_image_replacement.cc
- M chrome/browser/indigo/indigo_image_replacement.h
- M chrome/browser/indigo/indigo_image_replacement_manager.cc
- M chrome/browser/indigo/indigo_image_replacement_manager.h
- M chrome/browser/indigo/indigo_image_replacement_manager_browsertest.cc
- M chrome/browser/indigo/indigo_page_action_controller.cc
- M chrome/browser/indigo/indigo_page_action_controller.h
- M chrome/browser/indigo/indigo_page_action_controller_unittest.cc
- M chrome/browser/ui/views/indigo/indigo_toolbar.cc
- M chrome/browser/ui/views/indigo/indigo_toolbar.h
- M chrome/browser/ui/views/indigo/indigo_toolbar_unittest.cc
- M components/page_content_annotations/core/tracked_element_feature.h
- M components/viz/common/surfaces/tracked_element_rects.h
- M third_party/blink/public/mojom/image_replacement/image_replacement.mojom
- M third_party/blink/renderer/core/image_replacement/image_replacement.cc
- M third_party/blink/renderer/core/image_replacement/image_replacement.h
- M third_party/blink/renderer/core/image_replacement/image_replacement_test.cc
Code-Review: +1 by Andrew Verge, +1 by Vladimir Levin, +1 by Adithya Srinivasan, +1 by Ari Chivukula, +1 by Joe Mason
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuzu Saijo (Gerrit)
Yuzu Saijo has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |