Implement ScrollToolJavaScriptFeature. [chromium/src : main]
Kirubel Aklilu (Gerrit)
Kirubel Aklilu added 3 comments![]( "Open in Gerrit")
void ExecuteScrollAction(Kirubel AkliluNit: Would you mind elaborate the comments? Until I look into scroll_tool.ts, I keep wondering whether this executes as a "scroll from" or "scroll to" action.
The comments on `Scroll` and `ScrollTo` could use some elaboration as well. It would be nice to distinguish between "scrolling on an **already scrollable** element" and "scrolling something (that might be on out-of-frame on a parent scroll view) into visibility."
I elaborated a bit more here in the helper comment. I considered writing more in the main method comments but I didn't want to cover implementation in depth there
(e.g. the target may be scrollable, or might not exist, etc).
IIRC function comments are supposed to just explain the contract, not the details. WDYT?
CHECK(action.has_target());Kirubel AkliluPlease handle cases where there is not a target specified.
Thanks for catching this! I've added a TODO to handle this in a followup since we'll need to update both the JS and this class to handle when the target it omitted.
CHECK(target.has_coordinate() ||
(target.has_content_node_id() && target.has_document_identifier()));Kirubel AkliluI'm starting to think whether we should have performed `CHECK` on the `action` object or anything from it. IIUC the `action` object is provided by Gemini, so performing `CHECK` on this might violate this guideline: https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/checks.md#failures-beyond-chromium_s-control
Maybe we should just return an `ActorToolError` for these cases?
The plan is to return an ActorToolError before even hitting this line. For example, see [this validation](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/intelligence/actor/tools/model/click_tool.mm;l=23;drc=4adc3020e762918c9a0d1866d92abc58cad79099) in click_tool.mm before we CHECK for those invariants later on [here](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/intelligence/actor/tools/model/click_tool_java_script_feature.mm;l=42-44;drc=7738ab7b58441e7c78447a7e2154ca9943efb656).
You can get an early look at these checks in scroll_tool.mm in https://crrev.com/c/7727593.
Related details
- Ginny Huang
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. |
Ginny Huang (Gerrit)
Ginny Huang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ginny Huang
| Code-Review | +1 |
2 comments
void ExecuteScrollAction(Kirubel AkliluNit: Would you mind elaborate the comments? Until I look into scroll_tool.ts, I keep wondering whether this executes as a "scroll from" or "scroll to" action.
The comments on `Scroll` and `ScrollTo` could use some elaboration as well. It would be nice to distinguish between "scrolling on an **already scrollable** element" and "scrolling something (that might be on out-of-frame on a parent scroll view) into visibility."
I elaborated a bit more here in the helper comment. I considered writing more in the main method comments but I didn't want to cover implementation in depth there
(e.g. the target may be scrollable, or might not exist, etc).IIRC function comments are supposed to just explain the contract, not the details. WDYT?
LGTM to the current version of `ExecuteScrollAction`. I still think `Scroll` and `ScrollTo` should be better differentiated, but up to you :)
IIRC function comments are supposed to just explain the contract, not the details. WDYT?
Agree but we should still "explain the contract." Right now the difference between "Scroll" (I personally think its been given a bad name by the designer of actions_data) and "ScrollTo" is too far from self-explanatory for people without context. IMO, to unwitting debuggers, the difference between "Attempts to scroll an element in the given WebFrame." and "Attempts to scroll the given WebFrame to an element." looks a bit subtle 🤔
// TODO: crbug.com/472289079 - Add support for ScrollAction with the targetTIL... thanks!
(We usually do `TODO(crbug.com/123123123): Thing to do.` but if it passes presubmit check, it must be good to use!)
Related details
- Kirubel Aklilu
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. |
Kirubel Aklilu (Gerrit)
Kirubel Aklilu added 1 comment![]( "Open in Gerrit")
void ExecuteScrollAction(Kirubel AkliluNit: Would you mind elaborate the comments? Until I look into scroll_tool.ts, I keep wondering whether this executes as a "scroll from" or "scroll to" action.
The comments on `Scroll` and `ScrollTo` could use some elaboration as well. It would be nice to distinguish between "scrolling on an **already scrollable** element" and "scrolling something (that might be on out-of-frame on a parent scroll view) into visibility."
Ginny HuangI elaborated a bit more here in the helper comment. I considered writing more in the main method comments but I didn't want to cover implementation in depth there
(e.g. the target may be scrollable, or might not exist, etc).IIRC function comments are supposed to just explain the contract, not the details. WDYT?
LGTM to the current version of `ExecuteScrollAction`. I still think `Scroll` and `ScrollTo` should be better differentiated, but up to you :)
IIRC function comments are supposed to just explain the contract, not the details. WDYT?
Agree but we should still "explain the contract." Right now the difference between "Scroll" (I personally think its been given a bad name by the designer of actions_data) and "ScrollTo" is too far from self-explanatory for people without context. IMO, to unwitting debuggers, the difference between "Attempts to scroll an element in the given WebFrame." and "Attempts to scroll the given WebFrame to an element." looks a bit subtle 🤔
I agree that the naming could be confusing and that the prior comments only differed subtly. I elaborated a bit more in the latest comment and I think it should make it clearer.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kirubel Aklilu (Gerrit)
Kirubel Aklilu voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kirubel Aklilu (Gerrit)
Kirubel Aklilu added 1 comment![]( "Open in Gerrit")
gambard@, can you review the change to ios/chrome/browser/web/model/chrome_web_client.mm?
Related details
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kirubel Aklilu (Gerrit)
Kirubel Aklilu added 1 comment![]( "Open in Gerrit")
rohitrao@, can you review the change to ios/chrome/browser/web/model/chrome_web_client.mm?
Related details
- Rohit Rao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |