Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2981053002:
Remove methods from BrowserAccessibilityComWin which aren't needed. (Closed)
|
Created:
3 years, 5 months ago by dougt Modified:
3 years, 5 months ago Reviewers:
dmazzoni CC:
chromium-reviews, aboxhall+watch_chromium.org, nektar+watch_chromium.org, jam, yuzo+watch_chromium.org, dougt+watch_chromium.org, darin-cc_chromium.org, aleventhal+watch_chromium.org, dtseng+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove methods from BrowserAccessibilityComWin which aren't needed.
This is the first larger patch where we can start removing methods from
BrowserAccessibilityComWin that are completely implemented by
AXPlatformNodeWin.
Note, I believe the owner() check that is used in BrowserAccessibilityComWin
is not needed anymore. This is because the AXPlatformNodeWin should be
testing for a valid delegate in all of the exposed COM interfaces.
Also note, that I have removed changing the accessibility mode flags
from some of the unimplemented methods. I do not believe that this will be
a problem since these methods aren't really used.
There will be a follow up that moves more over after we work out what
to do about handling of the mode flags.
BUG=703369
Review-Url: https://codereview.chromium.org/2981053002
Cr-Commit-Position: refs/heads/master@{#487738}
Committed: https://chromium.googlesource.com/chromium/src/+/4ab0dd0c1a9df25e89f2a5f18cf051d7f368eaa4
Patch Set 1 : Remove methods #
Total comments: 5
Patch Set 2 : rebase #
Depends on Patchset: Messages
Total messages: 32 (22 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by dougt@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.
dougt@chromium.org changed reviewers: + dmazzoni@chromium.org
dmazzoni, ptal.
Mostly looks fantastic, but I'm slightly confused about one thing... https://codereview.chromium.org/2981053002/diff/20001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_win.cc (right): https://codereview.chromium.org/2981053002/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win.cc:935: WIN_ACCESSIBILITY_API_HISTOGRAM(UMA_API_GET_EXTENDED_ROLE); Wait, how does this even compile if this macro is defined in browser_accessibility_com_win.cc?
On 2017/07/17 08:32:40, dmazzoni wrote: > Mostly looks fantastic, but I'm slightly confused about > one thing... Thank you. > https://codereview.chromium.org/2981053002/diff/20001/ui/accessibility/platfo... > File ui/accessibility/platform/ax_platform_node_win.cc (right): > > https://codereview.chromium.org/2981053002/diff/20001/ui/accessibility/platfo... > ui/accessibility/platform/ax_platform_node_win.cc:935: > WIN_ACCESSIBILITY_API_HISTOGRAM(UMA_API_GET_EXTENDED_ROLE); > Wait, how does this even compile if this macro is defined in > browser_accessibility_com_win.cc? You're right. This CL depends on 2981023002. It's in your queue now.
https://codereview.chromium.org/2981053002/diff/20001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_com_win.cc (left): https://codereview.chromium.org/2981053002/diff/20001/content/browser/accessi... content/browser/accessibility/browser_accessibility_com_win.cc:1020: if (!owner()) Is there ever a case where owner_ is not equal to delegate_? It seems like now that BrowserAccessibilityComWin inherits from AXPlatformNodeWin, we could replace owner() with a function that takes delegate_ and casts it to a BrowserAccessibilityWin*. Then it would be more clear that this check isn't needed. https://codereview.chromium.org/2981053002/diff/20001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_com_win.cc (right): https://codereview.chromium.org/2981053002/diff/20001/content/browser/accessi... content/browser/accessibility/browser_accessibility_com_win.cc:761: AddAccessibilityModeFlags(kScreenReaderAndHTMLAccessibilityModes); For a future change, perhaps we should set up something in ui/accessibility/platform that keeps track of calls to various accessibility APIs that trigger mode flags, but in a generic way - then BrowserAccessibilityState could register to be called when those triggers happen. Then we wouldn't need to override all of these methods at all, since that's the only remaining reason we're overriding them.
https://codereview.chromium.org/2981053002/diff/20001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_com_win.cc (left): https://codereview.chromium.org/2981053002/diff/20001/content/browser/accessi... content/browser/accessibility/browser_accessibility_com_win.cc:1020: if (!owner()) On 2017/07/17 17:05:36, dmazzoni wrote: > Is there ever a case where owner_ is not equal to delegate_? As I understand things, owner_ and delegate_ in this case are always a BrowserAccessibilityWin. > It seems like now > that > BrowserAccessibilityComWin inherits from AXPlatformNodeWin, we could replace > owner() with a function that takes delegate_ and casts it to a > BrowserAccessibilityWin*. > Then it would be more clear that this check isn't needed. My plan is to essentially to remove all of these methods when we sort out the BrowserAccessibilityState stuff. I want to be able to completely depend on AXPlatformNodeWin for dealing with a null delegate. https://codereview.chromium.org/2981053002/diff/20001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_com_win.cc (right): https://codereview.chromium.org/2981053002/diff/20001/content/browser/accessi... content/browser/accessibility/browser_accessibility_com_win.cc:761: AddAccessibilityModeFlags(kScreenReaderAndHTMLAccessibilityModes); On 2017/07/17 17:05:36, dmazzoni wrote: > For a future change, perhaps we should set up something in > ui/accessibility/platform that keeps track of calls to various accessibility > APIs that trigger mode flags, but in a generic way - then > BrowserAccessibilityState could register to be called when those > triggers happen. > > Then we wouldn't need to override all of these methods at all, > since that's the only remaining reason we're overriding them. YES!! Lets discuss how to do this soon.
lgtm Great!
The CQ bit was checked by dougt@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-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...)
The CQ bit was checked by dougt@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:40001) has been deleted
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...) 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 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 dougt@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 dougt@chromium.org
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/2981053002/#ps60001 (title: "rebase")
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": 60001, "attempt_start_ts": 1500434981853140,
"parent_rev": "35760c4838b3b510efcf8623982a1d72a7afca0e", "commit_rev":
"4ab0dd0c1a9df25e89f2a5f18cf051d7f368eaa4"}
Message was sent while issue was closed.
Description was changed from ========== Remove methods from BrowserAccessibilityComWin which aren't needed. This is the first larger patch where we can start removing methods from BrowserAccessibilityComWin that are completely implemented by AXPlatformNodeWin. Note, I believe the owner() check that is used in BrowserAccessibilityComWin is not needed anymore. This is because the AXPlatformNodeWin should be testing for a valid delegate in all of the exposed COM interfaces. Also note, that I have removed changing the accessibility mode flags from some of the unimplemented methods. I do not believe that this will be a problem since these methods aren't really used. There will be a follow up that moves more over after we work out what to do about handling of the mode flags. BUG=703369 ========== to ========== Remove methods from BrowserAccessibilityComWin which aren't needed. This is the first larger patch where we can start removing methods from BrowserAccessibilityComWin that are completely implemented by AXPlatformNodeWin. Note, I believe the owner() check that is used in BrowserAccessibilityComWin is not needed anymore. This is because the AXPlatformNodeWin should be testing for a valid delegate in all of the exposed COM interfaces. Also note, that I have removed changing the accessibility mode flags from some of the unimplemented methods. I do not believe that this will be a problem since these methods aren't really used. There will be a follow up that moves more over after we work out what to do about handling of the mode flags. BUG=703369 Review-Url: https://codereview.chromium.org/2981053002 Cr-Commit-Position: refs/heads/master@{#487738} Committed: https://chromium.googlesource.com/chromium/src/+/4ab0dd0c1a9df25e89f2a5f18cf0... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/4ab0dd0c1a9df25e89f2a5f18cf0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
