Name calculation should not include nameFrom:author descendants.

Name calculation should not include nameFrom:author descendants.
Also, listitem should not compute name from descendants.

There are still a couple of failing tests that don't look related.

BUG=722145, 722146
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
NOTRY=true

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

[aleventhal, 2017-05-08 23:53:47]

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

[commit-bot: I haz the power, 2017-05-09 00:07:26]

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-09 02:05:14]

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

[commit-bot: I haz the power, 2017-05-09 02:05:15]

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

[aleventhal, 2017-05-10 16:35:28]

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

[commit-bot: I haz the power, 2017-05-10 16:36:30]

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-10 17:23:09]

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

[commit-bot: I haz the power, 2017-05-10 17:23:10]

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

[aleventhal, 2017-05-11 21:56:52]

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

[commit-bot: I haz the power, 2017-05-11 21:57:47]

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-11 22:23:46]

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

[commit-bot: I haz the power, 2017-05-11 22:23:47]

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_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[aleventhal, 2017-05-11 22:54:44]

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

[commit-bot: I haz the power, 2017-05-11 22:56:06]

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-11 23:05:25]

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

[commit-bot: I haz the power, 2017-05-11 23:05:26]

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

[aleventhal, 2017-05-12 14:56:48]

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

[commit-bot: I haz the power, 2017-05-12 14:57: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-05-12 15:27:17]

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

[commit-bot: I haz the power, 2017-05-12 15:27:18]

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

[aleventhal, 2017-05-12 16:40:05]

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

[commit-bot: I haz the power, 2017-05-12 16:40:52]

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-12 17:28:44]

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

[commit-bot: I haz the power, 2017-05-12 17:28:45]

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

[aleventhal, 2017-05-12 17:39:49]

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

[commit-bot: I haz the power, 2017-05-12 17:41: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-05-12 19:12:39]

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

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

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

[aleventhal1, 2017-05-12 20:03:08]

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

[commit-bot: I haz the power, 2017-05-12 20:03:50]

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-12 20:05:46]

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

[commit-bot: I haz the power, 2017-05-12 20:05:49]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  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_...)

[aleventhal, 2017-05-12 23:02:21]

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

[commit-bot: I haz the power, 2017-05-12 23:04: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-12 23:55:31]

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

[commit-bot: I haz the power, 2017-05-12 23:55:32]

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

[aleventhal, 2017-05-13 00:43:07]

Description was changed from

==========
Name calculation should not include nameFrom:author descendants.

BUG=None
==========

to

==========
Name calculation should not include nameFrom:author descendants.

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[aleventhal, 2017-05-13 00:50:05]

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

[commit-bot: I haz the power, 2017-05-13 00:50:37]

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

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

[commit-bot: I haz the power, 2017-05-13 01:23:22]

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-05-13 02:59:37]

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

[commit-bot: I haz the power, 2017-05-13 02:59:50]

Dry run: CQ is trying da patch.

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

[aleventhal, 2017-05-13 03:07:58]

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

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

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-13 03:38:46]

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

[commit-bot: I haz the power, 2017-05-13 03:38:47]

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-05-13 18:00:33]

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

[commit-bot: I haz the power, 2017-05-13 18:00: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-05-13 18:44:04]

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

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

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-05-13 18:55:24]

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

[commit-bot: I haz the power, 2017-05-13 18:55:34]

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-13 19:39:10]

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

[commit-bot: I haz the power, 2017-05-13 19:39:12]

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

[aleventhal, 2017-05-14 16:43:26]

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

[commit-bot: I haz the power, 2017-05-14 16:43:35]

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-14 17:37:49]

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

[commit-bot: I haz the power, 2017-05-14 17:37:50]

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

[aleventhal, 2017-05-14 17:57:35]

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

[commit-bot: I haz the power, 2017-05-14 17:57:46]

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-14 18:56:53]

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

[commit-bot: I haz the power, 2017-05-14 18:56:54]

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-05-14 21:52:58]

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

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

