Update json_schema_compiler to handle the Automation extension API

Update json_schema_compiler to handle the Automation extension API

This allows the automation externs file to be fully autogenerated.

Lots of tweaks to ChromeVox were required since we've been working with a
hand-tweaked externs file for a while.

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

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

[dmazzoni, 2016-12-29 22:27:55]

Description was changed from

==========
Update json_schema_compiler to handle the Automation extension API

This allows the automation externs file to be fully autogenerated.

Lots of tweaks to ChromeVox were required since we've been working with a
hand-tweaked externs file for a while.

BUG=none
==========

to

==========
Update json_schema_compiler to handle the Automation extension API

This allows the automation externs file to be fully autogenerated.

Lots of tweaks to ChromeVox were required since we've been working with a
hand-tweaked externs file for a while.

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

[dmazzoni, 2016-12-29 23:42:51]

dmazzoni@chromium.org changed reviewers:
+ dbeam@chromium.org, dtseng@chromium.org

[dmazzoni, 2016-12-29 23:42:52]

Work in progress. I realized after going down this
path that js_externs_generator converts enums to
UPPERCASE_WITH_UNDERSCORES when generating externs,
but the extensions API doesn't seem to convert enums
this way otherwise. Trying to figure out how to work
around that now.

[Dan Beam, 2016-12-30 03:28:23]

+rdevlin.cronin@ for tools/json_schema_compiler

[Dan Beam, 2016-12-30 03:30:36]

Description was changed from

==========
Update json_schema_compiler to handle the Automation extension API

This allows the automation externs file to be fully autogenerated.

Lots of tweaks to ChromeVox were required since we've been working with a
hand-tweaked externs file for a while.

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

to

==========
Update json_schema_compiler to handle the Automation extension API

This allows the automation externs file to be fully autogenerated.

Lots of tweaks to ChromeVox were required since we've been working with a
hand-tweaked externs file for a while.

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

[Dan Beam, 2016-12-30 03:30:36]

dbeam@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[dmazzoni, 2016-12-30 18:08:36]

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

[commit-bot: I haz the power, 2016-12-30 18:08:39]

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

[dmazzoni, 2016-12-30 18:09:52]

Updated to add a flag to disable normalization of enums.
Perhaps we could land the change like this, then try removing
the flag and updating the other externs files to see if it results
in any issues.

[commit-bot: I haz the power, 2016-12-30 18:18:09]

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

[commit-bot: I haz the power, 2016-12-30 18:18:10]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[David Tseng, 2016-12-30 19:00:27]

Took a very cursory glance and here are some initial thoughts:

We're not allowed to construct instances of extension objects, so would make
sense to let closure know. Haven't tried, but perhaps mark the constructors as
private.

I disagree with the changes to AutomationEvent. The properties like target are
meant to be read only and only set on construction. Make the listeners take an
AutomationEvent or a ChromeVoxAutomationEvent like in my cl.

The extension system binds the newer style enums itself as of early 2015 but
automation was not also updated. So the right fix is to remove the custom
binding from automation and update our usage.

[dmazzoni, 2016-12-30 23:35:27]

Thanks, that all makes sense. Making the constructor private and
making the event listeners take a ChromeVoxAutomationEvent is
easy, no problem.

I tried deleting the custom enum bindings but it wasn't working. I'll
have to dig into that some more. It's a pretty big refactoring because
we have to fix usage throughout automation internal js bindings,
automation tests, and ChromeVox.

[Dan Beam, 2017-01-04 03:19:49]

this seems super to me if it unblocks y'all

we should definitely get Devlin's opinion tomorrow, though

https://codereview.chromium.org/2601333002/diff/20001/tools/json_schema_compi...
File tools/json_schema_compiler/js_externs_generator.py (right):

https://codereview.chromium.org/2601333002/diff/20001/tools/json_schema_compi...
tools/json_schema_compiler/js_externs_generator.py:66:
self._js_util.GetInfo(tool) + (NOTE % namespace) + '\n' +
nit: maybe update tool += ' --no-normalize-enums' if self._no_normalize_enums?

https://codereview.chromium.org/2601333002/diff/20001/tools/json_schema_compi...
File tools/json_schema_compiler/js_util.py (right):

https://codereview.chromium.org/2601333002/diff/20001/tools/json_schema_compi...
tools/json_schema_compiler/js_util.py:108:
c.Concat(self._TypeToJsType(namespace_name, js_type), new_line = False)
new_line=False
        ^ no spaces

https://codereview.chromium.org/2601333002/diff/20001/tools/json_schema_compi...
File tools/json_schema_compiler/model.py (right):

https://codereview.chromium.org/2601333002/diff/20001/tools/json_schema_compi...
tools/json_schema_compiler/model.py:321: for i in range(len(params)):
params = json.get('parameters', [])
for i, param in enumerate(params):

[Devlin, 2017-01-04 23:54:21]

a few nits, but mostly good.  If we can get more people using autogenerated
enums, I'm happy :)

https://codereview.chromium.org/2601333002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js
(right):

