AX checked state changes

AX checked state work

1. Blink should let aria-checked override an input element's native semantics
2. We should support HTML radio/checkbox indeterminate states as mixed,
   even though this is an oversight in the specs
3. We should support aria-checked="mixed" on any kind of radio or checkbox
4. We should support aria-checked on <option> when used for
   role="menuitemcheckbox" or "menuitemradio"

Note: found and fixed at least some additional bugs while working on it:
- The action names for checked/unchecked where reversed
- Android didn't consider role="switch" checkable unless it was already checked

BUG=659835
R=dmazzoni
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2694903010
Cr-Commit-Position: refs/heads/master@{#465079}
Committed: https://chromium.googlesource.com/chromium/src/+/ae91e9c848e802335b5c424e62346b950b49cf31

[aleventhal, 2017-02-16 20:16:06]

Description was changed from

==========
AX checked state work

1. Blink should let aria-checked override an input element's native semantics
2. We should support HTML radio/checkbox indeterminate states as mixed,
   even though this is an oversight in the specs
3. We should support aria-checked="mixed" on any kind of radio or checkbox
4. We should support aria-checked on <option> when used for
   role="menuitemcheckbox" or "menuitemradio"

BUG=659835
R=dmazzoni
==========

to

==========
AX checked state work

1. Blink should let aria-checked override an input element's native semantics
2. We should support HTML radio/checkbox indeterminate states as mixed,
   even though this is an oversight in the specs
3. We should support aria-checked="mixed" on any kind of radio or checkbox
4. We should support aria-checked on <option> when used for
   role="menuitemcheckbox" or "menuitemradio"

BUG=659835
R=dmazzoni
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[aleventhal, 2017-02-16 20:18:29]

Description was changed from

==========
AX checked state work

1. Blink should let aria-checked override an input element's native semantics
2. We should support HTML radio/checkbox indeterminate states as mixed,
   even though this is an oversight in the specs
3. We should support aria-checked="mixed" on any kind of radio or checkbox
4. We should support aria-checked on <option> when used for
   role="menuitemcheckbox" or "menuitemradio"

BUG=659835
R=dmazzoni
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
AX checked state work

1. Blink should let aria-checked override an input element's native semantics
2. We should support HTML radio/checkbox indeterminate states as mixed,
   even though this is an oversight in the specs
3. We should support aria-checked="mixed" on any kind of radio or checkbox
4. We should support aria-checked on <option> when used for
   role="menuitemcheckbox" or "menuitemradio"

BUG=659835
R=dmazzoni
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[aleventhal, 2017-02-16 20:18:30]

Description was changed from

==========
AX checked state work

1. Blink should let aria-checked override an input element's native semantics
2. We should support HTML radio/checkbox indeterminate states as mixed,
   even though this is an oversight in the specs
3. We should support aria-checked="mixed" on any kind of radio or checkbox
4. We should support aria-checked on <option> when used for
   role="menuitemcheckbox" or "menuitemradio"

BUG=659835
R=dmazzoni
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
AX checked state work

1. Blink should let aria-checked override an input element's native semantics
2. We should support HTML radio/checkbox indeterminate states as mixed,
   even though this is an oversight in the specs
3. We should support aria-checked="mixed" on any kind of radio or checkbox
4. We should support aria-checked on <option> when used for
   role="menuitemcheckbox" or "menuitemradio"

BUG=659835
R=dmazzoni
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dmazzoni, 2017-02-16 20:56:05]

On the right track

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/accessibility/html-input-mixed.html (right):

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/html-input-mixed.html:8: <input
id="element1" type="checkbox" />  <!-- Will set indeterminate state via JS -->
Maybe put this comment on the line above so it fits in 80 cols

