A11y: Add/refactor methods for manipulating bitfields on AXNodeData.

A11y: Add/refactor methods for manipulating bitfields on AXNodeData.

In preparation for allowing a11y elements to indicate they support a
user-executable action (e.g. checking a checkbox), this change does two things:
 - Rename AXNodeData::AddStateFlag() / AXNodeData::HasStateFlag() to
   AXNodeData::AddState() for a consistent naming scheme.
 - Add a new method AXNodeData::ClearBitfields(), which will clear the bits set
   on AXNodeData::state. New bitfields added onto AXNodeData can also be cleared
   here.
 - Deletes AXNodeData::IsFlagSet() and replaces this with HasState() above.

BUG=717846
Review-Url: https://codereview.chromium.org/2860883003
Cr-Commit-Position: refs/heads/master@{#470245}
Committed: https://chromium.googlesource.com/chromium/src/+/34b559296ecf1005c41ea8278327ef5880553de0

[Patti Lor, 2017-05-04 07:08:19]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 07:08:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 07:48:50]

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

[commit-bot: I haz the power, 2017-05-04 07:48:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patti Lor, 2017-05-05 00:18:55]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 00:19:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patti Lor, 2017-05-05 00:54:41]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 00:55:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 02:12:10]

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

[commit-bot: I haz the power, 2017-05-05 02:12:10]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-05-05 05:00:42]

patricialor@chromium.org changed reviewers:
+ tapted@chromium.org

[Patti Lor, 2017-05-05 05:00:43]

Hey Trent, PTAL? The biggest change is probably ax_node_data.cc. If you want
more context in addition to the bug, see this CL
(https://codereview.chromium.org/2799413002/) which I am splitting up because of
the discussion I had with dmazzoni in it.

[tapted, 2017-05-05 05:53:37]

lgtm

some suggestions, but you'll need an a11y owner

https://codereview.chromium.org/2860883003/diff/40001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_manager_mac.mm (right):

https://codereview.chromium.org/2860883003/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_manager_mac.mm:139:
empty_document.state =
AddState()?

https://codereview.chromium.org/2860883003/diff/40001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2860883003/diff/40001/content/public/test/bro...
content/public/test/browser_test_utils.cc:1123: ui::AXNodeData data =
ui::AXNodeData();
nit: = `ui::AXNodeData();` not required

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

https://codereview.chromium.org/2860883003/diff/40001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:385:
dst->ClearBitfields();
this one isn't needed I guess, because of the line below?

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/ax_nod...
File ui/accessibility/ax_node_data.cc (right):

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/ax_nod...
ui/accessibility/ax_node_data.cc:31: return set ? (bitfield |= (1 << flag)) :
(bitfield &= ~(1 << flag));
The '=' on this line are not needed. i.e. '|=' -> '|' and '&=' -> '&'

Also I think there's only one caller? (and it always passes true) - perhaps just
put the line in AddState to set the state directly

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/ax_nod...
ui/accessibility/ax_node_data.cc:438: DCHECK(state == 0xFFFFFFFF);
DCHECK_EQ?

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/ax_nod...
ui/accessibility/ax_node_data.cc:447: DCHECK(state_flag != AX_STATE_NONE);
DCHECK_NE?

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/ax_nod...
File ui/accessibility/ax_node_data.h (right):

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/ax_nod...
ui/accessibility/ax_node_data.h:110: void ClearBitfields();
I feel like this should be called InitState() or PrepareForState() or something
like that. It looks too strange for consumers to set some fields, then call
`ClearBitfields` before setting state-specific stuff -- it's too vague.

Perhaps the constructor should even take an `initial_state` argument (or it
should just set state to 0 and we lose some safety..). That's a decision for the
a11y owners though.

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_auralinux.cc (right):

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_auralinux.cc:464: if
(GetData().HasState(ui::AX_STATE_DEFAULT))
Cache GetData() to a local var?

[Patti Lor, 2017-05-05 06:44:25]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 06:44:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patti Lor, 2017-05-05 08:43:43]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 08:43:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patti Lor, 2017-05-05 08:48:24]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-05-05 08:54:09]

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

[commit-bot: I haz the power, 2017-05-05 08:54:10]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Patti Lor, 2017-05-05 09:03:24]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 09:03:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patti Lor, 2017-05-05 09:13:58]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-05-05 10:04:28]

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

[commit-bot: I haz the power, 2017-05-05 10:04:29]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-05-08 00:28:06]

patricialor@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[Patti Lor, 2017-05-08 00:28:08]