Dry run: CQ is trying da patch.

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

[aleventhal, 2017-05-14 21:55:19]

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

[commit-bot: I haz the power, 2017-05-14 21:55:26]

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-14 22:32:05]

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

[commit-bot: I haz the power, 2017-05-14 22:32:06]

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-05-14 22:34:28]

Description was changed from

==========
Name calculation should not include nameFrom:author descendants.

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Name calculation should not include nameFrom:author descendants.

BUG=722145,722146
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[aleventhal, 2017-05-14 22:43:51]

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

[commit-bot: I haz the power, 2017-05-14 22:44: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-14 23:27:06]

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

[commit-bot: I haz the power, 2017-05-14 23:27:07]

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

[aleventhal, 2017-05-15 02:59:42]

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

[commit-bot: I haz the power, 2017-05-15 03:00: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-05-15 04:03:32]

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

[commit-bot: I haz the power, 2017-05-15 04:03: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-05-15 12:51:29]

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

[aleventhal, 2017-05-15 12:51:33]

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

[commit-bot: I haz the power, 2017-05-15 12:51:53]

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-15 12:56:00]

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

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

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

[aleventhal, 2017-05-15 13:14:20]

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

[commit-bot: I haz the power, 2017-05-15 13:14: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-05-15 13:42:20]

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

[commit-bot: I haz the power, 2017-05-15 13:42: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-05-15 16:16:23]

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

[commit-bot: I haz the power, 2017-05-15 16:17: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-15 16:57:45]

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

[commit-bot: I haz the power, 2017-05-15 16:57:46]

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-05-15 17:49:30]

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

[commit-bot: I haz the power, 2017-05-15 17:50: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-05-15 18:19:12]

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

[commit-bot: I haz the power, 2017-05-15 18:19:14]

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

[aleventhal1, 2017-05-15 19:44:26]

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

[commit-bot: I haz the power, 2017-05-15 19:45:21]

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-15 20:40:09]

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

[commit-bot: I haz the power, 2017-05-15 20:40:10]

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-05-15 23:04:47]

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

[commit-bot: I haz the power, 2017-05-15 23:06: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-05-16 00:56:28]

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

[commit-bot: I haz the power, 2017-05-16 00:56:29]

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-05-16 03:11:48]

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

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

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-16 05:11:14]

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

[commit-bot: I haz the power, 2017-05-16 05:11:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-05-16 11:01:17]

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

[commit-bot: I haz the power, 2017-05-16 11:01:26]

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-16 12:53:58]

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

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

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

[aleventhal, 2017-05-16 13:34:58]

Description was changed from

==========
Name calculation should not include nameFrom:author descendants.

BUG=722145,722146
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Name calculation should not include nameFrom:author descendants.
Also, listitem should not compute name from descendants.

There are still a couple of failing tests that don't look related.

BUG=722145,722146
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[aleventhal, 2017-05-16 13:34:59]

aleventhal@chromium.org changed reviewers:
+ aboxhall@chromium.org, dmazzoni@chromium.org, dtseng@google.com,
nektar@chromium.org
- aleventhal@google.com

[aleventhal, 2017-05-16 13:35:40]

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

[commit-bot: I haz the power, 2017-05-16 13:36:12]

Dry run: CQ is trying da patch.

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

[dmazzoni, 2017-05-16 16:25:39]

lgtm

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:304: //
group.
update this comment

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXMenuList.h (right):

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXMenuList.h:46: bool
NameFromContents(const bool recursive) const override;
I think const bool is redundant, I don't normally see
const for POD arguments because they're always pass-by-value.
Just use bool.

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:641: return
kGenericContainerRole;
Up to you, but I can see an argument for this
being "group", since it actually had a semantic role
but it was suppressed because it's inside a landmark
already.

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:647: return
kGenericContainerRole;
Same

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp (right):

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp:65: // SVG AAM 1.0
S8.2: the default role for an SVG root is "group".
what about this comment?

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp:69: role =
kGenericContainerRole;
Maybe this one should be group?

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

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