https://codereview.chromium.org/2601333002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:75:
var event = new chrome.automation.AutomationEvent();
rietveld thinks there's an illegal character here, I think (and on the following
lines)

https://codereview.chromium.org/2601333002/diff/20001/tools/json_schema_compi...
File tools/json_schema_compiler/compiler.py (right):

https://codereview.chromium.org/2601333002/diff/20001/tools/json_schema_compi...
tools/json_schema_compiler/compiler.py:189: dest='no_normalize_enums',
nit: no_normalize_enums sounds funny - maybe we could just do normalize_enums
and default to true?  It might also be worth describing when we'd want to do
this - and probably discouraging it in the general case, since we don't want
most APIs to.

https://codereview.chromium.org/2601333002/diff/20001/tools/json_schema_compi...
File tools/json_schema_compiler/js_externs_generator.py (right):

https://codereview.chromium.org/2601333002/diff/20001/tools/json_schema_compi...
tools/json_schema_compiler/js_externs_generator.py:143: self._class_name =
js_type.name
given this is designed to be popped on/off, maybe self._current_class_name? 
Also, is there ever a time when _class_name would not be null before we set it
here, or can we assert(!self._class_name)?

[David Tseng, 2017-01-05 17:59:32]

On 2016/12/30 23:35:27, dmazzoni wrote:
> Thanks, that all makes sense. Making the constructor private and
> making the event listeners take a ChromeVoxAutomationEvent is
> easy, no problem.
> 
> I tried deleting the custom enum bindings but it wasn't working. I'll
> have to dig into that some more. It's a pretty big refactoring because
> we have to fix usage throughout automation internal js bindings,
> automation tests, and ChromeVox.

Yup; that's why I wanted to get somethin in to:
1. unblock ChromeVox and Select to Speak work
2. getting the bits that could be auto generated, auto generated
3. the ungenerated pieces put in a hand written file and migrated over time

For future maintainability, we should try to conform to how extension api's work
and not have a one off for automation (e.g. the custom enum bindings and the
differences in enum style). There's also some stuff in the automation_node.js
custom binding that adds properties that don't exist in the idl...so, we're
definitely going to have to have some amount of hand rolled externs. For
example, AutomationNode has a toString function defined in the custom binding
which isn't in the idl.

I had proposed
https://codereview.chromium.org/2584513006/
to get us around all of the issues above.

But I went ahead and just continued to add ChromeVox only automation usage to
ChromeVox externs to land a few cls in the meantime.

Finally, also note that the js custom bindings contains much more semantics than
the idl like
inheritance (AutomationRootNode -> AutomationNode),  readonly, etc handled by
bindings.expose. This  makes me think extern generation should be done in the js
bindings themselves for some api's, but that would be up to rdevlin. Thanks!

[David Tseng, 2017-01-05 18:08:44]

On 2017/01/05 17:59:32, David Tseng wrote:
> On 2016/12/30 23:35:27, dmazzoni wrote:
> > Thanks, that all makes sense. Making the constructor private and
> > making the event listeners take a ChromeVoxAutomationEvent is
> > easy, no problem.
> > 
> > I tried deleting the custom enum bindings but it wasn't working. I'll
> > have to dig into that some more. It's a pretty big refactoring because
> > we have to fix usage throughout automation internal js bindings,
> > automation tests, and ChromeVox.
> 
> Yup; that's why I wanted to get somethin in to:
> 1. unblock ChromeVox and Select to Speak work
> 2. getting the bits that could be auto generated, auto generated
> 3. the ungenerated pieces put in a hand written file and migrated over time
> 
> For future maintainability, we should try to conform to how extension api's
work
> and not have a one off for automation (e.g. the custom enum bindings and the
> differences in enum style). There's also some stuff in the automation_node.js
> custom binding that adds properties that don't exist in the idl...so, we're
> definitely going to have to have some amount of hand rolled externs. For
> example, AutomationNode has a toString function defined in the custom binding
> which isn't in the idl.
> 
> I had proposed
> https://codereview.chromium.org/2584513006/
> to get us around all of the issues above.
> 
> But I went ahead and just continued to add ChromeVox only automation usage to
> ChromeVox externs to land a few cls in the meantime.
> 
> Finally, also note that the js custom bindings contains much more semantics
than
> the idl like
> inheritance (AutomationRootNode -> AutomationNode),  readonly, etc handled by
> bindings.expose. This  makes me think extern generation should be done in the
js
> bindings themselves for some api's, but that would be up to rdevlin. Thanks!

.... or we extend the idl parser to take the same attributes provided by
utils.expose like superClass. We should probably also add a test to make sure
the js custom bindings matches the idl. Happy to take some of these subtasks on.

[dmazzoni, 2017-01-10 22:19:11]

Ready for another look!

I changed all enums in ChromeVox to use the new style
and removed our old enum generator. I also added
CustomAutomationEvent for cases where we need to synthesize
an event, rather than constructing an AutomationEvent.

[dmazzoni, 2017-01-10 22:19:14]

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

