Extend AXTreeSourceViews to handle aura::Window and views::Widget.

Extend AXTreeSourceViews to handle aura::Window and views::Widget.


initial patch

BUG=

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

[David Tseng, 2014-04-22 23:22:17]

Still w.i.p. but ready for some feedback.

I still need to fill in removals, but the transitions from types should be
correct. Would also like your thoughts on what other types I need to wrap (e.g.
views existing under ash/).

[dmazzoni, 2014-04-23 05:06:44]

Great start!

This should be relatively easy to test. Consider writing an ash test - a lot of
the ash tests create a lot of Views like the status tray, launcher, and so on,
giving you lots of windows and views to explore.

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
File ui/views/accessibility/ax_node_source_obj_wrapper.h (right):

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
ui/views/accessibility/ax_node_source_obj_wrapper.h:19: class
AXNodeSourceObjWrapper {
Thinking about this some more, how about AXAuraObjWrapper?

Aura is a reasonable name to use that encompasses Windows, Widgets, and Views.
They're all Aura objects.

Putting NodeSource in the class name doesn't help, since this class doesn't
inherit from or use AXNodeSource, it's just meant to be used with it. But it
stands alone fine, and I could imagine writing tests for AXAuraObjWrapper that
don't involve AXNodeSource.

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
File ui/views/accessibility/ax_obj_cache.cc (right):

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
ui/views/accessibility/ax_obj_cache.cc:23: AXNodeSourceObjWrapper*
AXObjCache::GetOrCreate(View* view) {
I'm assuming these functions will register observers that listen for
destruction? Or should each wrapper do it?

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
File ui/views/accessibility/ax_obj_cache.h (right):

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
ui/views/accessibility/ax_obj_cache.h:24: class AXObjCache {
Then this could be AXAuraObjCache, which is good because AXObjCache sounds too
similar to AXObjectCache in blink.

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
File ui/views/accessibility/ax_view_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
ui/views/accessibility/ax_view_obj_wrapper.cc:26: // TODO(dtseng): Need to
handle |Widget| child of |View|.
Note that NativeViewHost has GetNativeViewAccessible. Maybe
GetNativeViewAccessible could return a AXAuraObjWrapper* on Chrome OS, and that
would be a solution to the problem in a way that doesn't require this class to
special-case NativeViewHost.

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
ui/views/accessibility/ax_view_obj_wrapper.cc:41: for (int32 state =
ui::AX_STATE_NONE;
Why can't you copy it directly? I think AXViewState::state interprets the state
bitmask the same way as AXNodeData::state.

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
File ui/views/accessibility/ax_widget_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
ui/views/accessibility/ax_widget_obj_wrapper.cc:16: #ifdef USE_AURA
Do we even want to compile this code on any non-Aura platforms? Note that
there's no non-Aura Windows code anymore, but honestly I think we should just
implement this code as Aura-only or possibly even Chrome OS only.

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
File ui/views/accessibility/ax_window_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
ui/views/accessibility/ax_window_obj_wrapper.cc:40: // TODO(dtseng): Window also
has title.
Hmmm, I think one of the two is just an internal name for debugging. You
probably just want the title passed through as AX_ATTR_NAME.

[David Tseng, 2014-04-23 18:20:00]

Still w.i.p.

PTAL for discussion. I added observers to remove our cached wrappers on
destruction, and a root wrapper that hooks up the ash shell. (which is currently
an illegal include). That root will need to be owned by an ash specific tree at
some point.

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
File ui/views/accessibility/ax_node_source_obj_wrapper.h (right):

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
ui/views/accessibility/ax_node_source_obj_wrapper.h:19: class
AXNodeSourceObjWrapper {
On 2014/04/23 05:06:44, dmazzoni wrote:
> Thinking about this some more, how about AXAuraObjWrapper?
> 
> Aura is a reasonable name to use that encompasses Windows, Widgets, and Views.
> They're all Aura objects.
> 
> Putting NodeSource in the class name doesn't help, since this class doesn't
> inherit from or use AXNodeSource, it's just meant to be used with it. But it
> stands alone fine, and I could imagine writing tests for AXAuraObjWrapper that
> don't involve AXNodeSource.

Well, AXNodeSource isn't an actual class, otherwise this would probably inherit
from it.

I think Aura in the name does make sense now that of the four platforms with
extension support, only one isn't using Aura (Mac).

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
File ui/views/accessibility/ax_obj_cache.cc (right):

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
ui/views/accessibility/ax_obj_cache.cc:23: AXNodeSourceObjWrapper*
AXObjCache::GetOrCreate(View* view) {
On 2014/04/23 05:06:44, dmazzoni wrote:
> I'm assuming these functions will register observers that listen for
> destruction? Or should each wrapper do it?

The widgets and windows will observe for destruction. Added.

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
File ui/views/accessibility/ax_obj_cache.h (right):

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
ui/views/accessibility/ax_obj_cache.h:24: class AXObjCache {
On 2014/04/23 05:06:44, dmazzoni wrote:
> Then this could be AXAuraObjCache, which is good because AXObjCache sounds too
> similar to AXObjectCache in blink.

There was a point where I would have liked to share the caching logic contained
in AXObjectCache since AXObjCache is heavily inspired by AXObjectCache, but yes,
the naming is probably too similar.

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
File ui/views/accessibility/ax_view_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
ui/views/accessibility/ax_view_obj_wrapper.cc:26: // TODO(dtseng): Need to
handle |Widget| child of |View|.
On 2014/04/23 05:06:44, dmazzoni wrote:
> Note that NativeViewHost has GetNativeViewAccessible. Maybe
> GetNativeViewAccessible could return a AXAuraObjWrapper* on Chrome OS, and
that
> would be a solution to the problem in a way that doesn't require this class to
> special-case NativeViewHost.



I think that could work. So, to reiterate, NativeViewHost being a subclass of
View, will be visited by traversing the children of a views::View hierarchy. We
would still need to know we're dealing with a NativeViewHost to call
GetNativeViewAccessible though?

The data we need at this stage (assuming the NativeViewHost wraps a web
contents) is the AXNodeData to be populated with routing/processid of the
ccontained web contents.

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
ui/views/accessibility/ax_view_obj_wrapper.cc:41: for (int32 state =
ui::AX_STATE_NONE;
On 2014/04/23 05:06:44, dmazzoni wrote:
> Why can't you copy it directly? I think AXViewState::state interprets the
state
> bitmask the same way as AXNodeData::state.

I could if I added an accesser...but AXViewState doesn't allow you to access the
state_ member as it's private.

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
File ui/views/accessibility/ax_widget_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
ui/views/accessibility/ax_widget_obj_wrapper.cc:16: #ifdef USE_AURA
On 2014/04/23 05:06:44, dmazzoni wrote:
> Do we even want to compile this code on any non-Aura platforms? Note that
> there's no non-Aura Windows code anymore, but honestly I think we should just
> implement this code as Aura-only or possibly even Chrome OS only.

sgtm; removed.

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
File ui/views/accessibility/ax_window_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
ui/views/accessibility/ax_window_obj_wrapper.cc:40: // TODO(dtseng): Window also
has title.
On 2014/04/23 05:06:44, dmazzoni wrote:
> Hmmm, I think one of the two is just an internal name for debugging. You
> probably just want the title passed through as AX_ATTR_NAME.

Done.

[David Tseng, 2014-04-23 21:32:55]

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
File ui/views/accessibility/ax_view_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/80001/ui/views/accessibility/a...
ui/views/accessibility/ax_view_obj_wrapper.cc:26: // TODO(dtseng): Need to
handle |Widget| child of |View|.
On 2014/04/23 18:20:01, David Tseng wrote:
> On 2014/04/23 05:06:44, dmazzoni wrote:
> > Note that NativeViewHost has GetNativeViewAccessible. Maybe
> > GetNativeViewAccessible could return a AXAuraObjWrapper* on Chrome OS, and
> that
> > would be a solution to the problem in a way that doesn't require this class
to
> > special-case NativeViewHost.
> 
> 
> 
> I think that could work. So, to reiterate, NativeViewHost being a subclass of
> View, will be visited by traversing the children of a views::View hierarchy.
We
> would still need to know we're dealing with a NativeViewHost to call
> GetNativeViewAccessible though?
> 
> The data we need at this stage (assuming the NativeViewHost wraps a web
> contents) is the AXNodeData to be populated with routing/processid of the
> ccontained web contents.

N/m; didn't check to see that GetNativeViewAccessible is defined on views::View
itself.

[dmazzoni, 2014-04-24 19:40:30]

>
> I think Aura in the name does make sense now that of the four platforms
> with extension support, only one isn't using Aura (Mac).


I don't quite follow - do we expect to have any desktop accessibility
support on any platform other than Chrome OS? I'm not sure it makes sense
even for Windows and Linux.

I think that could work. So, to reiterate, NativeViewHost being a
> subclass of View, will be visited by traversing the children of a
> views::View hierarchy. We would still need to know we're dealing with a
> NativeViewHost to call GetNativeViewAccessible though?
>

No, every view implements GetNativeViewAccessible already, but we currently
only use that method on Windows. I'm proposing that we make use of it here.

I could if I added an accesser...but AXViewState doesn't allow you to
> access the state_ member as it's private.


Oh...please just add one. That was done just to prevent bad usage of the
flags, but for copying it seems silly not to allow it.

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

[David Tseng, 2014-04-24 23:48:33]

PTAL. This is ready for a real review. I still have to make a pass to fixup
spacing/indentation, but otherwise, it should be up for scrutiny.

Added/revised the previous tests for AXTreeSourceViews. Also, added some
behaviors to hook up everything to our fake "root". The root still has no
role, so we'll have to come up with something there.



On Thu, Apr 24, 2014 at 12:40 PM, Dominic Mazzoni <dmazzoni@chromium.org>wrote:

> I think Aura in the name does make sense now that of the four platforms
>> with extension support, only one isn't using Aura (Mac).
>
>
> I don't quite follow - do we expect to have any desktop accessibility
> support on any platform other than Chrome OS? I'm not sure it makes sense
> even for Windows and Linux.
>
> I think that could work. So, to reiterate, NativeViewHost being a
>> subclass of View, will be visited by traversing the children of a
>> views::View hierarchy. We would still need to know we're dealing with a
>> NativeViewHost to call GetNativeViewAccessible though?
>>
>
> No, every view implements GetNativeViewAccessible already, but we
> currently only use that method on Windows. I'm proposing that we make use
> of it here.
>
> I could if I added an accesser...but AXViewState doesn't allow you to
>> access the state_ member as it's private.
>
>
> Oh...please just add one. That was done just to prevent bad usage of the
> flags, but for copying it seems silly not to allow it.
>
>  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.

[dmazzoni, 2014-04-25 05:55:23]

On Thu, Apr 24, 2014 at 4:48 PM, David Tseng <dtseng@chromium.org> wrote:

> PTAL. This is ready for a real review. I still have to make a pass to
> fixup spacing/indentation, but otherwise, it should be up for scrutiny.
>
> Added/revised the previous tests for AXTreeSourceViews. Also, added some
> behaviors to hook up everything to our fake "root". The root still has no
> role, so we'll have to come up with something there.
>

I'd be fine with adding a new role "desktop" to our enum. I'd rather have a
clear unambiguous role rather than try to repurpose somethign else.

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

[dmazzoni, 2014-04-25 06:13:58]

Some indentation issues, as you mentioned. Overall looks totally solid - if you
can address the only small concerns I have below this should be fine to submit,
maybe see if Alice can take a second pass for style if you're ready before I
return?

https://codereview.chromium.org/246433012/diff/210001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/automation_manager_views.cc (right):

https://codereview.chromium.org/246433012/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/automation_manager_views.cc:77:
AutomationManagerViews::  ~AutomationManagerViews() {}
Some extra whitespace here from a previous change

https://codereview.chromium.org/246433012/diff/210001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/ax_tree_source_views.cc (right):

https://codereview.chromium.org/246433012/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:12: namespace
views {
This lives in chrome/browser/ui/views now, so it shouldn't be in the views
namespace. Please fix throughout.

https://codereview.chromium.org/246433012/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:15:
root_.reset(new AXRootObjWrapper(0));
Could you have this assign the next id rather than using 0?

Also, ui/accessibility uses 0 to mean "no id" in a couple of places, so it's
better for "real" objects to start with an id of 1.

https://codereview.chromium.org/246433012/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:58: return
node1->GetID() == node2->GetID() && node1->GetID() != -1;
I think we use 0 for "no id" in ui/accessibility

https://codereview.chromium.org/246433012/diff/210001/ui/views/accessibility/...
File ui/views/accessibility/ax_view_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/210001/ui/views/accessibility/...
ui/views/accessibility/ax_view_obj_wrapper.cc:40: out_node_data->state = 0;
Please just add an accessor so you can copy it directly.

https://codereview.chromium.org/246433012/diff/210001/ui/views/accessibility/...
File ui/views/accessibility/ax_view_obj_wrapper.h (right):

https://codereview.chromium.org/246433012/diff/210001/ui/views/accessibility/...
ui/views/accessibility/ax_view_obj_wrapper.h:16: explicit AXViewObjWrapper(View*
view) : view_(view) {}
I wonder if creating a View object wrapper should:
* assert that is has a widget (so don't allow it on an unattached view)
* add the widget to the cache now if it wasn't already - thus guaranteeing that
we'll get a notification if this view is destroyed

[David Tseng, 2014-04-25 22:47:29]

+ aboxhall; please do a style check (and of course, feel free to comment on
anything else if you have the time).

https://codereview.chromium.org/246433012/diff/210001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/ax_tree_source_views.cc (right):

https://codereview.chromium.org/246433012/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:12: namespace
views {
On 2014/04/25 06:13:59, dmazzoni wrote:
> This lives in chrome/browser/ui/views now, so it shouldn't be in the views
> namespace. Please fix throughout.

Done.

https://codereview.chromium.org/246433012/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:15:
root_.reset(new AXRootObjWrapper(0));
On 2014/04/25 06:13:59, dmazzoni wrote:
> Could you have this assign the next id rather than using 0?
> 
> Also, ui/accessibility uses 0 to mean "no id" in a couple of places, so it's
> better for "real" objects to start with an id of 1.

Done.

https://codereview.chromium.org/246433012/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:58: return
node1->GetID() == node2->GetID() && node1->GetID() != -1;
On 2014/04/25 06:13:59, dmazzoni wrote:
> I think we use 0 for "no id" in ui/accessibility

Any distinction between no id and invalid id?

https://codereview.chromium.org/246433012/diff/210001/ui/views/accessibility/...
File ui/views/accessibility/ax_view_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/210001/ui/views/accessibility/...
ui/views/accessibility/ax_view_obj_wrapper.cc:40: out_node_data->state = 0;
On 2014/04/25 06:13:59, dmazzoni wrote:
> Please just add an accessor so you can copy it directly.

Done.

https://codereview.chromium.org/246433012/diff/210001/ui/views/accessibility/...
File ui/views/accessibility/ax_view_obj_wrapper.h (right):

https://codereview.chromium.org/246433012/diff/210001/ui/views/accessibility/...
ui/views/accessibility/ax_view_obj_wrapper.h:16: explicit AXViewObjWrapper(View*
view) : view_(view) {}
On 2014/04/25 06:13:59, dmazzoni wrote:
> I wonder if creating a View object wrapper should:
> * assert that is has a widget (so don't allow it on an unattached view)
> * add the widget to the cache now if it wasn't already - thus guaranteeing
that
> we'll get a notification if this view is destroyed

Done.

[dmazzoni, 2014-04-28 16:47:55]

On Fri, Apr 25, 2014 at 3:47 PM, <dtseng@chromium.org> wrote:

> https://codereview.chromium.org/246433012/diff/210001/
> chrome/browser/ui/views/accessibility/ax_tree_source_views.cc#newcode58
> chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:58: return
> node1->GetID() == node2->GetID() && node1->GetID() != -1;
> On 2014/04/25 06:13:59, dmazzoni wrote:
>
>> I think we use 0 for "no id" in ui/accessibility
>>
>
> Any distinction between no id and invalid id?


I don't think we're totally consistent except that we always use 1+ for
real ids. In ui/accessibility I've been using -1 for uninitialized id, 0
for anything else like "no id / not found in tree / placeholder" and 1+ for
real ids.

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

[David Tseng, 2014-04-28 17:11:41]

On Mon, Apr 28, 2014 at 9:47 AM, Dominic Mazzoni <dmazzoni@chromium.org>wrote:

> On Fri, Apr 25, 2014 at 3:47 PM, <dtseng@chromium.org> wrote:
>
>> https://codereview.chromium.org/246433012/diff/210001/
>> chrome/browser/ui/views/accessibility/ax_tree_source_views.cc#newcode58
>> chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:58: return
>> node1->GetID() == node2->GetID() && node1->GetID() != -1;
>> On 2014/04/25 06:13:59, dmazzoni wrote:
>>
>>> I think we use 0 for "no id" in ui/accessibility
>>>
>>
>> Any distinction between no id and invalid id?
>
>
> I don't think we're totally consistent except that we always use 1+ for
> real ids. In ui/accessibility I've been using -1 for uninitialized id, 0
> for anything else like "no id / not found in tree / placeholder" and 1+ for
> real ids.
>
> Ok; I've updated with  all real objects including the root getting an id
of >= 1 and -1 meaning "invalid".

 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-04-28 17:34:23]

Quick question before I do a proper review (and sorry for my slowness on that):
this doesn't actually implement chrome.automation.desktop (or getDesktop()),
right? Do you have a WIP for that at all? I'm curious to see how it ends up
looking.

Will do a pass on this CL now.

[David Tseng, 2014-04-28 17:54:10]

On Mon, Apr 28, 2014 at 10:34 AM, <aboxhall@chromium.org> wrote:

> Quick question before I do a proper review (and sorry for my slowness on
> that):
> this doesn't actually implement chrome.automation.desktop (or
> getDesktop()),
> right? Do you have a WIP for that at all? I'm curious to see how it ends up
> looking.
>

Because of the open questions in the spec, I haven't hooked up that part of
the cl. I figured this side was getting long enough on its own.
If it pans out like I think, the getDesktop part should be relatively
simple (it will just call AutomationManagerViews::Enable()). and
unserialize events on the js end

>
> Will do a pass on this CL now.
> Great; thanks!



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

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

[chromium-reviews, 2014-04-28 17:59:54]

Oh, no problem re not doing it in this CL; I was wondering whether you
might have a separate CL somewhere where we could take a look at what the
desktop API ends up looking like.


On Mon, Apr 28, 2014 at 10:54 AM, David Tseng <dtseng@chromium.org> wrote:

>
>
>
> On Mon, Apr 28, 2014 at 10:34 AM, <aboxhall@chromium.org> wrote:
>
>> Quick question before I do a proper review (and sorry for my slowness on
>> that):
>> this doesn't actually implement chrome.automation.desktop (or
>> getDesktop()),
>> right? Do you have a WIP for that at all? I'm curious to see how it ends
>> up
>> looking.
>>
>
> Because of the open questions in the spec, I haven't hooked up that part
> of the cl. I figured this side was getting long enough on its own.
> If it pans out like I think, the getDesktop part should be relatively
> simple (it will just call AutomationManagerViews::Enable()). and
> unserialize events on the js end
>
>>
>> Will do a pass on this CL now.
>> Great; thanks!
>
>
>
>> https://codereview.chromium.org/246433012/
>>
>
>

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

[David Tseng, 2014-04-28 18:37:34]

Nothing to show at the moment; I did update the spec with the latest
changes and hopefully we agree on that.


On Mon, Apr 28, 2014 at 10:59 AM, Alice Boxhall <aboxhall@google.com> wrote:

> Oh, no problem re not doing it in this CL; I was wondering whether you
> might have a separate CL somewhere where we could take a look at what the
> desktop API ends up looking like.
>
>
> On Mon, Apr 28, 2014 at 10:54 AM, David Tseng <dtseng@chromium.org> wrote:
>
>>
>>
>>
>> On Mon, Apr 28, 2014 at 10:34 AM, <aboxhall@chromium.org> wrote:
>>
>>> Quick question before I do a proper review (and sorry for my slowness on
>>> that):
>>> this doesn't actually implement chrome.automation.desktop (or
>>> getDesktop()),
>>> right? Do you have a WIP for that at all? I'm curious to see how it ends
>>> up
>>> looking.
>>>
>>
>> Because of the open questions in the spec, I haven't hooked up that part
>> of the cl. I figured this side was getting long enough on its own.
>> If it pans out like I think, the getDesktop part should be relatively
>> simple (it will just call AutomationManagerViews::Enable()). and
>> unserialize events on the js end
>>
>>>
>>> Will do a pass on this CL now.
>>> Great; thanks!
>>
>>
>>
>>> https://codereview.chromium.org/246433012/
>>>
>>
>>
>

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

[aboxhall, 2014-04-28 19:50:38]

lgtm

Generally looking great, just a bunch of nits and things I don't understand.

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/automation_manager_views.cc (right):

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/automation_manager_views.cc:27: if
(current_tree_serializer_.get())
Why reset when Enable() is called, rather than Disable()?

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/automation_manager_views.cc:64:
views::AXAuraObjWrapper* obj =
Could we call this aura_obj or similar?

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/ax_root_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_root_obj_wrapper.cc:19:
std::vector<views::AXAuraObjWrapper*> out_children;
Why is this called out_children? It's never sent out of the scope of this
function. I realise it's passed to GetChildren below, but I think it would be
less confusing to call it simply 'children' (and as a bonus the return statement
would fit on one line that way).

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_root_obj_wrapper.cc:42:
out_node_data->state = 0;
Is |state| not guaranteed to be initialized to 0? Should that be the
responsibility of the caller or the struct constructor instead?

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/ax_tree_source_views.cc (right):

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:21:
views::AXAuraObjWrapper* AXTreeSourceViews::GetRoot() const {
We could use a using-directive for views::AXAuraObjWrapper (and
views::AXAuraObjectCache) in this file, couldn't we? This would make at least
one function signature fit on one line.

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:26: if (id ==
root_->GetID())
Why special-case this? Is it not in the cache?

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:43: if (!parent &&
root_->HasChild(node))
Why would this happen? Does the root not get added as a parent?

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:54: if (!node1 ||
!node2)
I assume we don't want to return true in the case where both nodes are null?

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:72:
root->Serialize(&out_data);
Similarly, why is this called out_data, when it never exists outside of this
scope?

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/ax_tree_source_views_unittest.cc
(right):

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views_unittest.cc:72:
TEST_F(AXTreeSourceViewsTest, SimpleAccessers) {
Very minor nit: Accessors (with an o)

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views_unittest.cc:117:
content_->RemoveAllChildViews(false);
What does false signify?

https://codereview.chromium.org/246433012/diff/310001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/246433012/diff/310001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation_custom_bindings.js:31: //
TODO(aboxhall, dtseng): Make this return the speced AutomationRootNode obj.
Nit: align comment with function call below (indent 2 spaces).

https://codereview.chromium.org/246433012/diff/310001/ui/views/accessibility/...
File ui/views/accessibility/ax_aura_obj_cache.cc (right):

https://codereview.chromium.org/246433012/diff/310001/ui/views/accessibility/...
ui/views/accessibility/ax_aura_obj_cache.cc:27: std::map<View*, int32>::iterator
it =
Nit: combine with below into one line.

https://codereview.chromium.org/246433012/diff/310001/ui/views/accessibility/...
ui/views/accessibility/ax_aura_obj_cache.cc:51: widget_to_id_map_[widget] =
current_id_;
Perhaps this pattern (AuraObjWrapper* wrapper = new AXFooObjWrapper(foo);
foo_to_id_map_[foo] = current_id; cache[current_id_] = wrapper; current_id++;),
or some part of it, should be a method, given it's used in a few places? Up to
you.

https://codereview.chromium.org/246433012/diff/310001/ui/views/accessibility/...
File ui/views/accessibility/ax_view_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/310001/ui/views/accessibility/...
ui/views/accessibility/ax_view_obj_wrapper.cc:44:
const_cast<View*>(view_)->GetAccessibleState(&view_data);
Why is this cast necessary?

https://codereview.chromium.org/246433012/diff/310001/ui/views/accessibility/...
File ui/views/accessibility/ax_widget_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/310001/ui/views/accessibility/...
ui/views/accessibility/ax_widget_obj_wrapper.cc:51: void
AXWidgetObjWrapper::OnWillRemoveView(Widget* widget, View* view) {
We don't need to do anything with widget here?

[David Tseng, 2014-04-29 01:35:03]

+ sky for OWNERS approval.

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/automation_manager_views.cc (right):

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/automation_manager_views.cc:27: if
(current_tree_serializer_.get())
On 2014/04/28 19:50:38, aboxhall wrote:
> Why reset when Enable() is called, rather than Disable()?

In case Enable() gets called by multiple clients. I can add a TODO here for that
particular case, but it is under specified in the API in general.

For example,
client1 calls getDesktop()
focus moves somewhere
serializer has marked the entire tree as serialized
client2 calls getDesktop()
This client should get the entire tree (but wouldn't if we didn't Reset).

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/automation_manager_views.cc:64:
views::AXAuraObjWrapper* obj =
On 2014/04/28 19:50:38, aboxhall wrote:
> Could we call this aura_obj or similar?

Done.

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/ax_root_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_root_obj_wrapper.cc:19:
std::vector<views::AXAuraObjWrapper*> out_children;
On 2014/04/28 19:50:38, aboxhall wrote:
> Why is this called out_children? It's never sent out of the scope of this
> function. I realise it's passed to GetChildren below, but I think it would be
> less confusing to call it simply 'children' (and as a bonus the return
statement
> would fit on one line that way).
> 

I don't have a strong opinion here, but as you noted, it is an out parameter to
GetChildren (and the convention is to prefix without_, at least for parameter
names).

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_root_obj_wrapper.cc:42:
out_node_data->state = 0;
On 2014/04/28 19:50:38, aboxhall wrote:
> Is |state| not guaranteed to be initialized to 0? Should that be the
> responsibility of the caller or the struct constructor instead?

AXNodeData sets state to -1.

However, the root should have some better notion of state here, so I've added a
TODO.

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/ax_tree_source_views.cc (right):

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:21:
views::AXAuraObjWrapper* AXTreeSourceViews::GetRoot() const {
On 2014/04/28 19:50:38, aboxhall wrote:
> We could use a using-directive for views::AXAuraObjWrapper (and
> views::AXAuraObjectCache) in this file, couldn't we? This would make at least
> one function signature fit on one line.

Done.

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:26: if (id ==
root_->GetID())
On 2014/04/28 19:50:38, aboxhall wrote:
> Why special-case this? Is it not in the cache?

The root itself isn't stored in the AXAuraObjCache. The root isn't backed by an
Aura view for one and it only exists in the chrome/ layer (due to its dependency
on ash/shell.h).

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:43: if (!parent &&
root_->HasChild(node))
On 2014/04/28 19:50:38, aboxhall wrote:
> Why would this happen? Does the root not get added as a parent?

There is no "root" hooking up all aura::Window objects. The root here only
exists for the purpose of rooting a tree containing Window's, View's, and
Widget's. If you call aura::Window::parent() on a root Window, it will return
NULL, so this covers that particular case.

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:54: if (!node1 ||
!node2)
On 2014/04/28 19:50:38, aboxhall wrote:
> I assume we don't want to return true in the case where both nodes are null?

Yes, that's correct.

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:72:
root->Serialize(&out_data);
On 2014/04/28 19:50:38, aboxhall wrote:
> Similarly, why is this called out_data, when it never exists outside of this
> scope?

Same as above.

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/ax_tree_source_views_unittest.cc
(right):

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views_unittest.cc:72:
TEST_F(AXTreeSourceViewsTest, SimpleAccessers) {
On 2014/04/28 19:50:38, aboxhall wrote:
> Very minor nit: Accessors (with an o)

Done.

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views_unittest.cc:117:
content_->RemoveAllChildViews(false);
On 2014/04/28 19:50:38, aboxhall wrote:
> What does false signify?

/* delete_children */ (added in-line.

https://codereview.chromium.org/246433012/diff/310001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/246433012/diff/310001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation_custom_bindings.js:31: //
TODO(aboxhall, dtseng): Make this return the speced AutomationRootNode obj.
On 2014/04/28 19:50:38, aboxhall wrote:
> Nit: align comment with function call below (indent 2 spaces).

Done.

https://codereview.chromium.org/246433012/diff/310001/ui/views/accessibility/...
File ui/views/accessibility/ax_aura_obj_cache.cc (right):

https://codereview.chromium.org/246433012/diff/310001/ui/views/accessibility/...
ui/views/accessibility/ax_aura_obj_cache.cc:27: std::map<View*, int32>::iterator
it =
On 2014/04/28 19:50:38, aboxhall wrote:
> Nit: combine with below into one line.

Done.

https://codereview.chromium.org/246433012/diff/310001/ui/views/accessibility/...
ui/views/accessibility/ax_aura_obj_cache.cc:51: widget_to_id_map_[widget] =
current_id_;
On 2014/04/28 19:50:38, aboxhall wrote:
> Perhaps this pattern (AuraObjWrapper* wrapper = new AXFooObjWrapper(foo);
> foo_to_id_map_[foo] = current_id; cache[current_id_] = wrapper;
current_id++;),
> or some part of it, should be a method, given it's used in a few places? Up to
> you.

Made these into templates.

https://codereview.chromium.org/246433012/diff/310001/ui/views/accessibility/...
File ui/views/accessibility/ax_view_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/310001/ui/views/accessibility/...
ui/views/accessibility/ax_view_obj_wrapper.cc:44:
const_cast<View*>(view_)->GetAccessibleState(&view_data);
On 2014/04/28 19:50:38, aboxhall wrote:
> Why is this cast necessary?

Removed; (not sure, at some point, it was needed).

https://codereview.chromium.org/246433012/diff/310001/ui/views/accessibility/...
File ui/views/accessibility/ax_widget_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/310001/ui/views/accessibility/...
ui/views/accessibility/ax_widget_obj_wrapper.cc:51: void
AXWidgetObjWrapper::OnWillRemoveView(Widget* widget, View* view) {
On 2014/04/28 19:50:38, aboxhall wrote:
> We don't need to do anything with widget here?

No. Widget destruction is observed via the other method here.

Moving forward, we might want to send notifications to the extension process,,
but that's not clear as of this cl. The view removed will be sent when the next
notification comes through.

[aboxhall, 2014-04-29 02:19:22]

Still lgtm.

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/ax_root_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_root_obj_wrapper.cc:19:
std::vector<views::AXAuraObjWrapper*> out_children;
On 2014/04/29 01:35:03, David Tseng wrote:
> On 2014/04/28 19:50:38, aboxhall wrote:
> > Why is this called out_children? It's never sent out of the scope of this
> > function. I realise it's passed to GetChildren below, but I think it would
be
> > less confusing to call it simply 'children' (and as a bonus the return
> statement
> > would fit on one line that way).
> > 
> 
> I don't have a strong opinion here, but as you noted, it is an out parameter
to
> GetChildren (and the convention is to prefix without_, at least for parameter
> names).

I find it confusing, as I would expect out_foo to be an argument to the current
function (an out parameter) or else a parameter to an IPC or similar (this seems
to be the vast majority of cases when I search the codebase for "\bout_\w+"). In
the context of the implementation of GetChildren I would expect the argument to
be called out_children, but in the context of the caller it doesn't make sense
to me to call it that.

[sky, 2014-04-29 04:20:37]

How about updating c/b/u/views/OWNERS with at least dmazzoni so that I don't
have to review this:)

[David Tseng, 2014-04-29 16:44:29]

PTAL @dmazzoni

On 2014/04/29 04:20:37, sky wrote:
> How about updating c/b/u/views/OWNERS with at least dmazzoni so that I don't
> have to review this:)

I added an OWNERS file for c/b/u/v/accessibility; I can add dmazzoni for the
../OWNERS file, but I'm unsure if he wants that :).

[dmazzoni, 2014-04-29 17:11:25]

Here's a separate change to add myself to the parent owners file with a
comment.

https://codereview.chromium.org/256303008/

On Tue, Apr 29, 2014 at 9:44 AM, <dtseng@chromium.org> wrote:

> PTAL @dmazzoni
>
>
> On 2014/04/29 04:20:37, sky wrote:
>
>> How about updating c/b/u/views/OWNERS with at least dmazzoni so that I
>> don't
>> have to review this:)
>>
>
> I added an OWNERS file for c/b/u/v/accessibility; I can add dmazzoni for
> the
> ../OWNERS file, but I'm unsure if he wants that :).
>
>
> https://codereview.chromium.org/246433012/
>

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

[dmazzoni, 2014-04-29 17:12:03]

I'm taking a final look now.

[dmazzoni, 2014-04-29 17:20:59]

lgtm

https://codereview.chromium.org/246433012/diff/350001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/automation_manager_views.cc (right):

https://codereview.chromium.org/246433012/diff/350001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/automation_manager_views.cc:32: enabled_ =
false;
Do you also want to delete the serializer here to free up memory?

https://codereview.chromium.org/246433012/diff/350001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/automation_manager_views.h (right):

https://codereview.chromium.org/246433012/diff/350001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/automation_manager_views.h:24: class
AutomationManagerViews {
Aside from this living in the "ui/views" directory, maybe we should rename this
AutomationManagerAura, since Aura is a better umbrella term for what this class
manages.

OK to do in a subsequent change.

https://codereview.chromium.org/246433012/diff/350001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/automation_manager_views.h:29: // Enable
views-based automation.
That sounds backwards if you don't already understand what this class is for.
I'd say: Enable automation support for Views.

https://codereview.chromium.org/246433012/diff/350001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/ax_root_obj_wrapper.cc (right):

https://codereview.chromium.org/246433012/diff/350001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_root_obj_wrapper.cc:41: //
TODO(dtseng): Apply a richer set of states.
The desktop probably doesn't need any state, right?

https://codereview.chromium.org/246433012/diff/350001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/ax_tree_source_views.cc (right):

https://codereview.chromium.org/246433012/diff/350001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:29: if (id ==
root_->GetID())
Why does the root have to be special-cased? Isn't it in the cache?

https://codereview.chromium.org/246433012/diff/350001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:45: if (!parent &&
root_->HasChild(node))
Why can't an AXWindowWrapper return the desktop as its parent?

[David Tseng, 2014-04-29 17:37:25]

On Tue, Apr 29, 2014 at 10:20 AM, <dmazzoni@chromium.org> wrote:

> lgtm
>
>
>
>
> https://codereview.chromium.org/246433012/diff/350001/
> chrome/browser/ui/views/accessibility/automation_manager_views.cc
> File chrome/browser/ui/views/accessibility/automation_manager_views.cc
> (right):
>
> https://codereview.chromium.org/246433012/diff/350001/
> chrome/browser/ui/views/accessibility/automation_
> manager_views.cc#newcode32
> chrome/browser/ui/views/accessibility/automation_manager_views.cc:32:
> enabled_ = false;
> Do you also want to delete the serializer here to free up memory?
>
> Done.

> https://codereview.chromium.org/246433012/diff/350001/
> chrome/browser/ui/views/accessibility/automation_manager_views.h
> File chrome/browser/ui/views/accessibility/automation_manager_views.h
> (right):
>
> https://codereview.chromium.org/246433012/diff/350001/
> chrome/browser/ui/views/accessibility/automation_manager_views.h#newcode24
> chrome/browser/ui/views/accessibility/automation_manager_views.h:24:
> class AutomationManagerViews {
> Aside from this living in the "ui/views" directory, maybe we should
> rename this AutomationManagerAura, since Aura is a better umbrella term
> for what this class manages.
>
> OK to do in a subsequent change.
>
> Ok; I'll do it in a subsequent change.

> https://codereview.chromium.org/246433012/diff/350001/
> chrome/browser/ui/views/accessibility/automation_manager_views.h#newcode29
> chrome/browser/ui/views/accessibility/automation_manager_views.h:29: //
> Enable views-based automation.
> That sounds backwards if you don't already understand what this class is
> for. I'd say: Enable automation support for Views.
>
> Done.

> https://codereview.chromium.org/246433012/diff/350001/
> chrome/browser/ui/views/accessibility/ax_root_obj_wrapper.cc
> File chrome/browser/ui/views/accessibility/ax_root_obj_wrapper.cc
> (right):
>
> https://codereview.chromium.org/246433012/diff/350001/
> chrome/browser/ui/views/accessibility/ax_root_obj_wrapper.cc#newcode41
> chrome/browser/ui/views/accessibility/ax_root_obj_wrapper.cc:41: //
> TODO(dtseng): Apply a richer set of states.
> The desktop probably doesn't need any state, right?
>
> Busy? I think it's worth looking into.

> https://codereview.chromium.org/246433012/diff/350001/
> chrome/browser/ui/views/accessibility/ax_tree_source_views.cc
> File chrome/browser/ui/views/accessibility/ax_tree_source_views.cc
> (right):
>
> https://codereview.chromium.org/246433012/diff/350001/
> chrome/browser/ui/views/accessibility/ax_tree_source_views.cc#newcode29
> chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:29: if (id
> == root_->GetID())
> Why does the root have to be special-cased? Isn't it in the cache?
>
> No; it's not in the cache because it's owned by the AXTreeSourceViews
object. See below.

> https://codereview.chromium.org/246433012/diff/350001/
> chrome/browser/ui/views/accessibility/ax_tree_source_views.cc#newcode45
> chrome/browser/ui/views/accessibility/ax_tree_source_views.cc:45: if
> (!parent && root_->HasChild(node))
> Why can't an AXWindowWrapper return the desktop as its parent?
>
>
The root wrapper needs to depend on ash/shell.h.

>
> https://codereview.chromium.org/246433012/
>
> 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.

[chromium-reviews, 2014-04-29 17:41:27]

On Tue, Apr 29, 2014 at 10:37 AM, David Tseng <dtseng@chromium.org> wrote:

> Why does the root have to be special-cased? Isn't it in the cache?
>>
>> No; it's not in the cache because it's owned by the AXTreeSourceViews
> object. See below.
>

Got it, thanks.

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

[sky, 2014-04-29 19:11:41]

LGTM

[David Tseng, 2014-04-29 21:38:23]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-04-29 21:41:11]

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

[commit-bot: I haz the power, 2014-04-30 00:03:51]

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

[commit-bot: I haz the power, 2014-04-30 00:03:52]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_clang_dbg on tryserver.chromium

[David Tseng, 2014-04-30 14:48:53]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-04-30 14:50:05]

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

[commit-bot: I haz the power, 2014-04-30 14:54:48]

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

[commit-bot: I haz the power, 2014-04-30 14:54:49]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[David Tseng, 2014-04-30 16:47:40]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-04-30 16:49:42]

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

[commit-bot: I haz the power, 2014-04-30 16:53:34]

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

[commit-bot: I haz the power, 2014-04-30 16:53:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[David Tseng, 2014-04-30 17:40:55]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-04-30 17:41:33]

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

[commit-bot: I haz the power, 2014-04-30 17:45:55]

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

[commit-bot: I haz the power, 2014-04-30 17:45:56]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[David Tseng, 2014-04-30 19:11:11]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-04-30 19:12:31]

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

[commit-bot: I haz the power, 2014-04-30 19:26:37]

Change committed as 267297