[commit-bot: I haz the power, 2017-05-16 18:05:06]

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

[aleventhal1, 2017-05-16 18:11:24]

aleventhal@google.com changed reviewers:
+ aleventhal@google.com

[aleventhal1, 2017-05-16 18:11:26]

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXMenuList.h (right):

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXMenuList.h:46: bool
NameFromContents(const bool recursive) const override;
On 2017/05/16 16:25:39, dmazzoni wrote:
> I think const bool is redundant, I don't normally see
> const for POD arguments because they're always pass-by-value.
> Just use bool.

Done.

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:641: return
kGenericContainerRole;
On 2017/05/16 16:25:39, dmazzoni wrote:
> Up to you, but I can see an argument for this
> being "group", since it actually had a semantic role
> but it was suppressed because it's inside a landmark
> already.

Acknowledged.

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:647: return
kGenericContainerRole;
On 2017/05/16 16:25:39, dmazzoni wrote:
> Same

Acknowledged.

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp (right):

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp:69: role =
kGenericContainerRole;
On 2017/05/16 16:25:39, dmazzoni wrote:
> Maybe this one should be group?

Here's my general rule:
role="group" or "fieldset" -> Group
Any other container that needs to be exposed for any reason -> Generic Container

How's that?

[aleventhal1, 2017-05-16 18:15:43]

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

[chromium-reviews, 2017-05-16 18:16:28]

On 5/16/2017 2:11 PM, aleventhal@google.com wrote:
>
>
https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/accessibility/AXMenuList.h
> (right):
>
>
https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/accessibility/AXMenuList.h:46: bool
> NameFromContents(const bool recursive) const override;

I believe the Blink style requires us to drop the parameter names from 
function declarations and only put them in function definitions.
So, no need to include "recursive".

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blink-reviews, 2017-05-16 18:16:28]

On 5/16/2017 2:11 PM, aleventhal@google.com wrote:
>
>
https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/accessibility/AXMenuList.h
> (right):
>
>
https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/accessibility/AXMenuList.h:46: bool
> NameFromContents(const bool recursive) const override;

I believe the Blink style requires us to drop the parameter names from 
function declarations and only put them in function definitions.
So, no need to include "recursive".

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-05-16 18:17:07]

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-16 21:05:42]

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

[commit-bot: I haz the power, 2017-05-16 21:05:44]

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-05-16 21:07:27]

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

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

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-17 00:44:40]

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

[commit-bot: I haz the power, 2017-05-17 00:44:42]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2017-05-17 17:21:34]

dtseng@chromium.org changed reviewers:
+ dtseng@chromium.org
- dtseng@google.com

[David Tseng, 2017-05-17 17:22:04]

+ dtseng@chromium.org (not google).

[David Tseng, 2017-05-17 18:20:46]

https://codereview.chromium.org/2867073003/diff/610001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/output_test.extjs
(right):

https://codereview.chromium.org/2867073003/diff/610001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output_test.extjs:365:
{ value: 'role', start: 12, end: 16 }
What happened to the earcon?

[aleventhal1, 2017-05-17 19:45:51]

Added explicit earcon for listitem

[David Tseng, 2017-05-17 20:31:23]

lgtm

[aleventhal1, 2017-05-17 20:39:02]

The CQ bit was checked by aleventhal@google.com