We don't have a strict rule of 80 cols for html, but it's nice
since most other source files are 80 cols and the code review
tool wraps at 80 by default

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/html-input-mixed.html:19: <input
type="radio" name="radiogroup2" />
Might be nice to put an id on this one and check its
state too

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/html-input-mixed.html:31:
document.getElementById('element1').indeterminate = true;
You could put a <script> tag right after the element and set it
there, that might be cleaner

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/html-input-mixed.html:36:
debug("Role: " + element.role);
This is an old-style test, where you dump out text and check the
expectations. Please write a unit-style test like aria-modal.html
where you make individual assertions

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:424: // Only
checkboxes should support the mixed state.
This comment disagrees with the code below

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:426: if (role ==
CheckBoxRole || role == MenuItemCheckBoxRole ||
Is the set of roles checked here different than the set of
roles checked by isCheckable? Not even sure if it's needed.

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:458: return
input->shouldAppearIndeterminate();
Is it necessary to check the type? I wonder if
shouldAppearIndeterminate ever returns true for
something other than a checkbox or radio.

If not, just return shouldAppearIndeterminate()
directly.

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebAXObject.cpp (right):

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebAXObject.cpp:284: bool
WebAXObject::isButtonStateMixed() const {
i'd like you to replace this with a tristate, i.e.
rename this checkboxOrRadioState().

[dmazzoni, 2017-02-22 05:19:20]

As a general rule, please reply to all of the code review
comments indicating you've addressed them.
You can do this inline (preferred) just by clicking
on a comment and then clicking "Reply" or "Done".

Either way, no email is sent when you upload a new version
for review so please do send an email whenever it's ready
for a fresh look.

[aleventhal, 2017-02-24 21:05:16]

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

[commit-bot: I haz the power, 2017-02-24 21:06:14]

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-02-24 21:22:49]

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

[commit-bot: I haz the power, 2017-02-24 21:22:50]

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

[aleventhal, 2017-02-24 21:44:02]

Dominic, it's ready for another look.

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/accessibility/html-input-mixed.html (right):

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/html-input-mixed.html:8: <input
id="element1" type="checkbox" />  <!-- Will set indeterminate state via JS -->
On 2017/02/16 20:56:04, dmazzoni wrote:
> Maybe put this comment on the line above so it fits in 80 cols
> 
> We don't have a strict rule of 80 cols for html, but it's nice
> since most other source files are 80 cols and the code review
> tool wraps at 80 by default

Done.

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/html-input-mixed.html:19: <input
type="radio" name="radiogroup2" />
On 2017/02/16 20:56:04, dmazzoni wrote:
> Might be nice to put an id on this one and check its
> state too

Done.

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/html-input-mixed.html:31:
document.getElementById('element1').indeterminate = true;
On 2017/02/16 20:56:04, dmazzoni wrote:
> You could put a <script> tag right after the element and set it
> there, that might be cleaner

Done.

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/html-input-mixed.html:36:
debug("Role: " + element.role);
On 2017/02/16 20:56:04, dmazzoni wrote:
> This is an old-style test, where you dump out text and check the
> expectations. Please write a unit-style test like aria-modal.html
> where you make individual assertions

Instead of this one, right?

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:424: // Only
checkboxes should support the mixed state.
On 2017/02/16 20:56:04, dmazzoni wrote:
> This comment disagrees with the code below

Done.

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:426: if (role ==
CheckBoxRole || role == MenuItemCheckBoxRole ||
On 2017/02/16 20:56:04, dmazzoni wrote:
> Is the set of roles checked here different than the set of
> roles checked by isCheckable? Not even sure if it's needed.
> 

Yes, it doesn't include the Switch role.

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:458: return
input->shouldAppearIndeterminate();
On 2017/02/16 20:56:04, dmazzoni wrote:
> Is it necessary to check the type? I wonder if
> shouldAppearIndeterminate ever returns true for
> something other than a checkbox or radio.
> 
> If not, just return shouldAppearIndeterminate()
> directly.
> 

It doesn't right now. What do you think about keeping the checks or asserting?
HTML/CSS are fairly fluid. I feel like we should be intentional about any
changes in the upstream specs affecting our API.

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebAXObject.cpp (right):

https://codereview.chromium.org/2694903010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebAXObject.cpp:284: bool
WebAXObject::isButtonStateMixed() const {
On 2017/02/16 20:56:04, dmazzoni wrote:
> i'd like you to replace this with a tristate, i.e.
> rename this checkboxOrRadioState().

Done.

[aleventhal, 2017-02-27 16:05:17]

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

[commit-bot: I haz the power, 2017-02-27 16:05:39]

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-02-27 16:31:39]

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

[commit-bot: I haz the power, 2017-02-27 16:31:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[aleventhal, 2017-02-27 17:52:42]

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

[commit-bot: I haz the power, 2017-02-27 17:53:25]

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-02-27 18:29:15]

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

[commit-bot: I haz the power, 2017-02-27 18:29:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dmazzoni, 2017-02-27 19:54:55]

Will update this shortly, haven't forgotten

[aleventhal, 2017-02-27 20:27:10]

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

[commit-bot: I haz the power, 2017-02-27 20:27:40]

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-02-27 20:41:04]

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

[commit-bot: I haz the power, 2017-02-27 20:41:05]

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_...)