[commit-bot: I haz the power, 2017-01-10 22:19:47]

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

[dmazzoni, 2017-01-10 22:22:27]

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

[commit-bot: I haz the power, 2017-01-10 22:23:12]

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

[Dan Beam, 2017-01-10 22:54:43]

overall awesome

https://codereview.chromium.org/2601333002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/custom_automation_event.js
(right):

https://codereview.chromium.org/2601333002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/custom_automation_event.js:19:
* @param {string} eventFrom The source of this event.
could AutomationEvent just be an interface that gets @implemented by
CustomAutomationEvent?

https://codereview.chromium.org/2601333002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/custom_automation_event.js:21:
CustomAutomationEvent = function(type, target, eventFrom) {
var CustomAutomationEvent = function(...) {
};

or

function CustomAutomationEvent(...) {
}

https://codereview.chromium.org/2601333002/diff/60001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/automation/tests/tabs/find.js (right):

https://codereview.chromium.org/2601333002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/find.js:55:
assertEq(h1, rootNode.find({ role: RoleType.HEADING}));
{\s -> {
\s} -> }

https://codereview.chromium.org/2601333002/diff/60001/tools/json_schema_compi...
File tools/json_schema_compiler/js_externs_generator.py (right):

https://codereview.chromium.org/2601333002/diff/60001/tools/json_schema_compi...
tools/json_schema_compiler/js_externs_generator.py:223: else:
nit: need this else?

https://codereview.chromium.org/2601333002/diff/60001/tools/json_schema_compi...
File tools/json_schema_compiler/js_util.py (right):

https://codereview.chromium.org/2601333002/diff/60001/tools/json_schema_compi...
tools/json_schema_compiler/js_util.py:108:
c.Concat(self._TypeToJsType(namespace_name, js_type), new_line = False)
new_line=False

https://codereview.chromium.org/2601333002/diff/60001/tools/json_schema_compi...
File tools/json_schema_compiler/model.py (right):

https://codereview.chromium.org/2601333002/diff/60001/tools/json_schema_compi...
tools/json_schema_compiler/model.py:321: for i in range(len(params)):
for i, param in enumerate(params):

[commit-bot: I haz the power, 2017-01-10 23:05:52]

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

[commit-bot: I haz the power, 2017-01-10 23:05:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[David Tseng, 2017-01-10 23:24:33]

Thanks for doing the renaming. Still a lot of discrepancies between the hand
written and the auto gen versions; sorry.

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

https://codereview.chromium.org/2601333002/diff/60001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:268: dictionary NameFromType {
Is this missing?

https://codereview.chromium.org/2601333002/diff/60001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:588: //
Please do a diff between automation_node.js and this file. Not all properties
there are here e.g. startDuckingMedia.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
File third_party/closure_compiler/externs/automation.js (left):

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:234:
chrome.automation.NameFromType = {
What happened to this enum? I don't see it below.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:286: * @constructor
Did you try making this private?

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:299: * @type
{!chrome.automation.EventType}
Is this actually valid (i.e. !string is just string).

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:491: */
Ditto.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:500: */
Ditto.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:568:
chrome.automation.AutomationNode.prototype.containerLiveBusy;
Nullable for these live region attributes as well.

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

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:367: * @type
{!chrome.automation.AutomationNode}
This is nullable.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:381: * @type
{!chrome.automation.RoleType}
Ditto.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:390:
chrome.automation.AutomationNode.prototype.state;
This had a decent annotation before
Object<chrome.automation.StateType, boolean>

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:434: * @type {number}
Let's just make this a string and do the conversion in our custom bindings
(similar to the way we handle event from).

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:462: * @type {!Array<number>}
Nullable. Please make a ful pass between these autom generated annotations and
the hand written ones currently checked in.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:506:
chrome.automation.AutomationNode.prototype.activeDescendant;
Most, if not all of these, are nullable.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:602: * An array of indexes of
the start position of each text marker.
This property has no annotation?

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:624:
chrome.automation.AutomationNode.prototype.anchorObject;
This can only ever be present on an AutomationRootNode and even there, it is
nullable (if there's no selection).

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:628: * @type {number}
?number

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:715:
chrome.automation.AutomationNode.prototype.liveStatus;
All of the above are nullable.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:768: * @see
https://developer.chrome.com/extensions/automation#type-htmlAttributes
No annotation. Should be Object<string, string>

[dmazzoni, 2017-01-11 06:42:16]

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

[commit-bot: I haz the power, 2017-01-11 06:42:33]

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-01-11 07:24:20]

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

[commit-bot: I haz the power, 2017-01-11 07:24:21]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dmazzoni, 2017-01-11 22:19:46]

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

[dmazzoni, 2017-01-11 22:20:43]

Ready for another look.

I made some more changes to json_schema_compiler
to handle nullable types and to allow us to
pass through a jsexterns type declaration when
there's no other way for us to specify a proper
type.

https://codereview.chromium.org/2601333002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/custom_automation_event.js
(right):

https://codereview.chromium.org/2601333002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/custom_automation_event.js:19:
* @param {string} eventFrom The source of this event.
On 2017/01/10 22:54:43, Dan Beam wrote:
> could AutomationEvent just be an interface that gets @implemented by
> CustomAutomationEvent?

I don't think I could do that without major changes and maybe breaking
backwards compatibility...but I can do @extends {AutomationEvent} and
that works great, thanks.

https://codereview.chromium.org/2601333002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/custom_automation_event.js:21:
CustomAutomationEvent = function(type, target, eventFrom) {
On 2017/01/10 22:54:43, Dan Beam wrote:
> var CustomAutomationEvent = function(...) {
> };
> 
> or
> 
> function CustomAutomationEvent(...) {
> }

Done.

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

https://codereview.chromium.org/2601333002/diff/60001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:268: dictionary NameFromType {
On 2017/01/10 23:24:32, David Tseng wrote:
> Is this missing? 

It wasn't possible to express in IDL before. As suggested I changed it into a
standard
enum and wrote a custom binding for nameFrom.

https://codereview.chromium.org/2601333002/diff/60001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:588: //
On 2017/01/10 23:24:32, David Tseng wrote:
> Please do a diff between automation_node.js and this file. Not all properties
> there are here e.g. startDuckingMedia.

Done. Note that you had added the media ones to chrome_extension_externs.js
but I moved them here.

https://codereview.chromium.org/2601333002/diff/60001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/automation/tests/tabs/find.js (right):

https://codereview.chromium.org/2601333002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/find.js:55:
assertEq(h1, rootNode.find({ role: RoleType.HEADING}));
On 2017/01/10 22:54:43, Dan Beam wrote:
> {\s -> {
> \s} -> }

Done.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
File third_party/closure_compiler/externs/automation.js (left):

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:234:
chrome.automation.NameFromType = {
On 2017/01/10 23:24:32, David Tseng wrote:
> What happened to this enum? I don't see it below.

Done.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:286: * @constructor
On 2017/01/10 23:24:33, David Tseng wrote:
> Did you try making this private? 

Done, assuming that all classes generated by js_externs_generator should be
private
for extension APIs, otherwise I'm not sure how to express that in the IDL.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:299: * @type
{!chrome.automation.EventType}
On 2017/01/10 23:24:33, David Tseng wrote:
> Is this actually valid (i.e. !string is just string).

It's redundant but not illegal. The JSON schema compiler doesn't
keep track of whether ref types are enums or things like classes so
I can't easily fix the generator to handle this case.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:491: */
On 2017/01/10 23:24:33, David Tseng wrote:
> Ditto.

This one is good, the new type is {chrome.automation.Rect?}

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:500: */
On 2017/01/10 23:24:32, David Tseng wrote:
> Ditto.

Same

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:568:
chrome.automation.AutomationNode.prototype.containerLiveBusy;
On 2017/01/10 23:24:33, David Tseng wrote:
> Nullable for these live region attributes as well.

Done.

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

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:367: * @type
{!chrome.automation.AutomationNode}
On 2017/01/10 23:24:33, David Tseng wrote:
> This is nullable.

Done.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:381: * @type
{!chrome.automation.RoleType}
On 2017/01/10 23:24:32, David Tseng wrote:
> Ditto.

Done.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:390:
chrome.automation.AutomationNode.prototype.state;
On 2017/01/10 23:24:32, David Tseng wrote:
> This had a decent annotation before
> Object<chrome.automation.StateType, boolean>

I spent several hours trying to do this in a clean way.

json_schema_compiler uses the ppapi IDL parser,
which doesn't support maplike<> or any other similar
type expression. We could switch to a more complete
WebIDL parser, but that would be a major rewrite of
json_schema_compiler.

Instead I added support for adding formatted comments
to the IDL that are output to the jsexterns. So now we
can write this:

    // The $(ref:automation.StateType)s describing this node.
    // <jsexterns>
    //   @type {Object<chrome.automation.StateType, boolean>}
    // </jsexterns>
    object? state;

...and the resulting externs definition is this:

/**
 * The $(ref:automation.StateType)s describing this node.
 * @type {Object<chrome.automation.StateType, boolean>}
 * @see https://developer.chrome.com/extensions/automation#type-state
 */
chrome.automation.AutomationNode.prototype.state;

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:434: * @type {number}
On 2017/01/10 23:24:33, David Tseng wrote:
> Let's just make this a string and do the conversion in our custom bindings
> (similar to the way we handle event from).

Done.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:462: * @type {!Array<number>}
On 2017/01/10 23:24:32, David Tseng wrote:
> Nullable. Please make a ful pass between these autom generated annotations and
> the hand written ones currently checked in.

Done.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:506:
chrome.automation.AutomationNode.prototype.activeDescendant;
On 2017/01/10 23:24:33, David Tseng wrote:
> Most, if not all of these, are nullable.

Done.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:602: * An array of indexes of
the start position of each text marker.
On 2017/01/10 23:24:33, David Tseng wrote:
> This property has no annotation?

Done.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:624:
chrome.automation.AutomationNode.prototype.anchorObject;
On 2017/01/10 23:24:33, David Tseng wrote:
> This can only ever be present on an AutomationRootNode and even there, it is
> nullable (if there's no selection).

Done.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:628: * @type {number}
On 2017/01/10 23:24:32, David Tseng wrote:
> ?number

Done.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:715:
chrome.automation.AutomationNode.prototype.liveStatus;
On 2017/01/10 23:24:33, David Tseng wrote:
> All of the above are nullable.

Done.

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:768: * @see
https://developer.chrome.com/extensions/automation#type-htmlAttributes
On 2017/01/10 23:24:33, David Tseng wrote:
> No annotation. Should be Object<string, string>

Done.

https://codereview.chromium.org/2601333002/diff/60001/tools/json_schema_compi...
File tools/json_schema_compiler/js_externs_generator.py (right):

https://codereview.chromium.org/2601333002/diff/60001/tools/json_schema_compi...
tools/json_schema_compiler/js_externs_generator.py:223: else:
On 2017/01/10 22:54:43, Dan Beam wrote:
> nit: need this else?

Done.

https://codereview.chromium.org/2601333002/diff/60001/tools/json_schema_compi...
File tools/json_schema_compiler/js_util.py (right):

https://codereview.chromium.org/2601333002/diff/60001/tools/json_schema_compi...
tools/json_schema_compiler/js_util.py:108:
c.Concat(self._TypeToJsType(namespace_name, js_type), new_line = False)
On 2017/01/10 22:54:43, Dan Beam wrote:
> new_line=False

Done.

https://codereview.chromium.org/2601333002/diff/60001/tools/json_schema_compi...
File tools/json_schema_compiler/model.py (right):

https://codereview.chromium.org/2601333002/diff/60001/tools/json_schema_compi...
tools/json_schema_compiler/model.py:321: for i in range(len(params)):
On 2017/01/10 22:54:43, Dan Beam wrote:
> for i, param in enumerate(params):

Done.

[commit-bot: I haz the power, 2017-01-11 22:20:54]

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

[David Tseng, 2017-01-11 22:34:39]

https://codereview.chromium.org/2601333002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/custom_automation_event.js
(right):

https://codereview.chromium.org/2601333002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/custom_automation_event.js:26:
};
Override  stopPropagation and throw.

[dmazzoni, 2017-01-11 22:54:17]

https://codereview.chromium.org/2601333002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/custom_automation_event.js
(right):

https://codereview.chromium.org/2601333002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/custom_automation_event.js:26:
};
On 2017/01/11 22:34:39, David Tseng wrote:
> Override  stopPropagation and throw.

Done.

[dmazzoni, 2017-01-11 22:54:19]

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

[commit-bot: I haz the power, 2017-01-11 22:55:03]

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

[Dan Beam, 2017-01-11 23:04:27]

https://codereview.chromium.org/2601333002/diff/60001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/automation/tests/tabs/find.js (right):

https://codereview.chromium.org/2601333002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/find.js:55:
assertEq(h1, rootNode.find({ role: RoleType.HEADING}));
On 2017/01/11 22:20:42, dmazzoni wrote:
> On 2017/01/10 22:54:43, Dan Beam wrote:
> > {\s -> {
> > \s} -> }
> 
> Done.

for the sake of this awesome CL landing, i'll hold back comments about this.

it'd also be way faster to just run clang-format on these files.  it has these
style configurations (to omit spaces) already, i think, for JS

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

https://codereview.chromium.org/2601333002/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/automation.js:768: * @see
https://developer.chrome.com/extensions/automation#type-htmlAttributes
On 2017/01/10 23:24:33, David Tseng wrote:
> No annotation. Should be Object<string, string>

also note, Object<string> is pretty much exactly the same but shorter

https://codereview.chromium.org/2601333002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/2601333002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1267:
node.location || undefined);
i'm not exactly sure why you need to do this.  is this to change from null to
undefined?

https://codereview.chromium.org/2601333002/diff/100001/tools/json_schema_comp...
File tools/json_schema_compiler/idl_schema.py (right):

https://codereview.chromium.org/2601333002/diff/100001/tools/json_schema_comp...
tools/json_schema_compiler/idl_schema.py:62: match =
re.search('<jsexterns>(.*)</jsexterns>', comment, re.DOTALL)
this will probably not work with multiple <jsexterns> tags well, i.e.

<jsexterns>1</jsexterns>
<jsexterns>2</jsexterns>

will probably match

1</jsexterns>
<jsexterns>2

instead of 1 and 2 like you expect.

<jsexterns>(.*?)</jsexterns>

non-greedy question mark (?) might fix

https://codereview.chromium.org/2601333002/diff/100001/tools/json_schema_comp...
File tools/json_schema_compiler/js_util.py (right):

https://codereview.chromium.org/2601333002/diff/100001/tools/json_schema_comp...
tools/json_schema_compiler/js_util.py:104: def AppendTypeJsDoc(self, c,
namespace_name, js_type, optional):
s/optional/nullable/

optional means {type=} in JSDoc, not {?type}

maybe also make this a named key?  i.e. nullable=False

so from callsites instead of

  thing.AppendTypeJsDoc(c, 'namespace', 'Type', True)

you see (or at least could see):

  thing.AppendTypeJsDoc(c, 'namespace', 'Type', nullable=True)

https://codereview.chromium.org/2601333002/diff/100001/tools/json_schema_comp...
tools/json_schema_compiler/js_util.py:110: if js_type_str[0] == '!':
when does a nullable param have a !Type?  these are basically mutually exclusive

[dmazzoni, 2017-01-11 23:23:54]

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

[dmazzoni, 2017-01-11 23:23:57]

https://codereview.chromium.org/2601333002/diff/60001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/automation/tests/tabs/find.js (right):

https://codereview.chromium.org/2601333002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/find.js:55:
assertEq(h1, rootNode.find({ role: RoleType.HEADING}));
On 2017/01/11 23:04:26, Dan Beam wrote:
> On 2017/01/11 22:20:42, dmazzoni wrote:
> > On 2017/01/10 22:54:43, Dan Beam wrote:
> > > {\s -> {
> > > \s} -> }
> > 
> > Done.
> 
> for the sake of this awesome CL landing, i'll hold back comments about this.
> 
> it'd also be way faster to just run clang-format on these files.  it has these
> style configurations (to omit spaces) already, i think, for JS

Sure, went ahead and ran clang-format on this file, looks
fine.

We could probably clang-format 95% of our files and get
nothing but improvements. It's that last 5% that needs a few
tweaks that's the main roadblock to making it a presubmit
rule.

https://codereview.chromium.org/2601333002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/2601333002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1267:
node.location || undefined);
On 2017/01/11 23:04:26, Dan Beam wrote:
> i'm not exactly sure why you need to do this.  is this to change from null to
> undefined?

Because the externs file says that node.location can be valid or null, but the
second argument to the function is *optional*,
and optional means valid or undefined.

https://codereview.chromium.org/2601333002/diff/100001/tools/json_schema_comp...
File tools/json_schema_compiler/idl_schema.py (right):

https://codereview.chromium.org/2601333002/diff/100001/tools/json_schema_comp...
tools/json_schema_compiler/idl_schema.py:62: match =
re.search('<jsexterns>(.*)</jsexterns>', comment, re.DOTALL)
On 2017/01/11 23:04:26, Dan Beam wrote:
> this will probably not work with multiple <jsexterns> tags well, i.e.
> 
> <jsexterns>1</jsexterns>
> <jsexterns>2</jsexterns>
> 
> will probably match
> 
> 1</jsexterns>
> <jsexterns>2
> 
> instead of 1 and 2 like you expect.
> 
> <jsexterns>(.*?)</jsexterns>
> 
> non-greedy question mark (?) might fix

It'd require more work to iterate over them and append
them, and since we'd just concatenate them anyway, is it
okay to just allow a single <jsexterns> block? I can't
see a good reason to have more than one.

https://codereview.chromium.org/2601333002/diff/100001/tools/json_schema_comp...
File tools/json_schema_compiler/js_util.py (right):

https://codereview.chromium.org/2601333002/diff/100001/tools/json_schema_comp...
tools/json_schema_compiler/js_util.py:104: def AppendTypeJsDoc(self, c,
namespace_name, js_type, optional):
On 2017/01/11 23:04:26, Dan Beam wrote:
> s/optional/nullable/
> 
> optional means {type=} in JSDoc, not {?type}
> 
> maybe also make this a named key?  i.e. nullable=False
> 
> so from callsites instead of
> 
>   thing.AppendTypeJsDoc(c, 'namespace', 'Type', True)
> 
> you see (or at least could see):
> 
>   thing.AppendTypeJsDoc(c, 'namespace', 'Type', nullable=True)

Done.

https://codereview.chromium.org/2601333002/diff/100001/tools/json_schema_comp...
tools/json_schema_compiler/js_util.py:110: if js_type_str[0] == '!':
On 2017/01/11 23:04:26, Dan Beam wrote:
> when does a nullable param have a !Type?  these are basically mutually
exclusive

Exactly, but _TypeToJsType, below, always adds '!' by default
when it encounters a REF, so we have to undo it here.

[commit-bot: I haz the power, 2017-01-11 23:24:42]

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-01-12 00:21:30]

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