[aleventhal1, 2017-05-17 20:39:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2867073003/#ps630001
(title: "Explicitly use earcon for list items")

[commit-bot: I haz the power, 2017-05-17 20:40:49]

CQ is trying da patch.

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

[nektarios, 2017-05-17 21:05:24]

+bool AXObjectImpl::NameFromContents(bool recursive) const
bool result = faluse;

Why do we need this boolean? Could we simply return the result without storing
it somewhere first?




+    // Get their own name from contents, or contribute to ancestors
Please make sure that comments that are sentences end in a period. (Sorry Style
guide.)

+    // Some objects can contribute their contents to ancestor names, but
+    // only have their own name if they are focusable
Same.

+    // but for performance reasons we only do it
+    // if the row might receive focus
Same.
Also, first line appears too short.

+      result = recursive || (CanReceiveAccessibilityFocus() && !IsEditable());

Why are editable objects excluded?
What if the grid or tree grid is editable?

[aleventhal1, 2017-05-17 21:45:40]

On 2017/05/17 21:05:24, nektarios wrote:
> +bool AXObjectImpl::NameFromContents(bool recursive) const
> bool result = faluse;
> 
> Why do we need this boolean? Could we simply return the result without storing
> it somewhere first?

I'm not sure I understand this question.
 
> 
> +      result = recursive || (CanReceiveAccessibilityFocus() &&
!IsEditable());
> 
> Why are editable objects excluded?
> What if the grid or tree grid is editable?

It is supposed to mean things that are natively editable, like a contenteditable
or textfield item. Otherwise there are many redundant names in editable text
areas.

[aleventhal1, 2017-05-17 21:45:46]

On 2017/05/17 21:05:24, nektarios wrote:
> +bool AXObjectImpl::NameFromContents(bool recursive) const
> bool result = faluse;
> 
> Why do we need this boolean? Could we simply return the result without storing
> it somewhere first?

I'm not sure I understand this question.
 
> 
> +      result = recursive || (CanReceiveAccessibilityFocus() &&
!IsEditable());
> 
> Why are editable objects excluded?
> What if the grid or tree grid is editable?

It is supposed to mean things that are natively editable, like a contenteditable
or textfield item. Otherwise there are many redundant names in editable text
areas.

[dmazzoni, 2017-05-17 22:07:40]

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp (right):

https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp:69: role =
kGenericContainerRole;
On 2017/05/16 18:11:26, aleventhal1 wrote:
> On 2017/05/16 16:25:39, dmazzoni wrote:
> > Maybe this one should be group?
> 
> Here's my general rule:
> role="group" or "fieldset" -> Group
> Any other container that needs to be exposed for any reason -> Generic
Container
> 
> How's that?

Yeah, but the spec specifically says the default role for an SVG root is "group"
according to the comment above...

[dmazzoni, 2017-05-17 22:09:31]

>
>
>
https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/accessibility/AXMenuList.h:46: bool
> NameFromContents(const bool recursive) const override;
>
>
> I believe the Blink style requires us to drop the parameter names from
> function declarations and only put them in function definitions.
> So, no need to include "recursive".
>

No, that's only when the name is redundant. In this case "recursive" isn't
obvious or redundant so it should be included.

An example of redundant would be SetEnabled(bool enabled) or
SetName(std::string name) - in that case the parameter name is obvious, so
remove it from the declaration. But something like NameFromContents(bool
recursive) is fine.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dmazzoni, 2017-05-17 22:09:32]

>
>
>
https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/accessibility/AXMenuList.h:46: bool
> NameFromContents(const bool recursive) const override;
>
>
> I believe the Blink style requires us to drop the parameter names from
> function declarations and only put them in function definitions.
> So, no need to include "recursive".
>

No, that's only when the name is redundant. In this case "recursive" isn't
obvious or redundant so it should be included.

An example of redundant would be SetEnabled(bool enabled) or
SetName(std::string name) - in that case the parameter name is obvious, so
remove it from the declaration. But something like NameFromContents(bool
recursive) is fine.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-05-17 23:36:21]

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

[commit-bot: I haz the power, 2017-05-17 23:36:23]

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

[blink-reviews, 2017-05-17 23:50:19]

bool result = false;

What I meant is that I see you setting the result variable to either 
true or false and then returning it at the bottom of the method. Why not 
return right at the points where you are currently setting result?

IsEditable

