Add setSelection function to automation API.

Add setSelection function to automation API.

The new chrome.automation.setSelection function takes two (node, offset)
pairs, allowing selection to be set over a range in a tree.

BUG=523099
Committed: https://crrev.com/96201023fc5bcabb1f409d9d23fb9b53fcc44262
Cr-Commit-Position: refs/heads/master@{#352339}

[Peter Lundblad, 2015-09-23 08:29:42]

plundblad@chromium.org changed reviewers:
+ dtseng@chromium.org, nasko@chromium.org

[Peter Lundblad, 2015-09-23 08:29:42]

dtseng: for all of the cl.
nasko: for content/browser/frame_host/... 
content/public/browser/render_frame_host.h
content/common/accessibility_messages.h
kalman: for the api changes reflected in the .idl files.

[nasko, 2015-09-23 16:08:35]

I've only looked at content. The code looks good, but style is off. I would
suggest running "git cl format" on the changes to ensure the CL is style
compliant with very little effort.

https://codereview.chromium.org/1365433002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1365433002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:534: int focus_object_id,
int focus_offset) {
Each parameter should be on a new line.

https://codereview.chromium.org/1365433002/diff/40001/content/renderer/access...
File content/renderer/accessibility/renderer_accessibility.cc (right):

https://codereview.chromium.org/1365433002/diff/40001/content/renderer/access...
content/renderer/accessibility/renderer_accessibility.cc:506: int
anchor_acc_obj_id, int anchor_offset,
Each parameter on a new line.

https://codereview.chromium.org/1365433002/diff/40001/content/renderer/access...
File content/renderer/accessibility/renderer_accessibility.h (right):

https://codereview.chromium.org/1365433002/diff/40001/content/renderer/access...
content/renderer/accessibility/renderer_accessibility.h:113: void
OnSetSelection(int anchor_acc_obj_id, int anchor_offset,
Each parameter should be on a new line. Running "git cl format" helps avoid
thinking about style much.

[dmazzoni, 2015-09-23 17:57:04]

dmazzoni@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2015-09-23 17:57:05]

lgtm

[David Tseng, 2015-09-23 17:57:42]

https://codereview.chromium.org/1365433002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/automation/automation_apitest.cc (right):

https://codereview.chromium.org/1365433002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/automation/automation_apitest.cc:170:
IN_PROC_BROWSER_TEST_F(AutomationApiTest, DesktopSelection) {
FYI; I used desktop mainly to mean the views+web trees. Selection may better
belong under the tabs directory.

https://codereview.chromium.org/1365433002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/automation_internal/automation_action_adapter.h
(right):

https://codereview.chromium.org/1365433002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/automation_internal/automation_action_adapter.h:26:
// selection in text fields.
Needs more detail either here or in the idl (e.g. in the case of a frame, does
the whole frame get selected, if it supports reversed selections, how the
offsets work when applied to a non-static text node, etc).

https://codereview.chromium.org/1365433002/diff/40001/chrome/browser/ui/aura/...
File chrome/browser/ui/aura/accessibility/automation_manager_aura.cc (right):

https://codereview.chromium.org/1365433002/diff/40001/chrome/browser/ui/aura/...
chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:110: return;
NOTREACHED here, right?

https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:431: long? focusOffset;
Please document all of these -- they get picked up as part of the public
documentation for chrome.automation.

https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:566: // Sets the selection in a
tree.
Needs more detail. The anchor and focus nodes need to come from the same tree
right? (same tree here means node1.root === node2.root). Also, this doesn't work
for nodes where root.role == 'desktop'.

https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:567: [nocompile] static void
setSelection(
This is now a little confusing as there's two versions of setSelection (one on
chrome.automation and one on the AutomationNode). The AutomationNode version
needs its documentation updated as it's no longer restricted to text fields.
Maybe call the former chrome.automation.setDocumentSelection or something along
those lines.

https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:568: [instanceOf=AutomationNode]
object anchorObject,
Does the instanceOf property do anything here?

https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:569: long anchorOffset,
Rather than this list of params, use a dictionary (similar to find).

https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/automation_internal.idl (right):

https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation_internal.idl:41: // automationNodeID of
performActionRequiredParams is used for the
nit: one space between sentences in this file.
Comment is somewhat confusing. Maybe
Reuses ActionRequiredParams automationNodeID to mean anchor node id, and treeID
to apply to both anchor and focus node id's.

https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js
(right):

https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js:29:
assertTrue(!!textField);
nit: assertNotNullNorUndefined

https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js:31:
listenOnce(document, EventType.textSelectionChanged, function() {
Can you verify the event param's target as you expected? Also, is this
textSelectionChanged event triggered by focus above?

https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js:36:
// Wait for another text selection change event.  There's one for
Is this potentially flakey?

https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js:38:
listenOnce(document, EventType.textSelectionChanged, function() {
Ditto; vierfy event target.

https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js:40:
listenOnce(document, EventType.textSelectionChanged, function(evt) {
Ditto.

https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js:53:
runWithDocument(html, function(document) {
document here as a param looks pretty weird since we're in a dom.

https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js:54:
assertEq(undefined, document.anchorrObject);
Typo? s/anchorr/anchor

[David Tseng, 2015-09-23 19:10:42]

https://codereview.chromium.org/1365433002/diff/40001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/1365433002/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation_custom_bindings.js:139:
(anchorNodeImpl.id !== focusNodeImpl.id)) {
Also, forgot to add that I think this API should just disallow setting
selections to any node who's root is the desktop node. You can just use the
other setSelection API in that case.

[Peter Lundblad, 2015-09-25 14:45:48]

nasko@chromium.org writes:
> I've only looked at content. The code looks good, but style is off. I would
> suggest running "git cl format" on the changes to ensure the CL is style
> compliant with very little effort.

Tahks, I addressed all of your style comments below.

>
https://codereview.chromium.org/1365433002/diff/40001/content/browser/frame_h...
> File content/browser/frame_host/render_frame_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1365433002/diff/40001/content/browser/frame_h...
> content/browser/frame_host/render_frame_host_impl.cc:534: int
> focus_object_id, int focus_offset) {
> Each parameter should be on a new line.

Done.

>
https://codereview.chromium.org/1365433002/diff/40001/content/renderer/access...
> File content/renderer/accessibility/renderer_accessibility.cc (right):
> 
>
https://codereview.chromium.org/1365433002/diff/40001/content/renderer/access...
> content/renderer/accessibility/renderer_accessibility.cc:506: int
> anchor_acc_obj_id, int anchor_offset,
> Each parameter on a new line.

Done.

>
https://codereview.chromium.org/1365433002/diff/40001/content/renderer/access...
> File content/renderer/accessibility/renderer_accessibility.h (right):
> 
>
https://codereview.chromium.org/1365433002/diff/40001/content/renderer/access...
> content/renderer/accessibility/renderer_accessibility.h:113: void
> OnSetSelection(int anchor_acc_obj_id, int anchor_offset,
> Each parameter should be on a new line. Running "git cl format" helps
> avoid thinking about style much.

Done.

> https://codereview.chromium.org/1365433002/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Lundblad, 2015-09-25 14:59:40]

dtseng@chromium.org via codereview.chromium.org writes:
> 
> 
> 
> 
>
https://codereview.chromium.org/1365433002/diff/40001/chrome/browser/extensio...
> File chrome/browser/extensions/api/automation/automation_apitest.cc
> (right):
> 
>
https://codereview.chromium.org/1365433002/diff/40001/chrome/browser/extensio...
> chrome/browser/extensions/api/automation/automation_apitest.cc:170:
> IN_PROC_BROWSER_TEST_F(AutomationApiTest, DesktopSelection) {
> FYI; I used desktop mainly to mean the views+web trees. Selection may
> better belong under the tabs directory.

Moved.

>
https://codereview.chromium.org/1365433002/diff/40001/chrome/browser/extensio...
> File
> chrome/browser/extensions/api/automation_internal/automation_action_adapter.h
> (right):
> 
>
https://codereview.chromium.org/1365433002/diff/40001/chrome/browser/extensio...
>
chrome/browser/extensions/api/automation_internal/automation_action_adapter.h:26:
> // selection in text fields.
> Needs more detail either here or in the idl (e.g. in the case of a
> frame, does the whole frame get selected, if it supports reversed
> selections, how the offsets work when applied to a non-static text node,
> etc).

Documenting in the idl.  Trying to not be too detailed since the selection
semantics follows that of blink and I think it would be too much to document it
very detailed here (I mean 'there').


>
https://codereview.chromium.org/1365433002/diff/40001/chrome/browser/ui/aura/...
> File chrome/browser/ui/aura/accessibility/automation_manager_aura.cc
> (right):
> 
>
https://codereview.chromium.org/1365433002/diff/40001/chrome/browser/ui/aura/...
> chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:110:
> return;
> NOTREACHED here, right?

Yup.

>
https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
> File chrome/common/extensions/api/automation.idl (right):
> 
>
https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
> chrome/common/extensions/api/automation.idl:431: long? focusOffset;
> Please document all of these -- they get picked up as part of the public
> documentation for chrome.automation.

Added some documentation, but we might need more.  Is there a way of
documenting
all of these as a whole instead of repeting things four times?

>
https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
> chrome/common/extensions/api/automation.idl:566: // Sets the selection
> in a tree.
> Needs more detail. The anchor and focus nodes need to come from the same
> tree right? (same tree here means node1.root === node2.root). Also, this
> doesn't work for nodes where root.role == 'desktop'.

Done (see above).

>
https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
> chrome/common/extensions/api/automation.idl:567: [nocompile] static void
> setSelection(
> This is now a little confusing as there's two versions of setSelection
> (one on chrome.automation and one on the AutomationNode). The
> AutomationNode version needs its documentation updated as it's no longer
> restricted to text fields. Maybe call the former
> chrome.automation.setDocumentSelection or something along those lines.

I changed it to be setDocumentSelection. I am still not convinced that we want
to ahve this mixed semantics where document selection sometimes affects the text
field
selection,but leaving it as-is for now.

>
https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
> chrome/common/extensions/api/automation.idl:568:
> [instanceOf=AutomationNode] object anchorObject,
> Does the instanceOf property do anything here?

Yes, it validates that the passed in parameters are isntances of the node
(and the constructor exists in the context of validation). Saying
AutomationNOde as a parameter type had the nice effect of validating it as
a dict, recursively, and you can imagine how well that worked with our tree
structure with all kinds of links in both directions...

>
https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
> chrome/common/extensions/api/automation.idl:569: long anchorOffset,
> Rather than this list of params, use a dictionary (similar to find).

Unless we plan to make some parameters optional, I am not convinced
that's necessary.  Is there any style guide for when positional
parameters shouldn't be used because they are too many?

>
https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
> File chrome/common/extensions/api/automation_internal.idl (right):
> 
>
https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
> chrome/common/extensions/api/automation_internal.idl:41: //
> automationNodeID of performActionRequiredParams is used for the
> nit: one space between sentences in this file.

Alright, done.

> Comment is somewhat confusing. Maybe
> Reuses ActionRequiredParams automationNodeID to mean anchor node id, and
> treeID to apply to both anchor and focus node id's.

Done.

>
https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
> File
> chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js
> (right):
> 
>
https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
>
chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js:29:
> assertTrue(!!textField);
> nit: assertNotNullNorUndefined

Didn't find that in this framework and this is concise anyways.
Not changed.

>
https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
>
chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js:31:
> listenOnce(document, EventType.textSelectionChanged, function() {
> Can you verify the event param's target as you expected? Also, is this
> textSelectionChanged event triggered by focus above?

Yes, the textselchange is triggered by focusing the element.  Added checks for
target, trying to avoid depending on the order of the two selchange
events.

>
https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
>
chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js:36:
> // Wait for another text selection change event.  There's one for
> Is this potentially flakey?

I don't think so.  There are two text selection events fired when text
seledtion
changes in an edit field.  See https://codereview.chromium.org/1347243005.

>
https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
>
chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js:38:
> listenOnce(document, EventType.textSelectionChanged, function() {
> Ditto; vierfy event target.

Done.

>
https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
>
chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js:40:
> listenOnce(document, EventType.textSelectionChanged, function(evt) {
> Ditto.

Done.

>
https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
>
chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js:53:
> runWithDocument(html, function(document) {
> document here as a param looks pretty weird since we're in a dom.

Moot now that I got rid of this call.

>
https://codereview.chromium.org/1365433002/diff/40001/chrome/test/data/extens...
>
chrome/test/data/extensions/api_test/automation/tests/desktop/selection.js:54:
> assertEq(undefined, document.anchorrObject);
> Typo? s/anchorr/anchor

Yeah, thanks for spotting!

> https://codereview.chromium.org/1365433002/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Lundblad, 2015-09-25 15:03:38]

dtseng@chromium.org writes:
> 
>
https://codereview.chromium.org/1365433002/diff/40001/chrome/renderer/resourc...
> File chrome/renderer/resources/extensions/automation_custom_bindings.js
> (right):
> 
>
https://codereview.chromium.org/1365433002/diff/40001/chrome/renderer/resourc...
> chrome/renderer/resources/extensions/automation_custom_bindings.js:139:
> (anchorNodeImpl.id !== focusNodeImpl.id)) {
> Also, forgot to add that I think this API should just disallow setting
> selections to any node who's root is the desktop node. You can just use
> the other setSelection API in that case.

I changed the code to that effect until we decide if we want
setDocumentSelection
to set selection in a text field.  If we want that, then I think it should be
consistent
between the trees.

Another thinkg I changed was to change the AutomationNode.setSelection method
to only work on text fields and text boxes. We shouldn't relax the restriction
that it only works on those kinds of nodes.

Thanks,
//Peter

> https://codereview.chromium.org/1365433002/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2015-09-29 22:17:16]

lgtm

https://codereview.chromium.org/1365433002/diff/40001/chrome/common/extension...
> chrome/common/extensions/api/automation.idl:431: long? focusOffset;
> Please document all of these -- they get picked up as part of the public
> documentation for chrome.automation.

Added some documentation, but we might need more.  Is there a way of
documenting
all of these as a whole instead of repeting things four times?
I think this is the only place we need to write docs. You can try generating the
public docs (see chrome/common/extensions/docs/README).

>

[nasko, 2015-09-30 17:33:02]

content/ LGTM

[Peter Lundblad, 2015-10-05 11:28:49]

plundblad@chromium.org changed reviewers:
+ kalman@chromium.org

[Peter Lundblad, 2015-10-05 11:28:49]

@kalman, could you have a look at the api changes in the idl file?

[not at google - send to devlin, 2015-10-05 12:56:02]

lgtm with a suggested API change.

https://codereview.chromium.org/1365433002/diff/140001/chrome/common/extensio...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1365433002/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/automation.idl:582: long focusOffset);
I'd make this an object with properties "anchorObject", "anchorOffset",
"focusObject", and "focusOffset". I.e. you'd call the function like:

chrome.automation.setDocumentSelection({
  anchorObject: ...,
  anchorOffset: ...,
  focusObject: ...,
  focusOffset: ...,
});

It'd more similar to the OO Range object
(https://developer.mozilla.org/en-US/docs/Web/API/Range) in that it has named
properties vs having a long constructor. Also, it's generally easier to read,
and idiomatic for extension APIs.

You could consider lifting the properties you've added to AutomationNode into a
shared data type (like SelectionRange) but personally I would't bother, since
it's simpler the way you have it, and more consistent with AutomationNode.

[Peter Lundblad, 2015-10-05 14:59:05]

kalman@chromium.org writes:
> lgtm with a suggested API change.
> 
> 
>
https://codereview.chromium.org/1365433002/diff/140001/chrome/common/extensio...
> File chrome/common/extensions/api/automation.idl (right):
> 
>
https://codereview.chromium.org/1365433002/diff/140001/chrome/common/extensio...
> chrome/common/extensions/api/automation.idl:582: long focusOffset);
> I'd make this an object with properties "anchorObject", "anchorOffset",
> "focusObject", and "focusOffset". I.e. you'd call the function like:
> 
> chrome.automation.setDocumentSelection({
>    anchorObject: ...,
>    anchorOffset: ...,
>    focusObject: ...,
>    focusOffset: ...,
> });
> 
Alright, as you maybe saw, David had the same preference, you guys convinced me
now;)
Done.  I might change anchorObject -> anchorNode and the same for focusObject
in a follow up cl that also does it for the properties on the node because
it is more descriptive, but leaving that for another dya...

Thanks,
//Peter

> https://codereview.chromium.org/1365433002/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Lundblad, 2015-10-05 15:01:53]

The CQ bit was checked by plundblad@chromium.org

[Peter Lundblad, 2015-10-05 15:01:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, dtseng@chromium.org, nasko@chromium.org,
kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1365433002/#ps160001
(title: "Change setDocumentSelection to take named arguments.")

[commit-bot: I haz the power, 2015-10-05 15:02:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1365433002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1365433002/160001

[commit-bot: I haz the power, 2015-10-05 17:27:51]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-10-05 17:28:37]

Patchset 9 (id:??) landed as
https://crrev.com/96201023fc5bcabb1f409d9d23fb9b53fcc44262
Cr-Commit-Position: refs/heads/master@{#352339}