[aleventhal, 2017-02-27 22:56:30]

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

[commit-bot: I haz the power, 2017-02-27 22:57:04]

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-02-27 23:22:29]

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

[commit-bot: I haz the power, 2017-02-27 23:22:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dmazzoni, 2017-02-28 00:10:50]

Coming along!

For automation API support you'll need to add something
to:

chrome/renderer/extensions/automation_internal_custom_bindings.cc

Do something similar to RouteNodeIDFunction("GetRole").

https://codereview.chromium.org/2694903010/diff/180001/ash/common/system/chro...
File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right):

https://codereview.chromium.org/2694903010/diff/180001/ash/common/system/chro...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:67:
ui::AX_CHECKED_STATE_FALSE);
nit: line continuations are indented 4 spaces - but rather
than memorizing, just run "git cl format" and it should
do that automatically

https://codereview.chromium.org/2694903010/diff/180001/ash/common/system/chro...
File ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc (right):

https://codereview.chromium.org/2694903010/diff/180001/ash/common/system/chro...
ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:75: const int
checkedState =
checked_state (variable naming in C++ code)

https://codereview.chromium.org/2694903010/diff/180001/ash/common/system/chro...
ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:78: checkedState
== ui::AX_CHECKED_STATE_MIXED) {
I'd skip MIXED here unless there's any reason
to believe it'd actually be used.

https://codereview.chromium.org/2694903010/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebAXObject.cpp (right):

https://codereview.chromium.org/2694903010/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebAXObject.cpp:292: WebString
WebAXObject::checked() const {
You shouldn't need this. checkedState() should be
all we need to expose.

The code below looks like it belongs only in test code.

https://codereview.chromium.org/2694903010/diff/180001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebAXObject.h (right):

https://codereview.chromium.org/2694903010/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebAXObject.h:131: BLINK_EXPORT WebString
checked() const;
delete this one

https://codereview.chromium.org/2694903010/diff/180001/ui/accessibility/ax_en...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/2694903010/diff/180001/ui/accessibility/ax_en...
ui/accessibility/ax_enums.idl:433: // Unchecked (0), Checked (1), Mixed (2)
Don't specify the enum values, just say it's of
type AXCheckedState

[aleventhal, 2017-03-01 02:27:18]

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

[commit-bot: I haz the power, 2017-03-01 02:28:12]

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-03-01 03:32:33]

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

[commit-bot: I haz the power, 2017-03-01 03:32:34]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[aleventhal, 2017-03-06 16:15:23]

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

[commit-bot: I haz the power, 2017-03-06 16:15:39]

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-03-06 17:11:51]

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

[commit-bot: I haz the power, 2017-03-06 17:11:52]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[aleventhal, 2017-03-06 18:17:08]

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

[commit-bot: I haz the power, 2017-03-06 18:17:42]

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-03-06 19:11:27]

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

[commit-bot: I haz the power, 2017-03-06 19:11:28]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[aleventhal, 2017-03-06 19:15:42]

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