[commit-bot: I haz the power, 2017-01-12 00:21:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dan Beam, 2017-01-12 03:12:50]

lgtm

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/idl_schema.py (right):

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/idl_schema.py:62: match =
re.search('<jsexterns>(.*)</jsexterns>', comment, re.DOTALL)
oh, i guess this is only on the comment, not the whole file.  probably less of
an issue then

I'm trusting Devlin's instincts on whether this mechanism is actually a good
idea.  It basically seems like static_cast<> for externs generation ;)

[Devlin, 2017-01-12 17:28:48]

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

https://codereview.chromium.org/2601333002/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/automation.idl:380: // <jsexterns>@type
{Object<chrome.automation.StateType, boolean>}
This is kind of unfortunate... Maybe we should introduce a map type to avoid the
need for this.  Well, probably out of scope for this cl.

https://codereview.chromium.org/2601333002/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/automation.idl:495: // <jsexterns>@type
{Array<number>}</jsexterns>
Why can't we just do
double[]? lineBreaks;
?

Similar for below.

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/idl_schema.py (right):

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/idl_schema.py:62: match =
re.search('<jsexterns>(.*)</jsexterns>', comment, re.DOTALL)
On 2017/01/12 03:12:50, Dan Beam wrote:
> oh, i guess this is only on the comment, not the whole file.  probably less of
> an issue then
> 
> I'm trusting Devlin's instincts on whether this mechanism is actually a good
> idea.  It basically seems like static_cast<> for externs generation ;)