I agree with the issues that you point out with editable controls, but I 
was concerned about the editable grid case, because it's an important 
use case. What if every cell is content editable and the user moves from 
cell to cell using the cursors, the name of the cell shouldn't have its 
contents? I guess not. The contents should be in the value attribute if 
it's editable. Is that it?

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-05-17 23:50:19]

bool result = false;

What I meant is that I see you setting the result variable to either 
true or false and then returning it at the bottom of the method. Why not 
return right at the points where you are currently setting result?

IsEditable

I agree with the issues that you point out with editable controls, but I 
was concerned about the editable grid case, because it's an important 
use case. What if every cell is content editable and the user moves from 
cell to cell using the cursors, the name of the cell shouldn't have its 
contents? I guess not. The contents should be in the value attribute if 
it's editable. Is that it?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aleventhal1, 2017-05-18 13:30:40]

On 2017/05/17 23:50:19, chromium-reviews wrote:
> bool result = false;
> 
> What I meant is that I see you setting the result variable to either 
> true or false and then returning it at the bottom of the method. Why not 
> return right at the points where you are currently setting result?

Ah, yes that was my first implementation, but some of the try bot compilers were
not smart and thought that some control paths did not return a value. After some
annoying experimentation I found that this redundant setting of a false default
value satisfied all the compilers. Normally this is not a problem because most
switch statements include a default case, but I wanted to ensure that when we
add a new role we must add it to one of the 3 places in this switch statement.

> 
> IsEditable
> 
> I agree with the issues that you point out with editable controls, but I 
> was concerned about the editable grid case, because it's an important 
> use case. What if every cell is content editable and the user moves from 
> cell to cell using the cursors, the name of the cell shouldn't have its 
> contents? I guess not. The contents should be in the value attribute if 
> it's editable. Is that it?
You're right, we should check this out. If it's ok I'd like to get this in, and
we try it. If it causes an issue we can rethink the logic. I believe that when
the caret enters an editable area we should be ok.

> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[aleventhal1, 2017-05-18 13:30:44]

On 2017/05/17 23:50:19, chromium-reviews wrote:
> bool result = false;
> 
> What I meant is that I see you setting the result variable to either 
> true or false and then returning it at the bottom of the method. Why not 
> return right at the points where you are currently setting result?

Ah, yes that was my first implementation, but some of the try bot compilers were
not smart and thought that some control paths did not return a value. After some
annoying experimentation I found that this redundant setting of a false default
value satisfied all the compilers. Normally this is not a problem because most
switch statements include a default case, but I wanted to ensure that when we
add a new role we must add it to one of the 3 places in this switch statement.

> 
> IsEditable
> 
> I agree with the issues that you point out with editable controls, but I 
> was concerned about the editable grid case, because it's an important 
> use case. What if every cell is content editable and the user moves from 
> cell to cell using the cursors, the name of the cell shouldn't have its 
> contents? I guess not. The contents should be in the value attribute if 
> it's editable. Is that it?
You're right, we should check this out. If it's ok I'd like to get this in, and
we try it. If it causes an issue we can rethink the logic. I believe that when
the caret enters an editable area we should be ok.

> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[aleventhal1, 2017-05-18 13:32:35]

On 2017/05/17 22:07:40, dmazzoni wrote:
>
https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp (right):
> 
>
https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp:69: role =
> kGenericContainerRole;
> On 2017/05/16 18:11:26, aleventhal1 wrote:
> > On 2017/05/16 16:25:39, dmazzoni wrote:
> > > Maybe this one should be group?
> > 
> > Here's my general rule:
> > role="group" or "fieldset" -> Group
> > Any other container that needs to be exposed for any reason -> Generic
> Container
> > 
> > How's that?
> 
> Yeah, but the spec specifically says the default role for an SVG root is
"group"
> according to the comment above...

Ok, my mistake. I'll fix it.

[aleventhal1, 2017-05-18 13:32:40]