[commit-bot: I haz the power, 2017-03-06 19:16:14]

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-03-06 20:13:23]

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

[commit-bot: I haz the power, 2017-03-06 20:13:24]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[aleventhal, 2017-03-07 15:15:50]

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

[commit-bot: I haz the power, 2017-03-07 15:16: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-03-07 16:08:58]

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

[commit-bot: I haz the power, 2017-03-07 16:08:59]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[aleventhal, 2017-03-07 16:51:00]

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

[commit-bot: I haz the power, 2017-03-07 16:52:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2017-03-07 17:35:36]

https://codereview.chromium.org/2694903010/diff/300001/chrome/renderer/resour...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/2694903010/diff/300001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation/automation_node.js:289: get
role() {
You need to implement checked() here, something like:

get checked() {
  return GetChecked(this.treeID, this.id);
}

https://codereview.chromium.org/2694903010/diff/300001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation/automation_node.js:1110: 'role',
You also need to add 'checked' to this list so that the
attribute is actually exposed

[commit-bot: I haz the power, 2017-03-07 17:39:58]

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

[commit-bot: I haz the power, 2017-03-07 17:39:59]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[aleventhal, 2017-03-07 21:45:13]

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

[commit-bot: I haz the power, 2017-03-07 21:46:15]

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-03-07 23:40:19]

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

[commit-bot: I haz the power, 2017-03-07 23:40:21]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[aleventhal, 2017-03-08 16:57:39]

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

[commit-bot: I haz the power, 2017-03-08 16:58:02]

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-03-08 18:52:29]

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

[commit-bot: I haz the power, 2017-03-08 18:52:30]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[aleventhal, 2017-03-08 22:42:12]

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

[commit-bot: I haz the power, 2017-03-08 22:42:58]

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-03-08 22:52:30]

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

[commit-bot: I haz the power, 2017-03-08 22:52:31]

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_...)

[aleventhal, 2017-03-09 02:46:42]

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

[commit-bot: I haz the power, 2017-03-09 02:47:14]

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-03-09 04:15:07]

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

[commit-bot: I haz the power, 2017-03-09 04:15:08]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[aleventhal, 2017-03-09 15:27:06]

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

[commit-bot: I haz the power, 2017-03-09 15:27:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-03-09 16:24:54]

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

[commit-bot: I haz the power, 2017-03-09 16:25:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-03-09 16:59:58]

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

[commit-bot: I haz the power, 2017-03-09 17:00:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-03-09 17:10:51]

Ready for review

https://codereview.chromium.org/2694903010/diff/400001/content/browser/access...
File content/browser/accessibility/browser_accessibility_android.cc (right):

https://codereview.chromium.org/2694903010/diff/400001/content/browser/access...
content/browser/accessibility/browser_accessibility_android.cc:147: //
TODO(aleventhal) does this ever happen when checkable is not true yet?
Dominic, we had this extra logic, but I'm not sure what else could be checkable
other than the roles above.

https://codereview.chromium.org/2694903010/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.h (right):

https://codereview.chromium.org/2694903010/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.h:126: 
The methods have been moved up to AXObject so that <option> elements can
benefit.

https://codereview.chromium.org/2694903010/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (left):

https://codereview.chromium.org/2694903010/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:932: return
isChecked() ? AXSupportedAction::Check
This was the reverse of what it should be.

https://codereview.chromium.org/2694903010/diff/440001/content/test/data/acce...
File content/test/data/accessibility/aria/aria-switch-expected-android.txt
(left):

https://codereview.chromium.org/2694903010/diff/440001/content/test/data/acce...
content/test/data/accessibility/aria/aria-switch-expected-android.txt:1:
android.webkit.WebView focusable focused scrollable
A switch should always be checkable unless it's disabled.

[commit-bot: I haz the power, 2017-03-09 17:34:01]

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

[commit-bot: I haz the power, 2017-03-09 17:34:02]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[aleventhal, 2017-03-09 18:22:26]

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

[commit-bot: I haz the power, 2017-03-09 18:23:01]

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-03-09 19:04:05]

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