I'm not in love with it, but there's also not a great alternative yet.  But
there are a few places it looks like we can avoid it here - see idl comments.

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/js_externs_generator.py (right):

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/js_externs_generator.py:72: self._AppendTypeJsDoc(c,
js_type, nullable=False)
False is the default; no need to pass it in.

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/js_externs_generator.py:127: c.Comment('@private',
comment_prefix = '', wrap_indent=4)
are all ctors unconditionally private?  That seems a bit odd.

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/js_externs_generator_test.py (right):

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/js_externs_generator_test.py:50: dictionary Qux {
Can we expand this to use nullables and jsexterns?

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/js_util.py (right):

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/js_util.py:112: js_type_str = '?' + js_type_str
Note: This means the possible values for the type are existent or null, but
*not* undefined.  Since undefined is the default value in js for nonexistent
properties on an object, it seems like maybe this should be either:
?foo|undefined   -> null, undefined, or existent
or
!foo|undefined   -> undefined or existent, not null

In my experience, usually the latter is sufficient, but I'm not sure how all of
automation uses it.

[dmazzoni, 2017-01-12 17:44:16]

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

https://codereview.chromium.org/2601333002/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/automation.idl:380: // <jsexterns>@type
{Object<chrome.automation.StateType, boolean>}
On 2017/01/12 17:28:47, Devlin wrote:
> This is kind of unfortunate... Maybe we should introduce a map type to avoid
the
> need for this.  Well, probably out of scope for this cl.

FWIW, the *other* instance of idl_parser in the codebase
(in tools/idl_parser as opposed to ppapi/generators)
already supports maplike and setlike. Any idea how similar
or different the two parsers are otherwise and if there's
any chance we could converge?

I can look into adding support for maplike. Do you see any
problem with adding it just for jsexterns initially, without
the ability to handle it for ppapi bindings and extension
function bindings and other uses of the code?

Note that in all of the cases where we return a maplike
in our IDL we use custom bindings, so we have no need for
an auto bindings generator.

I'm just not sure whether it'd be more confusing to
explicitly parse maplike<> and not fully support it,
vs just having a way to indicate a more specific type
for the rare cases where we have a custom binding.

[Devlin, 2017-01-12 21:54:06]

On 2017/01/12 17:44:16, dmazzoni wrote:
>
https://codereview.chromium.org/2601333002/diff/140001/chrome/common/extensio...
> File chrome/common/extensions/api/automation.idl (right):
> 
>
https://codereview.chromium.org/2601333002/diff/140001/chrome/common/extensio...
> chrome/common/extensions/api/automation.idl:380: // <jsexterns>@type
> {Object<chrome.automation.StateType, boolean>}
> On 2017/01/12 17:28:47, Devlin wrote:
> > This is kind of unfortunate... Maybe we should introduce a map type to avoid
> the
> > need for this.  Well, probably out of scope for this cl.
> 
> FWIW, the *other* instance of idl_parser in the codebase
> (in tools/idl_parser as opposed to ppapi/generators)
> already supports maplike and setlike. Any idea how similar
> or different the two parsers are otherwise and if there's
> any chance we could converge?
> 
> I can look into adding support for maplike. Do you see any
> problem with adding it just for jsexterns initially, without
> the ability to handle it for ppapi bindings and extension
> function bindings and other uses of the code?
> 
> Note that in all of the cases where we return a maplike
> in our IDL we use custom bindings, so we have no need for
> an auto bindings generator.
> 
> I'm just not sure whether it'd be more confusing to
> explicitly parse maplike<> and not fully support it,
> vs just having a way to indicate a more specific type
> for the rare cases where we have a custom binding.

I think for this CL, the jsexterns tag approach is probably sufficient.  I don't
know how much further benefit we'd get from it immediately, so probably not
worth the investment at this time.

[dmazzoni, 2017-01-13 21:22:54]

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

https://codereview.chromium.org/2601333002/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/automation.idl:495: // <jsexterns>@type
{Array<number>}</jsexterns>
On 2017/01/12 17:28:47, Devlin wrote:
> Why can't we just do
> double[]? lineBreaks;
> ?
> 
> Similar for below.

Done.

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/idl_schema.py (right):

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/idl_schema.py:62: match =
re.search('<jsexterns>(.*)</jsexterns>', comment, re.DOTALL)
On 2017/01/12 17:28:47, Devlin wrote:
> On 2017/01/12 03:12:50, Dan Beam wrote:
> > oh, i guess this is only on the comment, not the whole file.  probably less
of
> > an issue then
> > 
> > I'm trusting Devlin's instincts on whether this mechanism is actually a good
> > idea.  It basically seems like static_cast<> for externs generation ;)
> 
> I'm not in love with it, but there's also not a great alternative yet.  But
> there are a few places it looks like we can avoid it here - see idl comments.