On 2017/05/17 22:07:40, dmazzoni wrote:
>
https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp (right):
> 
>
https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp:69: role =
> kGenericContainerRole;
> On 2017/05/16 18:11:26, aleventhal1 wrote:
> > On 2017/05/16 16:25:39, dmazzoni wrote:
> > > Maybe this one should be group?
> > 
> > Here's my general rule:
> > role="group" or "fieldset" -> Group
> > Any other container that needs to be exposed for any reason -> Generic
> Container
> > 
> > How's that?
> 
> Yeah, but the spec specifically says the default role for an SVG root is
"group"
> according to the comment above...

Ok, my mistake. I'll fix it.

[aleventhal1, 2017-05-18 13:50:18]

The CQ bit was checked by aleventhal@google.com

[commit-bot: I haz the power, 2017-05-18 13:50:43]

CQ is trying da patch.

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

[aleventhal1, 2017-05-18 13:53:31]

On 2017/05/18 13:32:40, aleventhal1 wrote:
> On 2017/05/17 22:07:40, dmazzoni wrote:
> >
>
https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2867073003/diff/550001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp:69: role =
> > kGenericContainerRole;
> > On 2017/05/16 18:11:26, aleventhal1 wrote:
> > > On 2017/05/16 16:25:39, dmazzoni wrote:
> > > > Maybe this one should be group?
> > > 
> > > Here's my general rule:
> > > role="group" or "fieldset" -> Group
> > > Any other container that needs to be exposed for any reason -> Generic
> > Container
> > > 
> > > How's that?
> > 
> > Yeah, but the spec specifically says the default role for an SVG root is
> "group"
> > according to the comment above...
> 
> Ok, my mistake. I'll fix it.

Actually, this change was necessary so that the svg platform tests in
content/test/data/accessibility/html still pass.
We previously exposed SVG as MSAA role group but IA2 role section. Do we no
longer want to do that?

kRoleGroup now ends up always just being strictly a group, whereas
kRoleGenericContainer becomes group/section on Windows.

[commit-bot: I haz the power, 2017-05-18 13:58:40]

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

[commit-bot: I haz the power, 2017-05-18 13:58:43]

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

[chromium-reviews, 2017-05-18 14:18:16]

On 5/18/2017 9:30 AM, aleventhal@google.com wrote:
> On 2017/05/17 23:50:19, chromium-reviews wrote:
> > bool result = false;
> >
> > What I meant is that I see you setting the result variable to either
> > true or false and then returning it at the bottom of the method. Why 
> not
> > return right at the points where you are currently setting result?
>
> Ah, yes that was my first implementation, but some of the try bot 
> compilers were
> not smart and thought that some control paths did not return a value. 
> After some
> annoying experimentation I found that this redundant setting of a 
> false default
> value satisfied all the compilers. Normally this is not a problem 
> because most
> switch statements include a default case, but I wanted to ensure that 
> when we
> add a new role we must add it to one of the 3 places in this switch 
> statement.
>
Okay, understood.
Yes, the style guide says that there should be no default case when the 
switch statement covers the values of an enumeration. This is to prevent 
adding values to the enumeration and forgetting to update any switch 
statements that reference it. Compilers will warn if the enumeration 
changes but any switch statements are not. They will not warn when there 
is a default case.


> >
> > IsEditable
> >
> > I agree with the issues that you point out with editable controls, 
> but I
> > was concerned about the editable grid case, because it's an important
> > use case. What if every cell is content editable and the user moves 
> from
> > cell to cell using the cursors, the name of the cell shouldn't have its
> > contents? I guess not. The contents should be in the value attribute if
> > it's editable. Is that it?
> You're right, we should check this out. If it's ok I'd like to get 
> this in, and
> we try it. If it causes an issue we can rethink the logic. I believe 
> that when
> the caret enters an editable area we should be ok.
Agreed.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blink-reviews, 2017-05-18 14:18:16]