[commit-bot: I haz the power, 2017-03-09 19:04:07]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-03-09 19:33:39]

dmazzoni@chromium.org changed reviewers:
+ dglazkov@chromium.org, dtseng@chromium.org

[dmazzoni, 2017-03-09 19:33:41]

lgtm

Excellent job, just some minor nits!

+dglazkov for third_party/WebKit/public
since that code looks good.

+dtseng to review the ChromeVox changes

I suggest adding sky@ to review all views/
code once you've addressed the nits.

Then run 'git cl owners' to see if any more
owners are needed after the three of us approve

https://codereview.chromium.org/2694903010/diff/460001/ash/common/system/chro...
File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right):

https://codereview.chromium.org/2694903010/diff/460001/ash/common/system/chro...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:65: const int checkedState
=
const ui::AXCheckedState checkedState (instead of int)

https://codereview.chromium.org/2694903010/diff/460001/ash/common/system/chro...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:161: const int checkedState
=
same

https://codereview.chromium.org/2694903010/diff/460001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/2694903010/diff/460001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:728: *
represents something 'truthy', e.g.: for checked:
As defined by Colbert? :)

https://codereview.chromium.org/2694903010/diff/460001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:733:
switch(attrib) {
nit: just 2 space indent here

https://codereview.chromium.org/2694903010/diff/460001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/accessibility/input-mixed.html (right):

https://codereview.chromium.org/2694903010/diff/460001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/accessibility/input-mixed.html:14: <!-- Will be
::intederminate in CSS because no radio item selected yet -->
spelling

https://codereview.chromium.org/2694903010/diff/460001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/accessibility/input-mixed.html:20: <!-- NOT
mixed/intederminate because group has a selected radio button -->
same

https://codereview.chromium.org/2694903010/diff/460001/ui/views/controls/butt...
File ui/views/controls/button/toggle_button.cc (right):

https://codereview.chromium.org/2694903010/diff/460001/ui/views/controls/butt...
ui/views/controls/button/toggle_button.cc:236: const int checkedState =
checked_state

https://codereview.chromium.org/2694903010/diff/460001/ui/views/controls/menu...
File ui/views/controls/menu/menu_item_view.cc (right):

https://codereview.chromium.org/2694903010/diff/460001/ui/views/controls/menu...
ui/views/controls/menu/menu_item_view.cc:178: const bool isChecked =
GetDelegate()->IsItemChecked(GetCommand());
is_checked

https://codereview.chromium.org/2694903010/diff/460001/ui/views/controls/menu...
ui/views/controls/menu/menu_item_view.cc:179: const int checkedState =
checked_state

Also type AXCheckedState

[aleventhal, 2017-03-09 21:34:41]

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

[commit-bot: I haz the power, 2017-03-09 21:35:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-03-09 21:39:17]

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

[aleventhal, 2017-03-09 21:40:49]

https://codereview.chromium.org/2694903010/diff/460001/ash/common/system/chro...
File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right):

https://codereview.chromium.org/2694903010/diff/460001/ash/common/system/chro...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:65: const int checkedState
=
On 2017/03/09 19:33:41, dmazzoni wrote:
> const ui::AXCheckedState checkedState (instead of int)

Done.

https://codereview.chromium.org/2694903010/diff/460001/ash/common/system/chro...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:161: const int checkedState
=
On 2017/03/09 19:33:41, dmazzoni wrote:
> same

Done.

https://codereview.chromium.org/2694903010/diff/460001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/2694903010/diff/460001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:728: *
represents something 'truthy', e.g.: for checked:
On 2017/03/09 19:33:41, dmazzoni wrote:
> As defined by Colbert? :)

Actually yeah this is popular in the JS community, it's great IMO. 
Also falsey.

See https://developer.mozilla.org/en-US/docs/Glossary/Truthy

