Add AXTreeDelegate and refactor other AXTree classes slightly.

Add AXTreeDelegate and refactor other AXTree classes slightly.

AXTreeDelegate will allow content/browser to make BrowserAccessibility
a wrapper around AXNode. Test it using gmock.

Change AXTreeSource to take a typename rather than a class so that
we can use it with WebAXObject (which is an object that owns a pointer,
not a pointer itself).

Finally, replace GetChildAtIndex with a function GetChildren that
populates a vector. This allows content/renderer to more efficiently
filter out children that don't belong in the tree.

BUG=316726
R=dtseng@chromium.org

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

[tfarina, 2014-01-11 23:58:50]

Please, do not land this with gmock dep.

https://codereview.chromium.org/125783002/diff/1/ui/accessibility/accessibili...
File ui/accessibility/accessibility.gyp (right):

https://codereview.chromium.org/125783002/diff/1/ui/accessibility/accessibili...
ui/accessibility/accessibility.gyp:46: '../../testing/gmock.gyp:gmock',
no, no. Do not use Gmock. Ben banned it!

[dmazzoni, 2014-01-12 08:10:15]

+ben@chromium.org

I just re-read the thread on gmock from a year ago, and I’m pretty sure no
overall consensus was reached - most people including Ben acknolwedged that
gmock can be used correctly and we don’t need to completely ban it. Ben
banned gmock for *integration* tests in directories that he owns - not for
unit tests. Lots of uses of gmock have been added since then.

Ben, please correct me if I'm wrong.

Thiago, if you have a suggestion for a better way to test this particular
interface other than gmock, I'm open to suggestions.

On Sat, Jan 11, 2014 at 3:58 PM, <tfarina@chromium.org> wrote:

> Please, do not land this with gmock dep.
>
>
> https://codereview.chromium.org/125783002/diff/1/ui/
> accessibility/accessibility.gyp
> File ui/accessibility/accessibility.gyp (right):
>
> https://codereview.chromium.org/125783002/diff/1/ui/
> accessibility/accessibility.gyp#newcode46
> ui/accessibility/accessibility.gyp:46: '../../testing/gmock.gyp:gmock',
> no, no. Do not use Gmock. Ben banned it!
>
> https://codereview.chromium.org/125783002/
>

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

[Ben Goodger (Google), 2014-01-13 22:39:03]

I guess my question would be - what's the footprint of gmock in
ui/accessibility today?

The test example you showed could be trivially manually mocked which is
what we do in other libs under src/ui.

-Ben


On Sun, Jan 12, 2014 at 12:10 AM, Dominic Mazzoni <dmazzoni@chromium.org>wrote:

> +ben@chromium.org
>
> I just re-read the thread on gmock from a year ago, and I’m pretty sure no
> overall consensus was reached - most people including Ben acknolwedged that
> gmock can be used correctly and we don’t need to completely ban it. Ben
> banned gmock for *integration* tests in directories that he owns - not for
> unit tests. Lots of uses of gmock have been added since then.
>
> Ben, please correct me if I'm wrong.
>
> Thiago, if you have a suggestion for a better way to test this particular
> interface other than gmock, I'm open to suggestions.
>
> On Sat, Jan 11, 2014 at 3:58 PM, <tfarina@chromium.org> wrote:
>
>> Please, do not land this with gmock dep.
>>
>>
>> https://codereview.chromium.org/125783002/diff/1/ui/
>> accessibility/accessibility.gyp
>> File ui/accessibility/accessibility.gyp (right):
>>
>> https://codereview.chromium.org/125783002/diff/1/ui/
>> accessibility/accessibility.gyp#newcode46
>> ui/accessibility/accessibility.gyp:46: '../../testing/gmock.gyp:gmock',
>> no, no. Do not use Gmock. Ben banned it!
>>
>> https://codereview.chromium.org/125783002/
>>
>
>

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

[tfarina, 2014-01-13 22:44:37]

You can get out of the mock pretty easily, I also think it helps clarifying what
the exact sequence things will happen.

Let me know if you get in trouble.

https://codereview.chromium.org/125783002/diff/1/ui/accessibility/ax_tree_uni...
File ui/accessibility/ax_tree_unittest.cc (right):

https://codereview.chromium.org/125783002/diff/1/ui/accessibility/ax_tree_uni...
ui/accessibility/ax_tree_unittest.cc:20: MOCK_METHOD1(OnNodeWillBeDeleted,
void(AXNode*));
yeah, something like:

class AXTreeDelegateImpl : public AXTreeDelegate {
 public:
  virtual void OnNodeWillBeDeleted(AXNode*) {
    deleted_ = true;
  }

  ...

  bool deleted() const { return deleted_; }