Thanks Trent! Dominic could you take a look? I decided to split up the changes
in crrev.com/2799413002 since they seem a bit more isolated now. Please see
https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/ax_nod...
as well for Trent's suggestion about clearing bitfields.

https://codereview.chromium.org/2860883003/diff/40001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_manager_mac.mm (right):

https://codereview.chromium.org/2860883003/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_manager_mac.mm:139:
empty_document.state =
On 2017/05/05 05:53:36, tapted wrote:
> AddState()?

Done.

https://codereview.chromium.org/2860883003/diff/40001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2860883003/diff/40001/content/public/test/bro...
content/public/test/browser_test_utils.cc:1123: ui::AXNodeData data =
ui::AXNodeData();
On 2017/05/05 05:53:36, tapted wrote:
> nit: = `ui::AXNodeData();` not required

Oops, thanks.

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

https://codereview.chromium.org/2860883003/diff/40001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:385:
dst->ClearBitfields();
On 2017/05/05 05:53:36, tapted wrote:
> this one isn't needed I guess, because of the line below?

Reverted! (This will come back in a later patch though, since I'm planning to
add another bitfield.)

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/ax_nod...
File ui/accessibility/ax_node_data.cc (right):

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/ax_nod...
ui/accessibility/ax_node_data.cc:31: return set ? (bitfield |= (1 << flag)) :
(bitfield &= ~(1 << flag));
On 2017/05/05 05:53:36, tapted wrote:
> The '=' on this line are not needed. i.e. '|=' -> '|' and '&=' -> '&'
> 
> Also I think there's only one caller? (and it always passes true) - perhaps
just
> put the line in AddState to set the state directly

Deleted :)

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/ax_nod...
ui/accessibility/ax_node_data.cc:438: DCHECK(state == 0xFFFFFFFF);
On 2017/05/05 05:53:36, tapted wrote:
> DCHECK_EQ?

Done.

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/ax_nod...
ui/accessibility/ax_node_data.cc:447: DCHECK(state_flag != AX_STATE_NONE);
On 2017/05/05 05:53:36, tapted wrote:
> DCHECK_NE?

Done.

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/ax_nod...
File ui/accessibility/ax_node_data.h (right):

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/ax_nod...
ui/accessibility/ax_node_data.h:110: void ClearBitfields();
On 2017/05/05 05:53:36, tapted wrote:
> I feel like this should be called InitState() or PrepareForState() or
something
> like that. It looks too strange for consumers to set some fields, then call
> `ClearBitfields` before setting state-specific stuff -- it's too vague.
> 
> Perhaps the constructor should even take an `initial_state` argument (or it
> should just set state to 0 and we lose some safety..). That's a decision for
the
> a11y owners though.

Renamed to Init() for now, will link owners this comment.

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_auralinux.cc (right):

https://codereview.chromium.org/2860883003/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_auralinux.cc:464: if
(GetData().HasState(ui::AX_STATE_DEFAULT))
On 2017/05/05 05:53:36, tapted wrote:
> Cache GetData() to a local var?

Done.

[dmazzoni, 2017-05-08 04:28:27]

lgtm

Thanks for doing this!

https://codereview.chromium.org/2860883003/diff/100001/ui/accessibility/ax_no...
File ui/accessibility/ax_node_data.h (right):

https://codereview.chromium.org/2860883003/diff/100001/ui/accessibility/ax_no...
ui/accessibility/ax_node_data.h:110: void Init();
Perhaps we should just put this in the constructor?

A long time ago, I left state_ initialized to a bogus value purely
as a means to make sure that it was always being set deliberately
and not forgotten.

I think it may no longer be a good idea. All of these calls to Init()
are just a distraction, it would be much cleaner to just allow creating
an AXNodeData and having |state| set to a reasonable default.

Totally fine to just do that in this patch, or in a separate patch.

[Patti Lor, 2017-05-08 07:05:51]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 07:06:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patti Lor, 2017-05-08 07:44:46]

patricialor@chromium.org changed reviewers:
+ sky@chromium.org

[Patti Lor, 2017-05-08 07:44:47]

Thanks Dominic - I made the change to clear the |state| bitfield in AXNodeData's
constructor in this patchset.

+sky, PTAL? Thanks!

https://codereview.chromium.org/2860883003/diff/100001/ui/accessibility/ax_no...
File ui/accessibility/ax_node_data.h (right):

