Refactor content/renderer/accessibility to use AXTreeSerializer (re-land).

Refactor content/renderer/accessibility to use AXTreeSerializer (re-land).

This removes the confusing code in content/renderer/accessibility that
serializes updates from Blink's accessibility tree, and replaces it with
calls to the new cleaner and fully-unit-tested AXTreeSerializer class.

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

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

[dmazzoni, 2014-01-07 16:29:36]

Depends on the previous change, https://codereview.chromium.org/125783002/

[aboxhall, 2014-02-25 17:45:35]

https://codereview.chromium.org/125763003/diff/190001/content/common/accessib...
File content/common/accessibility_messages.h (right):

https://codereview.chromium.org/125763003/diff/190001/content/common/accessib...
content/common/accessibility_messages.h:49:
IPC_STRUCT_TRAITS_MEMBER(node_id_to_clear)
Can we have comments for these too?

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:48: bool
IsParentUnignoredOf(WebAXObject ancestor,
Why are these pass by value now? I guess it doesn't matter too much since it
essentially just wraps a pointer...

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:121: // TODO(ctguil): We
may want to remove this check as webkit stabilizes.
Might be time to remove this TODO?

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
File content/renderer/accessibility/blink_ax_tree_source.h (right):

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.h:22: // AXTreeSource
implementation..
Stray . ?

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
File content/renderer/accessibility/renderer_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
content/renderer/accessibility/renderer_accessibility_browsertest.cc:223: // The
job of TestRendererAccessibilityComplete is to serialize the
What happened here?

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
File content/renderer/accessibility/renderer_accessibility_complete.cc (right):

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
content/renderer/accessibility/renderer_accessibility_complete.cc:250:
new_locations[id] = new_location;
This map never seems to be read from?

[dmazzoni, 2014-02-27 20:51:43]

https://codereview.chromium.org/125763003/diff/190001/content/common/accessib...
File content/common/accessibility_messages.h (right):

https://codereview.chromium.org/125763003/diff/190001/content/common/accessib...
content/common/accessibility_messages.h:49:
IPC_STRUCT_TRAITS_MEMBER(node_id_to_clear)
On 2014/02/25 17:45:35, aboxhall wrote:
> Can we have comments for these too?

This isn't the struct, this is just the macro that allows the struct to be
passed via IPC

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:48: bool
IsParentUnignoredOf(WebAXObject ancestor,
On 2014/02/25 17:45:35, aboxhall wrote:
> Why are these pass by value now? I guess it doesn't matter too much since it
> essentially just wraps a pointer...

I had to switch to just use WebAXObject consistently everywhere in order to make
this work with AXTreeSource. Yeah, this should be extremely lightweight.

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:121: // TODO(ctguil): We
may want to remove this check as webkit stabilizes.
On 2014/02/25 17:45:35, aboxhall wrote:
> Might be time to remove this TODO?

Agreed!  :)

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
File content/renderer/accessibility/blink_ax_tree_source.h (right):

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.h:22: // AXTreeSource
implementation..
On 2014/02/25 17:45:35, aboxhall wrote:
> Stray . ?

Done.

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
File content/renderer/accessibility/renderer_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
content/renderer/accessibility/renderer_accessibility_browsertest.cc:223: // The
job of TestRendererAccessibilityComplete is to serialize the
On 2014/02/25 17:45:35, aboxhall wrote:
> What happened here?

Oops, I got too aggressive with search-and-replace.

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
File content/renderer/accessibility/renderer_accessibility_complete.cc (right):

https://codereview.chromium.org/125763003/diff/190001/content/renderer/access...
content/renderer/accessibility/renderer_accessibility_complete.cc:250:
new_locations[id] = new_location;
On 2014/02/25 17:45:35, aboxhall wrote:
> This map never seems to be read from?

Good catch! I was missing a "swap" at the end.'

[aboxhall, 2014-02-27 20:57:39]

lgtm

https://codereview.chromium.org/125763003/diff/190001/content/common/accessib...
File content/common/accessibility_messages.h (right):

https://codereview.chromium.org/125763003/diff/190001/content/common/accessib...
content/common/accessibility_messages.h:49:
IPC_STRUCT_TRAITS_MEMBER(node_id_to_clear)
On 2014/02/27 20:51:44, dmazzoni wrote:
> On 2014/02/25 17:45:35, aboxhall wrote:
> > Can we have comments for these too?
> 
> This isn't the struct, this is just the macro that allows the struct to be
> passed via IPC

Ah, I see.

[dmazzoni, 2014-02-28 17:51:55]

+jam for content
+inferno for *_messages

[jam, 2014-02-28 18:08:34]

lgtm

[Tom Sepez, 2014-03-03 21:18:39]

LGTM

[dmazzoni, 2014-04-02 06:06:36]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-04-02 06:06:40]

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

[commit-bot: I haz the power, 2014-04-02 08:38:46]

Change committed as 261099

[pfeldman, 2014-04-02 11:29:44]

A revert of this CL has been created in
https://codereview.chromium.org/222073002/ by pfeldman@chromium.org.

The reason for reverting is: Breaks asan builders:
http://build.chromium.org/p/chromium.memory/buildstatus?builder=Linux%20ASan%....

[dmazzoni, 2014-04-03 05:26:41]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-04-03 05:28:20]

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

[commit-bot: I haz the power, 2014-04-03 08:32:33]

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

[commit-bot: I haz the power, 2014-04-03 08:32:34]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[dmazzoni, 2014-04-03 15:15:37]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-04-03 15:15:53]

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

[commit-bot: I haz the power, 2014-04-03 15:25:32]

Change committed as 261408