 private:
  bool deleted_;  # Or you could you use an int for a counter, deleted_count_
};

[dmazzoni, 2014-01-13 22:46:37]

OK, I'll switch it to a manual mock. Just an FYI, I'm on leave now so this
changelist will stay open for a while.

On Mon, Jan 13, 2014 at 2:44 PM, <tfarina@chromium.org> wrote:

> You can get out of the mock pretty easily, I also think it helps
> clarifying what
> the exact sequence things will happen.
>
> Let me know if you get in trouble.
>
>
> https://codereview.chromium.org/125783002/diff/1/ui/accessibility/ax_tree_
> unittest.cc
> File ui/accessibility/ax_tree_unittest.cc (right):
>
> https://codereview.chromium.org/125783002/diff/1/ui/accessibility/ax_tree_
> unittest.cc#newcode20
> ui/accessibility/ax_tree_unittest.cc:20:
> MOCK_METHOD1(OnNodeWillBeDeleted, void(AXNode*));
> yeah, something like:
>
> class AXTreeDelegateImpl : public AXTreeDelegate {
>  public:
>   virtual void OnNodeWillBeDeleted(AXNode*) {
>     deleted_ = true;
>   }
>
>   ...
>
>   bool deleted() const { return deleted_; }
>
>  private:
>   bool deleted_;  # Or you could you use an int for a counter,
> deleted_count_
> };
>
> https://codereview.chromium.org/125783002/
>

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

[dmazzoni, 2014-02-19 22:25:04]

Thanks for your patience, I've been on leave for a while. I'm back now and I
just updated this change to remove the dependence on gmock.

Ready for a fresh look.

[tfarina, 2014-02-19 22:30:43]

unittests lgtm (just looked at them).

https://codereview.chromium.org/125783002/diff/200001/ui/accessibility/ax_tre...
File ui/accessibility/ax_tree_unittest.cc (right):

https://codereview.chromium.org/125783002/diff/200001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree_unittest.cc:17: class MockAXTreeDelegate : public
AXTreeDelegate {
Mock is fine by me if you want to use it, but I'd suggest AXTreeDelegateImpl or
FakeAXTreeDelegate or AXTreeDelegateTest, or whatever.

I wouldn't use Mock, since you are not using gmock here anymore.

https://codereview.chromium.org/125783002/diff/200001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree_unittest.cc:54: }  // anonymous namespace
nit: please remove the 'anonymous' from this comment. And it is unnamed! :)

[dmazzoni, 2014-02-19 22:37:02]

https://codereview.chromium.org/125783002/diff/200001/ui/accessibility/ax_tre...
File ui/accessibility/ax_tree_unittest.cc (right):

https://codereview.chromium.org/125783002/diff/200001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree_unittest.cc:17: class MockAXTreeDelegate : public
AXTreeDelegate {
On 2014/02/19 22:30:43, tfarina wrote:
> Mock is fine by me if you want to use it, but I'd suggest AXTreeDelegateImpl
or
> FakeAXTreeDelegate or AXTreeDelegateTest, or whatever.
> 
> I wouldn't use Mock, since you are not using gmock here anymore.

I'm happy to change it to Fake, but I don't see why we shouldn't be allowed to
use the word "Mock" when it's a well-understood generic term for a class that
asserts that an interface is used correctly.

We have ProfileMock and dozens of others in the Chrome codebase, none of which
use gmock.

("Fake" is still correct, though - it's just that Mock is a specific type of
Fake.)

https://codereview.chromium.org/125783002/diff/200001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree_unittest.cc:54: }  // anonymous namespace
On 2014/02/19 22:30:43, tfarina wrote:
> nit: please remove the 'anonymous' from this comment. And it is unnamed! :)

Done.

[David Tseng, 2014-02-21 00:33:22]

https://codereview.chromium.org/125783002/diff/1/ui/accessibility/ax_tree.cc
File ui/accessibility/ax_tree.cc (left):

https://codereview.chromium.org/125783002/diff/1/ui/accessibility/ax_tree.cc#...
ui/accessibility/ax_tree.cc:168: node->Destroy();
Does this keep an AXNode's data valid for all platforms?

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

https://codereview.chromium.org/125783002/diff/1/ui/accessibility/ax_tree.h#n...
ui/accessibility/ax_tree.h:27: // but its links to parents and children are
invalid. This is called
s/This/If this

https://codereview.chromium.org/125783002/diff/1/ui/accessibility/ax_tree.h#n...
ui/accessibility/ax_tree.h:29: virtual void OnNodeWillBeDeleted(AXNode* node) =
0;
nit: how about just OnWillDeleteNode

