Add support for in-page links in ChromeVox.

Add support for in-page links in ChromeVox.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2795083003
Cr-Commit-Position: refs/heads/master@{#461913}
Committed: https://chromium.googlesource.com/chromium/src/+/f23e01c5bf039c5a4c95b5448288e3b079ce7ddc

[David Tseng, 2017-04-03 23:32:45]

Description was changed from

==========
Add support for in page links in ChromeVox.

BUG=
==========

to

==========
Add support for in page links in ChromeVox.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2017-04-03 23:33:04]

dtseng@chromium.org changed reviewers:
+ nektar@chromium.org

[nektarios, 2017-04-04 01:53:06]

I didn't understand what the member_of attribute has to do with links. Is using
member_of just an optimization over the previous code that had a while loop that
found the live container manually. Or are you trying to fix another bug?

Also, sorry I am a bit new to this: Why are we returning undefined if an
attribute is not found instead of null?
I remember on previous Closure projects we used to simply make the type nullable
in cases that there was nothing to return and simply returned null.

[nektarios, 2017-04-04 02:01:27]

Could you please explain two more things?
@internal_link is a message. Shouldn't it be defined in our strings file?
What is the $nameFromNode property in the link's enter rule. I can't find its
definition anywhere.

[nektarios, 2017-04-04 02:02:32]

lgtm

[David Tseng, 2017-04-04 16:33:25]

I didn't understand what the member_of attribute has to do with links. Is using
member_of just an optimization over the previous code that had a while loop that
found the live container manually. Or are you trying to fix another bug?

Optimization / completeness since I was in that part of the code already. There
are a number of attributes that have not been hooked up to automation/ChromeVox.
I took it out for clarity on this cl and will follow up on the other missing
attributes.


Also, sorry I am a bit new to this: Why are we returning undefined if an
attribute is not found instead of null?
I remember on previous Closure projects we used to simply make the type nullable
in cases that there was nothing to return and simply returned null.

The practical answer is that while porting to C++, the bindings like
getIntAttribute don't return anything when the attribute is not found.

The js bindings look like
return getIntAttribute(...);

where getIntAttribute is a native binding.

So, when getIntAttribute returns nothing, js I believe defaults to undefined.

The higher level answer is I don't think we thought about this explicitly.


@internal_link is a message. Shouldn't it be defined in our strings file?

Nope. It's a message id. Look at the parser below for '@'. It uses the parsed
value as a lookup for the actual message using this message id.


What is the $nameFromNode property in the link's enter rule. I can't find its
definition anywhere.


Look at the parser below for the block that deals with 'nameFromNode'.

[chromium-reviews, 2017-04-04 17:00:53]

are a number of attributes that have not been hooked up to 
automation/ChromeVox.
> I took it out for clarity on this cl and will follow up on the other 
> missing
> attributes.
>
You may leave it in if you wish.

>
> @internal_link is a message. Shouldn't it be defined in our strings file?
>
> Nope. It's a message id. Look at the parser below for '@'. It uses the 
> parsed
> value as a lookup for the actual message using this message id.
>
Yes, I understand that it's a message ID, but shouldn't the message 
itself be defined somewhere in a strings file?

>
> What is the $nameFromNode property in the link's enter rule. I can't 
> find its
> definition anywhere.
>
>
> Look at the parser below for the block that deals with 'nameFromNode'.

Okay, got it.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2017-04-04 17:26:29]

The internal link message was already defined for ChromeVox classic, so no
need to do anything in the strings file.
On Tue, Apr 4, 2017 at 10:00 AM 'Nektarios Paisios' via Chromium-reviews <
chromium-reviews@chromium.org> wrote:

> are a number of attributes that have not been hooked up to
> automation/ChromeVox.
>
> I took it out for clarity on this cl and will follow up on the other
> missing
> attributes.
>
> You may leave it in if you wish.
>
>
>
> @internal_link is a message. Shouldn't it be defined in our strings file?
>
> Nope. It's a message id. Look at the parser below for '@'. It uses the
> parsed
> value as a lookup for the actual message using this message id.
>
> Yes, I understand that it's a message ID, but shouldn't the message itself
> be defined somewhere in a strings file?
>
>
>
> What is the $nameFromNode property in the link's enter rule. I can't find
> its
> definition anywhere.
>
>
> Look at the parser below for the block that deals with 'nameFromNode'.
>
>
> Okay, got it.
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2017-04-04 17:40:01]

dtseng@chromium.org changed reviewers:
+ michaelpg@chromium.org

[David Tseng, 2017-04-04 17:40:02]

+ michaelpg for t/c/externs/automation.js

[Dan Beam, 2017-04-04 17:49:14]

https://codereview.chromium.org/2795083003/diff/20001/third_party/closure_com...
File third_party/closure_compiler/externs/automation.js (right):

https://codereview.chromium.org/2795083003/diff/20001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:6: //  
tools/json_schema_compiler/compiler.py.
are you running the script?

$ tools/json_schema_compiler/compiler.py -g externs
chrome/common/extensions/api/automation.idl >
third_party/closure_compiler/externs/automation.js

because it seems like this is a hand edit

[David Tseng, 2017-04-04 18:11:03]

