Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2736293002:
Add chrome flags entry for ChromeVox ARC support (Closed)
|
Created:
3 years, 9 months ago by David Tseng Modified:
3 years, 9 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, victorhsieh+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, asvitkine+watch_chromium.org, srahim+watch_chromium.org, davemoore+watch_chromium.org, yusukes+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd chrome flags entry for ChromeVox ARC support
Also, enforces only at most one form of Android spoken feedback at any given time.
BUG=683396
Review-Url: https://codereview.chromium.org/2736293002
Cr-Commit-Position: refs/heads/master@{#456862}
Committed: https://chromium.googlesource.com/chromium/src/+/431540d32b0ef55df2fe7d3df12958b4c47c3435
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address feedback. #
Total comments: 3
Patch Set 3 : Address feedback. #
Total comments: 2
Patch Set 4 : Add name string. #Patch Set 5 : Rebase. #Patch Set 6 : Try getting boolean pref. #Patch Set 7 : Boolean managed pref. #
Messages
Total messages: 50 (35 generated)
Description was changed from ========== Add a flag to enable ChromeVox ARC++ support BUG= ========== to ========== Add a flag to enable ChromeVox ARC++ support Also, enforces only at most one form of Android spoken feedback at any given time. ==========
dtseng@chromium.org changed reviewers: + yawano@chromium.org
Description was changed from ========== Add a flag to enable ChromeVox ARC++ support Also, enforces only at most one form of Android spoken feedback at any given time. ========== to ========== Add chrome flags entry for ChromeVox ARC support Also, enforces only at most one form of Android spoken feedback at any given time. BUG=683396 ==========
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by https://codereview.chromium.org/2736293002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2736293002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:14961: + <message name="IDS_FLAGS_ENABLE_CHROMEVOX_ARC_SUPPORT_DESCRIPTION" desc="Name of the flag that enables ChromeVox support for ARC++."> nit: Use "ARC" instead of "ARC++", for consistency. https://codereview.chromium.org/2736293002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right): https://codereview.chromium.org/2736293002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:599: bool value_exists = pref->GetValue()->GetAsInteger(&val); maybe pref->GetValue()->is_int(); to avoid the temporary |val|?
dtseng@chromium.org changed reviewers: + isherman@chromium.org
PTAL. Thanks lhchavez (for OWNERS) and +isherman for histograms.xml. https://codereview.chromium.org/2736293002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2736293002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:14961: + <message name="IDS_FLAGS_ENABLE_CHROMEVOX_ARC_SUPPORT_DESCRIPTION" desc="Name of the flag that enables ChromeVox support for ARC++."> On 2017/03/08 19:17:44, Luis Héctor Chávez wrote: > nit: Use "ARC" instead of "ARC++", for consistency. Done. https://codereview.chromium.org/2736293002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right): https://codereview.chromium.org/2736293002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:599: bool value_exists = pref->GetValue()->GetAsInteger(&val); On 2017/03/08 19:17:44, Luis Héctor Chávez wrote: > maybe pref->GetValue()->is_int(); to avoid the temporary |val|? Done.
histograms.xml lgtm
https://codereview.chromium.org/2736293002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right): https://codereview.chromium.org/2736293002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:530: if (enabled) { I think this will disable focus highlight if user has enabled TalkBack as focus highlight also relies on accessibility helper. We will need to have an additional check here or ArcAccessibilityHelper side.
PTAL. https://codereview.chromium.org/2736293002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right): https://codereview.chromium.org/2736293002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:530: if (enabled) { On 2017/03/10 05:05:47, yawano wrote: > I think this will disable focus highlight if user has enabled TalkBack as focus > highlight also relies on accessibility helper. We will need to have an > additional check here or ArcAccessibilityHelper side. I'm not sure I follow. Are you saying if someone has ChromeVox enabled and focus highlight enabled? In that case, removing this check is fine as accessibility helper when loaded, sends a filter type to the service from the Chrome end. Nothing else is needed (i.e. this would retain the previous behavior). In general though, it doesn't make sense to have focus highlight and ChromeVox enabled at the same time since both ChromeVox and Talkback do their own focus highlighting, right?
lgtm. Thank you! https://codereview.chromium.org/2736293002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right): https://codereview.chromium.org/2736293002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:530: if (enabled) { Sorry, I missed the point that TalkBack also does focus highlight. Yes, we should be okay to disable accessibility helper. I think both leaving accessibility helper as is and disabling it should be okay. Thank you.
lgtm with a nit. https://codereview.chromium.org/2736293002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2736293002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2350: {"enable-chromevox-arc-support", IDS_FLAGS_SHOW_ARC_FILES_APP_NAME, nit: IDS_FLAGS_ENABLE_CHROMEVOX_ARC_SUPPORT_NAME
lgtm
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
https://codereview.chromium.org/2736293002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2736293002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2350: {"enable-chromevox-arc-support", IDS_FLAGS_SHOW_ARC_FILES_APP_NAME, On 2017/03/13 02:07:18, yawano wrote: > nit: IDS_FLAGS_ENABLE_CHROMEVOX_ARC_SUPPORT_NAME Ugg. Yup added. Thanks.
The CQ bit was checked by dtseng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, lhchavez@chromium.org, yawano@chromium.org Link to the patchset: https://codereview.chromium.org/2736293002/#ps60001 (title: "Add name string.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dtseng@chromium.org
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by dtseng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, isherman@chromium.org, yawano@chromium.org Link to the patchset: https://codereview.chromium.org/2736293002/#ps80001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dtseng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, isherman@chromium.org, yawano@chromium.org Link to the patchset: https://codereview.chromium.org/2736293002/#ps120001 (title: "Boolean managed pref.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489529367114670,
"parent_rev": "40f48059f7f123778ac5ae4a746968f60f174f4e", "commit_rev":
"431540d32b0ef55df2fe7d3df12958b4c47c3435"}
Message was sent while issue was closed.
Description was changed from ========== Add chrome flags entry for ChromeVox ARC support Also, enforces only at most one form of Android spoken feedback at any given time. BUG=683396 ========== to ========== Add chrome flags entry for ChromeVox ARC support Also, enforces only at most one form of Android spoken feedback at any given time. BUG=683396 Review-Url: https://codereview.chromium.org/2736293002 Cr-Commit-Position: refs/heads/master@{#456862} Committed: https://chromium.googlesource.com/chromium/src/+/431540d32b0ef55df2fe7d3df129... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/431540d32b0ef55df2fe7d3df129... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
