First step to move common accessibility code out of content.

First step to move common accessibility code out of content.

This changelist defines new classes and types for
accessibility in ui/accessibility. Subsequent changes will
refactor code inside content/ to inherit from AXTree and
AXNode, then we can start exposing the accessibility tree
outside of content/.

In addition, we can now use these same types in Views
accessibility and share more code.

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

[dmazzoni, 2013-11-08 21:08:54]

Ready for an initial look.

This code violates the content API guidelines because it
actually has some implementation in content/public.

Take AXTree as an example - the reason is because we need
code inside content/ and code outside content/ to actually
inherit from it as well as take advantage of this common
functionality.

Using the pimpl pattern would make it impossible for code
outside of content/ to subclass an AXTree.

Suggestions welcome.

[dmazzoni, 2013-11-09 00:09:17]

OK, I tried to refactor this so that the public API
doesn't have any implementation. This should be legal
now.

[aboxhall, 2013-11-11 18:20:34]

I'm disappointed git didn't do a better job detecting copies.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
File content/common/ax_node_impl.cc (right):

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
content/common/ax_node_impl.cc:18: fprintf(stderr, "Deleting node %p\n", this);
fflush(stderr);
I assume this isn't meant to be checked in?

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
File content/common/ax_tree_impl.cc (right):

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:57: int32 AXTreeImpl::GetId(const AXNode* node)
const {
What is the purpose of this and the below two methods, given they just call a
public method on the node in each case? Is it in case some platform wants to
override them?

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:131: // Delete any previous children of this node
that are no longer
Suggestion:
// (1) Delete ...
(to refer back to steps above.)

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_se...
File content/common/ax_tree_serializer.h (right):

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_se...
content/common/ax_tree_serializer.h:82: client_root_ = NULL;
Why aren't we calling delete client_root_ here?

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_se...
content/common/ax_tree_serializer.h:88: AXTreeUpdate* out_update) {
Nit: alignment.

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
File content/public/common/ax_node_data.cc (right):

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
content/public/common/ax_node_data.cc:20: std::string IntVectorToString(const
std::vector<int>& items) {
Is this used, given all the debugging logic was removed?

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
File content/public/common/ax_tree_update.h (right):

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
content/public/common/ax_tree_update.h:40: // TODO: location changes
TODO(dmazzoni)

[David Tseng, 2013-11-11 18:43:51]

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
File content/public/common/ax_node.h (right):

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
content/public/common/ax_node.h:18: // Accessors.
The traversal API mirrors some of the same functionality provided by AXTree. Is
this intentional; if so, why?

Should the caller traverse using AXNode or AXtree?

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
File content/public/common/ax_node_data.h (left):

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
content/public/common/ax_node_data.h:21: // the renderer process to the browser
process.
"from a source process to a receiver process" (since the extension process is
technically a renderer, we will be sending browser process -> renderer process).

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
File content/public/common/ax_tree.h (right):

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
content/public/common/ax_tree.h:18: static AXTree* Create();
An AXTree initialized without any AXTreeUpdate's seems ambiguous to me. Why do
we need this at all?

[David Tseng, 2013-11-11 19:27:18]

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
File content/common/ax_node_impl.h (right):

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
content/common/ax_node_impl.h:20: // AXNode.
nit: AXNode implementation.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
content/common/ax_node_impl.h:29: // Initialize the node. This sets up its place
in the tree but does
A little confused. Does this mean this is initialization done before or after
children are available? Recall that some platforms (i.e. win) need to compute
some members based on fully initialized children.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
content/common/ax_node_impl.h:33: AXTreeImpl* tree, AXNodeImpl* parent, int32
id, int32 index_in_parent);
I'm a little surprised to see this take an AXTreeImpl. Who keeps track of
parent/child links (the node or the tree class)?

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
content/common/ax_node_impl.h:44: virtual void
DetachTree(std::vector<AXNodeImpl*>* nodes);
out_nodes

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
content/common/ax_node_impl.h:46: // Swap the internal children vector with
|children|.
This instance takes ownership?

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
File content/common/ax_tree_impl.cc (right):

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:78: for (size_t i = 0; i < update.nodes.size();
i++) {
nit: ++i

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:80: return false;
DCHECK? Is this ever acceptable?

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:102: for (size_t i = 0; i < src.child_ids.size();
++i) {
ditto

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:104: return false;
DCHECK?

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:122: // Update the children in three steps:
This huge comment block indicates that the below should probably belong in its
own method (which should then be unit tested separately).

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:170: // Handle the case where this node is the
new root of the tree.
And perhaps a separate method for this as well. (i.e. OnDoUpdateChilfdren,
OnDoUpdateRoot, or the like).

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
File content/common/ax_tree_impl.h (right):

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.h:19: // AXTree is a live, managed tree of AXNode
objects that can receive
AXTreeImpl.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.h:28: AXTreeImpl();
Again, I'm not sure why we need this?

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.h:32: // AXTreeSource and AXTree.
nit: ... implementation (or just make separate sections with comment).

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.h:35: virtual AXNode* GetFromId(int32 id) const
OVERRIDE;
Do the various platforms need to implement these id getters? It seems like it
could be handled easily by the base class(es); in this case, AXTree.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.h:44: // AXTree.
nit: ... implementation.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.h:49: void NodeWasDestroyed(AXNodeImpl* node);
OnNodeDestroyed

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_se...
File content/common/ax_tree_serializer.h (right):

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_se...
content/common/ax_tree_serializer.h:33: class CONTENT_EXPORT AXTreeSerializer {
So, what's the difference between calling Serialize on an AXTree vs using this
class?

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_se...
content/common/ax_tree_serializer.h:35:
AXTreeSerializer(AXTreeSource<AXSourceNode>* tree);
explicit

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_se...
content/common/ax_tree_serializer.h:37: void Reset();
What and why? Comment  please.

[dmazzoni, 2013-11-12 00:03:03]

Most feedback addressed. Will do a bit more refactoring as
requested, but please let me know if this explains the high-level
design now.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
File content/common/ax_node_impl.cc (right):

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
content/common/ax_node_impl.cc:18: fprintf(stderr, "Deleting node %p\n", this);
fflush(stderr);
On 2013/11/11 18:20:35, aboxhall wrote:
> I assume this isn't meant to be checked in?

Done.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
File content/common/ax_node_impl.h (right):

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
content/common/ax_node_impl.h:20: // AXNode.
On 2013/11/11 19:27:18, David Tseng wrote:
> nit: AXNode implementation.

Done.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
content/common/ax_node_impl.h:29: // Initialize the node. This sets up its place
in the tree but does
On 2013/11/11 19:27:18, David Tseng wrote:
> A little confused. Does this mean this is initialization done before or after
> children are available? Recall that some platforms (i.e. win) need to compute
> some members based on fully initialized children.

I haven't even added the hook to do platform-specific initialization after its
children are available.

I'll add interfaces like that in the next changelist when I refactor
BrowserAccessibility into AXPlatformNode and make it inherit from this.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
content/common/ax_node_impl.h:33: AXTreeImpl* tree, AXNodeImpl* parent, int32
id, int32 index_in_parent);
On 2013/11/11 19:27:18, David Tseng wrote:
> I'm a little surprised to see this take an AXTreeImpl. Who keeps track of
> parent/child links (the node or the tree class)?

Good point. I got rid of the reference to the tree, making AXNode even simpler.

It's possible the platform node may want a pointer to the tree, but this base
class
doesn't need it.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
content/common/ax_node_impl.h:44: virtual void
DetachTree(std::vector<AXNodeImpl*>* nodes);
On 2013/11/11 19:27:18, David Tseng wrote:
> out_nodes

Done.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_node_im...
content/common/ax_node_impl.h:46: // Swap the internal children vector with
|children|.
On 2013/11/11 19:27:18, David Tseng wrote:
> This instance takes ownership?

Done.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
File content/common/ax_tree_impl.cc (right):

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:57: int32 AXTreeImpl::GetId(const AXNode* node)
const {
On 2013/11/11 18:20:35, aboxhall wrote:
> What is the purpose of this and the below two methods, given they just call a
> public method on the node in each case? Is it in case some platform wants to
> override them?

This is because AXSourceTree is an abstraction of an accessibility tree where
the type (AXNode here) is opaque. AXSourceTree allows us to abstract the
Blink accessibility tree and implement AXTreeSerializer once rather than
needing to duplicate that code.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:78: for (size_t i = 0; i < update.nodes.size();
i++) {
On 2013/11/11 19:27:18, David Tseng wrote:
> nit: ++i

Done.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:80: return false;
On 2013/11/11 19:27:18, David Tseng wrote:
> DCHECK? Is this ever acceptable?

It's not acceptable, but what we're currently doing in
BrowserAccessibilityManager is sending a message to crash the renderer, rather
than doing a DCHECK and crashing the browser.

It's also better at this level of abstraction to return false because then we
can just assert it in a test, and test that bad cases fail.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:102: for (size_t i = 0; i < src.child_ids.size();
++i) {
On 2013/11/11 19:27:18, David Tseng wrote:
> ditto

?

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:104: return false;
On 2013/11/11 19:27:18, David Tseng wrote:
> DCHECK?

Same

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
File content/common/ax_tree_impl.h (right):

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.h:19: // AXTree is a live, managed tree of AXNode
objects that can receive
On 2013/11/11 19:27:18, David Tseng wrote:
> AXTreeImpl.

Done.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.h:28: AXTreeImpl();
On 2013/11/11 19:27:18, David Tseng wrote:
> Again, I'm not sure why we need this?

The content API can only contain interfaces.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.h:32: // AXTreeSource and AXTree.
On 2013/11/11 19:27:18, David Tseng wrote:
> nit: ... implementation (or just make separate sections with comment).

Done.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.h:35: virtual AXNode* GetFromId(int32 id) const
OVERRIDE;
On 2013/11/11 19:27:18, David Tseng wrote:
> Do the various platforms need to implement these id getters? It seems like it
> could be handled easily by the base class(es); in this case, AXTree.

This is the base class. Think of AXTree as just the interface.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.h:44: // AXTree.
On 2013/11/11 19:27:18, David Tseng wrote:
> nit: ... implementation.

Done.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.h:49: void NodeWasDestroyed(AXNodeImpl* node);
On 2013/11/11 19:27:18, David Tseng wrote:
> OnNodeDestroyed

Done.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_se...
File content/common/ax_tree_serializer.h (right):

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_se...
content/common/ax_tree_serializer.h:33: class CONTENT_EXPORT AXTreeSerializer {
On 2013/11/11 19:27:18, David Tseng wrote:
> So, what's the difference between calling Serialize on an AXTree vs using this
> class?

AXTree just has a method to serialize one node. I renamed that one SerializeNode
to make it more clear.

The reason it has that is because of AXSourceTree.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_se...
content/common/ax_tree_serializer.h:35:
AXTreeSerializer(AXTreeSource<AXSourceNode>* tree);
On 2013/11/11 19:27:18, David Tseng wrote:
> explicit

Done.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_se...
content/common/ax_tree_serializer.h:37: void Reset();
On 2013/11/11 19:27:18, David Tseng wrote:
> What and why? Comment  please.

Done.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_se...
content/common/ax_tree_serializer.h:82: client_root_ = NULL;
On 2013/11/11 18:20:35, aboxhall wrote:
> Why aren't we calling delete client_root_ here?

That was a bug. Good catch.

Actually it looks easier to just delete it. I renamed it.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_se...
content/common/ax_tree_serializer.h:88: AXTreeUpdate* out_update) {
On 2013/11/11 18:20:35, aboxhall wrote:
> Nit: alignment.

Done.

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
File content/public/common/ax_node.h (right):

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
content/public/common/ax_node.h:18: // Accessors.
On 2013/11/11 18:43:51, David Tseng wrote:
> The traversal API mirrors some of the same functionality provided by AXTree.
Is
> this intentional; if so, why?
> 
> Should the caller traverse using AXNode or AXtree?

Hmmm, good point.

The only reason AXTree provides this is because of the AXSourceTree abstraction.

Perhaps AXSourceTree should be an adaptor rather than an actualy class? That
might simplify things...let me know what you think once you understand why I did
it this way.

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
File content/public/common/ax_node_data.cc (right):

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
content/public/common/ax_node_data.cc:20: std::string IntVectorToString(const
std::vector<int>& items) {
On 2013/11/11 18:20:35, aboxhall wrote:
> Is this used, given all the debugging logic was removed?

Done.

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
File content/public/common/ax_node_data.h (left):

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
content/public/common/ax_node_data.h:21: // the renderer process to the browser
process.
On 2013/11/11 18:43:51, David Tseng wrote:
> "from a source process to a receiver process" (since the extension process is
> technically a renderer, we will be sending browser process -> renderer
process).

Done.

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
File content/public/common/ax_tree.h (right):

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
content/public/common/ax_tree.h:18: static AXTree* Create();
On 2013/11/11 18:43:51, David Tseng wrote:
> An AXTree initialized without any AXTreeUpdate's seems ambiguous to me. Why do
> we need this at all?

It was useful for unit testing...maybe we should have a
CreateEmptyTreeForTesting?

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
File content/public/common/ax_tree_update.h (right):

https://codereview.chromium.org/67283004/diff/40001/content/public/common/ax_...
content/public/common/ax_tree_update.h:40: // TODO: location changes
On 2013/11/11 18:20:35, aboxhall wrote:
> TODO(dmazzoni)

Done.

[dmazzoni, 2013-11-12 07:20:19]

OK, to reduce confusion, now AXTreeImpl no longer double-inherits from
AXTreeSource.

AXTreeSource is still useful so that it will be possible to use AXTreeSerializer
on either a real AXTree, or on the Blink accessibility tree. Instead of having
AXTreeImpl inherit, though, now it just exposes a method to return an
AXTreeSource for itself, implemented using a small adapter class.

[dmazzoni, 2013-11-12 07:48:23]

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
File content/common/ax_tree_impl.cc (right):

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:122: // Update the children in three steps:
On 2013/11/11 19:27:18, David Tseng wrote:
> This huge comment block indicates that the below should probably belong in its
> own method (which should then be unit tested separately).

Refactored. More comprehensive unit tests coming.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:131: // Delete any previous children of this node
that are no longer
On 2013/11/11 18:20:35, aboxhall wrote:
> Suggestion:
> // (1) Delete ...
> (to refer back to steps above.)

Refactored, please take a look.

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:170: // Handle the case where this node is the
new root of the tree.
On 2013/11/11 19:27:18, David Tseng wrote:
> And perhaps a separate method for this as well. (i.e. OnDoUpdateChilfdren,
> OnDoUpdateRoot, or the like).

I thought this was fine here now that everything else was in another method,
take a look.

[jam, 2013-11-12 16:28:13]

Just had a chance to look at this, sorry for the delay.

Looking at these new files, given that they don't depend on content I'm
wondering why you put them in content/public instead of ui/base/accessibility?
Is it because of the one WebKit enum that's included? I thought we talked about
having that be a duplicate until the repos are merged, and having compile time
asserts ensuring that the values match (say in content)?

The reason why I say this is that for helper-style classes that are used by
multiple top level modules and which inherently have nothing to do with content,
we usually put them in a lower-level module like base or ui. Then they don't
have to have a public interface and hidden implementation like they do now.

[dmazzoni, 2013-11-12 18:54:20]

On Tue, Nov 12, 2013 at 8:28 AM, <jam@chromium.org> wrote:

> Is it because of the one WebKit enum that's included?


Yes.


> I thought we talked about
> having that be a duplicate until the repos are merged, and having compile
> time
> asserts ensuring that the values match (say in content)?
>

Unfortunately compile-time asserts aren't very helpful, because that makes
it impossible to add or delete an enum in Blink without breaking Chromium.
The only possibility is a massive switch statement.

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

[jam, 2013-11-12 21:06:14]

On 2013/11/12 18:54:20, Dominic Mazzoni wrote:
> On Tue, Nov 12, 2013 at 8:28 AM, <mailto:jam@chromium.org> wrote:
> 
> > Is it because of the one WebKit enum that's included?
> 
> 
> Yes.
> 
> 
> > I thought we talked about
> > having that be a duplicate until the repos are merged, and having compile
> > time
> > asserts ensuring that the values match (say in content)?
> >
> 
> Unfortunately compile-time asserts aren't very helpful, because that makes
> it impossible to add or delete an enum in Blink without breaking Chromium.

Can you explain some more why it's impossible? I've seen this pattern in a
number of places; people have come up with ways of doing patches across repos to
deal with this (i.e. make chrome's code conditional on something that's defined
in the patch which lands later in blink)

> The only possibility is a massive switch statement.

[dmazzoni, 2013-11-12 21:13:18]

On Tue, Nov 12, 2013 at 1:06 PM, <jam@chromium.org> wrote:

> Can you explain some more why it's impossible? I've seen this pattern in a
> number of places; people have come up with ways of doing patches across
> repos to
> deal with this (i.e. make chrome's code conditional on something that's
> defined
> in the patch which lands later in blink)


Please point me to any examples of Blink -> Chromium enum mapping that
you're aware of that tries to make it easy to add/delete enums.

Within Blink, the standard pattern is to have parallel enums, with no
switch needed, just a direct copy - and then assertions to ensure they're
in sync.

I guess I'll go this route because I can't see an alternative. It's just
frustrating to have to main two sets of enums that can't even share the
same style guidelines.

- Dominic

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

[jam, 2013-11-12 21:42:03]

On 2013/11/12 21:13:18, Dominic Mazzoni wrote:
> On Tue, Nov 12, 2013 at 1:06 PM, <mailto:jam@chromium.org> wrote:
> 
> > Can you explain some more why it's impossible? I've seen this pattern in a
> > number of places; people have come up with ways of doing patches across
> > repos to
> > deal with this (i.e. make chrome's code conditional on something that's
> > defined
> > in the patch which lands later in blink)
> 
> 
> Please point me to any examples of Blink -> Chromium enum mapping 

Can you explain exactly what you mean by blink->chromium enum mapping, to make
sure we're talking about the same thing? I'm talking about an enum that has two
copies, one in blink and the other in chrome.

> that you're aware of that tries to make it easy to add/delete enums.

I was just referring to the impossible comment, I agree it's not easy as it does
take a 3-sided careful patch.

> 
> Within Blink, the standard pattern is to have parallel enums, with no
> switch needed, just a direct copy - and then assertions to ensure they're
> in sync.
> 
> I guess I'll go this route because I can't see an alternative. It's just
> frustrating to have to main two sets of enums that can't even share the
> same style guidelines.
> 
> - Dominic
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2013-11-13 15:27:45]

A quick suggestion.

Could we split this cl into a few pieces?

Let's try to land the AXTree/AXNode interfaces first and defer the more
difficult sections of this cl.


On Tue, Nov 12, 2013 at 1:42 PM, <jam@chromium.org> wrote:

> On 2013/11/12 21:13:18, Dominic Mazzoni wrote:
>
>  On Tue, Nov 12, 2013 at 1:06 PM, <mailto:jam@chromium.org> wrote:
>>
>
>  > Can you explain some more why it's impossible? I've seen this pattern
>> in a
>> > number of places; people have come up with ways of doing patches across
>> > repos to
>> > deal with this (i.e. make chrome's code conditional on something that's
>> > defined
>> > in the patch which lands later in blink)
>>
>
>
>  Please point me to any examples of Blink -> Chromium enum mapping
>>
>
> Can you explain exactly what you mean by blink->chromium enum mapping, to
> make
> sure we're talking about the same thing? I'm talking about an enum that
> has two
> copies, one in blink and the other in chrome.
>
>
>  that you're aware of that tries to make it easy to add/delete enums.
>>
>
> I was just referring to the impossible comment, I agree it's not easy as
> it does
> take a 3-sided careful patch.
>
>
>
>  Within Blink, the standard pattern is to have parallel enums, with no
>> switch needed, just a direct copy - and then assertions to ensure they're
>> in sync.
>>
>
>  I guess I'll go this route because I can't see an alternative. It's just
>> frustrating to have to main two sets of enums that can't even share the
>> same style guidelines.
>>
>
>  - Dominic
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/67283004/
>

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

[David Tseng, 2013-11-13 18:00:07]

https://codereview.chromium.org/67283004/diff/270001/content/common/ax_tree_s...
File content/common/ax_tree_source.h (right):

https://codereview.chromium.org/67283004/diff/270001/content/common/ax_tree_s...
content/common/ax_tree_source.h:20: template<class AXSourceNode>
AXNodeSource

https://codereview.chromium.org/67283004/diff/270001/content/common/ax_tree_s...
content/common/ax_tree_source.h:23: AXTreeSource() {}
These should probably be protected.

https://codereview.chromium.org/67283004/diff/270001/content/common/ax_tree_s...
content/common/ax_tree_source.h:26: virtual AXSourceNode* GetRoot() const = 0;
Do you need GetRoot*? If not, this class could be made entirely stateless which
has its advantages.

https://codereview.chromium.org/67283004/diff/270001/content/public/common/ax...
File content/public/common/ax_tree_update.h (right):

https://codereview.chromium.org/67283004/diff/270001/content/public/common/ax...
content/public/common/ax_tree_update.h:34: struct CONTENT_EXPORT AXTreeUpdate {
Btw, what do you see the purpose of this class considering that we will need
another structure to be defined as an IPC message?

[David Tseng, 2013-11-13 18:08:08]

lgtm

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
File content/common/ax_tree_impl.cc (right):

https://codereview.chromium.org/67283004/diff/40001/content/common/ax_tree_im...
content/common/ax_tree_impl.cc:57: int32 AXTreeImpl::GetId(const AXNode* node)
const {
On 2013/11/12 00:03:04, Dominic Mazzoni wrote:
> On 2013/11/11 18:20:35, aboxhall wrote:
> > What is the purpose of this and the below two methods, given they just call
a
> > public method on the node in each case? Is it in case some platform wants to
> > override them?
> 
> This is because AXSourceTree is an abstraction of an accessibility tree where
> the type (AXNode here) is opaque. AXSourceTree allows us to abstract the
> Blink accessibility tree and implement AXTreeSerializer once rather than
> needing to duplicate that code.

I don't entirely understand this; the methods duplicated in this cl are on both
AXNode and AXTree; if you don't mean to potential subclasses to override them,
then make them private perhaps?

[David Tseng, 2013-11-13 18:08:11]

lgtm

[dmazzoni, 2013-11-13 19:35:58]

OK, I moved this from content/ to ui/accessibility.

We're going to go the route of having separate
accessibility enums in Chromium.

After this comment I'm going to update the change
title and description to match, and update the list
of reviewers.

> Could we split this cl into a few pieces?
> Let's try to land the AXTree/AXNode interfaces
> first and defer the more difficult sections of this cl.

It should be simpler now that there's no need for
separate interfaces and implementation. I'm not sure
there's much more I could remove and have it still
compile.

https://codereview.chromium.org/67283004/diff/270001/content/common/ax_tree_s...
File content/common/ax_tree_source.h (right):

https://codereview.chromium.org/67283004/diff/270001/content/common/ax_tree_s...
content/common/ax_tree_source.h:20: template<class AXSourceNode>
On 2013/11/13 18:00:07, David Tseng wrote:
> AXNodeSource

Done.

https://codereview.chromium.org/67283004/diff/270001/content/common/ax_tree_s...
content/common/ax_tree_source.h:23: AXTreeSource() {}
On 2013/11/13 18:00:07, David Tseng wrote:
> These should probably be protected.

Done.

https://codereview.chromium.org/67283004/diff/270001/content/common/ax_tree_s...
content/common/ax_tree_source.h:26: virtual AXSourceNode* GetRoot() const = 0;
On 2013/11/13 18:00:07, David Tseng wrote:
> Do you need GetRoot*? If not, this class could be made entirely stateless
which
> has its advantages.

Not currently used so I deleted it. I think it is stateless either way, right?

https://codereview.chromium.org/67283004/diff/270001/content/public/common/ax...
File content/public/common/ax_tree_update.h (right):

https://codereview.chromium.org/67283004/diff/270001/content/public/common/ax...
content/public/common/ax_tree_update.h:34: struct CONTENT_EXPORT AXTreeUpdate {
On 2013/11/13 18:00:07, David Tseng wrote:
> Btw, what do you see the purpose of this class considering that we will need
> another structure to be defined as an IPC message?

I expect this to be contained in the IPC message. It allows
testing serialization and unserialization of an
accessibility tree without any IPC infrastructure. It also
allows us to send updates in more than one IPC message -
for example I expect the new extension code will use a
different message.

[David Tseng, 2013-11-15 18:05:31]

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_node.h
File ui/accessibility/ax_node.h (right):

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_node...
ui/accessibility/ax_node.h:31: virtual void Init(AXNode* parent, int32 id, int32
index_in_parent);
Where does this id come from? Id is part of the AXNodeData structure right?

IMO, would be better to just use SetData and eliminate Init.

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_node...
ui/accessibility/ax_node.h:38: void UpdateIndexInParent(int index_in_parent);
Just a setter; call it SetIndexInParent.

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_node...
File ui/accessibility/ax_node_data.cc (right):

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_node...
ui/accessibility/ax_node_data.cc:22: state(-1) {
What does it mean to have state of -1?

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_roles.h
File ui/accessibility/ax_roles.h (right):

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_role...
ui/accessibility/ax_roles.h:8: // These should be kept in sync with
third_party/WebKit/public/web/WebAXEnums.h
Maybe name this file ax_enums.h?

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree.h
File ui/accessibility/ax_tree.h (right):

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree.h:22: // It's designed to be subclassed to ement
support for native
Optional: I still think this design isn't a good abstraction for all of our use
cases. Namely, in the extension process, we don't need any of the facilities
provided by AXTreeSource.

Proposed alternative:
AXTreeSource stays as is.
Create a new interface called AXTreeReceiver which implements an
Unserialize(AXTreeUpdate) method along with anything else needed.

Thus,
Blink: only needs to implement the AXTreeSource interface.
Browser: needs to implement both AXTreeSource and AXTreeReceiver interfaces.
Extension: only needs to implement the AXTreeReceiver interface.

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree.h:22: // It's designed to be subclassed to ement
support for native
s/ement/implement

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree.h:36: virtual AXTreeSource<AXNode>* CreateTreeSource();
I think this method belongs on the AXTreeSource interface.

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree...
File ui/accessibility/ax_tree_serializer.cc (left):

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.cc:12: }  // namespace ui
This file looks empty; are you missing a commit?

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree...
File ui/accessibility/ax_tree_source.h (right):

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_source.h:33: virtual void SerializeNode(
Why not just serialize the entire subtree here and eliminate the need for a
serializer class?

[dmazzoni, 2013-11-18 08:09:25]

@ben I added ax_export.h, as requested - there are
no dependencies on ui/base now.

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_node.h
File ui/accessibility/ax_node.h (right):

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_node...
ui/accessibility/ax_node.h:31: virtual void Init(AXNode* parent, int32 id, int32
index_in_parent);
On 2013/11/15 18:05:31, David Tseng wrote:
> Where does this id come from? Id is part of the AXNodeData structure right?
> 
> IMO, would be better to just use SetData and eliminate Init.

The node isn't valid without a parent, id, and index in parent, but it is valid
with empty data. Also, the parent and id aren't allowed to change, so I don't
want a method that implies they can.

How about this, I moved this to the constructor. That should be more clear.
That means we can't inherit from this class for platform-specific classes, but
we were already moving that direction.

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_node...
ui/accessibility/ax_node.h:38: void UpdateIndexInParent(int index_in_parent);
On 2013/11/15 18:05:31, David Tseng wrote:
> Just a setter; call it SetIndexInParent.

Done.

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_node...
File ui/accessibility/ax_node_data.cc (right):

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_node...
ui/accessibility/ax_node_data.cc:22: state(-1) {
On 2013/11/15 18:05:31, David Tseng wrote:
> What does it mean to have state of -1?

Since it's a bitmask, it means everything is on - it makes it really glaringly
obvious if it's left uninitialized because so many uncommon state flags are on.

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_roles.h
File ui/accessibility/ax_roles.h (right):

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_role...
ui/accessibility/ax_roles.h:8: // These should be kept in sync with
third_party/WebKit/public/web/WebAXEnums.h
On 2013/11/15 18:05:31, David Tseng wrote:
> Maybe name this file ax_enums.h?

Done.

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree.h
File ui/accessibility/ax_tree.h (right):

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree.h:22: // It's designed to be subclassed to ement
support for native
On 2013/11/15 18:05:31, David Tseng wrote:
> s/ement/implement

Done.

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree.h:22: // It's designed to be subclassed to ement
support for native
On 2013/11/15 18:05:31, David Tseng wrote:
> Optional: I still think this design isn't a good abstraction for all of our
use
> cases. Namely, in the extension process, we don't need any of the facilities
> provided by AXTreeSource.

OK, I moved that part into a subclass, AXSerializableTree.

Right now I can't think of a time when we'd want an AXTree that's not a
receiver tree, so rather than split, let's leave it combined and refactor later
when needed. I don't want to add complexity to the class hierarchy that
we won't actually use.

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree.h:36: virtual AXTreeSource<AXNode>* CreateTreeSource();
On 2013/11/15 18:05:31, David Tseng wrote:
> I think this method belongs on the AXTreeSource interface.

This is now in AXSerializableTree.

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree...
File ui/accessibility/ax_tree_serializer.cc (left):

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.cc:12: }  // namespace ui
On 2013/11/15 18:05:31, David Tseng wrote:
> This file looks empty; are you missing a commit?

There's an empty constructor and descructor in this file, that's it. The rest is
in the header file because it's templatized, but this constructor and descructor
must be in the source file.

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree...
File ui/accessibility/ax_tree_source.h (right):

https://codereview.chromium.org/67283004/diff/490001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_source.h:33: virtual void SerializeNode(
On 2013/11/15 18:05:31, David Tseng wrote:
> Why not just serialize the entire subtree here and eliminate the need for a
> serializer class?

Because the serializer needs to maintain state and stay in sync with its
destination.
It makes it possible for there to be multiple destinations all listening to the
same
source tree that aren't all in sync with each other. Or, it allows you to
temporarily get a
snapshot of the whole tree without disturbing an existing serializer.

[aboxhall, 2013-11-18 21:57:10]

lgtm

Are the parallel enum assertions meant to be included in this change too?

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_enums.h
File ui/accessibility/ax_enums.h (right):

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_enum...
ui/accessibility/ax_enums.h:88: AX_ROLE_S_V_G_ROOT,
Could this be AX_ROLE_SVG_ROOT?

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_seri...
File ui/accessibility/ax_serializable_tree.h (right):

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_seri...
ui/accessibility/ax_serializable_tree.h:19: // Get a TreeSource interface for
this tree. The client gets ownership
I was momentarily confused by 'Get' here. Even though it's redundant with the
method name, perhaps change to 'Create'. Also, 'interface' confuses me here too
(possibly I've just done too much Java programming). Perhaps just leave it out?
I assume it's intended to mean "TreeSource [which acts as an] interface"?

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree.cc
File ui/accessibility/ax_tree.cc (right):

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree.cc:21: Unserialize(initial_state);
Why not just call UpdateNode with root directly? Do we expect the Unserialize
logic to change/be overridden?

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
File ui/accessibility/ax_tree_serializer.h (right):

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:28: // every node that changes state.
What's handled automatically is if
Some sample code might be useful here. Up to you.

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:49: void
DeleteClientTreeNodeAndDescendants(ClientTreeNode* client_node);
Could this be accurately renamed ClearClientTreeNode? The comment provides extra
information on what's happening.

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
File ui/accessibility/ax_tree_unittest.cc (right):

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_unittest.cc:14: TEST(AXTreeTest, TestSerialize) {
It'd be nice to have a test for the update case as well.

[dmazzoni, 2013-11-19 17:13:54]

On 2013/11/18 21:57:10, aboxhall wrote:
> Are the parallel enum assertions meant to be included in this change too?

No, I'll include those when I refactor content/renderer/accessibility to use
these classes.

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_enums.h
File ui/accessibility/ax_enums.h (right):

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_enum...
ui/accessibility/ax_enums.h:88: AX_ROLE_S_V_G_ROOT,
On 2013/11/18 21:57:10, aboxhall wrote:
> Could this be AX_ROLE_SVG_ROOT?

Good catch, thanks. That's what I get for trying to apply an automatic rule to
rename the Blink enums.

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_seri...
File ui/accessibility/ax_serializable_tree.h (right):

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_seri...
ui/accessibility/ax_serializable_tree.h:19: // Get a TreeSource interface for
this tree. The client gets ownership
On 2013/11/18 21:57:10, aboxhall wrote:
> I was momentarily confused by 'Get' here. Even though it's redundant with the
> method name, perhaps change to 'Create'. Also, 'interface' confuses me here
too
> (possibly I've just done too much Java programming). Perhaps just leave it
out?
> I assume it's intended to mean "TreeSource [which acts as an] interface"?

Changed to "Create a TreeSource adapter".

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree.cc
File ui/accessibility/ax_tree.cc (right):

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree.cc:21: Unserialize(initial_state);
On 2013/11/18 21:57:10, aboxhall wrote:
> Why not just call UpdateNode with root directly? Do we expect the Unserialize
> logic to change/be overridden?

Yes - in BrowserAccessibilityManager the equivalent method does a little
postprocessing. It wasn't needed when I simplified the code here, but my guess
is that when BrowserAccessibilityManager is refactored to inherit from this,
it will need to extend Unserialize.

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
File ui/accessibility/ax_tree_serializer.h (right):

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:28: // every node that changes state.
What's handled automatically is if
On 2013/11/18 21:57:10, aboxhall wrote:
> Some sample code might be useful here. Up to you.

I think it will become more apparent in the next code review, when
content/renderer/accessibility is refactored to implement AXTreeSource.

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:49: void
DeleteClientTreeNodeAndDescendants(ClientTreeNode* client_node);
On 2013/11/18 21:57:10, aboxhall wrote:
> Could this be accurately renamed ClearClientTreeNode? The comment provides
extra
> information on what's happening.

Clear sounds confusing to me, it implies clearing out its data but not deleting
the object itself. How about DeleteClientTreeNodeSubtree?

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
File ui/accessibility/ax_tree_unittest.cc (right):

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_unittest.cc:14: TEST(AXTreeTest, TestSerialize) {
On 2013/11/18 21:57:10, aboxhall wrote:
> It'd be nice to have a test for the update case as well.

I have a more comprehensive unit test already written and
coming in the next changelist! I left it out to keep this
change from getting too large, I want to focus on the
interface here and not the algorithm.

[aboxhall, 2013-11-19 17:18:23]

lgtm

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
File ui/accessibility/ax_tree_serializer.h (right):

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:49: void
DeleteClientTreeNodeAndDescendants(ClientTreeNode* client_node);
On 2013/11/19 17:13:54, Dominic Mazzoni wrote:
> On 2013/11/18 21:57:10, aboxhall wrote:
> > Could this be accurately renamed ClearClientTreeNode? The comment provides
> extra
> > information on what's happening.
> 
> Clear sounds confusing to me, it implies clearing out its data but not
deleting
> the object itself. How about DeleteClientTreeNodeSubtree?

Sure, or even DeleteSubtree (since the ClientTreeNode is implied by the
argument). Either way.

[David Tseng, 2013-11-20 23:31:04]

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
File ui/accessibility/ax_tree_serializer.h (right):

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:28: // every node that changes state.
What's handled automatically is if
On 2013/11/19 17:13:54, Dominic Mazzoni wrote:
> On 2013/11/18 21:57:10, aboxhall wrote:
> > Some sample code might be useful here. Up to you.
> 
> I think it will become more apparent in the next code review, when
> content/renderer/accessibility is refactored to implement AXTreeSource.

How about removing the comment block here and adding a TODO to put sample usage
code here? I'm slightly confused by the comment overall and would find usage
code much clearer.

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:129: // about. If we don't find it, then
this must be the new root of the
This assumes that the caller always first tries to serialize the root node
right? Would we ever try serializing a non root node first?

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:144: //client_node->location = ...;
TODO? Do we need to save any more info (besides id, and parent/children).

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:232: //reused_child->location =
obj.boundingBoxRect();
?

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:248: }
Move these methods to impl file (.cc).

[dmazzoni, 2013-11-21 17:27:43]

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
File ui/accessibility/ax_tree_serializer.h (right):

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:28: // every node that changes state.
What's handled automatically is if
On 2013/11/20 23:31:05, David Tseng wrote:
> On 2013/11/19 17:13:54, Dominic Mazzoni wrote:
> > On 2013/11/18 21:57:10, aboxhall wrote:
> > > Some sample code might be useful here. Up to you.
> > 
> > I think it will become more apparent in the next code review, when
> > content/renderer/accessibility is refactored to implement AXTreeSource.
> 
> How about removing the comment block here and adding a TODO to put sample
usage
> code here? I'm slightly confused by the comment overall and would find usage
> code much clearer.

I clarified the comment and added a TODO.

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:49: void
DeleteClientTreeNodeAndDescendants(ClientTreeNode* client_node);
On 2013/11/19 17:18:23, aboxhall wrote:
> On 2013/11/19 17:13:54, Dominic Mazzoni wrote:
> > On 2013/11/18 21:57:10, aboxhall wrote:
> > > Could this be accurately renamed ClearClientTreeNode? The comment provides
> > extra
> > > information on what's happening.
> > 
> > Clear sounds confusing to me, it implies clearing out its data but not
> deleting
> > the object itself. How about DeleteClientTreeNodeSubtree?
> 
> Sure, or even DeleteSubtree (since the ClientTreeNode is implied by the
> argument). Either way.

How about DeleteClientSubtree?

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:129: // about. If we don't find it, then
this must be the new root of the
On 2013/11/20 23:31:05, David Tseng wrote:
> This assumes that the caller always first tries to serialize the root node
> right? Would we ever try serializing a non root node first?

Added a TODO. Previously we handled that case but not in this part of the code.
A better fix is coming in the next change.

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:144: //client_node->location = ...;
On 2013/11/20 23:31:05, David Tseng wrote:
> TODO? Do we need to save any more info (besides id, and parent/children).

I just deleted those for now, I'll add them back in a later change. I just
wanted to keep this one simpler and get the tree changing semantics right first
before worrying about location changes, focus changes, and other higher-level
things like that.

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:232: //reused_child->location =
obj.boundingBoxRect();
On 2013/11/20 23:31:05, David Tseng wrote:
> ?

Done.

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:248: }
On 2013/11/20 23:31:05, David Tseng wrote:
> Move these methods to impl file (.cc).

I can't because the class is templatized.

If I made it not templatized, it'd have to essentially take a void* insetad of
AXSourceNode. I hate putting code in a header file, but in this case it seems
preferable to do that rather than lose type-safety for this class.

[aboxhall, 2013-11-21 22:15:46]

lgtm

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
File ui/accessibility/ax_tree_serializer.h (right):

https://codereview.chromium.org/67283004/diff/640001/ui/accessibility/ax_tree...
ui/accessibility/ax_tree_serializer.h:49: void
DeleteClientTreeNodeAndDescendants(ClientTreeNode* client_node);
On 2013/11/21 17:27:43, Dominic Mazzoni wrote:
> On 2013/11/19 17:18:23, aboxhall wrote:
> > On 2013/11/19 17:13:54, Dominic Mazzoni wrote:
> > > On 2013/11/18 21:57:10, aboxhall wrote:
> > > > Could this be accurately renamed ClearClientTreeNode? The comment
provides
> > > extra
> > > > information on what's happening.
> > > 
> > > Clear sounds confusing to me, it implies clearing out its data but not
> > deleting
> > > the object itself. How about DeleteClientTreeNodeSubtree?
> > 
> > Sure, or even DeleteSubtree (since the ClientTreeNode is implied by the
> > argument). Either way.
> 
> How about DeleteClientSubtree?

Seems legit.

[Ben Goodger (Google), 2013-11-21 23:15:41]

https://codereview.chromium.org/67283004/diff/820001/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/67283004/diff/820001/ui/ui.gyp#newcode49
ui/ui.gyp:49: 'accessibility/ax_enums.h',
since this has a subdir in src/ui, it should also have its own gyp file and
targets. I know src/ui/ui.gyp is confusing in this regard but the intent is to
move it into src/ui/base to avoid this confusion.

[dmazzoni, 2013-11-22 00:07:14]

https://codereview.chromium.org/67283004/diff/820001/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/67283004/diff/820001/ui/ui.gyp#newcode49
ui/ui.gyp:49: 'accessibility/ax_enums.h',
On 2013/11/21 23:15:42, Ben Goodger (Google) wrote:
> since this has a subdir in src/ui, it should also have its own gyp file and
> targets. I know src/ui/ui.gyp is confusing in this regard but the intent is to
> move it into src/ui/base to avoid this confusion.

Got it. Done.

[Ben Goodger (Google), 2013-11-22 05:54:03]

cool! lgtm. don't forget to file a ticket to get your unittests run by the bots.

[dmazzoni, 2013-11-22 06:30:20]

On 2013/11/22 05:54:03, Ben Goodger (Google) wrote:
> cool! lgtm. don't forget to file a ticket to get your unittests run by the
bots.

Thanks! Will do.

[commit-bot: I haz the power, 2013-11-22 06:30:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/67283004/990001

[commit-bot: I haz the power, 2013-11-22 08:59:05]

Change committed as 236733