Make AccessibilityNodeData more compact.

Make AccessibilityNodeData more compact.

The only goal of this tedious changelist is
less memory usage when storing the accessible tree.

The major changes are:
* Make almost everything a "sparse" attribute.
  For example, not every element has a name or
  value, so make them optional attributes rather
  than string fields that always take up memory.
* Use UTF-8 std::strings instead of string16s.
* Use vectors of pairs rather than maps.

If there is any slowdown after this change, we
can speed up lookup in vectors by sorting the
attributes and doing a binary search. There are
also opportunities to do more referencing and
less string copying.

When reviewing, look at the changes to the header
files first and make sure you understand them.
Everything else is just a consequence of those
changes.

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

[aboxhall, 2013-07-31 18:34:47]

First batch of comments.

https://chromiumcodereview.appspot.com/21269002/diff/6001/content/browser/acc...
File content/browser/accessibility/browser_accessibility.cc (right):

https://chromiumcodereview.appspot.com/21269002/diff/6001/content/browser/acc...
content/browser/accessibility/browser_accessibility.cc:263: std::string* value)
const {
Nit: this will fit on the previous line.

https://chromiumcodereview.appspot.com/21269002/diff/6001/content/browser/acc...
File content/browser/accessibility/browser_accessibility.h (right):

https://chromiumcodereview.appspot.com/21269002/diff/6001/content/browser/acc...
content/browser/accessibility/browser_accessibility.h:188: const
std::vector<int32>& GetIntListAttribute(
Why does this accessor return the value when others assign it to an out
parameter?

https://chromiumcodereview.appspot.com/21269002/diff/6001/content/browser/acc...
content/browser/accessibility/browser_accessibility.h:249:
std::vector<std::pair<
Could we add using statements for each attribute type to make this a bit more
readable?

https://chromiumcodereview.appspot.com/21269002/diff/6001/content/browser/acc...
content/browser/accessibility/browser_accessibility.h:250:
AccessibilityNodeData::BoolAttribute, bool> > bool_attributes_;
Could we add using statements for AccessibilityNodeData::FooAttribute to make
these a little more readable?

https://chromiumcodereview.appspot.com/21269002/diff/6001/content/browser/acc...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

https://chromiumcodereview.appspot.com/21269002/diff/6001/content/browser/acc...
content/browser/accessibility/browser_accessibility_cocoa.mm:472: std::string
value;
I'm finding the name of this variable a little confusing with the other senses
of 'value', and the various roles it plays throughout this method. How much of a
problem would it be to have a new string for each call to GetStringAttribute,
with a descriptive name for the attribute that's being retrieved?

https://chromiumcodereview.appspot.com/21269002/diff/6001/content/browser/acc...
content/browser/accessibility/browser_accessibility_cocoa.mm:482: if
(browserAccessibility_->GetStringAttribute(
Should this be negated, or the value of value (argh) checked inside the
if-block?

https://chromiumcodereview.appspot.com/21269002/diff/6001/content/browser/acc...
content/browser/accessibility/browser_accessibility_cocoa.mm:495: size_t
leftIndex = value.size();
I realise this is effectively unchanged in this change, but could this use
rfind() instead?

https://chromiumcodereview.appspot.com/21269002/diff/6001/content/browser/acc...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://chromiumcodereview.appspot.com/21269002/diff/6001/content/browser/acc...
content/browser/accessibility/browser_accessibility_win.cc:3114: string16
BrowserAccessibilityWin::GetValueText() {
Where is this method used?

[dmazzoni, 2013-08-06 17:36:34]

Ready for another look.

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility.cc:263: std::string* value)
const {
On 2013/07/31 18:34:47, aboxhall wrote:
> Nit: this will fit on the previous line.

Done.

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility.h (right):

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility.h:188: const
std::vector<int32>& GetIntListAttribute(
On 2013/07/31 18:34:47, aboxhall wrote:
> Why does this accessor return the value when others assign it to an out
> parameter?

The distinction is whether you need to know whether the attribute
is present or not. As an example, there's a difference between
VALUE_FOR_RANGE being present and it being zero.

It's worth improving this too while I'm at it. I made all of the
attribute accessors have two variants - one that returns the
value or a default value if it's missing, and another that takes
a pointer and returns a bool indicating if the attribute was
present.

That made it possible to save a few lines of code in some places
where we don't care whether the attribute is present or not.

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility.h:249:
std::vector<std::pair<
On 2013/07/31 18:34:47, aboxhall wrote:
> Could we add using statements for each attribute type to make this a bit more
> readable?

That's generally frowned-upon in header files.

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility.h:250:
AccessibilityNodeData::BoolAttribute, bool> > bool_attributes_;
On 2013/07/31 18:34:47, aboxhall wrote:
> Could we add using statements for AccessibilityNodeData::FooAttribute to make
> these a little more readable?

Same.

However, a longer-term plan is to move these enums to
Blink, and we can come up with more compact names at
that time.

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_cocoa.mm:482: if
(browserAccessibility_->GetStringAttribute(
On 2013/07/31 18:34:47, aboxhall wrote:
> Should this be negated, or the value of value (argh) checked inside the
> if-block?

I replaced this with HasStringAttribute to be more clear.

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_cocoa.mm:495: size_t
leftIndex = value.size();
On 2013/07/31 18:34:47, aboxhall wrote:
> I realise this is effectively unchanged in this change, but could this use
> rfind() instead?

Done.

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_win.cc:3114: string16
BrowserAccessibilityWin::GetValueText() {
On 2013/07/31 18:34:47, aboxhall wrote:
> Where is this method used?

It's private, it's called by both get_accValue, and in PreInitialize to
initialize the attributes returned by get_attributes.

[aboxhall, 2013-08-07 17:16:08]

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_win.cc (left):

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_win.cc:2786: if (ia2_role_
== IA2_ROLE_COLOR_CHOOSER) {
I guess this was never doing anything, but should it have been fixed rather than
being removed?

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_win.cc:571: if (ia2_role_ ==
IA2_ROLE_COLOR_CHOOSER) {
Does this block belong in GetValueText() as well?

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
File content/browser/accessibility/browser_accessibility_win.cc (left):

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_win.cc:2786: if (ia2_role_
== IA2_ROLE_COLOR_CHOOSER) {
Why does this no longer expose the color well value?

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_win.cc:3244: string16
html_tag = GetString16Attribute(
Nit: fits in 80 chars

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_win.cc:3357: string16
aria_role = GetString16Attribute(
Does this work? You're passing in &aria_role at the same time as you're defining
it. Presumably this should be
GetString16Attribute(AccessibilityNodeData::ATTR_ROLE)?

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_win.cc:3557: string16
aria_role = GetString16Attribute(
Nit: fits in 80 chars.

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
File content/browser/accessibility/browser_accessibility_win_unittest.cc
(right):

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_win_unittest.cc:482: const
std::string button1_text_name = "red";
Should this and the below be used on lines 564 and 573?

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
File content/browser/accessibility/cross_platform_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
content/browser/accessibility/cross_platform_accessibility_browsertest.cc:432:
EXPECT_EQ(AccessibilityNodeData::ATTR_CELL_IDS,
I think this should be an assert.

https://codereview.chromium.org/21269002/diff/20001/content/common/accessibil...
File content/common/accessibility_node_data.cc (left):

https://codereview.chromium.org/21269002/diff/20001/content/common/accessibil...
content/common/accessibility_node_data.cc:273: std::string tmp =
UTF16ToUTF8(name);
Why is this no longer necessary? (Why was it ever necessary?)

[aboxhall, 2013-08-07 17:31:40]

https://codereview.chromium.org/21269002/diff/20001/content/common/accessibil...
File content/common/accessibility_node_data.h (right):

https://codereview.chromium.org/21269002/diff/20001/content/common/accessibil...
content/common/accessibility_node_data.h:324:
std::vector<std::pair<StringAttribute, std::string> > string_attributes;
I forgot to ask: how do we ensure we don't end up with duplicate attributes in
here?

[dmazzoni, 2013-08-07 17:48:18]

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_win.cc (left):

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_win.cc:2786: if (ia2_role_
== IA2_ROLE_COLOR_CHOOSER) {
On 2013/08/07 17:16:09, aboxhall wrote:
> I guess this was never doing anything, but should it have been fixed rather
than
> being removed?

It's just being moved.

The main reason I moved it was to avoid changing value_ - I'd like to eventually
get rid of code that modifies attributes on initialization and instead just
compute them all dynamically. Or do something else smart like cache the derived
attributes the first time they're accessed or something.

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/21269002/diff/6001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility_win.cc:571: if (ia2_role_ ==
IA2_ROLE_COLOR_CHOOSER) {
On 2013/08/07 17:16:09, aboxhall wrote:
> Does this block belong in GetValueText() as well?

Possibly, but that would be a change in functionality -
I really want to make this a purely refactoring change.

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
File content/browser/accessibility/browser_accessibility_win.cc (left):

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_win.cc:2786: if (ia2_role_
== IA2_ROLE_COLOR_CHOOSER) {
On 2013/08/07 17:16:09, aboxhall wrote:
> Why does this no longer expose the color well value?

Just moved this code, didn't delete it. See other comment.

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_win.cc:3244: string16
html_tag = GetString16Attribute(
On 2013/08/07 17:16:09, aboxhall wrote:
> Nit: fits in 80 chars

81 according to my editor

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_win.cc:3357: string16
aria_role = GetString16Attribute(
On 2013/08/07 17:16:09, aboxhall wrote:
> Does this work? You're passing in &aria_role at the same time as you're
defining
> it. Presumably this should be
> GetString16Attribute(AccessibilityNodeData::ATTR_ROLE)?

You're right, this was totally wrong.

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
File content/browser/accessibility/browser_accessibility_win_unittest.cc
(right):

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_win_unittest.cc:482: const
std::string button1_text_name = "red";
On 2013/08/07 17:16:09, aboxhall wrote:
> Should this and the below be used on lines 564 and 573?

Done.

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
File content/browser/accessibility/cross_platform_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
content/browser/accessibility/cross_platform_accessibility_browsertest.cc:432:
EXPECT_EQ(AccessibilityNodeData::ATTR_CELL_IDS,
On 2013/08/07 17:16:09, aboxhall wrote:
> I think this should be an assert.

Done.

https://codereview.chromium.org/21269002/diff/20001/content/common/accessibil...
File content/common/accessibility_node_data.cc (left):

https://codereview.chromium.org/21269002/diff/20001/content/common/accessibil...
content/common/accessibility_node_data.cc:273: std::string tmp =
UTF16ToUTF8(name);
On 2013/08/07 17:16:09, aboxhall wrote:
> Why is this no longer necessary? (Why was it ever necessary?)

"name" and "value" used to be explicit fields in
AccessibilityNodeData. Now they're string attributes,
which saves memory for all of the nodes that don't have
a name or value. It also simplifies this logic because
we don't have to special-case printing them.

https://codereview.chromium.org/21269002/diff/20001/content/common/accessibil...
File content/common/accessibility_node_data.h (right):

https://codereview.chromium.org/21269002/diff/20001/content/common/accessibil...
content/common/accessibility_node_data.h:324:
std::vector<std::pair<StringAttribute, std::string> > string_attributes;
On 2013/08/07 17:31:40, aboxhall wrote:
> I forgot to ask: how do we ensure we don't end up with duplicate attributes in
> here?

We don't really, but they're only written in one place, in
AccessibilityNodeSerializer.

They are currently also written in browser_accessibility.cc (SetStringAttribute)
but I'm hoping to get rid of that to keep the logic really simple.

[aboxhall, 2013-08-07 20:22:05]

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/21269002/diff/20001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_win.cc:3244: string16
html_tag = GetString16Attribute(
On 2013/08/07 17:48:18, Dominic Mazzoni wrote:
> On 2013/08/07 17:16:09, aboxhall wrote:
> > Nit: fits in 80 chars
> 
> 81 according to my editor

Oops, left out the indent :) (And below.)

https://codereview.chromium.org/21269002/diff/40001/content/common/accessibil...
File content/common/accessibility_node_data.h (right):

https://codereview.chromium.org/21269002/diff/40001/content/common/accessibil...
content/common/accessibility_node_data.h:312: void SetName(std::string name);
This is only used in testing, right?

[dmazzoni, 2013-08-07 20:23:39]

https://codereview.chromium.org/21269002/diff/40001/content/common/accessibil...
File content/common/accessibility_node_data.h (right):

https://codereview.chromium.org/21269002/diff/40001/content/common/accessibil...
content/common/accessibility_node_data.h:312: void SetName(std::string name);
On 2013/08/07 20:22:06, aboxhall wrote:
> This is only used in testing, right?

Yes.

[aboxhall, 2013-08-07 20:25:17]

LGTM from me.

https://codereview.chromium.org/21269002/diff/40001/content/common/accessibil...
File content/common/accessibility_node_data.h (right):

https://codereview.chromium.org/21269002/diff/40001/content/common/accessibil...
content/common/accessibility_node_data.h:312: void SetName(std::string name);
On 2013/08/07 20:23:40, Dominic Mazzoni wrote:
> On 2013/08/07 20:22:06, aboxhall wrote:
> > This is only used in testing, right?
> 
> Yes.

Is that worth a comment?

[dmazzoni, 2013-08-18 22:06:55]

+jschuh for accessibility_messages
+phajdan.jr for content/test

[Paweł Hajdan Jr., 2013-08-19 19:06:55]

content/test LGTM

[dmazzoni, 2013-08-20 15:30:17]

+tsepez for content/common/accessibility_messages.h

[Tom Sepez, 2013-08-20 16:29:21]

On 2013/08/20 15:30:17, Dominic Mazzoni wrote:
> +tsepez for content/common/accessibility_messages.h

Messages LGTM.

[commit-bot: I haz the power, 2013-08-20 16:34:33]

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

[commit-bot: I haz the power, 2013-08-20 23:41:54]

Change committed as 218572