Great, now down to just two special cases, both maps.

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/js_externs_generator.py (right):

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/js_externs_generator.py:72: self._AppendTypeJsDoc(c,
js_type, nullable=False)
On 2017/01/12 17:28:47, Devlin wrote:
> False is the default; no need to pass it in.

Done.

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/js_externs_generator.py:127: c.Comment('@private',
comment_prefix = '', wrap_indent=4)
On 2017/01/12 17:28:47, Devlin wrote:
> are all ctors unconditionally private?  That seems a bit odd.

Considering the context that we're only generating externs
for classes defined as part of Chrome extension APIs, I think
that's true.

If this code is being used for APIs other than Chrome
extension APIs and it actually generates bindings for
constructors that can be used, we could remove this and
add support to the IDL.

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/js_externs_generator_test.py (right):

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/js_externs_generator_test.py:50: dictionary Qux {
On 2017/01/12 17:28:47, Devlin wrote:
> Can we expand this to use nullables and jsexterns?

Done.

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/js_util.py (right):

https://codereview.chromium.org/2601333002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/js_util.py:112: js_type_str = '?' + js_type_str
On 2017/01/12 17:28:48, Devlin wrote:
> Note: This means the possible values for the type are existent or null, but
> *not* undefined.  Since undefined is the default value in js for nonexistent
> properties on an object, it seems like maybe this should be either:
> ?foo|undefined   -> null, undefined, or existent
> or
> !foo|undefined   -> undefined or existent, not null
> 
> In my experience, usually the latter is sufficient, but I'm not sure how all
of
> automation uses it.

You're totally right.

Pretty much everything returned from our native bindings
should either be existent or undefined, with a couple of
things that we can guarantee are existent.

In IDL the only thing we can express is "optional", but
that should translate into !foo|undefined, not ?foo.

That's also consistent with how js_externs_generator
is handling optional fields in record types, too.

Fixed.

I also renamed "nullable" back to "optional" in the
json_schema_compiler code.

[dmazzoni, 2017-01-13 21:22:55]

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

[commit-bot: I haz the power, 2017-01-13 21:23:49]

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

[dmazzoni, 2017-01-13 22:06:38]

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

[commit-bot: I haz the power, 2017-01-13 22:07:07]

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-01-13 23:36:43]

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

[commit-bot: I haz the power, 2017-01-13 23:36:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2017-01-17 21:28:49]

it looks like there are still a few tests failing, but idl + compiler stuff lgtm

[dmazzoni, 2017-01-20 17:24:36]

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

[commit-bot: I haz the power, 2017-01-20 17:24:47]

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-01-20 19:08:20]

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

[commit-bot: I haz the power, 2017-01-20 19:08:21]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-01-20 20:07:32]

On 2017/01/17 21:28:49, Devlin (slow) wrote:
> it looks like there are still a few tests failing, but idl + compiler stuff
lgtm

Thanks for the reviews!

The test failures were due to accidentally deleting an attribute I thought was
unused from: chrome/renderer/resources/extensions/automation/automation_node.js
-
reverted that line and now trybots are green.

Ready to land.

[dmazzoni, 2017-01-20 20:07:45]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2017-01-20 20:07:46]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2601333002/#ps220001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-20 20:08:06]

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-01-20 20:14:52]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484942865943220,
"parent_rev": "df8a372f6687f04e8ee911388bd9186f926c10b3", "commit_rev":
"f1cbfff18b19ff99310ffbfb94ca75462da7a859"}

[commit-bot: I haz the power, 2017-01-20 20:15:45]

Description was changed from

==========
Update json_schema_compiler to handle the Automation extension API

This allows the automation externs file to be fully autogenerated.

Lots of tweaks to ChromeVox were required since we've been working with a
hand-tweaked externs file for a while.

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

to

==========
Update json_schema_compiler to handle the Automation extension API

This allows the automation externs file to be fully autogenerated.

Lots of tweaks to ChromeVox were required since we've been working with a
hand-tweaked externs file for a while.

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

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

[commit-bot: I haz the power, 2017-01-20 20:15:47]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/f1cbfff18b19ff99310ffbfb94ca...

[sebsg, 2017-01-23 14:55:47]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2650733002/ by sebsg@chromium.org.

The reason for reverting is: interactive_uitests failing for 50 times in a row

https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C...

see crbug.com/683915.