Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2806093002:
Forward AXPlatformNodeWin calls to the right child. (Closed)
|
Created:
3 years, 8 months ago by dougt Modified:
3 years, 8 months ago Reviewers:
dmazzoni CC:
chromium-reviews, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, dougt+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAXPlatformNodeWin implements IAccessibility. Many of these MSAA methods take a
var_id parameter indicating that the operation should be performed on a
particular child ID, rather than the called object.
This CL mimics the approach taken by BrowserAccessibilityWin.
BUG=703369
Review-Url: https://codereview.chromium.org/2806093002
Cr-Commit-Position: refs/heads/master@{#465288}
Committed: https://chromium.googlesource.com/chromium/src/+/6a3aeb67d211a7c37fc2ce6355898d27fdfdad04
Patch Set 1 : Convert get_accChild #
Total comments: 2
Patch Set 2 : Forward to child #Patch Set 3 : Reverting to using existing error codes for failures. #Patch Set 4 : do not use IsValidId() when retargeting #
Total comments: 1
Patch Set 5 : rebase #Patch Set 6 : feedback from dmazzoni #
Messages
Total messages: 44 (36 generated)
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...
Description was changed from ========== Forward to child BUG= ========== to ========== AXPlatformNodeWin implements IAccessibility. Many of these MSAA methods take a var_id parameter indicating that the operation should be performed on a particular child ID, rather than the called object. This CL mimics the approach taken by BrowserAccessibilityWin. BUG=703369 ==========
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 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 #1 (id:1) has been deleted
dougt@chromium.org changed reviewers: + dmazzoni@chromium.org
ptal
https://codereview.chromium.org/2806093002/diff/20001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_win.cc (right): https://codereview.chromium.org/2806093002/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win.cc:259: auto* target = GetTargetFromChildID(var_id); What do you think about putting GetTargetFromChildID in the COM_OBJECT_VALIDATE_VAR_ID macro? auto* target = COM_OBJECT_VALIDATE_VAR_ID(var_id); That cuts down on the boilerplate a lot. https://codereview.chromium.org/2806093002/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win.cc:1234: return static_cast<AXPlatformNodeWin*>( Note that a child may not necessarily be an AXPlatformNodeWin, so this cast isn't safe. In particular, right now the child of an AXPlatformNodeWin may be a BrowserAccessibilityWin. Instead, use FromNativeViewAccessible(), which will check if the object is the right type and cast it if so. If you get a AXPlatformNodeBase, it is safe to cast that to an AXPlatformNodeWIn.
On 2017/04/10 07:20:20, dmazzoni wrote: > https://codereview.chromium.org/2806093002/diff/20001/ui/accessibility/platfo... > File ui/accessibility/platform/ax_platform_node_win.cc (right): > > https://codereview.chromium.org/2806093002/diff/20001/ui/accessibility/platfo... > ui/accessibility/platform/ax_platform_node_win.cc:259: auto* target = > GetTargetFromChildID(var_id); > What do you think about putting GetTargetFromChildID in the > COM_OBJECT_VALIDATE_VAR_ID macro? > > auto* target = COM_OBJECT_VALIDATE_VAR_ID(var_id); > > That cuts down on the boilerplate a lot. Hrm. We could do something like: #define COM_OBJECT_VALIDATE_VAR_ID(var_id) \ if (!delegate_) return E_FAIL; \ if (!IsValidId(var_id)) return E_INVALIDARG \ auto* target = GetTargetFromChildID(var_id); \ if (!target) return E_INVALIDARG; Which would declare |target| if you use that macro. I am not sure I like that sort of magic, but it does cut down on boilerplate. WDYT?
On 2017/04/10 20:02:54, dougt wrote: > On 2017/04/10 07:20:20, dmazzoni wrote: > > > https://codereview.chromium.org/2806093002/diff/20001/ui/accessibility/platfo... > > File ui/accessibility/platform/ax_platform_node_win.cc (right): > > > > > https://codereview.chromium.org/2806093002/diff/20001/ui/accessibility/platfo... > > ui/accessibility/platform/ax_platform_node_win.cc:259: auto* target = > > GetTargetFromChildID(var_id); > > What do you think about putting GetTargetFromChildID in the > > COM_OBJECT_VALIDATE_VAR_ID macro? > > > > auto* target = COM_OBJECT_VALIDATE_VAR_ID(var_id); > > > > That cuts down on the boilerplate a lot. > > Hrm. We could do something like: > > #define COM_OBJECT_VALIDATE_VAR_ID(var_id) \ > if (!delegate_) return E_FAIL; \ > if (!IsValidId(var_id)) return E_INVALIDARG \ > auto* target = GetTargetFromChildID(var_id); \ > if (!target) return E_INVALIDARG; > > Which would declare |target| if you use that macro. I am not sure I like that > sort of magic, but it does cut down on boilerplate. WDYT? I agree it should look less magic. How about something like: #define COM_OBJECT_VALIDATE_AND_GET_TARGET_FROM_VARID(var_id, target) \ if (!delegate_) return E_FAIL; \ if (!IsValidId(var_id)) return E_INVALIDARG \ target = GetTargetFromChildID(var_id); \ if (!target) return E_INVALIDARG; Then use it like this: AXPlatformNodeWin* target; COM_OBJECT_VALIDATE_AND_GET_TARGET_FROM_VARID(var_id, target); WDYT? In general macros should be avoided, but I think this is exactly the sort of place where they should be used. Another option would be to try to auto-generate this file from the IAccessible idl files.
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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #2 (id:40001) 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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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 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.
On 2017/04/11 02:48:12, dmazzoni wrote: > On 2017/04/10 20:02:54, dougt wrote: > > On 2017/04/10 07:20:20, dmazzoni wrote: > > > > > > https://codereview.chromium.org/2806093002/diff/20001/ui/accessibility/platfo... > > > File ui/accessibility/platform/ax_platform_node_win.cc (right): > > > > > > > > > https://codereview.chromium.org/2806093002/diff/20001/ui/accessibility/platfo... > > > ui/accessibility/platform/ax_platform_node_win.cc:259: auto* target = > > > GetTargetFromChildID(var_id); > > > What do you think about putting GetTargetFromChildID in the > > > COM_OBJECT_VALIDATE_VAR_ID macro? > > > > > > auto* target = COM_OBJECT_VALIDATE_VAR_ID(var_id); > > > > > > That cuts down on the boilerplate a lot. > > > > Hrm. We could do something like: > > > > #define COM_OBJECT_VALIDATE_VAR_ID(var_id) \ > > if (!delegate_) return E_FAIL; \ > > if (!IsValidId(var_id)) return E_INVALIDARG \ > > auto* target = GetTargetFromChildID(var_id); \ > > if (!target) return E_INVALIDARG; > > > > Which would declare |target| if you use that macro. I am not sure I like that > > sort of magic, but it does cut down on boilerplate. WDYT? > > I agree it should look less magic. > > How about something like: > > #define COM_OBJECT_VALIDATE_AND_GET_TARGET_FROM_VARID(var_id, target) \ > if (!delegate_) return E_FAIL; \ > if (!IsValidId(var_id)) return E_INVALIDARG \ > target = GetTargetFromChildID(var_id); \ > if (!target) return E_INVALIDARG; > > Then use it like this: > > AXPlatformNodeWin* target; > COM_OBJECT_VALIDATE_AND_GET_TARGET_FROM_VARID(var_id, target); > > WDYT? We need two different forms of the macros which isn't too bad. Also note that this new form does not call IsValidId() since we expect the var_id to resolve into a valid target (and if it doesn't, we treat that as an error).
lgtm https://codereview.chromium.org/2806093002/diff/100001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_win.cc (right): https://codereview.chromium.org/2806093002/diff/100001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_win.cc:67: #define COM_OBJECT_VALIDATE_VAR_ID(var_id) \ I think you can get rid of all of the COM_OBJECT_VALIDATE_VAR_ID formas that don't include AND_GET_TARGET. We shouldn't ever need them because if there's a VAR_ID, we must get the target. The only exception in the current code is something unimplemented. I'd rather just use the AND_GET_TARGET macro there.
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/2806093002/#ps140001 (title: "feedback from dmazzoni")
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": 140001, "attempt_start_ts": 1492538600402760,
"parent_rev": "341c0454a9e0c39895d2228e25a9fd57f919e16e", "commit_rev":
"6a3aeb67d211a7c37fc2ce6355898d27fdfdad04"}
Message was sent while issue was closed.
Description was changed from ========== AXPlatformNodeWin implements IAccessibility. Many of these MSAA methods take a var_id parameter indicating that the operation should be performed on a particular child ID, rather than the called object. This CL mimics the approach taken by BrowserAccessibilityWin. BUG=703369 ========== to ========== AXPlatformNodeWin implements IAccessibility. Many of these MSAA methods take a var_id parameter indicating that the operation should be performed on a particular child ID, rather than the called object. This CL mimics the approach taken by BrowserAccessibilityWin. BUG=703369 Review-Url: https://codereview.chromium.org/2806093002 Cr-Commit-Position: refs/heads/master@{#465288} Committed: https://chromium.googlesource.com/chromium/src/+/6a3aeb67d211a7c37fc2ce635589... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/6a3aeb67d211a7c37fc2ce635589... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