https://codereview.chromium.org/2694903010/diff/460001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:733:
switch(attrib) {
On 2017/03/09 19:33:41, dmazzoni wrote:
> nit: just 2 space indent here

Done.

https://codereview.chromium.org/2694903010/diff/460001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/accessibility/input-mixed.html (right):

https://codereview.chromium.org/2694903010/diff/460001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/accessibility/input-mixed.html:14: <!-- Will be
::intederminate in CSS because no radio item selected yet -->
On 2017/03/09 19:33:41, dmazzoni wrote:
> spelling

Done.

https://codereview.chromium.org/2694903010/diff/460001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/accessibility/input-mixed.html:20: <!-- NOT
mixed/intederminate because group has a selected radio button -->
On 2017/03/09 19:33:41, dmazzoni wrote:
> same

Done.

https://codereview.chromium.org/2694903010/diff/460001/ui/views/controls/butt...
File ui/views/controls/button/toggle_button.cc (right):

https://codereview.chromium.org/2694903010/diff/460001/ui/views/controls/butt...
ui/views/controls/button/toggle_button.cc:236: const int checkedState =
On 2017/03/09 19:33:41, dmazzoni wrote:
> checked_state

Done.

https://codereview.chromium.org/2694903010/diff/460001/ui/views/controls/menu...
File ui/views/controls/menu/menu_item_view.cc (right):

https://codereview.chromium.org/2694903010/diff/460001/ui/views/controls/menu...
ui/views/controls/menu/menu_item_view.cc:179: const int checkedState =
On 2017/03/09 19:33:41, dmazzoni wrote:
> checked_state
> 
> Also type AXCheckedState

Done.

[commit-bot: I haz the power, 2017-03-09 21:44:47]

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

[commit-bot: I haz the power, 2017-03-09 21:44:52]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[aleventhal, 2017-03-09 21:49:24]

Description was changed from

==========
AX checked state work

1. Blink should let aria-checked override an input element's native semantics
2. We should support HTML radio/checkbox indeterminate states as mixed,
   even though this is an oversight in the specs
3. We should support aria-checked="mixed" on any kind of radio or checkbox
4. We should support aria-checked on <option> when used for
   role="menuitemcheckbox" or "menuitemradio"

BUG=659835
R=dmazzoni
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
AX checked state work

1. Blink should let aria-checked override an input element's native semantics
2. We should support HTML radio/checkbox indeterminate states as mixed,
   even though this is an oversight in the specs
3. We should support aria-checked="mixed" on any kind of radio or checkbox
4. We should support aria-checked on <option> when used for
   role="menuitemcheckbox" or "menuitemradio"

Note: found and fixed at least some additional bugs while working on it:
- The action names for checked/unchecked where reversed
- Android didn't consider role="switch" checkable unless it was already checked

BUG=659835
R=dmazzoni
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[aleventhal, 2017-03-09 21:53:08]

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

[commit-bot: I haz the power, 2017-03-09 21:53:32]

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-03-09 22:57:11]

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

[commit-bot: I haz the power, 2017-03-09 22:57:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[David Tseng, 2017-03-13 18:10:54]

https://codereview.chromium.org/2694903010/diff/500001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/2694903010/diff/500001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:727: *
For a given automation property, return true if the the value
nit: remove dup the

https://codereview.chromium.org/2694903010/diff/500001/chrome/renderer/extens...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/2694903010/diff/500001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:1014: const
char* AutomationInternalCustomBindings::GetCheckedStateName(
ax_enums.h should generate this for you (ToString(enum_value))

[David Tseng, 2017-03-17 16:10:18]

lgtm with comments addressed. Thanks.

[dglazkov, 2017-03-18 23:29:42]

lgtm

[aleventhal, 2017-04-14 18:59:26]

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

[commit-bot: I haz the power, 2017-04-14 18:59:58]

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-04-14 19:37:35]

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

[commit-bot: I haz the power, 2017-04-14 19:37:37]

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_...)

[aleventhal, 2017-04-14 23:02:52]

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

[commit-bot: I haz the power, 2017-04-14 23:09:08]

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-04-14 23:43:29]

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

[commit-bot: I haz the power, 2017-04-14 23:43:31]

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_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[aleventhal, 2017-04-15 03:17:27]

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

