Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2453583003:
Clean up two AccessibilityNodeInfo.addAction APIs (Closed)
|
Created:
4 years, 1 month ago by dmazzoni Modified:
4 years, 1 month ago Reviewers:
sgurun-gerrit only CC:
chromium-reviews, aboxhall+watch_chromium.org, nektar+watch_chromium.org, jam, yuzo+watch_chromium.org, je_julie, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClean up two AccessibilityNodeInfo.addAction APIs
The interface to AccessibilityNodeInfo.addAction changed in the L SDK.
Rather than duplicating a large method twice (and forgetting to keep them
in sync), have one method that adds actions to a node, and a small helper
with two implementations, one for L and one for pre-L.
BUG=656104
Committed: https://crrev.com/c9e5aa6d8c9b24910dce0ab1de29e045555ba5ec
Cr-Commit-Position: refs/heads/master@{#427737}
Patch Set 1 #
Total comments: 4
Messages
Total messages: 15 (7 generated)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
dmazzoni@chromium.org changed reviewers: + sgurun@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2453583003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java (right): https://codereview.chromium.org/2453583003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java:23: private SparseArray<AccessibilityAction> mAccessibilityActionMap = does this really help for reducing the # of objects created?
one more forgotten comment. https://codereview.chromium.org/2453583003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java (left): https://codereview.chromium.org/2453583003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java:113: protected void addAccessibilityNodeInfoActions(AccessibilityNodeInfo node, I think there is a behavior change here, this method does not add ACTION_SHOW_ON_SCREEN (and similar ones). After your changes, it seems to me that they will be added for LollippopBrowserAccessibilityManager, is this what you want?
https://codereview.chromium.org/2453583003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java (left): https://codereview.chromium.org/2453583003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java:113: protected void addAccessibilityNodeInfoActions(AccessibilityNodeInfo node, On 2016/10/26 17:09:47, sgurun wrote: > I think there is a behavior change here, this method does not add > ACTION_SHOW_ON_SCREEN (and similar ones). After your changes, it seems to me > that they will be added for LollippopBrowserAccessibilityManager, is this what > you want? Yeah, so it was a bug that ACTION_SHOW_ON_SCREEN wasn't added for LollipopBrowserAccessibilityManager. Because there were two different versions of the same method I forgot to update both, which suggested I should refactor it like this so that there's less duplication. https://codereview.chromium.org/2453583003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java (right): https://codereview.chromium.org/2453583003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java:23: private SparseArray<AccessibilityAction> mAccessibilityActionMap = On 2016/10/26 17:05:46, sgurun wrote: > does this really help for reducing the # of objects created? Are you asking about SparseArray vs HashMap? Or why we need some sort of map at all? SparseArray was suggested by a lint check The reason we have a map at all is because we end up passing the same objects every time addAccessibilityNodeInfoActions is called, I think it's definitely useful to only create the actions once.
On 2016/10/26 17:28:14, dmazzoni wrote: > https://codereview.chromium.org/2453583003/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java > (left): > > https://codereview.chromium.org/2453583003/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java:113: > protected void addAccessibilityNodeInfoActions(AccessibilityNodeInfo node, > On 2016/10/26 17:09:47, sgurun wrote: > > I think there is a behavior change here, this method does not add > > ACTION_SHOW_ON_SCREEN (and similar ones). After your changes, it seems to me > > that they will be added for LollippopBrowserAccessibilityManager, is this what > > you want? > > Yeah, so it was a bug that ACTION_SHOW_ON_SCREEN wasn't > added for LollipopBrowserAccessibilityManager. Because there > were two different versions of the same method I forgot to > update both, which suggested I should refactor it like this > so that there's less duplication. > > https://codereview.chromium.org/2453583003/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java > (right): > > https://codereview.chromium.org/2453583003/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java:23: > private SparseArray<AccessibilityAction> mAccessibilityActionMap = > On 2016/10/26 17:05:46, sgurun wrote: > > does this really help for reducing the # of objects created? > > Are you asking about SparseArray vs HashMap? Or why we need some sort of map at > all? > > SparseArray was suggested by a lint check > > The reason we have a map at all is because we end up passing the same objects > every time addAccessibilityNodeInfoActions is called, I think it's definitely > useful to only create the actions once. lgtm
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...
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Clean up two AccessibilityNodeInfo.addAction APIs The interface to AccessibilityNodeInfo.addAction changed in the L SDK. Rather than duplicating a large method twice (and forgetting to keep them in sync), have one method that adds actions to a node, and a small helper with two implementations, one for L and one for pre-L. BUG=656104 ========== to ========== Clean up two AccessibilityNodeInfo.addAction APIs The interface to AccessibilityNodeInfo.addAction changed in the L SDK. Rather than duplicating a large method twice (and forgetting to keep them in sync), have one method that adds actions to a node, and a small helper with two implementations, one for L and one for pre-L. BUG=656104 Committed: https://crrev.com/c9e5aa6d8c9b24910dce0ab1de29e045555ba5ec Cr-Commit-Position: refs/heads/master@{#427737} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/c9e5aa6d8c9b24910dce0ab1de29e045555ba5ec Cr-Commit-Position: refs/heads/master@{#427737} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
