Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1010083006:
Implement AtkComponent interface for chrome UI. (Closed)
|
Created:
5 years, 9 months ago by shreeramk Modified:
5 years, 7 months ago CC:
chromium-reviews, je_julie(Not used), plundblad+watch_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement AtkComponent interface for chrome UI.
Following functions has been implemented in this CL:
get_extents, get_size, get_position.
BUG=463671
Committed: https://crrev.com/90027a2ffbdf01ead6730f2e23d0bc9dcc814108
Cr-Commit-Position: refs/heads/master@{#327656}
Patch Set 1 #
Total comments: 2
Patch Set 2 : window coords #
Total comments: 15
Patch Set 3 : incorporating comments #
Total comments: 2
Patch Set 4 : #
Total comments: 1
Patch Set 5 : #
Total comments: 4
Patch Set 6 : fixing nits #
Messages
Total messages: 21 (3 generated)
PTAL. Need feedback. Not ready for commit. https://codereview.chromium.org/1010083006/diff/1/ui/accessibility/platform/a... File ui/accessibility/platform/ax_platform_node_auralinux.cc (right): https://codereview.chromium.org/1010083006/diff/1/ui/accessibility/platform/a... ui/accessibility/platform/ax_platform_node_auralinux.cc:468: // Not able to find any api to get screen coords for window Need some input here.
shreeram.k@samsung.com changed reviewers: + aboxhall@chromium.org, dmazzoni@chromium.org
https://codereview.chromium.org/1010083006/diff/1/ui/accessibility/platform/a... File ui/accessibility/platform/ax_platform_node_auralinux.cc (right): https://codereview.chromium.org/1010083006/diff/1/ui/accessibility/platform/a... ui/accessibility/platform/ax_platform_node_auralinux.cc:468: // Not able to find any api to get screen coords for window On 2015/03/26 05:25:15, shreeramk wrote: > Need some input here. Keep taking the parent of this object until you reach one that has a role of "window". Take the upper-left corner of that window object and subtract it from the coordinates of this object, and now you have window-relative coordinates.
Implemented calculation wrt window coords. PTAL. Thanks! https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_auralinux.cc (right): https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:478: *height -= window_coords.height(); This calculation is is not required for width n height. Will update this in next patch set.
Have you tested this? I don't see how the AtkComponent interface can be accessed. https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_auralinux.cc (right): https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:294: GetWindowCoords(); I don't think you need to call it here. https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:397: // ax_platform_node_auralinux AtkComponent interface implementation. This belongs up above, just before the comment "The rest of the AXPlatformNodeAuraLinux code" https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:400: static void ax_component_interface_get_extents(AtkComponent* atk_component, This should be named ax_platform_node_auralinux_get_extents for consistency. https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:440: iface->get_extents = ax_component_interface_get_extents; nit: only indent 2 chars https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:445: GType ax_component_auralinux_get_type() { Where is this called? I don't think we want a separate type for this. https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:506: AtkObjectToAXPlatformNodeAuraLinux(parent); You need to test this. It's possible that the parent is an AtkObject that isn't an AXPlatformNodeAuraLinux object. You should work with it as an AtkObject. https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:509: gfx::Rect window_coords = obj->GetData().location; Do this using its AtkComponent interface https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_auralinux.h (right): https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.h:32: gfx::Rect GetWindowCoords(); This should be private. Move it down under the private section below, and move the implementation in the source file to match (we try to have the order of functions in the header and file the same, though not everyone enforces this).
I haven't tested this yet. Rest other comments I would address in the next patchset. https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_auralinux.cc (right): https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:294: GetWindowCoords(); On 2015/03/27 19:09:14, dmazzoni wrote: > I don't think you need to call it here. Sorry, by mistake I wrote this. :( https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:445: GType ax_component_auralinux_get_type() { On 2015/03/27 19:09:14, dmazzoni wrote: > Where is this called? I don't think we want a separate type for this. Actually I don't know/understand like from where it'd be called or who would call this. I asked this doubt in the last mail.
Type this command to see an earlier version of ATK support in Chrome. You can use that as a guide for some things. git checkout 404d9a54 content/browser/accessibility/browser_accessibility_gtk.cc I think you're going to need to figure out how to get ATK support working in order to keep working on this. I don't know why it's not working for you. https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_auralinux.cc (right): https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:240: GType type = g_type_register_static( Immediately after this call, you should add a line of code like this: g_type_add_interface_static( type, ATK_TYPE_COMPONENT, &ComponentInfo); Then add something like this above it: static const GInterfaceInfo ComponentInfo = { reinterpret_cast<GInterfaceInitFunc>(ax_component_interface_base_init), 0, 0 }; Obviously ax_component_interface_base_init needs to be moved above that. Once you do that you shouldn't need ax_component_auralinux_get_type. https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:445: GType ax_component_auralinux_get_type() { On 2015/03/28 16:59:48, shreeramk wrote: > On 2015/03/27 19:09:14, dmazzoni wrote: > > Where is this called? I don't think we want a separate type for this. > > Actually I don't know/understand like from where it'd be called or who would > call this. I asked this doubt in the last mail. This is what you do when you define a new type. What you want here is to add an interface to an existing type using g_type_add_interface_static. See my comment above, then delete this function.
https://codereview.chromium.org/1010083006/diff/40001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_auralinux.cc (right): https://codereview.chromium.org/1010083006/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:481: gfx::Rect rect_size = GetData().location; I think better would be to move this calculations to ax_platform_node_auralinux_get_size, instead of creating a new method. And similarly for GetExtents & GetPosition. Need feedback on this.
https://codereview.chromium.org/1010083006/diff/40001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_auralinux.cc (right): https://codereview.chromium.org/1010083006/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:481: gfx::Rect rect_size = GetData().location; On 2015/03/31 14:47:10, shreeramk wrote: > I think better would be to move this calculations to > ax_platform_node_auralinux_get_size, instead of creating a new method. > And similarly for GetExtents & GetPosition. > Need feedback on this. No, I prefer it this way. The C functions (ax_platform_node_auralinux_get_size) should just be tiny wrappers that don't do any logic, they should just convert types and then call a C++ function. Thanks.
(Comment on PatchSet 2). https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_auralinux.cc (right): https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:506: AtkObjectToAXPlatformNodeAuraLinux(parent); On 2015/03/27 19:09:14, dmazzoni wrote: > You need to test this. It's possible that the parent is an AtkObject that isn't > an AXPlatformNodeAuraLinux object. You should work with it as an AtkObject. Already a check is made in AtkObjectToAXPlatformNodeAuraLinux to check whether its an IS_AX_PLATFORM_NODE_AURALINUX() object or not. Do you mean to put a check like this if(!obj) return..... Or, "You should work with it as an AtkObject." Did you mean to say - direclty use it as an AtkObject? But if I use it as an AtkObject then I won't be able to make below recursive call obj->GetWindowCoords();
https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_auralinux.cc (right): https://codereview.chromium.org/1010083006/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:506: AtkObjectToAXPlatformNodeAuraLinux(parent); On 2015/04/01 11:50:14, shreeramk wrote: > On 2015/03/27 19:09:14, dmazzoni wrote: > > You need to test this. It's possible that the parent is an AtkObject that > isn't > > an AXPlatformNodeAuraLinux object. You should work with it as an AtkObject. > > Already a check is made in AtkObjectToAXPlatformNodeAuraLinux to check whether > its an IS_AX_PLATFORM_NODE_AURALINUX() object or not. I think that's checking whether |this| is an AXPlatformNodeAuraLinux. I'm saying that the parent may not be one. > "You should work with it as an AtkObject." > > Did you mean to say - direclty use it as an AtkObject? > But if I use it as an AtkObject then I won't be able to make below recursive > call > obj->GetWindowCoords(); That's right, I think you need to call a helper function that recursively walks up the parents of an AtkObject and calls things like ax_component_interface_get_position on it. The situation you need to handle is that there's a GTK window, for example, which has its own AtkObject, and this object is a child of that. You can't call GetWindowCoords on the GTK window, but you can call ax_component_interface_get_position on it.
https://codereview.chromium.org/1010083006/diff/60001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_auralinux.cc (right): https://codereview.chromium.org/1010083006/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:486: gfx::Rect AXPlatformNodeAuraLinux::GetWindowCoords() { Sorry :( . I am not able to figure out how to implement this part. If you don't mind, could you please implement this?
PTAL. I haven't been able to verify since python script to dump tree for chrome UI is not working for me. Thanks.
PTAL. Thanks.
lgtm I tested it and it worked for me! https://codereview.chromium.org/1010083006/diff/80001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_auralinux.cc (right): https://codereview.chromium.org/1010083006/diff/80001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:189: static gfx::Rect findAtkObjectParentCoords(AtkObject* atk_object) { Nit: FindAtkObjectParentCoords Also, have it return a gfx::Point rather than a gfx::Rect with no width and height. https://codereview.chromium.org/1010083006/diff/80001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:194: gfx::Rect window_coords(x,y,0,0); nit: space after comma: (x, y, 0, 0) https://codereview.chromium.org/1010083006/diff/80001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:217: gint* x, gint* y, nit: indentation https://codereview.chromium.org/1010083006/diff/80001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_auralinux.cc:230: gint* width, gint* height) { nit: indentation
The CQ bit was checked by shreeram.k@samsung.com
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/1010083006/#ps100001 (title: "fixing nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1010083006/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/90027a2ffbdf01ead6730f2e23d0bc9dcc814108 Cr-Commit-Position: refs/heads/master@{#327656}
Message was sent while issue was closed.
Causes this (I'm on Debian jessie):
../../ui/accessibility/platform/ax_platform_node_auralinux.cc:193:5: error:
'atk_component_get_position' is deprecated [-Werror,-Wdeprecated-declarations]
atk_component_get_position(ATK_COMPONENT(atk_object),
^
/usr/include/atk-1.0/atk/atkcomponent.h:187:23: note:
'atk_component_get_position' has been explicitly marked deprecated here
void atk_component_get_position (AtkComponent
*component,
^
1 error generated.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