[commit-bot: I haz the power, 2017-04-15 03:17:41]

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-04-15 05:18:19]

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

[commit-bot: I haz the power, 2017-04-15 05:18:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aleventhal, 2017-04-15 17:47:31]

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

[commit-bot: I haz the power, 2017-04-15 17:47:44]

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-04-15 19:15:06]

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

[commit-bot: I haz the power, 2017-04-15 19:15:07]

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

[aleventhal, 2017-04-15 19:56:31]

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

[commit-bot: I haz the power, 2017-04-15 19:56:40]

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-04-15 21:12:41]

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

[commit-bot: I haz the power, 2017-04-15 21:12:43]

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_...)

[aleventhal1, 2017-04-17 15:27:50]

The CQ bit was checked by aleventhal@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 15:28:19]

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-04-17 17:12:55]

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

[commit-bot: I haz the power, 2017-04-17 17:12:57]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[aleventhal, 2017-04-17 22:15:30]

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

[commit-bot: I haz the power, 2017-04-17 22:16:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-04-17 22:19:17]

aleventhal@chromium.org changed reviewers:
- aleventhal@google.com

[sky, 2017-04-17 23:34:38]

LGTM

[commit-bot: I haz the power, 2017-04-18 00:16:36]

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

[commit-bot: I haz the power, 2017-04-18 00:16:38]

Dry run: This issue passed the CQ dry run.

[aleventhal, 2017-04-18 00:28:49]

The CQ bit was checked by aleventhal@chromium.org

[aleventhal, 2017-04-18 00:28:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, dtseng@chromium.org, dglazkov@chromium.org
Link to the patchset: https://codereview.chromium.org/2694903010/#ps640001
(title: "git cl try")

[commit-bot: I haz the power, 2017-04-18 00:29:21]

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-04-18 00:37:10]

CQ is committing da patch.

Bot data: {"patchset_id": 640001, "attempt_start_ts": 1492475329086740,
"parent_rev": "ada0f67fca5c772aac731925e3ce4918548a04a5", "commit_rev":
"ae91e9c848e802335b5c424e62346b950b49cf31"}

[commit-bot: I haz the power, 2017-04-18 00:37:40]

Description was changed from

==========
AX checked state work

1. Blink should let aria-checked override an input element's native semantics
2. We should support HTML radio/checkbox indeterminate states as mixed,
   even though this is an oversight in the specs
3. We should support aria-checked="mixed" on any kind of radio or checkbox
4. We should support aria-checked on <option> when used for
   role="menuitemcheckbox" or "menuitemradio"

Note: found and fixed at least some additional bugs while working on it:
- The action names for checked/unchecked where reversed
- Android didn't consider role="switch" checkable unless it was already checked

BUG=659835
R=dmazzoni
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
AX checked state work

1. Blink should let aria-checked override an input element's native semantics
2. We should support HTML radio/checkbox indeterminate states as mixed,
   even though this is an oversight in the specs
3. We should support aria-checked="mixed" on any kind of radio or checkbox
4. We should support aria-checked on <option> when used for
   role="menuitemcheckbox" or "menuitemradio"

Note: found and fixed at least some additional bugs while working on it:
- The action names for checked/unchecked where reversed
- Android didn't consider role="switch" checkable unless it was already checked

BUG=659835
R=dmazzoni
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2694903010
Cr-Commit-Position: refs/heads/master@{#465079}
Committed:
https://chromium.googlesource.com/chromium/src/+/ae91e9c848e802335b5c424e6234...
==========

[commit-bot: I haz the power, 2017-04-18 00:37:43]

Committed patchset #33 (id:640001) as
https://chromium.googlesource.com/chromium/src/+/ae91e9c848e802335b5c424e6234...

[aleventhal, 2017-04-19 19:29:32]

https://codereview.chromium.org/2694903010/diff/500001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/2694903010/diff/500001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:727: *
For a given automation property, return true if the the value
On 2017/03/13 18:10:54, David Tseng wrote:
> nit: remove dup the

Done.