On 5/18/2017 9:30 AM, aleventhal@google.com wrote:
> On 2017/05/17 23:50:19, chromium-reviews wrote:
> > bool result = false;
> >
> > What I meant is that I see you setting the result variable to either
> > true or false and then returning it at the bottom of the method. Why 
> not
> > return right at the points where you are currently setting result?
>
> Ah, yes that was my first implementation, but some of the try bot 
> compilers were
> not smart and thought that some control paths did not return a value. 
> After some
> annoying experimentation I found that this redundant setting of a 
> false default
> value satisfied all the compilers. Normally this is not a problem 
> because most
> switch statements include a default case, but I wanted to ensure that 
> when we
> add a new role we must add it to one of the 3 places in this switch 
> statement.
>
Okay, understood.
Yes, the style guide says that there should be no default case when the 
switch statement covers the values of an enumeration. This is to prevent 
adding values to the enumeration and forgetting to update any switch 
statements that reference it. Compilers will warn if the enumeration 
changes but any switch statements are not. They will not warn when there 
is a default case.


> >
> > IsEditable
> >
> > I agree with the issues that you point out with editable controls, 
> but I
> > was concerned about the editable grid case, because it's an important
> > use case. What if every cell is content editable and the user moves 
> from
> > cell to cell using the cursors, the name of the cell shouldn't have its
> > contents? I guess not. The contents should be in the value attribute if
> > it's editable. Is that it?
> You're right, we should check this out. If it's ok I'd like to get 
> this in, and
> we try it. If it causes an issue we can rethink the logic. I believe 
> that when
> the caret enters an editable area we should be ok.
Agreed.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[aleventhal, 2017-05-18 18:27:59]

The CQ bit was checked by aleventhal@chromium.org

[aleventhal, 2017-05-18 18:28:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/2867073003/#ps650001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-18 18:28:46]

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-18 20:43:04]

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

[commit-bot: I haz the power, 2017-05-18 20:43:07]

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-05-18 21:13:23]

The CQ bit was checked by aleventhal@chromium.org

[commit-bot: I haz the power, 2017-05-18 21:14:46]

CQ is trying da patch.

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

[aleventhal, 2017-05-18 23:18:08]

Description was changed from

==========
Name calculation should not include nameFrom:author descendants.
Also, listitem should not compute name from descendants.

There are still a couple of failing tests that don't look related.

BUG=722145,722146
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Name calculation should not include nameFrom:author descendants.
Also, listitem should not compute name from descendants.

There are still a couple of failing tests that don't look related.

BUG=722145,722146
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
NOTRY=true
==========

[aleventhal, 2017-05-18 23:39:18]

The CQ bit was unchecked by aleventhal@chromium.org

[aleventhal, 2017-05-18 23:39:30]

The CQ bit was checked by aleventhal@chromium.org

[commit-bot: I haz the power, 2017-05-18 23:41:15]

CQ is committing da patch.

Bot data: {"patchset_id": 650001, "attempt_start_ts": 1495142003065760,
"parent_rev": "84fa8b0864ae839a74233d9cb0bc10fee9a892df", "commit_rev":
"b70906e105560268db0efb67c3e072265e3d9582"}

[commit-bot: I haz the power, 2017-05-18 23:41:29]

Description was changed from

==========
Name calculation should not include nameFrom:author descendants.
Also, listitem should not compute name from descendants.

There are still a couple of failing tests that don't look related.

BUG=722145,722146
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
NOTRY=true
==========

to

==========
Name calculation should not include nameFrom:author descendants.
Also, listitem should not compute name from descendants.

There are still a couple of failing tests that don't look related.

BUG=722145,722146
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
NOTRY=true

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

[commit-bot: I haz the power, 2017-05-18 23:41:31]

Committed patchset #34 (id:650001) as
https://chromium.googlesource.com/chromium/src/+/b70906e105560268db0efb67c3e0...

[commit-bot: I haz the power, 2017-05-18 23:41:52]

CQ is trying da patch.

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