On 2017/04/04 17:49:14, Dan Beam (slow) wrote:
>
https://codereview.chromium.org/2795083003/diff/20001/third_party/closure_com...
> File third_party/closure_compiler/externs/automation.js (right):
> 
>
https://codereview.chromium.org/2795083003/diff/20001/third_party/closure_com...
> third_party/closure_compiler/externs/automation.js:6: //  
> tools/json_schema_compiler/compiler.py.
> are you running the script?
> 
> $ tools/json_schema_compiler/compiler.py -g externs
> chrome/common/extensions/api/automation.idl >
> third_party/closure_compiler/externs/automation.js
> 
> because it seems like this is a hand edit

Yes, you're right. I updated this cl with the auto generated contents that my cl
touched.

[David Tseng, 2017-04-04 18:18:22]

On 2017/04/04 18:11:03, David Tseng wrote:
> On 2017/04/04 17:49:14, Dan Beam (slow) wrote:
> >
>
https://codereview.chromium.org/2795083003/diff/20001/third_party/closure_com...
> > File third_party/closure_compiler/externs/automation.js (right):
> > 
> >
>
https://codereview.chromium.org/2795083003/diff/20001/third_party/closure_com...
> > third_party/closure_compiler/externs/automation.js:6: //  
> > tools/json_schema_compiler/compiler.py.
> > are you running the script?
> > 
> > $ tools/json_schema_compiler/compiler.py -g externs
> > chrome/common/extensions/api/automation.idl >
> > third_party/closure_compiler/externs/automation.js
> > 
> > because it seems like this is a hand edit
> 
> Yes, you're right. I updated this cl with the auto generated contents that my
cl
> touched.

I'll fix the tabId fragment in a follow up. I don't know if previous
cls/reviewers in this file ever caught that one or were fine with that hand
edit, but I'll have to change this api to probably take an non-optional object
with one optional property (tabId).

[David Tseng, 2017-04-04 19:18:17]

Actually, PTAL. Just one small change to make getTree take two optional
params, and now all of automation.js can be auto generated.

On Tue, Apr 4, 2017 at 11:18 AM, <dtseng@chromium.org> wrote:

> On 2017/04/04 18:11:03, David Tseng wrote:
> > On 2017/04/04 17:49:14, Dan Beam (slow) wrote:
> > >
> >
> https://codereview.chromium.org/2795083003/diff/20001/
> third_party/closure_compiler/externs/automation.js
> > > File third_party/closure_compiler/externs/automation.js (right):
> > >
> > >
> >
> https://codereview.chromium.org/2795083003/diff/20001/
> third_party/closure_compiler/externs/automation.js#newcode6
> > > third_party/closure_compiler/externs/automation.js:6: //
> > > tools/json_schema_compiler/compiler.py.
> > > are you running the script?
> > >
> > > $ tools/json_schema_compiler/compiler.py -g externs
> > > chrome/common/extensions/api/automation.idl >
> > > third_party/closure_compiler/externs/automation.js
> > >
> > > because it seems like this is a hand edit
> >
> > Yes, you're right. I updated this cl with the auto generated contents
> that my
> cl
> > touched.
>
> I'll fix the tabId fragment in a follow up. I don't know if previous
> cls/reviewers in this file ever caught that one or were fine with that hand
> edit, but I'll have to change this api to probably take an non-optional
> object
> with one optional property (tabId).
>
> https://codereview.chromium.org/2795083003/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[michaelpg, 2017-04-04 20:29:49]

externs lgtm

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

https://codereview.chromium.org/2795083003/diff/60001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:445: // The target of an in page
link.
nit: hyphenate compound adjective, "in-page" (similar to "pre-tax").

[David Tseng, 2017-04-04 21:40:45]

Description was changed from

==========
Add support for in page links in ChromeVox.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add support for in-page links in ChromeVox.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2017-04-04 21:41:32]

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

https://codereview.chromium.org/2795083003/diff/60001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:445: // The target of an in page
link.
On 2017/04/04 20:29:48, michaelpg wrote:
> nit: hyphenate compound adjective, "in-page" (similar to "pre-tax").

Done throughout and re-generated externs.

[David Tseng, 2017-04-04 21:42:22]

The CQ bit was checked by dtseng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 21:43:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 22:53:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 22:53:49]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[David Tseng, 2017-04-04 23:11:55]

The CQ bit was checked by dtseng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 23:12:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 23:43:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 23:43:21]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2017-04-05 00:17:43]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2017-04-05 00:17:43]

The patchset sent to the CQ was uploaded after l-g-t-m from nektar@chromium.org,
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2795083003/#ps80001
(title: "Address nit.")

[commit-bot: I haz the power, 2017-04-05 00:18:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 00:26:12]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491351463164880,
"parent_rev": "4add44729fbb67e384a602c9d26dc12245058025", "commit_rev":
"f23e01c5bf039c5a4c95b5448288e3b079ce7ddc"}

[commit-bot: I haz the power, 2017-04-05 00:27:18]

Description was changed from

==========
Add support for in-page links in ChromeVox.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add support for in-page links in ChromeVox.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2795083003
Cr-Commit-Position: refs/heads/master@{#461913}
Committed:
https://chromium.googlesource.com/chromium/src/+/f23e01c5bf039c5a4c95b5448288...
==========

[commit-bot: I haz the power, 2017-04-05 00:27:19]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/f23e01c5bf039c5a4c95b5448288...