[AiOverlayDialog] SetText tool [chromium/src : main]
Gary Klassen (Gerrit)
Gary Klassen added 9 comments![]( "Open in Gerrit")
Thanks for the reviews! This change is muuuuuuch better for it. Look ok?
} else {Gary KlassenWhat is `element_id` representing here? The comment in the mojom says this has to be a numeric ID but we're falling back to alpha identifier below?
Done
base::WriteJson(base::Value(selector_expr)).value_or("\"\"");Gary KlassenWhy `WriteJson`? What does this do? Why can't we just put the selector directly in the string below?
Done
" const plain = raw.replace(/<[^>]*>/g, '');\n",Gary KlassenA large block of JS is very hard to parse what's going on for a reader...if we keep this, we should at least add some comments to make it more digestable.
Done
" el.dispatchEvent(new ClipboardEvent('paste', {\n",Gary KlassenAre we...trying to paste the data _in addition to_ using a property setter / innerHTML above? This isn't in an `else` block...
Done
rfh->ExecuteJavaScriptInIsolatedWorld(Gary KlassenThis is suspect...injecting JS into the page for a browser feature is very unusual. On a practical level I suspect this causes a bunch of extra memory and device usage to spin up the JS machinery for a new world. It also introduces the possibility of colliding with page JS and is generally opaque about what we're doing.
Dictation will be adding some machinery here that could help. In the mean time I'd suggest re-using or taking an approach similar to actor [TypeTool](https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/actor.mojom;l=71;drc=25b5ecdbad3b09f037021101fa78c28fa258ca5f)
Done
element->setAttribute(AtomicString("data-element-id"),Gary KlassenWe shouldn't mutate an author's DOM for a builtin browser feature. APC already gives you a DOMNodeId which is stable. We need a object in the renderer for AiOverlay to take that DOMNodeId+string, convert back from DOMNodeId to DOM element and send it KeyEvents/DataTransfer like GiC
Done
element->setAttribute(AtomicString("data-element-id"),Gary KlassenWe shouldn't mutate an author's DOM for a builtin browser feature. APC already gives you a DOMNodeId which is stable. We need a object in the renderer for AiOverlay to take that DOMNodeId+string, convert back from DOMNodeId to DOM element and send it KeyEvents/DataTransfer like GiC
Done
Related details
- David Bokan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan voted and added 6 comments![]( "Open in Gerrit")
Votes added by David Bokan
| Code-Review | +1 |
6 comments
few minor comments but lgtm!
base::WeakPtrFactory<AiOverlayTools> weak_ptr_factory_{this};Unused
#include "chrome/common/chrome_isolated_world_ids.h"This and json_writer are unused now
std::move(callback).Run(base::unexpected("Invalid numeric element id"));Any reason we cant define element_id as an int in the mojom API? (i.e. client has to convert the markdown extracted value to int)
if (res && res->code == actor::mojom::ActionResultCode::kOk) {This should be guaranteed non-null, you can omit or CHECK this
GURL("data:text/html,<html><body><input id='test_input' "nit: omit the html and body tags in tests to avoid clutter and avoid the trailing '/' in <input>'s closing tag
Related details
- Gary Klassen
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. |
Gary Klassen (Gerrit)
Gary Klassen abandoned this change.
Gary Klassen abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |