Generate ax enums from idl.

Generate ax enums from idl.

This cl lays the groundwork for sharing all ax enums between platfrom accessibility and extension accessibility.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249993

[aboxhall, 2014-01-22 17:49:45]

Initial comments.

https://codereview.chromium.org/143473003/diff/50001/tools/json_schema_compil...
File tools/json_schema_compiler/h_generator.py (right):

https://codereview.chromium.org/143473003/diff/50001/tools/json_schema_compil...
tools/json_schema_compiler/h_generator.py:211: # Add any non-explicit enum
values last.
Why is this change necessary, here and above?

https://codereview.chromium.org/143473003/diff/50001/ui/accessibility/ax.idl
File ui/accessibility/ax.idl (right):

https://codereview.chromium.org/143473003/diff/50001/ui/accessibility/ax.idl#...
ui/accessibility/ax.idl:8: enum AXEvent {
Given this is all in the ax namespace, do we need to preface everything with AX
as well?

https://codereview.chromium.org/143473003/diff/50001/ui/accessibility/ax_node...
File ui/accessibility/ax_node_data.h (right):

https://codereview.chromium.org/143473003/diff/50001/ui/accessibility/ax_node...
ui/accessibility/ax_node_data.h:18: using namespace ax;
This is forbidden by the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces

[David Tseng, 2014-01-22 20:53:53]

+ kalman for more feedback and general commentary on this approach.

PTAL.

Due to some of the restrictions in the idl parser generator, we've had to rename
our idl to ui.idl and use the namespace ui.

I tried to limit my changes to the idl/json compiler, but we need the support
for root namespace set to "".

The idea is that we'll eventually include ui.idl using the extensions
infrastructure and be able to use it from javascript.

https://codereview.chromium.org/143473003/diff/50001/tools/json_schema_compil...
File tools/json_schema_compiler/h_generator.py (right):

https://codereview.chromium.org/143473003/diff/50001/tools/json_schema_compil...
tools/json_schema_compiler/h_generator.py:211: # Add any non-explicit enum
values last.
On 2014/01/22 17:49:45, aboxhall wrote:
> Why is this change necessary, here and above?

This was the easiest way to allow us to generate h/cc enums that keep in sync
with the ones in Blink.

https://codereview.chromium.org/143473003/diff/50001/ui/accessibility/ax.idl
File ui/accessibility/ax.idl (right):

https://codereview.chromium.org/143473003/diff/50001/ui/accessibility/ax.idl#...
ui/accessibility/ax.idl:8: enum AXEvent {
On 2014/01/22 17:49:45, aboxhall wrote:
> Given this is all in the ax namespace, do we need to preface everything with
AX
> as well?

We do because the idl parser generator uses the enum name to form the native
enum value
i.e.
ax::AXEvent::X_EVENT_LOAD_COMPLETE

https://codereview.chromium.org/143473003/diff/50001/ui/accessibility/ax_node...
File ui/accessibility/ax_node_data.h (right):

https://codereview.chromium.org/143473003/diff/50001/ui/accessibility/ax_node...
ui/accessibility/ax_node_data.h:18: using namespace ax;
On 2014/01/22 17:49:45, aboxhall wrote:
> This is forbidden by the style guide:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces

Done.

[aboxhall, 2014-01-22 20:58:46]

On 2014/01/22 20:53:53, David Tseng wrote:
> Due to some of the restrictions in the idl parser generator, we've had to
rename
> our idl to ui.idl and use the namespace ui.

I think this wasn't added.

[David Tseng, 2014-01-22 21:32:50]

On Wed, Jan 22, 2014 at 12:58 PM, <aboxhall@chromium.org> wrote:

> On 2014/01/22 20:53:53, David Tseng wrote:
>
>> Due to some of the restrictions in the idl parser generator, we've had to
>>
> rename
>
>> our idl to ui.idl and use the namespace ui.
>>
>
> I think this wasn't added.
> Thanks. Added.
>
>
> https://codereview.chromium.org/143473003/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

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

[aboxhall, 2014-01-22 23:28:31]

https://codereview.chromium.org/143473003/diff/290002/content/renderer/access...
File content/renderer/accessibility/blink_ax_enum_conversion.h (right):

https://codereview.chromium.org/143473003/diff/290002/content/renderer/access...
content/renderer/accessibility/blink_ax_enum_conversion.h:9: #include
"ui/accessibility/ui.h"
This name bothers me a bit. Are you able to go into more detail as to why this
needs to be named this way?

[David Tseng, 2014-01-22 23:41:25]

On Wed, Jan 22, 2014 at 3:28 PM, <aboxhall@chromium.org> wrote:

>
> https://codereview.chromium.org/143473003/diff/290002/content/renderer/
> accessibility/blink_ax_enum_conversion.h
> File content/renderer/accessibility/blink_ax_enum_conversion.h (right):
>
> https://codereview.chromium.org/143473003/diff/290002/content/renderer/
> accessibility/blink_ax_enum_conversion.h#newcode9
> content/renderer/accessibility/blink_ax_enum_conversion.h:9: #include
> "ui/accessibility/ui.h"
> This name bothers me a bit. Are you able to go into more detail as to
> why this needs to be named this way?
>
> The tool (json_schema_compiler) explicitly disallows a file not named
after its namespace. Maybe kalman can chime in here, but I wasn't sure if I
could / should change that restriction.

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

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

[aboxhall, 2014-01-22 23:45:10]

On 2014/01/22 23:41:25, David Tseng wrote:
> On Wed, Jan 22, 2014 at 3:28 PM, <mailto:aboxhall@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/143473003/diff/290002/content/renderer/
> > accessibility/blink_ax_enum_conversion.h
> > File content/renderer/accessibility/blink_ax_enum_conversion.h (right):
> >
> > https://codereview.chromium.org/143473003/diff/290002/content/renderer/
> > accessibility/blink_ax_enum_conversion.h#newcode9
> > content/renderer/accessibility/blink_ax_enum_conversion.h:9: #include
> > "ui/accessibility/ui.h"
> > This name bothers me a bit. Are you able to go into more detail as to
> > why this needs to be named this way?
> >
> > The tool (json_schema_compiler) explicitly disallows a file not named
> after its namespace. Maybe kalman can chime in here, but I wasn't sure if I
> could / should change that restriction.

I see. Why couldn't we call it ax_enums or something?

[not at google - send to devlin, 2014-01-23 00:04:39]

https://codereview.chromium.org/143473003/diff/290002/tools/json_schema_compi...
File tools/json_schema_compiler/cpp_util.py (right):

https://codereview.chromium.org/143473003/diff/290002/tools/json_schema_compi...
tools/json_schema_compiler/cpp_util.py:113: if namespace == "":
use '' not "".

https://codereview.chromium.org/143473003/diff/290002/tools/json_schema_compi...
File tools/json_schema_compiler/h_generator.py (right):

https://codereview.chromium.org/143473003/diff/290002/tools/json_schema_compi...
tools/json_schema_compiler/h_generator.py:212: c.Append('%s,' %
self._type_helper.GetEnumNoneValue(type_))
Explain this a bit more (why you need it last). I can see why but without the
context of this patch (making the enums match up with some other enum list) it
seems arbitrary.

also, same comment needed where you changed it above.

https://codereview.chromium.org/143473003/diff/290002/ui/accessibility/ui.idl
File ui/accessibility/ui.idl (right):

https://codereview.chromium.org/143473003/diff/290002/ui/accessibility/ui.idl...
ui/accessibility/ui.idl:178: [absolute] enum AXStringAttribute {
what does [absolute] mean? I can see you updating it in the py files but I still
can't understand it.

[David Tseng, 2014-01-23 00:31:42]

https://codereview.chromium.org/143473003/diff/290002/tools/json_schema_compi...
File tools/json_schema_compiler/h_generator.py (right):

https://codereview.chromium.org/143473003/diff/290002/tools/json_schema_compi...
tools/json_schema_compiler/h_generator.py:212: c.Append('%s,' %
self._type_helper.GetEnumNoneValue(type_))
On 2014/01/23 00:04:40, kalman wrote:
> Explain this a bit more (why you need it last). I can see why but without the
> context of this patch (making the enums match up with some other enum list) it
> seems arbitrary.
> 
> also, same comment needed where you changed it above.

This is mostly paranoia on my part because of the various comments stating to
keep enums in sync. I'm not sure if we ever cast from a Blink enum role to a
Chrome specific one.

Added comment for the other instance.

https://codereview.chromium.org/143473003/diff/290002/ui/accessibility/ui.idl
File ui/accessibility/ui.idl (right):

https://codereview.chromium.org/143473003/diff/290002/ui/accessibility/ui.idl...
ui/accessibility/ui.idl:178: [absolute] enum AXStringAttribute {
On 2014/01/23 00:04:40, kalman wrote:
> what does [absolute] mean? I can see you updating it in the py files but I
still
> can't understand it.

It means do not use the enum name as a prefix for the enum value(s). How about
literalvalues?

[not at google - send to devlin, 2014-01-23 00:38:06]

https://codereview.chromium.org/143473003/diff/290002/ui/accessibility/ui.idl
File ui/accessibility/ui.idl (right):

https://codereview.chromium.org/143473003/diff/290002/ui/accessibility/ui.idl...
ui/accessibility/ui.idl:178: [absolute] enum AXStringAttribute {
On 2014/01/23 00:31:43, David Tseng wrote:
> On 2014/01/23 00:04:40, kalman wrote:
> > what does [absolute] mean? I can see you updating it in the py files but I
> still
> > can't understand it.
> 
> It means do not use the enum name as a prefix for the enum value(s). How about
> literalvalues?

Literal looks better - but is there a reason you can't remove the ax_attr_
prefix from each of these enum values and beyond that resign yourself to needing
to type AX_STRING_ATTIRBUTE_DOC_URL (could be AX_STIRNG_ATTR_DOC_URL) rather
than AX_ATTR_DOC_URL?

for all instance of [literalvalue]

[David Tseng, 2014-01-23 00:54:16]

https://codereview.chromium.org/143473003/diff/290002/ui/accessibility/ui.idl
File ui/accessibility/ui.idl (right):

https://codereview.chromium.org/143473003/diff/290002/ui/accessibility/ui.idl...
ui/accessibility/ui.idl:178: [absolute] enum AXStringAttribute {
On 2014/01/23 00:38:07, kalman wrote:
> On 2014/01/23 00:31:43, David Tseng wrote:
> > On 2014/01/23 00:04:40, kalman wrote:
> > > what does [absolute] mean? I can see you updating it in the py files but I
> > still
> > > can't understand it.
> > 
> > It means do not use the enum name as a prefix for the enum value(s). How
about
> > literalvalues?
> 
> Literal looks better - but is there a reason you can't remove the ax_attr_
> prefix from each of these enum values and beyond that resign yourself to
needing
> to type AX_STRING_ATTIRBUTE_DOC_URL (could be AX_STIRNG_ATTR_DOC_URL) rather
> than AX_ATTR_DOC_URL?
> 
> for all instance of [literalvalue]

I was just trying to minimize the number of actual renames and generate what was
in ax_enums.h. I can change all such instances to AX_STRING_ATTRIBUTE_* and
remove the literalvalues property if you prefer that.

[not at google - send to devlin, 2014-01-23 00:56:48]

https://codereview.chromium.org/143473003/diff/290002/ui/accessibility/ui.idl
File ui/accessibility/ui.idl (right):

https://codereview.chromium.org/143473003/diff/290002/ui/accessibility/ui.idl...
ui/accessibility/ui.idl:178: [absolute] enum AXStringAttribute {
On 2014/01/23 00:54:17, David Tseng wrote:
> On 2014/01/23 00:38:07, kalman wrote:
> > On 2014/01/23 00:31:43, David Tseng wrote:
> > > On 2014/01/23 00:04:40, kalman wrote:
> > > > what does [absolute] mean? I can see you updating it in the py files but
I
> > > still
> > > > can't understand it.
> > > 
> > > It means do not use the enum name as a prefix for the enum value(s). How
> about
> > > literalvalues?
> > 
> > Literal looks better - but is there a reason you can't remove the ax_attr_
> > prefix from each of these enum values and beyond that resign yourself to
> needing
> > to type AX_STRING_ATTIRBUTE_DOC_URL (could be AX_STIRNG_ATTR_DOC_URL) rather
> > than AX_ATTR_DOC_URL?
> > 
> > for all instance of [literalvalue]
> 
> I was just trying to minimize the number of actual renames and generate what
was
> in ax_enums.h. I can change all such instances to AX_STRING_ATTRIBUTE_* and
> remove the literalvalues property if you prefer that.

Ah right. I don't mind something like this then.

However, another question - probably with a simple answer - why not have the
enum be called AXAttr and have all of these be doc_url, doc_title, etc?

[not at google - send to devlin, 2014-01-23 16:04:35]

Alice explained the reasoning behind the int/string/bool/etc enum
differentiation.

And presumably the reason you can't lump it all together in a single enum here
is that it would put the ordering of the enums out compared to blink? I.e.
something along the lines of [literalvalues] really is necessary?

I'm coming from it like this: I would rather not creep domain specific (in this
case C++) features into the IDL files. I fear the complexity growing and IDL
files becoming more arcane.

However in the interest of not holding y'all up this seems like a special enough
special case. As a nit I would ask that the [literalvalues] attribute name be
called out as a C++ specific option. Because it is; the reason they're prefixed
with the enum name is because C++ enums are uniquely awkward in that way.

Perhaps call it [cpp_omit_enum_type] ?

[not at google - send to devlin, 2014-01-23 16:07:12]

https://codereview.chromium.org/143473003/diff/350001/tools/json_schema_compi...
File tools/json_schema_compiler/cpp_util.py (right):

https://codereview.chromium.org/143473003/diff/350001/tools/json_schema_compi...
tools/json_schema_compiler/cpp_util.py:114: return c
why do you need these changes?

[David Tseng, 2014-01-23 17:34:51]

https://codereview.chromium.org/143473003/diff/290002/ui/accessibility/ui.idl
File ui/accessibility/ui.idl (right):

https://codereview.chromium.org/143473003/diff/290002/ui/accessibility/ui.idl...
ui/accessibility/ui.idl:178: [absolute] enum AXStringAttribute {
On 2014/01/23 00:56:48, kalman wrote:
> On 2014/01/23 00:54:17, David Tseng wrote:
> > On 2014/01/23 00:38:07, kalman wrote:
> > > On 2014/01/23 00:31:43, David Tseng wrote:
> > > > On 2014/01/23 00:04:40, kalman wrote:
> > > > > what does [absolute] mean? I can see you updating it in the py files
but
> I
> > > > still
> > > > > can't understand it.
> > > > 
> > > > It means do not use the enum name as a prefix for the enum value(s). How
> > about
> > > > literalvalues?
> > > 
> > > Literal looks better - but is there a reason you can't remove the ax_attr_
> > > prefix from each of these enum values and beyond that resign yourself to
> > needing
> > > to type AX_STRING_ATTIRBUTE_DOC_URL (could be AX_STIRNG_ATTR_DOC_URL)
rather
> > > than AX_ATTR_DOC_URL?
> > > 
> > > for all instance of [literalvalue]
> > 
> > I was just trying to minimize the number of actual renames and generate what
> was
> > in ax_enums.h. I can change all such instances to AX_STRING_ATTRIBUTE_* and
> > remove the literalvalues property if you prefer that.
> 
> Ah right. I don't mind something like this then.
> 
> However, another question - probably with a simple answer - why not have the
> enum be called AXAttr and have all of these be doc_url, doc_title, etc?

Mostly because on some platforms like Windows, we need to return type-specific
attributes to the platform (i.e. BSTR's for string attributes like get_url), so
knowing the type is necessary.

[David Tseng, 2014-01-23 17:44:42]

https://codereview.chromium.org/143473003/diff/350001/tools/json_schema_compi...
File tools/json_schema_compiler/cpp_util.py (right):

https://codereview.chromium.org/143473003/diff/350001/tools/json_schema_compi...
tools/json_schema_compiler/cpp_util.py:114: return c
On 2014/01/23 16:07:12, kalman wrote:
> why do you need these changes?

Check out ui/accessibility/accessibility.gyp and ax:root_namespace.

The namespace generated by the tool is root_namespace + namespace of the idl.

Unfortunately, if root namespace is empty, the tool generates an anonymous
namespace in the header file which isn't what we ever want I think.

For the existing ui/accessibility files, everything's placed under the ui
namespace, so we're forced to use ui.idl and namespace it into ui with an empty
namespace.

Would love to hear any alternatives to achieve the above.

[David Tseng, 2014-01-23 17:53:45]

On Thu, Jan 23, 2014 at 8:04 AM, <kalman@chromium.org> wrote:

> Alice explained the reasoning behind the int/string/bool/etc enum
> differentiation.
>
> And presumably the reason you can't lump it all together in a single enum
> here
> is that it would put the ordering of the enums out compared to blink? I.e.
> something along the lines of [literalvalues] really is necessary?
>
> I'm coming from it like this: I would rather not creep domain specific (in
> this
> case C++) features into the IDL files. I fear the complexity growing and
> IDL
> files becoming more arcane.
>
> However in the interest of not holding y'all up this seems like a special
> enough
> special case. As a nit I would ask that the [literalvalues] attribute name
> be
> called out as a C++ specific option. Because it is; the reason they're
> prefixed
> with the enum name is because C++ enums are uniquely awkward in that way.
>
> Perhaps call it [cpp_omit_enum_type] ?
>
> Done.

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

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

[not at google - send to devlin, 2014-01-23 18:01:22]

https://codereview.chromium.org/143473003/diff/350001/tools/json_schema_compi...
File tools/json_schema_compiler/cpp_util.py (right):

https://codereview.chromium.org/143473003/diff/350001/tools/json_schema_compi...
tools/json_schema_compiler/cpp_util.py:114: return c
On 2014/01/23 17:44:42, David Tseng wrote:
> On 2014/01/23 16:07:12, kalman wrote:
> > why do you need these changes?
> 
> Check out ui/accessibility/accessibility.gyp and ax:root_namespace.
> 
> The namespace generated by the tool is root_namespace + namespace of the idl.
> 
> Unfortunately, if root namespace is empty, the tool generates an anonymous
> namespace in the header file which isn't what we ever want I think.
> 
> For the existing ui/accessibility files, everything's placed under the ui
> namespace, so we're forced to use ui.idl and namespace it into ui with an
empty
> namespace.
> 
> Would love to hear any alternatives to achieve the above.

I see. Can you leave out the root_namespace from the gyp file, or set it to None
or something? Having it as empty-string seems wrong if this is the intention.

[David Tseng, 2014-01-23 18:18:24]

https://codereview.chromium.org/143473003/diff/350001/tools/json_schema_compi...
File tools/json_schema_compiler/cpp_util.py (right):

https://codereview.chromium.org/143473003/diff/350001/tools/json_schema_compi...
tools/json_schema_compiler/cpp_util.py:114: return c
On 2014/01/23 18:01:23, kalman wrote:
> On 2014/01/23 17:44:42, David Tseng wrote:
> > On 2014/01/23 16:07:12, kalman wrote:
> > > why do you need these changes?
> > 
> > Check out ui/accessibility/accessibility.gyp and ax:root_namespace.
> > 
> > The namespace generated by the tool is root_namespace + namespace of the
idl.
> > 
> > Unfortunately, if root namespace is empty, the tool generates an anonymous
> > namespace in the header file which isn't what we ever want I think.
> > 
> > For the existing ui/accessibility files, everything's placed under the ui
> > namespace, so we're forced to use ui.idl and namespace it into ui with an
> empty
> > namespace.
> > 
> > Would love to hear any alternatives to achieve the above.
> 
> I see. Can you leave out the root_namespace from the gyp file, or set it to
None
> or something? Having it as empty-string seems wrong if this is the intention.

Agreed. However, root_namespace is required and gyp doesn't like root_namespace
set to None (complains about Type error for NoneType).

[not at google - send to devlin, 2014-01-23 18:26:46]

json schema compiler changes lgtm, and a drive-by in accessibility (sorry).

https://codereview.chromium.org/143473003/diff/290004/tools/json_schema_compi...
File tools/json_schema_compiler/cpp_util.py (right):

https://codereview.chromium.org/143473003/diff/290004/tools/json_schema_compi...
tools/json_schema_compiler/cpp_util.py:113: if namespace == '':
ok, add a comment then.

# In lieu of GYP supporting None for the namespace variable the ''
# namespace implies there is no root namespace.

however also see (my last comment I promise!) the comment in the gyp file about
having a namespace anyway, because namespaces can be nice, and that saves this
from needing to change.

https://codereview.chromium.org/143473003/diff/290004/ui/accessibility/access...
File ui/accessibility/accessibility.gyp (right):

https://codereview.chromium.org/143473003/diff/290004/ui/accessibility/access...
ui/accessibility/accessibility.gyp:76: 'root_namespace': '',
one last question: why not just use a namespace here ("ax_gen" for example, or
whatever you want) and then add 'using namespace ax_gen' in all of the files
which need to include it?

https://codereview.chromium.org/143473003/diff/290004/ui/accessibility/ax_nod...
File ui/accessibility/ax_node_data.cc (right):

https://codereview.chromium.org/143473003/diff/290004/ui/accessibility/ax_nod...
ui/accessibility/ax_node_data.cc:221: default:
drive-by: you're probably better off handing NONE here explicitly (and
everywhere else) so that you get compile safety when enum values are added.

[David Tseng, 2014-01-23 19:12:40]

https://codereview.chromium.org/143473003/diff/290004/ui/accessibility/access...
File ui/accessibility/accessibility.gyp (right):

https://codereview.chromium.org/143473003/diff/290004/ui/accessibility/access...
ui/accessibility/accessibility.gyp:76: 'root_namespace': '',
On 2014/01/23 18:26:47, kalman wrote:
> one last question: why not just use a namespace here ("ax_gen" for example, or
> whatever you want) and then add 'using namespace ax_gen' in all of the files
> which need to include it?

I would do this but the files under ui/accessibility use the top level ui
namespace. Since using directives are disallowed by the style guide inside of
headers, this makes things pretty verbose.
If we change ui/accessibility to actually use a more specific namespace like
ui::ax, I'll change this. Added TODO.

https://codereview.chromium.org/143473003/diff/290004/ui/accessibility/ax_nod...
File ui/accessibility/ax_node_data.cc (right):

https://codereview.chromium.org/143473003/diff/290004/ui/accessibility/ax_nod...
ui/accessibility/ax_node_data.cc:221: default:
On 2014/01/23 18:26:47, kalman wrote:
> drive-by: you're probably better off handing NONE here explicitly (and
> everywhere else) so that you get compile safety when enum values are added.

Done.

[David Tseng, 2014-01-23 20:12:17]

+ avi for content/content_common.gypi

[David Tseng, 2014-01-27 19:52:40]

PTAL all.

@kalman:
Discussed the filename restrictions with @aboxhall, and we agreed that the
filename in previous patchsets doesn't make sense. I therefore added a
"allow_customfilename" attribute to the idl.

@jam for content/ changes.

[aboxhall1, 2014-01-27 19:54:52]

On Mon, Jan 27, 2014 at 11:52 AM, <dtseng@chromium.org> wrote:

> PTAL all.
>
> @kalman:
> Discussed the filename restrictions with @aboxhall, and we agreed that the
> filename in previous patchsets doesn't make sense. I therefore added a
> "allow_customfilename" attribute to the idl.
>

Expanding on this: we agree that it would be preferable to move the "ax"
prefix into the namespace; however, dmazzoni@ implemented these enums
originally, and we're not sure whether there was some subtle reason for the
current naming style, so we'd rather wait until he returns from paternity
leave before making that change.

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

[not at google - send to devlin, 2014-01-27 19:58:31]

https://codereview.chromium.org/143473003/diff/580001/ui/accessibility/access...
File ui/accessibility/accessibility.gyp (right):

https://codereview.chromium.org/143473003/diff/580001/ui/accessibility/access...
ui/accessibility/accessibility.gyp:72: 'ax_enums.idl',
The reason why the filename thing is enforced is because:
1- this file needs to be generated into ax_enums.h/cc for gyp reasons (otherwise
changes won't get picked up properly IIRC)
2- the namespace determines the generated file names, not the original file name

I'd rather not introduce another annotation for this special case. Instead we
can change point (2) to generate into a file based on the original filename
rather than the namespace.

Unless it already does. I can't remember.

[David Tseng, 2014-01-27 20:04:23]

https://codereview.chromium.org/143473003/diff/580001/ui/accessibility/access...
File ui/accessibility/accessibility.gyp (right):

https://codereview.chromium.org/143473003/diff/580001/ui/accessibility/access...
ui/accessibility/accessibility.gyp:72: 'ax_enums.idl',
On 2014/01/27 19:58:32, kalman wrote:
> The reason why the filename thing is enforced is because:
> 1- this file needs to be generated into ax_enums.h/cc for gyp reasons
(otherwise
> changes won't get picked up properly IIRC)
> 2- the namespace determines the generated file names, not the original file
name
> 
> I'd rather not introduce another annotation for this special case. Instead we
> can change point (2) to generate into a file based on the original filename
> rather than the namespace.
> 
> Unless it already does. I can't remember.

That's exactly it. The cc generator and friends use the "unix_name" which is
just the namespace to create the various filenames.

I can remove the annotation and just leave my changes to use the short filename
as opposed to the unix name. Is that what you're saying?

[not at google - send to devlin, 2014-01-27 20:07:16]

On 2014/01/27 20:04:23, David Tseng wrote:
>
https://codereview.chromium.org/143473003/diff/580001/ui/accessibility/access...
> File ui/accessibility/accessibility.gyp (right):
> 
>
https://codereview.chromium.org/143473003/diff/580001/ui/accessibility/access...
> ui/accessibility/accessibility.gyp:72: 'ax_enums.idl',
> On 2014/01/27 19:58:32, kalman wrote:
> > The reason why the filename thing is enforced is because:
> > 1- this file needs to be generated into ax_enums.h/cc for gyp reasons
> (otherwise
> > changes won't get picked up properly IIRC)
> > 2- the namespace determines the generated file names, not the original file
> name
> > 
> > I'd rather not introduce another annotation for this special case. Instead
we
> > can change point (2) to generate into a file based on the original filename
> > rather than the namespace.
> > 
> > Unless it already does. I can't remember.
> 
> That's exactly it. The cc generator and friends use the "unix_name" which is
> just the namespace to create the various filenames.
> 
> I can remove the annotation and just leave my changes to use the short
filename
> as opposed to the unix name. Is that what you're saying?

Yes that's what I'm saying, thanks.

[David Tseng, 2014-01-27 20:07:20]

https://codereview.chromium.org/143473003/diff/580001/tools/json_schema_compi...
File tools/json_schema_compiler/compiler.py (left):

https://codereview.chromium.org/143473003/diff/580001/tools/json_schema_compi...
tools/json_schema_compiler/compiler.py:78: sys.exit("Filename %s is illegal.
Name files using unix_hacker style." %
To expand, I'll have to get rid of this check. Is that fine?

[not at google - send to devlin, 2014-01-27 20:08:39]

https://codereview.chromium.org/143473003/diff/580001/tools/json_schema_compi...
File tools/json_schema_compiler/compiler.py (left):

https://codereview.chromium.org/143473003/diff/580001/tools/json_schema_compi...
tools/json_schema_compiler/compiler.py:78: sys.exit("Filename %s is illegal.
Name files using unix_hacker style." %
On 2014/01/27 20:07:21, David Tseng wrote:
> To expand, I'll have to get rid of this check. Is that fine?

Yes that's fine.

[David Tseng, 2014-01-27 20:12:23]

On Mon, Jan 27, 2014 at 12:07 PM, <kalman@chromium.org> wrote:

> On 2014/01/27 20:04:23, David Tseng wrote:
>
> https://codereview.chromium.org/143473003/diff/580001/ui/
> accessibility/accessibility.gyp
>
>> File ui/accessibility/accessibility.gyp (right):
>>
>
>
> https://codereview.chromium.org/143473003/diff/580001/ui/
> accessibility/accessibility.gyp#newcode72
>
>> ui/accessibility/accessibility.gyp:72: 'ax_enums.idl',
>> On 2014/01/27 19:58:32, kalman wrote:
>> > The reason why the filename thing is enforced is because:
>> > 1- this file needs to be generated into ax_enums.h/cc for gyp reasons
>> (otherwise
>> > changes won't get picked up properly IIRC)
>> > 2- the namespace determines the generated file names, not the original
>> file
>> name
>> >
>> > I'd rather not introduce another annotation for this special case.
>> Instead
>>
> we
>
>> > can change point (2) to generate into a file based on the original
>> filename
>> > rather than the namespace.
>> >
>> > Unless it already does. I can't remember.
>>
>
>  That's exactly it. The cc generator and friends use the "unix_name" which
>> is
>> just the namespace to create the various filenames.
>>
>
>  I can remove the annotation and just leave my changes to use the short
>>
> filename
>
>> as opposed to the unix name. Is that what you're saying?
>>
>
> Yes that's what I'm saying, thanks.
> Done
>
> https://codereview.chromium.org/143473003/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

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

[jam, 2014-01-27 21:36:16]

On 2014/01/27 19:52:40, David Tseng wrote:
> PTAL all.
> 
> @kalman:
> Discussed the filename restrictions with @aboxhall, and we agreed that the
> filename in previous patchsets doesn't make sense. I therefore added a
> "allow_customfilename" attribute to the idl.
> 
> @jam for content/ changes.

content_common.gypi lgtm, the rest you have owners in accessibility directories

[not at google - send to devlin, 2014-01-28 18:51:52]

lgtm

[David Tseng, 2014-01-29 19:52:06]

@kalman or @jam, I'm having all sorts of problems with ensuring the ax_enums.h
(ui/accessibility/accessibility.gyp:ax) target gets "built" before any
dependents attempt to build. I tried applying hard_dependency to the target, but
it appears (at least for the case of clank/natives/framework/clank.gyp), my
target isn't being evaluated/built before. I verified it's in the dependency
graph.

Appreciate any insights. Thanks.

[cjhopman, 2014-01-29 23:30:53]

On 2014/01/29 19:52:06, David Tseng wrote:
> @kalman or @jam, I'm having all sorts of problems with ensuring the ax_enums.h
> (ui/accessibility/accessibility.gyp:ax) target gets "built" before any
> dependents attempt to build. I tried applying hard_dependency to the target,
but
> it appears (at least for the case of clank/natives/framework/clank.gyp), my
> target isn't being evaluated/built before. I verified it's in the dependency
> graph.
> 
> Appreciate any insights. Thanks.

It's possible that your issue is actually caused by those dependent targets not
getting the 'include_dirs' from the 'ax_gen' target's
'direct_dependent_settings'. I think you should be using:
'export_dependent_settings': [
  'ax_gen',
],
in the 'accessibility' target. Then, any target that has an include of
ax_enums.h should depend on the 'accessibility' target.

[David Tseng, 2014-01-30 17:06:40]

Thanks; that appears to do it and obviate re-stating dependencies wherever
the accessibility target is listed.


On Wed, Jan 29, 2014 at 3:30 PM, <cjhopman@chromium.org> wrote:

> On 2014/01/29 19:52:06, David Tseng wrote:
>
>> @kalman or @jam, I'm having all sorts of problems with ensuring the
>> ax_enums.h
>> (ui/accessibility/accessibility.gyp:ax) target gets "built" before any
>> dependents attempt to build. I tried applying hard_dependency to the
>> target,
>>
> but
>
>> it appears (at least for the case of clank/natives/framework/clank.gyp),
>> my
>> target isn't being evaluated/built before. I verified it's in the
>> dependency
>> graph.
>>
>
>  Appreciate any insights. Thanks.
>>
>
> It's possible that your issue is actually caused by those dependent
> targets not
> getting the 'include_dirs' from the 'ax_gen' target's
> 'direct_dependent_settings'. I think you should be using:
> 'export_dependent_settings': [
>   'ax_gen',
> ],
> in the 'accessibility' target. Then, any target that has an include of
> ax_enums.h should depend on the 'accessibility' target.
>
>
> https://codereview.chromium.org/143473003/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

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

[David Tseng, 2014-02-06 17:33:39]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-02-06 17:38:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/143473003/930001

[commit-bot: I haz the power, 2014-02-06 18:43:25]

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

[commit-bot: I haz the power, 2014-02-06 18:43:26]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[David Tseng, 2014-02-06 20:45:36]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-02-06 20:47:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/143473003/1330001

[commit-bot: I haz the power, 2014-02-07 00:42:25]

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

[commit-bot: I haz the power, 2014-02-07 00:42:26]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[David Tseng, 2014-02-07 01:01:23]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-02-07 01:06:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/143473003/1330001

[commit-bot: I haz the power, 2014-02-07 02:22:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/143473003/1330001

[commit-bot: I haz the power, 2014-02-07 05:50:15]

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

[commit-bot: I haz the power, 2014-02-07 05:50:16]

Retried try job too often on win_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[David Tseng, 2014-02-07 19:09:03]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-02-07 19:09:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/143473003/1330001

[commit-bot: I haz the power, 2014-02-07 22:14:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/143473003/1330001

[commit-bot: I haz the power, 2014-02-08 02:01:51]

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

[commit-bot: I haz the power, 2014-02-08 02:01:52]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[David Tseng, 2014-02-08 23:54:47]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-02-08 23:55:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/143473003/2190001

[David Tseng, 2014-02-09 03:57:13]

FYI.

Dump accessibility tree test caught a bug!

Blink enum conversions
was still using blink::AXStateEnabled when setting states to the resulting
ax node data. Should have been using ui::ui::AX_STATE_ENABLED




On Sat, Feb 8, 2014 at 3:55 PM, <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/dtseng@
> chromium.org/143473003/2190001
>
>
> https://codereview.chromium.org/143473003/
>

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

[David Tseng, 2014-02-09 03:58:02]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-02-09 03:58:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/143473003/2410001

[commit-bot: I haz the power, 2014-02-09 10:45:13]

Change committed as 249993

[Avi (use Gerrit), 2014-02-13 17:21:38]

Why does this not have a bug in the description?

It seems like this broke Mac 64 hard.

http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Mac%2010.8%20x64...

[ RUN      ] AXTreeTest.SerializeSimpleAXTree
../../ui/accessibility/ax_tree_unittest.cc:70: Failure
Value of: dst_tree.ToString()
  Actual: "id=1 root_web_area FOCUSABLE FOCUSED (0, 0)-(800, 600)
child_ids=2,3\n  id=2 button (20, 20)-(200, 30)\n  id=3 check_box (20, 50)-(200,
30)\n"
Expected: "id=1 ROOT_WEB_AREA FOCUSABLE FOCUSED (0, 0)-(800, 600)
child_ids=2,3\n" "  id=2 BUTTON (20, 20)-(200, 30)\n" "  id=3 CHECKBOX (20,
50)-(200, 30)\n"
Which is: "id=1 ROOT_WEB_AREA FOCUSABLE FOCUSED (0, 0)-(800, 600)
child_ids=2,3\n  id=2 BUTTON (20, 20)-(200, 30)\n  id=3 CHECKBOX (20, 50)-(200,
30)\n"
[  FAILED  ] AXTreeTest.SerializeSimpleAXTree (0 ms)

[Avi (use Gerrit), 2014-02-13 17:23:45]

Bug filed.

http://crbug.com/343580

[dmazzoni at google, 2014-02-13 17:34:31]

Mac 64 must be an unofficial builder? It wasn't caught by the CQ...


On Thu, Feb 13, 2014 at 9:23 AM, <avi@chromium.org> wrote:

> Bug filed.
>
> http://crbug.com/343580
>
>
> https://codereview.chromium.org/143473003/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

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

[Avi (use Gerrit), 2014-02-13 17:39:34]

On 2014/02/13 17:34:31, dmazzoni at google wrote:
> Mac 64 must be an unofficial builder? It wasn't caught by the CQ...

It's FYI right now. It builds the world and tests the world, and I manually
harass people when it goes red.

Maybe someday there will be a tester bot on the waterfall,
http://crbug.com/311694 , but that day is not yet here.

[David Tseng, 2014-02-13 18:35:34]

I have a fix. Sorry for the breakage.


On Thu, Feb 13, 2014 at 9:39 AM, <avi@chromium.org> wrote:

> On 2014/02/13 17:34:31, dmazzoni at google wrote:
>
>> Mac 64 must be an unofficial builder? It wasn't caught by the CQ...
>>
>
> It's FYI right now. It builds the world and tests the world, and I manually
> harass people when it goes red.
>
> Maybe someday there will be a tester bot on the waterfall,
> http://crbug.com/311694 , but that day is not yet here.
>
> https://codereview.chromium.org/143473003/
>

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