https://codereview.chromium.org/2860883003/diff/100001/ui/accessibility/ax_no...
ui/accessibility/ax_node_data.h:110: void Init();
On 2017/05/08 04:28:26, dmazzoni wrote:
> Perhaps we should just put this in the constructor?
> 
> A long time ago, I left state_ initialized to a bogus value purely
> as a means to make sure that it was always being set deliberately
> and not forgotten.
> 
> I think it may no longer be a good idea. All of these calls to Init()
> are just a distraction, it would be much cleaner to just allow creating
> an AXNodeData and having |state| set to a reasonable default.
> 
> Totally fine to just do that in this patch, or in a separate patch.

I've deleted this Init() method and just set |state| to 0 in the constructor.

[commit-bot: I haz the power, 2017-05-08 08:55:10]

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

[commit-bot: I haz the power, 2017-05-08 08:55:11]

Dry run: This issue passed the CQ dry run.

[sky, 2017-05-08 15:56:27]

LGTM - assuming state is already cleared.

https://codereview.chromium.org/2860883003/diff/120001/ui/views/accessibility...
File ui/views/accessibility/ax_view_obj_wrapper.cc (left):

https://codereview.chromium.org/2860883003/diff/120001/ui/views/accessibility...
ui/views/accessibility/ax_view_obj_wrapper.cc:49: out_node_data->state = 0;
How come the new code doesn't need a ClearState() or something? Is it assumed
that Serialize() is called with the state set to 0 already?

[Patti Lor, 2017-05-09 06:03:40]

Thanks all!

https://codereview.chromium.org/2860883003/diff/120001/ui/views/accessibility...
File ui/views/accessibility/ax_view_obj_wrapper.cc (left):

https://codereview.chromium.org/2860883003/diff/120001/ui/views/accessibility...
ui/views/accessibility/ax_view_obj_wrapper.cc:49: out_node_data->state = 0;
On 2017/05/08 15:56:27, sky wrote:
> How come the new code doesn't need a ClearState() or something? Is it assumed
> that Serialize() is called with the state set to 0 already?

The current AXNodeData class initialises its |state| member to be 0xffffff,
which was an invalid state (see
https://codereview.chromium.org/2860883003/#msg28 - Dominic's comment - for the
reason if you're interested). This patch is changing it so that it is inited to
0x0 instead, so this is fine :)

[Patti Lor, 2017-05-09 06:03:47]

The CQ bit was checked by patricialor@chromium.org

[Patti Lor, 2017-05-09 06:03:47]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2860883003/#ps120001
(title: "Delete AXNodeData::Init() and clear bitfields in AXNodeData()
instead.")

[commit-bot: I haz the power, 2017-05-09 06:04:13]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 06:09:42]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494309827334810,
"parent_rev": "93a9fd92093885c40743e2f6ea044d6565337909", "commit_rev":
"34b559296ecf1005c41ea8278327ef5880553de0"}

[commit-bot: I haz the power, 2017-05-09 06:09:52]

Description was changed from

==========
A11y: Add/refactor methods for manipulating bitfields on AXNodeData.

In preparation for allowing a11y elements to indicate they support a
user-executable action (e.g. checking a checkbox), this change does two things:
 - Rename AXNodeData::AddStateFlag() / AXNodeData::HasStateFlag() to
   AXNodeData::AddState() for a consistent naming scheme.
 - Add a new method AXNodeData::ClearBitfields(), which will clear the bits set
   on AXNodeData::state. New bitfields added onto AXNodeData can also be cleared
   here.
 - Deletes AXNodeData::IsFlagSet() and replaces this with HasState() above.

BUG=717846
==========

to

==========
A11y: Add/refactor methods for manipulating bitfields on AXNodeData.

In preparation for allowing a11y elements to indicate they support a
user-executable action (e.g. checking a checkbox), this change does two things:
 - Rename AXNodeData::AddStateFlag() / AXNodeData::HasStateFlag() to
   AXNodeData::AddState() for a consistent naming scheme.
 - Add a new method AXNodeData::ClearBitfields(), which will clear the bits set
   on AXNodeData::state. New bitfields added onto AXNodeData can also be cleared
   here.
 - Deletes AXNodeData::IsFlagSet() and replaces this with HasState() above.

BUG=717846

Review-Url: https://codereview.chromium.org/2860883003
Cr-Commit-Position: refs/heads/master@{#470245}
Committed:
https://chromium.googlesource.com/chromium/src/+/34b559296ecf1005c41ea8278327...
==========

[commit-bot: I haz the power, 2017-05-09 06:09:53]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/34b559296ecf1005c41ea8278327...