Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2447013009:
Implement Increment and Decrement accessible actions (Closed)
|
Created:
4 years, 1 month ago by dmazzoni Modified:
4 years, 1 month ago CC:
aboxhall+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, jam, je_julie, kalyank, mac-reviews_chromium.org, mlamouri+watch-content_chromium.org, nektar+watch_chromium.org, sadrul, yuzo+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement Increment and Decrement accessible actions
This is much easier now that we have the AXAction enum!
BUG=660227
Committed: https://crrev.com/1ca764663ddb69354f1de83396b1cd12205527c3
Cr-Commit-Position: refs/heads/master@{#430231}
Patch Set 1 #Patch Set 2 : Address feedback #Patch Set 3 : Rebase #Patch Set 4 : Rebase #Patch Set 5 : Rebase #Patch Set 6 : Change to make test pass on Android #
Messages
Total messages: 47 (34 generated)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
dmazzoni@chromium.org changed reviewers: + nektar@chromium.org
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...)
Only a few minor points.
class AccessibilityActionBrowserTest : public ContentBrowserTest {
I think the Style Guide says to put declarations that should not be visible
outside this file into an unnamed namespace to avoid potential name clashes.
+ BrowserAccessibility* FindNodeWithRoleAndNameHelper(
+ BrowserAccessibility* node,
+ ui::AXRole role,
+ const std::string& name) {
Nit: Would you mind modifying the names a bit, making the input node a
reference and moving this to the private section?
+ BrowserAccessibility* FindNodeInSubTree(
+ BrowserAccessibility& root,
+ ui::AXRole role,
+ const std::string& name) {
+ if (node->GetRole() == role && ...
Shouldn't we check if node != nullptr?
Could you also modify the name of the primary function?
+ BrowserAccessibility* FindNode(
+ ui::AXRole role,
+ const std::string& name) {
+ BrowserAccessibility* target = FindNodeWithRoleAndName(
+ ui::AX_ROLE_BUTTON, "One");
+ CHECK(target);
Shouldn't it be ASERT_NE(nullptr, target)? Same in other test cases.
+ GetManager()->SetFocus(*target);
Shouldn't we assert that manager is not nullptr above? Same in other test cases.
+ // Increment, should result in value staying the same (max)
Nit: Put a period at the end of the comment. Same for decrement comment.
+ // Accessibility actions. All of these are implemented asynchronously
+ // by sending a message to the renderer to perform the respective action
+ // on the given node. See the definition of ui::AXActionData for more
Nit: Enclose ui::AXActionData in |s.
lgtm
On 2016/10/28 16:49:30, nektarios wrote:
> Only a few minor points.
>
> class AccessibilityActionBrowserTest : public ContentBrowserTest {
> I think the Style Guide says to put declarations that should not be visible
> outside this file into an unnamed namespace to avoid potential name clashes.
Done.
> + BrowserAccessibility* FindNodeWithRoleAndNameHelper(
> + BrowserAccessibility* node,
> + ui::AXRole role,
> + const std::string& name) {
> Nit: Would you mind modifying the names a bit, making the input node a
> reference and moving this to the private section?
Done.
> + BrowserAccessibility* FindNodeInSubTree(
> + BrowserAccessibility& root,
> + ui::AXRole role,
> + const std::string& name) {
>
> + if (node->GetRole() == role && ...
> Shouldn't we check if node != nullptr?
Made a reference, so not needed anymore
> Could you also modify the name of the primary function?
>
> + BrowserAccessibility* FindNode(
> + ui::AXRole role,
> + const std::string& name) {
Done
> + BrowserAccessibility* target = FindNodeWithRoleAndName(
> + ui::AX_ROLE_BUTTON, "One");
> + CHECK(target);
> Shouldn't it be ASERT_NE(nullptr, target)? Same in other test cases.
Done
> + GetManager()->SetFocus(*target);
> Shouldn't we assert that manager is not nullptr above? Same in other test
cases.
There's no reason it should be null, though. If it was null for some reason the
test would certainly crash.
> + // Increment, should result in value staying the same (max)
> Nit: Put a period at the end of the comment. Same for decrement comment.
Done
> + // Accessibility actions. All of these are implemented asynchronously
> + // by sending a message to the renderer to perform the respective action
> + // on the given node. See the definition of ui::AXActionData for more
> Nit: Enclose ui::AXActionData in |s.
Done
The CQ bit was checked by dmazzoni@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...
dmazzoni@chromium.org changed reviewers: + phajdan.jr@chromium.org
+phajdan.jr to add a new test to content/test/BUILD.gn
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by dmazzoni@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...)
LGTM
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nektar@chromium.org Link to the patchset: https://codereview.chromium.org/2447013009/#ps40001 (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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dmazzoni@chromium.org
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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_...)
The CQ bit was checked by dmazzoni@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by dmazzoni@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by dmazzoni@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 dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nektar@chromium.org, phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/2447013009/#ps100001 (title: "Change to make test pass on Android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Implement Increment and Decrement accessible actions This is much easier now that we have the AXAction enum! BUG=660227 ========== to ========== Implement Increment and Decrement accessible actions This is much easier now that we have the AXAction enum! BUG=660227 Committed: https://crrev.com/1ca764663ddb69354f1de83396b1cd12205527c3 Cr-Commit-Position: refs/heads/master@{#430231} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1ca764663ddb69354f1de83396b1cd12205527c3 Cr-Commit-Position: refs/heads/master@{#430231} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
