Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2838903002:
Add accessibility related notification to notification center (Closed)
|
Created:
3 years, 8 months ago by yiyix Modified:
3 years, 7 months ago CC:
chromium-reviews, sadrul, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, dougt+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, kalyank, je_julie, tdanderson Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd accessibility related notification into notification center
When chromevox is enabled or the braille is connected, a notification will be shown to
users through the notification center. The screenshot of the notifications can be found
in the crbug.
TEST=TrayAccessibilityTest.ShowNotification
BUG=685845
Review-Url: https://codereview.chromium.org/2838903002
Cr-Commit-Position: refs/heads/master@{#473939}
Committed: https://chromium.googlesource.com/chromium/src/+/2df25af11bbad658a261076cef020590b9988f9b
Patch Set 1 : nits #
Total comments: 23
Patch Set 2 : fix nits #
Total comments: 28
Patch Set 3 : address comments #
Total comments: 5
Patch Set 4 : comments #
Total comments: 2
Patch Set 5 : nits #
Messages
Total messages: 77 (64 generated)
Description was changed from ========== format rebase fix notification accessibility BUG= ========== to ========== BUG= ==========
The CQ bit was checked by yiyix@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by yiyix@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== BUG= ========== to ========== BUG=685845 ==========
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yiyix@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...
Patchset #2 (id:80001) has been deleted
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yiyix@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== BUG=685845 ========== to ========== [WIP] Add accessibility stuff to notification center BUG=685845 ==========
The CQ bit was checked by yiyix@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.
Patchset #3 (id:120001) has been deleted
The CQ bit was checked by yiyix@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_compile_dbg_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...)
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Patchset #3 (id:140001) has been deleted
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.
Patchset #1 (id:60001) has been deleted
Description was changed from ========== [WIP] Add accessibility stuff to notification center BUG=685845 ========== to ========== Add accessibility related notification into notification center When chromevox is enabled or the braille is connected, a notification will be shown to users through the notification center. The screenshot of the notifications can be found in the crbug. TEST=TrayAccessibilityTest.ShowNotification BUG=685845 ==========
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:160001) has been deleted
yiyix@chromium.org changed reviewers: + tdanderson@chromium.org
@tdanderson, could you please review this patch? Thank you.
I have left a first pass of comments below. https://codereview.chromium.org/2838903002/diff/180001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/2838903002/diff/180001/ash/ash_strings.grd#ne... ash/ash_strings.grd:6: nit: please run 'git cl try' after uploading the next patch set, it would be good to double check there are no regressions and that the modified tests pass. https://codereview.chromium.org/2838903002/diff/180001/ash/ash_strings.grd#ne... ash/ash_strings.grd:181: Press Ctrl+Alt+Z to disable. The spacing around the + signs should be consistent with the spacing on line 190. We should pick whichever of the two options is read back correctly by ChromeVox (I suspect that will be the one on line 190). https://codereview.chromium.org/2838903002/diff/180001/ash/ash_strings.grd#ne... ash/ash_strings.grd:186: <message name="IDS_ASH_STATUS_TRAY_SPOKEN_FEEDBACK_BRAILLE_ENABLED_TITLE" desc="The message shown on a notification when spoken feedback is enabled"> nit: update desc https://codereview.chromium.org/2838903002/diff/180001/ash/ash_strings.grd#ne... ash/ash_strings.grd:187: Braille and Chromevox are enabled Should be ChromeVox instead of Chromevox https://codereview.chromium.org/2838903002/diff/180001/ash/ash_strings.grd#ne... ash/ash_strings.grd:190: Press Ctrl + Alt + Z to disable spoken feedback. I suspect that it is sufficient for IDS_ASH_STATUS_TRAY_SPOKEN_FEEDBACK_ENABLED and IDS_ASH_STATUS_TRAY_SPOKEN_FEEDBACK_BRAILLE_ENABLED to just say "Press Ctrl + Alt + Z to disable spoken feedback." (in which case you can delete one of them). I don't see any reason why only one should say "Spoken feedback is enabled" https://codereview.chromium.org/2838903002/diff/180001/ash/resources/vector_i... File ash/resources/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2838903002/diff/180001/ash/resources/vector_i... ash/resources/vector_icons/BUILD.gn:94: "system_menu_accessibility_braille.icon", We use the system_menu_ prefix to describe icons that can be shown in the system menu itself. Since this is only used within a notification, I suggest re-naming as notification_braille.icon. https://codereview.chromium.org/2838903002/diff/180001/ash/system/tray_access... File ash/system/tray_accessibility.cc (right): https://codereview.chromium.org/2838903002/diff/180001/ash/system/tray_access... ash/system/tray_accessibility.cc:97: if (enabled_accessibility & A11Y_BRAILLE_DISPLAY_CONNECTED) { nit: no {}'s in this inner block since everything is a single line https://codereview.chromium.org/2838903002/diff/180001/ash/system/tray_access... ash/system/tray_accessibility.cc:407: ash::UMA_STATUS_AREA_DETAILED_ACCESSABILITY); Can you please fix the pre-existing typo in this enum (but we'll have to keep the typo in the user action name StatusArea_Accessability_DetailedView, otherwise this will make the collected data inconsistent) https://codereview.chromium.org/2838903002/diff/180001/ash/system/tray_access... ash/system/tray_accessibility.cc:454: // enabled or the braille is connected, which is laid out like: nit: "or if a braille device is connected" https://codereview.chromium.org/2838903002/diff/180001/ash/system/tray_access... ash/system/tray_accessibility.cc:455: // -----------------x- nit: no need for lines 455-6 any more since we're now using the standard notification appearance https://codereview.chromium.org/2838903002/diff/180001/ash/system/tray_access... ash/system/tray_accessibility.cc:458: if ((notify == A11Y_NOTIFICATION_SHOW) && being_enabled && nit: innermost () not needed https://codereview.chromium.org/2838903002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/system/tray_accessibility_browsertest.cc (right): https://codereview.chromium.org/2838903002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:932: const base::string16 CHROMEVOX_ENABLED_TITILE = typo: _TITLE
yiyix@chromium.org changed reviewers: + jamescook@chromium.org - tdanderson@chromium.org
@jamescook, could you please review this change? Thank you https://codereview.chromium.org/2838903002/diff/180001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/2838903002/diff/180001/ash/ash_strings.grd#ne... ash/ash_strings.grd:6: On 2017/05/08 21:55:27, tdanderson (OOO til 5-23) wrote: > nit: please run 'git cl try' after uploading the next patch set, it would be > good to double check there are no regressions and that the modified tests pass. I did it in the other patches. I did several patches before submitting this one (as shown in the generated messages.) I will remember to include the result in the final version. https://codereview.chromium.org/2838903002/diff/180001/ash/ash_strings.grd#ne... ash/ash_strings.grd:181: Press Ctrl+Alt+Z to disable. On 2017/05/08 21:55:27, tdanderson (OOO til 5-23) wrote: > The spacing around the + signs should be consistent with the spacing on line > 190. We should pick whichever of the two options is read back correctly by > ChromeVox (I suspect that will be the one on line 190). Good call. updated. https://codereview.chromium.org/2838903002/diff/180001/ash/ash_strings.grd#ne... ash/ash_strings.grd:186: <message name="IDS_ASH_STATUS_TRAY_SPOKEN_FEEDBACK_BRAILLE_ENABLED_TITLE" desc="The message shown on a notification when spoken feedback is enabled"> On 2017/05/08 21:55:27, tdanderson (OOO til 5-23) wrote: > nit: update desc Sorry, I forget to update it as i copy it. https://codereview.chromium.org/2838903002/diff/180001/ash/ash_strings.grd#ne... ash/ash_strings.grd:187: Braille and Chromevox are enabled On 2017/05/08 21:55:27, tdanderson (OOO til 5-23) wrote: > Should be ChromeVox instead of Chromevox Done. https://codereview.chromium.org/2838903002/diff/180001/ash/resources/vector_i... File ash/resources/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2838903002/diff/180001/ash/resources/vector_i... ash/resources/vector_icons/BUILD.gn:94: "system_menu_accessibility_braille.icon", On 2017/05/08 21:55:27, tdanderson (OOO til 5-23) wrote: > We use the system_menu_ prefix to describe icons that can be shown in the system > menu itself. Since this is only used within a notification, I suggest re-naming > as notification_braille.icon. Done. https://codereview.chromium.org/2838903002/diff/180001/ash/system/tray_access... File ash/system/tray_accessibility.cc (right): https://codereview.chromium.org/2838903002/diff/180001/ash/system/tray_access... ash/system/tray_accessibility.cc:97: if (enabled_accessibility & A11Y_BRAILLE_DISPLAY_CONNECTED) { On 2017/05/08 21:55:27, tdanderson (OOO til 5-23) wrote: > nit: no {}'s in this inner block since everything is a single line Done. https://codereview.chromium.org/2838903002/diff/180001/ash/system/tray_access... ash/system/tray_accessibility.cc:407: ash::UMA_STATUS_AREA_DETAILED_ACCESSABILITY); On 2017/05/08 21:55:27, tdanderson (OOO til 5-23) wrote: > Can you please fix the pre-existing typo in this enum (but we'll have to keep > the typo in the user action name StatusArea_Accessability_DetailedView, > otherwise this will make the collected data inconsistent) I missed it. Make sense. Corrected. https://codereview.chromium.org/2838903002/diff/180001/ash/system/tray_access... ash/system/tray_accessibility.cc:454: // enabled or the braille is connected, which is laid out like: On 2017/05/08 21:55:27, tdanderson (OOO til 5-23) wrote: > nit: "or if a braille device is connected" Done. https://codereview.chromium.org/2838903002/diff/180001/ash/system/tray_access... ash/system/tray_accessibility.cc:455: // -----------------x- On 2017/05/08 21:55:27, tdanderson (OOO til 5-23) wrote: > nit: no need for lines 455-6 any more since we're now using the standard > notification appearance Done. https://codereview.chromium.org/2838903002/diff/180001/ash/system/tray_access... ash/system/tray_accessibility.cc:458: if ((notify == A11Y_NOTIFICATION_SHOW) && being_enabled && On 2017/05/08 21:55:27, tdanderson (OOO til 5-23) wrote: > nit: innermost () not needed Done. https://codereview.chromium.org/2838903002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/system/tray_accessibility_browsertest.cc (right): https://codereview.chromium.org/2838903002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:932: const base::string16 CHROMEVOX_ENABLED_TITILE = On 2017/05/08 21:55:27, tdanderson (OOO til 5-23) wrote: > typo: _TITLE Done.
The CQ bit was checked by yiyix@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Approach seems fine, just little things. https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... File ash/system/tray_accessibility.cc (right): https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:93: if (enabled_accessibility & A11Y_BRAILLE_DISPLAY_CONNECTED && optional: Consider () around (a & b). The operator precedence is correct here, but sometimes I forget & vs. &&. https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:95: return ash::kSystemMenuAccessibilityIcon; nit: ash:: not needed, here or below https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:96: } else { nit: else not needed https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:345: const char kNotificationId[] = "chrome://settings/accessibility"; Can this go in the anonymous namespace above? https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:453: // enabled or if a braille is connected super nit: "a braille display" and end with . https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:456: message_center::MessageCenter* message_center = nit: move above line 444 and use |message_center| there https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:458: if (!message_center) Does this happen in practice? In tests? You should add a comment. Also, wouldn't the code crash on line 444 if this was null? https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:467: text.append(l10n_util::GetStringUTF16( just =, not append, here and below https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:472: if (being_enabled & A11Y_BRAILLE_DISPLAY_CONNECTED) { combine with else above to make else-if https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:483: notification = base::MakeUnique<message_center::Notification>( move declaration here from 461 https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:486: ash::kMenuIconSize, nit: no ash:: https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:492: message_center->AddNotification(std::move(notification)); or you could create the notification here with MakeUnique https://codereview.chromium.org/2838903002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/system/tray_accessibility_browsertest.cc (right): https://codereview.chromium.org/2838903002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:954: message_center::MessageCenter::Get()->GetVisibleNotifications(); optional: if you want to make lines like this (and 945) shorter, feel free to add "using message_center::MessageCenter" at file scope https://codereview.chromium.org/2838903002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:982: } Thanks for fixing up the tests!
https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... File ash/system/tray_accessibility.cc (right): https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:93: if (enabled_accessibility & A11Y_BRAILLE_DISPLAY_CONNECTED && On 2017/05/16 00:56:18, James Cook wrote: > optional: Consider () around (a & b). The operator precedence is correct here, > but sometimes I forget & vs. &&. Updated, it's easier for readability. https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:95: return ash::kSystemMenuAccessibilityIcon; On 2017/05/16 00:56:18, James Cook wrote: > nit: ash:: not needed, here or below would it be a good idea to remove all ash:: in this class? we use ash:: for all UMA values. https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:96: } else { On 2017/05/16 00:56:19, James Cook wrote: > nit: else not needed Done. https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:345: const char kNotificationId[] = "chrome://settings/accessibility"; On 2017/05/16 00:56:18, James Cook wrote: > Can this go in the anonymous namespace above? Yea, i was not sure where to put it. it's just a private variable to this class, so i tried to put closer to where we use it. https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:453: // enabled or if a braille is connected On 2017/05/16 00:56:18, James Cook wrote: > super nit: "a braille display" and end with . Done. https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:456: message_center::MessageCenter* message_center = On 2017/05/16 00:56:18, James Cook wrote: > nit: move above line 444 and use |message_center| there Done. https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:458: if (!message_center) On 2017/05/16 00:56:18, James Cook wrote: > Does this happen in practice? In tests? You should add a comment. Also, wouldn't > the code crash on line 444 if this was null? I was copying this dcheck from some test cases. I did a code search after you pointing it out, you are right. In the message center code, we have: DCHECK(g_message_center); return g_message_center; so it can never be null https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:467: text.append(l10n_util::GetStringUTF16( On 2017/05/16 00:56:18, James Cook wrote: > just =, not append, here and below Done. https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:472: if (being_enabled & A11Y_BRAILLE_DISPLAY_CONNECTED) { On 2017/05/16 00:56:19, James Cook wrote: > combine with else above to make else-if Done. https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:483: notification = base::MakeUnique<message_center::Notification>( On 2017/05/16 00:56:18, James Cook wrote: > move declaration here from 461 Done. I always forget that I suppose to declare it when i use it. https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:486: ash::kMenuIconSize, On 2017/05/16 00:56:18, James Cook wrote: > nit: no ash:: Done. https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:492: message_center->AddNotification(std::move(notification)); On 2017/05/16 00:56:19, James Cook wrote: > or you could create the notification here with MakeUnique Done. I also realize that i removed the notification for both if and else before the condition statement. I should remove the else statement. Thank you so much for the detailed review. https://codereview.chromium.org/2838903002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/system/tray_accessibility_browsertest.cc (right): https://codereview.chromium.org/2838903002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:982: } On 2017/05/16 00:56:19, James Cook wrote: > Thanks for fixing up the tests! :) you are welcome
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
On 2017/05/19 02:29:56, yiyix wrote: > https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... > File ash/system/tray_accessibility.cc (right): > > https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... > ash/system/tray_accessibility.cc:93: if (enabled_accessibility & > A11Y_BRAILLE_DISPLAY_CONNECTED && > On 2017/05/16 00:56:18, James Cook wrote: > > optional: Consider () around (a & b). The operator precedence is correct here, > > but sometimes I forget & vs. &&. > > Updated, it's easier for readability. > > https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... > ash/system/tray_accessibility.cc:95: return ash::kSystemMenuAccessibilityIcon; > On 2017/05/16 00:56:18, James Cook wrote: > > nit: ash:: not needed, here or below > > would it be a good idea to remove all ash:: in this class? we use ash:: for all > UMA values. > > https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... > ash/system/tray_accessibility.cc:96: } else { > On 2017/05/16 00:56:19, James Cook wrote: > > nit: else not needed > > Done. > > https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... > ash/system/tray_accessibility.cc:345: const char kNotificationId[] = > "chrome://settings/accessibility"; > On 2017/05/16 00:56:18, James Cook wrote: > > Can this go in the anonymous namespace above? > > Yea, i was not sure where to put it. it's just a private variable to this class, > so i tried to put closer to where we use it. > > https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... > ash/system/tray_accessibility.cc:453: // enabled or if a braille is connected > On 2017/05/16 00:56:18, James Cook wrote: > > super nit: "a braille display" and end with . > > Done. > > https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... > ash/system/tray_accessibility.cc:456: message_center::MessageCenter* > message_center = > On 2017/05/16 00:56:18, James Cook wrote: > > nit: move above line 444 and use |message_center| there > > Done. > > https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... > ash/system/tray_accessibility.cc:458: if (!message_center) > On 2017/05/16 00:56:18, James Cook wrote: > > Does this happen in practice? In tests? You should add a comment. Also, > wouldn't > > the code crash on line 444 if this was null? > > I was copying this dcheck from some test cases. I did a code search after you > pointing it out, you are right. In the message center code, we have: > DCHECK(g_message_center); > return g_message_center; > > so it can never be null > > https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... > ash/system/tray_accessibility.cc:467: text.append(l10n_util::GetStringUTF16( > On 2017/05/16 00:56:18, James Cook wrote: > > just =, not append, here and below > > Done. > > https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... > ash/system/tray_accessibility.cc:472: if (being_enabled & > A11Y_BRAILLE_DISPLAY_CONNECTED) { > On 2017/05/16 00:56:19, James Cook wrote: > > combine with else above to make else-if > > Done. > > https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... > ash/system/tray_accessibility.cc:483: notification = > base::MakeUnique<message_center::Notification>( > On 2017/05/16 00:56:18, James Cook wrote: > > move declaration here from 461 > > Done. I always forget that I suppose to declare it when i use it. > > https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... > ash/system/tray_accessibility.cc:486: ash::kMenuIconSize, > On 2017/05/16 00:56:18, James Cook wrote: > > nit: no ash:: > > Done. > > https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... > ash/system/tray_accessibility.cc:492: > message_center->AddNotification(std::move(notification)); > On 2017/05/16 00:56:19, James Cook wrote: > > or you could create the notification here with MakeUnique > > Done. I also realize that i removed the notification for both if and else before > the condition statement. I should remove the else statement. Thank you so much > for the detailed review. > > https://codereview.chromium.org/2838903002/diff/200001/chrome/browser/chromeo... > File chrome/browser/chromeos/system/tray_accessibility_browsertest.cc (right): > > https://codereview.chromium.org/2838903002/diff/200001/chrome/browser/chromeo... > chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:982: } > On 2017/05/16 00:56:19, James Cook wrote: > > Thanks for fixing up the tests! > > :) you are welcome @jamescook, could you please review this patch again? Thank you so much
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM with nits https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... File ash/system/tray_accessibility.cc (right): https://codereview.chromium.org/2838903002/diff/200001/ash/system/tray_access... ash/system/tray_accessibility.cc:95: return ash::kSystemMenuAccessibilityIcon; On 2017/05/19 02:29:55, yiyix wrote: > On 2017/05/16 00:56:18, James Cook wrote: > > nit: ash:: not needed, here or below > > would it be a good idea to remove all ash:: in this class? we use ash:: for all > UMA values. Yes, removing all ash:: is fine. We avoid extra namespace specifiers when we can. https://codereview.chromium.org/2838903002/diff/220001/ash/system/tray_access... File ash/system/tray_accessibility.cc (right): https://codereview.chromium.org/2838903002/diff/220001/ash/system/tray_access... ash/system/tray_accessibility.cc:101: else if (enabled_accessibility & A11Y_SPOKEN_FEEDBACK) nit: no need for "else", here or below https://codereview.chromium.org/2838903002/diff/220001/ash/system/tray_access... ash/system/tray_accessibility.cc:454: // enabled or if a braille is connected. nit: a braille *display* https://codereview.chromium.org/2838903002/diff/220001/ash/system/tray_access... ash/system/tray_accessibility.cc:455: if (notify == A11Y_NOTIFICATION_SHOW && being_enabled && nit: I think you should just check "being_enabled != A11Y_NONE". Otherwise you're checking for 0 twice.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you so much for detailed review, James. https://codereview.chromium.org/2838903002/diff/220001/ash/system/tray_access... File ash/system/tray_accessibility.cc (right): https://codereview.chromium.org/2838903002/diff/220001/ash/system/tray_access... ash/system/tray_accessibility.cc:101: else if (enabled_accessibility & A11Y_SPOKEN_FEEDBACK) On 2017/05/19 04:11:09, James Cook wrote: > nit: no need for "else", here or below true, i only call this function if braille or spoken feedback is enabled. I don't really need the return gfx::kNoneIcon; I really start to focus on these little details. https://codereview.chromium.org/2838903002/diff/220001/ash/system/tray_access... ash/system/tray_accessibility.cc:454: // enabled or if a braille is connected. On 2017/05/19 04:11:10, James Cook wrote: > nit: a braille *display* Done.
The CQ bit was checked by yiyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2838903002/#ps240001 (title: "comments")
The CQ bit was unchecked by yiyix@chromium.org
yiyix@chromium.org changed reviewers: + stevenjb@chromium.org
@stevenjb, could you please review changes in chrome/browser? Thank you very much.
Hooray! Thanks for working on this! lgtm w/ one suggestion. https://codereview.chromium.org/2838903002/diff/240001/ash/system/tray_access... File ash/system/tray_accessibility.cc (right): https://codereview.chromium.org/2838903002/diff/240001/ash/system/tray_access... ash/system/tray_accessibility.cc:484: previous_accessibility_state_ = accessibility_state; Can we move this to line 449, then invert and early exit at line 452?
https://codereview.chromium.org/2838903002/diff/240001/ash/system/tray_access... File ash/system/tray_accessibility.cc (right): https://codereview.chromium.org/2838903002/diff/240001/ash/system/tray_access... ash/system/tray_accessibility.cc:484: previous_accessibility_state_ = accessibility_state; On 2017/05/19 20:18:34, stevenjb wrote: > Can we move this to line 449, then invert and early exit at line 452? Yes, it will be looking better that way. Thank you!
The CQ bit was checked by yiyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2838903002/#ps260001 (title: "nits")
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": 260001, "attempt_start_ts": 1495552262059410,
"parent_rev": "7e55c99e9413ad1689b59652ad7df1cadb792010", "commit_rev":
"2df25af11bbad658a261076cef020590b9988f9b"}
Message was sent while issue was closed.
Description was changed from ========== Add accessibility related notification into notification center When chromevox is enabled or the braille is connected, a notification will be shown to users through the notification center. The screenshot of the notifications can be found in the crbug. TEST=TrayAccessibilityTest.ShowNotification BUG=685845 ========== to ========== Add accessibility related notification into notification center When chromevox is enabled or the braille is connected, a notification will be shown to users through the notification center. The screenshot of the notifications can be found in the crbug. TEST=TrayAccessibilityTest.ShowNotification BUG=685845 Review-Url: https://codereview.chromium.org/2838903002 Cr-Commit-Position: refs/heads/master@{#473939} Committed: https://chromium.googlesource.com/chromium/src/+/2df25af11bbad658a261076cef02... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:260001) as https://chromium.googlesource.com/chromium/src/+/2df25af11bbad658a261076cef02... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