https://codereview.chromium.org/125783002/diff/1/ui/accessibility/ax_tree.h#n...
ui/accessibility/ax_tree.h:35: virtual void OnNodeCreated(AXNode* node) = 0;
OnDidCreateNode

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_ser...
File ui/accessibility/ax_serializable_tree.cc (right):

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_ser...
ui/accessibility/ax_serializable_tree.cc:55: virtual const AXNode* GetNull()
const OVERRIDE {
Is there ever an instance where GetNull wouldn't return NULL?

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_tre...
File ui/accessibility/ax_tree_serializer.h (right):

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree_serializer.h:128: AXSourceNode* lca);
out_lca; also, is the above comment obsolete? You used to pass in AXSourceNode**
(and so can change the pointer to the new lca); or is there something I'm
missing with the template.

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree_serializer.h:175: if (client_root_) {
Also, can we move these non-trivial functions out of this header into the impl
file?

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_tre...
File ui/accessibility/ax_tree_source.h (right):

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree_source.h:36: 
Please add some documentation for potential implementers.

[dmazzoni, 2014-02-21 06:44:59]

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_ser...
File ui/accessibility/ax_serializable_tree.cc (right):

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_ser...
ui/accessibility/ax_serializable_tree.cc:55: virtual const AXNode* GetNull()
const OVERRIDE {
On 2014/02/21 00:33:23, David Tseng wrote:
> Is there ever an instance where GetNull wouldn't return NULL?

Yes - I had to add this so I can make AXTreeSource<WebAXObject>, because
WebAXObject is just a ref-counted wrapper class, not a pointer - it's never null
itself, you have to call isNull() on it to see if doesn't point to anything.

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_tre...
File ui/accessibility/ax_tree_serializer.h (right):

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree_serializer.h:128: AXSourceNode* lca);
On 2014/02/21 00:33:23, David Tseng wrote:
> out_lca; also, is the above comment obsolete? You used to pass in
AXSourceNode**
> (and so can change the pointer to the new lca); or is there something I'm
> missing with the template.

The comment is still valid. Remember that AXSourceNode is now a typename. In
content/browser, AXSourceNode will be a AXNode* - but in content/renderer,
AXSourceNode will be a WebAXObject - an object, not a pointer.

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree_serializer.h:175: if (client_root_) {
On 2014/02/21 00:33:23, David Tseng wrote:
> Also, can we move these non-trivial functions out of this header into the impl
> file?

Sadly no - templatized classes in C++ have to be fully defined in the header
file because the compiler has to actually expand out the class every time you
specialize it with a specific type.

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_tre...
File ui/accessibility/ax_tree_source.h (right):

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree_source.h:36: 
On 2014/02/21 00:33:23, David Tseng wrote:
> Please add some documentation for potential implementers.

Done.

[David Tseng, 2014-02-21 23:51:12]

LGTM with comment.

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_tre...
File ui/accessibility/ax_tree_serializer.h (right):

https://codereview.chromium.org/125783002/diff/280001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree_serializer.h:175: if (client_root_) {
On 2014/02/21 06:44:59, dmazzoni wrote:
> On 2014/02/21 00:33:23, David Tseng wrote:
> > Also, can we move these non-trivial functions out of this header into the
impl
> > file?
> 
> Sadly no - templatized classes in C++ have to be fully defined in the header
> file because the compiler has to actually expand out the class every time you
> specialize it with a specific type.

There are ways around this:
http://www.parashift.com/c++-faq-lite/separate-template-fn-defn-from-decl.html
http://www.parashift.com/c++-faq-lite/separate-template-fn-defn-from-decl-exp...
Not sure if it's a concern at this point, but eventually in-lining all of these
functions may be undesirable.

[dmazzoni, 2014-02-21 23:55:19]

On Fri, Feb 21, 2014 at 3:51 PM, <dtseng@chromium.org> wrote:

> Sadly no - templatized classes in C++ have to be fully defined in the
>>
> header
>
>> file because the compiler has to actually expand out the class every
>>
> time you
>
>> specialize it with a specific type.
>>
>
> There are ways around this:
> http://www.parashift.com/c++-faq-lite/separate-template-fn-
> defn-from-decl.html
> http://www.parashift.com/c++-faq-lite/separate-template-fn-
> defn-from-decl-export-keyword.html
> Not sure if it's a concern at this point, but eventually in-lining all
> of these functions may be undesirable.
>

This is a great trick when the template argument is something like <int>,
but here we need it for types that aren't known to this module.

It looks like c++11 might give us a working solution to this, so once c++11
syntax is allowed in Chrome we can fix it then.

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

[dmazzoni, 2014-02-24 07:16:06]

Committed patchset #9 manually as r252888 (presubmit successful).