Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
667713006:
Implement automatic load of composed/embedded automation trees (Closed)
|
Created:
6 years, 1 month ago by David Tseng Modified:
6 years, 1 month ago CC:
chromium-reviews, extensions-reviews_chromium.org, sadrul, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, tfarina, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, yuzo+watch_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, kalyank, chromium-apps-reviews_chromium.org, ben+ash_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkcr Project:
chromium Visibility:
Public. |
DescriptionImplement automatic load of composed/embedded automation trees
Introduces the metadata required to link together the desktop tree and a views::WebView. Additionally, implements the js side logic to resolve these links.
A views::WebView now gets assigned an AXRole of webView and its immediate child resolves to the rootWebArea it hosts. This resolution is done dynamically at runtime using the underlying AX tree id which itself is mapped using AXTreeIDRegistry back in the browser. If the hosting webView has yet to load its child rootWebArea, enableFrame will be called to and a callback waiting for the subroot's data; this auto load occurs as soon as a webView is seen during a tree update and results in a childrenChanged event once its child rootWebArea is available. When a caller asks for the parent of a rootWebArea, a dynamic lookup is also performed to retrieve a hosting webView, if any.
Committed: https://crrev.com/ad8ae0f7f45d921efd8da77ffecbcd7253c87870
Cr-Commit-Position: refs/heads/master@{#302648}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Address comments. #Patch Set 3 : #
Total comments: 15
Patch Set 4 : Address comments. #Patch Set 5 : Variant with load() and AutomationnRootNode interface intact. #Patch Set 6 : #
Total comments: 1
Patch Set 7 : Get test to pass. #
Total comments: 7
Patch Set 8 : #
Total comments: 6
Patch Set 9 : indent, rename, map win role #Patch Set 10 : #
Total comments: 26
Patch Set 11 : indexInParent #Patch Set 12 : #Patch Set 13 : #
Total comments: 1
Patch Set 14 : #Patch Set 15 : json #Patch Set 16 : bad export #Patch Set 17 : Fix UAF #Messages
Total messages: 65 (25 generated)
dtseng@chromium.org changed reviewers: + aboxhall@chromium.org, dmazzoni@chromium.org
WIP. Some illegal includes ui -> content in here which I'd like some feedback on/suggestions to avoid. Punting on the tree id issue as had some trouble getting this to work at all. Approach/strategy feedback welcome; had to also move some api definitions because our unserialization logic doesn't have role at the time of object creation.
Great first draft! More comments coming. https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... File ui/views/accessibility/ax_view_obj_wrapper.cc (right): https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... ui/views/accessibility/ax_view_obj_wrapper.cc:52: view_data.role = ui::AX_ROLE_WEB_AREA; Why not just fix this in WebView? Is the WebView a web area, or is it a host to a web area? If it's a host, maybe it should have a role like IFRAME or FRAME, or maybe even a new role like WEB_VIEW?
https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:179: content::RenderFrameHost::FromID(params->process_id, I think this is what we want eventually, but aren't the routing ids used in the automation id right now RenderView routing ids and not RenderFrame ids? It seems confusing to mix them. https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:184: content::WebContents* contents = nit: indent https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/automation_internal/automation_internal_api.h (right): https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.h:63: DECLARE_EXTENSION_FUNCTION("automationInternal.enableRenderer", How about enableFrame or enableWebFrame? Renderer is a bit of an overloaded term in the Chromium codebase, it doesn't feel very precise. https://codereview.chromium.org/667713006/diff/1/chrome/browser/ui/ash/access... File chrome/browser/ui/ash/accessibility/automation_manager_ash.cc (right): https://codereview.chromium.org/667713006/diff/1/chrome/browser/ui/ash/access... chrome/browser/ui/ash/accessibility/automation_manager_ash.cc:46: current_tree_serializer_->Reset(); Why is this needed? https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/automation.idl:290: boolean loaded; Should this be boolean? so that it can be undefined on other nodes? https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/automation.idl:294: static void load(RootCallback callback); can a method be optional? https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/automation_internal.idl (right): https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/automation_internal.idl:101: // Enable automation of the renderer with the given process id and routing Change renderer to Frame or WebContents or whatever specifically this applies to. Looking towards the future, I think this will just give you one frame, right? Yes, accessibility will be enabled for the whole WebContents, but you'll only be subscribed to one frame and not subframes by default. https://codereview.chromium.org/667713006/diff/1/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/automation/tests/desktop/load_tabs.js (right): https://codereview.chromium.org/667713006/diff/1/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/automation/tests/desktop/load_tabs.js:9: nodes.push(node); nit: indent https://codereview.chromium.org/667713006/diff/1/content/public/browser/brows... File content/public/browser/browser_accessibility_state.h (right): https://codereview.chromium.org/667713006/diff/1/content/public/browser/brows... content/public/browser/browser_accessibility_state.h:39: // Disables automation for all running tabs. (Only if accessibility is not nit: finish comment https://codereview.chromium.org/667713006/diff/1/ui/accessibility/ax_enums.idl File ui/accessibility/ax_enums.idl (right): https://codereview.chromium.org/667713006/diff/1/ui/accessibility/ax_enums.id... ui/accessibility/ax_enums.idl:299: // Internal only; do not expose to platform APIs. I don't think we should put these here. Let's add ax_frame_id that specifies the frame id of an axtree, and ax_child_frame_id that specifies the id of a child axtree contained within this tree. https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... File ui/views/accessibility/ax_view_obj_wrapper.cc (right): https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... ui/views/accessibility/ax_view_obj_wrapper.cc:8: #include "content/public/browser/render_process_host.h" Yeah, you can't include these from here. I think the best place to put this added code is in AXTreeSourceAsh. AXTreeSourceAsh::SerializeNode, just call node->Serialize(out_data) but then do some postprocessing there to handle web areas.
Also, added a test to demonstrate events work across trees. On Tue 28 Oct 2014 05:14:11 PM PDT, dmazzoni@chromium.org wrote: > Great first draft! More comments coming. > > > > https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... > File ui/views/accessibility/ax_view_obj_wrapper.cc (right): > > https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... > ui/views/accessibility/ax_view_obj_wrapper.cc:52: view_data.role = > ui::AX_ROLE_WEB_AREA; > Why not just fix this in WebView? There are possibly implications for other platforms; IIRC, VoiceOver expects a web area role to have some specific children (e.g. scrollbars). > > Is the WebView a web area, or is it a host to a web area? If it's a > host, maybe it should have a role like IFRAME or FRAME, or maybe even a > new role like WEB_VIEW? The original tree just has a client as a child with no other descendants, so I suppose it's a host? > > https://codereview.chromium.org/667713006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PTAL On Tue 28 Oct 2014 11:53:30 PM PDT, dmazzoni@chromium.org wrote: > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... > File > chrome/browser/extensions/api/automation_internal/automation_internal_api.cc > (right): > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... > chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:179: > content::RenderFrameHost::FromID(params->process_id, > I think this is what we want eventually, but aren't the routing ids used > in the automation id right now RenderView routing ids and not > RenderFrame ids? It seems confusing to mix them. Yeah; we're mixing them already. enableTab uses rfh's routing id and that's what I'm using to do the lookup when moving from host webArea to rootWebArea. > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... > chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:184: > content::WebContents* contents = > nit: indent > Done. > https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... > File > chrome/browser/extensions/api/automation_internal/automation_internal_api.h > (right): > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... > chrome/browser/extensions/api/automation_internal/automation_internal_api.h:63: > DECLARE_EXTENSION_FUNCTION("automationInternal.enableRenderer", > How about enableFrame or enableWebFrame? Renderer is a bit of an > overloaded term in the Chromium codebase, it doesn't feel very precise. > True; done. > https://codereview.chromium.org/667713006/diff/1/chrome/browser/ui/ash/access... > File chrome/browser/ui/ash/accessibility/automation_manager_ash.cc > (right): > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/ui/ash/access... > chrome/browser/ui/ash/accessibility/automation_manager_ash.cc:46: > current_tree_serializer_->Reset(); > Why is this needed? > Good catch; It's not; done during testing; removed. > https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... > File chrome/common/extensions/api/automation.idl (right): > > https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/automation.idl:290: boolean loaded; > Should this be boolean? so that it can be undefined on other nodes? Done. > > https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/automation.idl:294: static void > load(RootCallback callback); > can a method be optional? > Don't think so; compiler complained when I tried suffix ?. > https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... > File chrome/common/extensions/api/automation_internal.idl (right): > > https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/automation_internal.idl:101: // Enable > automation of the renderer with the given process id and routing > Change renderer to Frame or WebContents or whatever specifically this > applies to. > > Looking towards the future, I think this will just give you one frame, > right? Yes, accessibility will be enabled for the whole WebContents, but > you'll only be subscribed to one frame and not subframes by default. > Well, the subscription is via WebContentsObserver, so I think it's both frames and subframes. perhaps on the whole conte > https://codereview.chromium.org/667713006/diff/1/chrome/test/data/extensions/... > File > chrome/test/data/extensions/api_test/automation/tests/desktop/load_tabs.js > (right): > > https://codereview.chromium.org/667713006/diff/1/chrome/test/data/extensions/... > chrome/test/data/extensions/api_test/automation/tests/desktop/load_tabs.js:9: > nodes.push(node); > nit: indent Done.> > https://codereview.chromium.org/667713006/diff/1/content/public/browser/brows... > File content/public/browser/browser_accessibility_state.h (right): > > https://codereview.chromium.org/667713006/diff/1/content/public/browser/brows... > content/public/browser/browser_accessibility_state.h:39: // Disables > automation for all running tabs. (Only if accessibility is not > nit: finish comment > I'm removing these changes since turning on automation requires we register observers on all web contents including newly created ones. The original idea was to not have to explicitly load() every subframe, but it turns out that it's easier to have the client call load() and it's probably better for performance/memory. > https://codereview.chromium.org/667713006/diff/1/ui/accessibility/ax_enums.idl > File ui/accessibility/ax_enums.idl (right): > > https://codereview.chromium.org/667713006/diff/1/ui/accessibility/ax_enums.id... > ui/accessibility/ax_enums.idl:299: // Internal only; do not expose to > platform APIs. > I don't think we should put these here. > > Let's add ax_frame_id that specifies the frame id of an axtree, and > ax_child_frame_id that specifies the id of a child axtree contained > within this tree. So, these ids would (for now) be process id and routing id under the hood? Done. > https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... > File ui/views/accessibility/ax_view_obj_wrapper.cc (right): > > https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... > ui/views/accessibility/ax_view_obj_wrapper.cc:8: #include > "content/public/browser/render_process_host.h" > Yeah, you can't include these from here. > > I think the best place to put this added code is in AXTreeSourceAsh. > > AXTreeSourceAsh::SerializeNode, just call node->Serialize(out_data) but > then do some postprocessing there to handle web areas. > Yeah; that's breaking the abstraction of ax objects to View, but I guess we can do that. Any other suggestions? > https://codereview.chromium.org/667713006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PTAL On Tue 28 Oct 2014 11:53:30 PM PDT, dmazzoni@chromium.org wrote: > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... > File > chrome/browser/extensions/api/automation_internal/automation_internal_api.cc > (right): > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... > chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:179: > content::RenderFrameHost::FromID(params->process_id, > I think this is what we want eventually, but aren't the routing ids used > in the automation id right now RenderView routing ids and not > RenderFrame ids? It seems confusing to mix them. Yeah; we're mixing them already. enableTab uses rfh's routing id and that's what I'm using to do the lookup when moving from host webArea to rootWebArea. > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... > chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:184: > content::WebContents* contents = > nit: indent > Done. > https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... > File > chrome/browser/extensions/api/automation_internal/automation_internal_api.h > (right): > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... > chrome/browser/extensions/api/automation_internal/automation_internal_api.h:63: > DECLARE_EXTENSION_FUNCTION("automationInternal.enableRenderer", > How about enableFrame or enableWebFrame? Renderer is a bit of an > overloaded term in the Chromium codebase, it doesn't feel very precise. > True; done. > https://codereview.chromium.org/667713006/diff/1/chrome/browser/ui/ash/access... > File chrome/browser/ui/ash/accessibility/automation_manager_ash.cc > (right): > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/ui/ash/access... > chrome/browser/ui/ash/accessibility/automation_manager_ash.cc:46: > current_tree_serializer_->Reset(); > Why is this needed? > Good catch; It's not; done during testing; removed. > https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... > File chrome/common/extensions/api/automation.idl (right): > > https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/automation.idl:290: boolean loaded; > Should this be boolean? so that it can be undefined on other nodes? Done. > > https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/automation.idl:294: static void > load(RootCallback callback); > can a method be optional? > Don't think so; compiler complained when I tried suffix ?. > https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... > File chrome/common/extensions/api/automation_internal.idl (right): > > https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/automation_internal.idl:101: // Enable > automation of the renderer with the given process id and routing > Change renderer to Frame or WebContents or whatever specifically this > applies to. > > Looking towards the future, I think this will just give you one frame, > right? Yes, accessibility will be enabled for the whole WebContents, but > you'll only be subscribed to one frame and not subframes by default. > Well, the subscription is via WebContentsObserver, so I think it's both frames and subframes. perhaps on the whole conte > https://codereview.chromium.org/667713006/diff/1/chrome/test/data/extensions/... > File > chrome/test/data/extensions/api_test/automation/tests/desktop/load_tabs.js > (right): > > https://codereview.chromium.org/667713006/diff/1/chrome/test/data/extensions/... > chrome/test/data/extensions/api_test/automation/tests/desktop/load_tabs.js:9: > nodes.push(node); > nit: indent Done.> > https://codereview.chromium.org/667713006/diff/1/content/public/browser/brows... > File content/public/browser/browser_accessibility_state.h (right): > > https://codereview.chromium.org/667713006/diff/1/content/public/browser/brows... > content/public/browser/browser_accessibility_state.h:39: // Disables > automation for all running tabs. (Only if accessibility is not > nit: finish comment > I'm removing these changes since turning on automation requires we register observers on all web contents including newly created ones. The original idea was to not have to explicitly load() every subframe, but it turns out that it's easier to have the client call load() and it's probably better for performance/memory. > https://codereview.chromium.org/667713006/diff/1/ui/accessibility/ax_enums.idl > File ui/accessibility/ax_enums.idl (right): > > https://codereview.chromium.org/667713006/diff/1/ui/accessibility/ax_enums.id... > ui/accessibility/ax_enums.idl:299: // Internal only; do not expose to > platform APIs. > I don't think we should put these here. > > Let's add ax_frame_id that specifies the frame id of an axtree, and > ax_child_frame_id that specifies the id of a child axtree contained > within this tree. So, these ids would (for now) be process id and routing id under the hood? Done. > https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... > File ui/views/accessibility/ax_view_obj_wrapper.cc (right): > > https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... > ui/views/accessibility/ax_view_obj_wrapper.cc:8: #include > "content/public/browser/render_process_host.h" > Yeah, you can't include these from here. > > I think the best place to put this added code is in AXTreeSourceAsh. > > AXTreeSourceAsh::SerializeNode, just call node->Serialize(out_data) but > then do some postprocessing there to handle web areas. > Yeah; that's breaking the abstraction of ax objects to View, but I guess we can do that. Any other suggestions? It's ok as is I suppose. > https://codereview.chromium.org/667713006/ > On Wed, Oct 29, 2014 at 9:02 AM, <dtseng@chromium.org> wrote: > Also, added a test to demonstrate events work across trees. > > On Tue 28 Oct 2014 05:14:11 PM PDT, dmazzoni@chromium.org wrote: > > > Great first draft! More comments coming. > > > > > > > > > https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... > > File ui/views/accessibility/ax_view_obj_wrapper.cc (right): > > > > > https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... > > ui/views/accessibility/ax_view_obj_wrapper.cc:52: view_data.role = > > ui::AX_ROLE_WEB_AREA; > > Why not just fix this in WebView? > > There are possibly implications for other platforms; IIRC, VoiceOver > expects a web area role to have some specific children (e.g. scrollbars). > > > > Is the WebView a web area, or is it a host to a web area? If it's a > > host, maybe it should have a role like IFRAME or FRAME, or maybe even a > > new role like WEB_VIEW? > > The original tree just has a client as a child with no other > descendants, so I suppose it's a host? > > > > > https://codereview.chromium.org/667713006/ > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Looking - sorry for slowness, still feeling a bit under the weather and trying to get my thoughts together before I send out a review. On Wed, Oct 29, 2014 at 11:31 AM, David Tseng <dtseng@chromium.org> wrote: > PTAL > > > On Tue 28 Oct 2014 11:53:30 PM PDT, dmazzoni@chromium.org wrote: > > > > > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... > > File > > > chrome/browser/extensions/api/automation_internal/automation_internal_api.cc > > (right): > > > > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... > > > chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:179: > > content::RenderFrameHost::FromID(params->process_id, > > I think this is what we want eventually, but aren't the routing ids used > > in the automation id right now RenderView routing ids and not > > RenderFrame ids? It seems confusing to mix them. > > Yeah; we're mixing them already. enableTab uses rfh's routing id and > that's what I'm using to do the lookup when moving from host webArea to > rootWebArea. > > > > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... > > > chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:184: > > content::WebContents* contents = > > nit: indent > > > Done. > > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... > > File > > > chrome/browser/extensions/api/automation_internal/automation_internal_api.h > > (right): > > > > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/extensions/ap... > > > chrome/browser/extensions/api/automation_internal/automation_internal_api.h:63: > > DECLARE_EXTENSION_FUNCTION("automationInternal.enableRenderer", > > How about enableFrame or enableWebFrame? Renderer is a bit of an > > overloaded term in the Chromium codebase, it doesn't feel very precise. > > > > True; done. > > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/ui/ash/access... > > File chrome/browser/ui/ash/accessibility/automation_manager_ash.cc > > (right): > > > > > https://codereview.chromium.org/667713006/diff/1/chrome/browser/ui/ash/access... > > chrome/browser/ui/ash/accessibility/automation_manager_ash.cc:46: > > current_tree_serializer_->Reset(); > > Why is this needed? > > > Good catch; It's not; done during testing; removed. > > > https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... > > File chrome/common/extensions/api/automation.idl (right): > > > > > https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... > > chrome/common/extensions/api/automation.idl:290: boolean loaded; > > Should this be boolean? so that it can be undefined on other nodes? > > Done. > > > > > https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... > > chrome/common/extensions/api/automation.idl:294: static void > > load(RootCallback callback); > > can a method be optional? > > > Don't think so; compiler complained when I tried suffix ?. > > > https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... > > File chrome/common/extensions/api/automation_internal.idl (right): > > > > > https://codereview.chromium.org/667713006/diff/1/chrome/common/extensions/api... > > chrome/common/extensions/api/automation_internal.idl:101: // Enable > > automation of the renderer with the given process id and routing > > Change renderer to Frame or WebContents or whatever specifically this > > applies to. > > > > Looking towards the future, I think this will just give you one frame, > > right? Yes, accessibility will be enabled for the whole WebContents, but > > you'll only be subscribed to one frame and not subframes by default. > > > Well, the subscription is via WebContentsObserver, so I think it's > both frames and subframes. > perhaps on the whole conte > > > https://codereview.chromium.org/667713006/diff/1/chrome/test/data/extensions/... > > File > > > chrome/test/data/extensions/api_test/automation/tests/desktop/load_tabs.js > > (right): > > > > > https://codereview.chromium.org/667713006/diff/1/chrome/test/data/extensions/... > > > chrome/test/data/extensions/api_test/automation/tests/desktop/load_tabs.js:9: > > nodes.push(node); > > nit: indent > > Done.> > > > https://codereview.chromium.org/667713006/diff/1/content/public/browser/brows... > > File content/public/browser/browser_accessibility_state.h (right): > > > > > https://codereview.chromium.org/667713006/diff/1/content/public/browser/brows... > > content/public/browser/browser_accessibility_state.h:39: // Disables > > automation for all running tabs. (Only if accessibility is not > > nit: finish comment > > > I'm removing these changes since turning on automation requires we > register observers on all web contents including newly created > ones. The original idea was to not have to explicitly load() every > subframe, but it turns out that it's easier to have the client call > load() and it's probably better for performance/memory. > > > > https://codereview.chromium.org/667713006/diff/1/ui/accessibility/ax_enums.idl > > File ui/accessibility/ax_enums.idl (right): > > > > > https://codereview.chromium.org/667713006/diff/1/ui/accessibility/ax_enums.id... > > ui/accessibility/ax_enums.idl:299: // Internal only; do not expose to > > platform APIs. > > I don't think we should put these here. > > > > Let's add ax_frame_id that specifies the frame id of an axtree, and > > ax_child_frame_id that specifies the id of a child axtree contained > > within this tree. > > > So, these ids would (for now) be process id and routing id under the > hood? Done. > > > https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... > > File ui/views/accessibility/ax_view_obj_wrapper.cc (right): > > > > > https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... > > ui/views/accessibility/ax_view_obj_wrapper.cc:8: #include > > "content/public/browser/render_process_host.h" > > Yeah, you can't include these from here. > > > > I think the best place to put this added code is in AXTreeSourceAsh. > > > > AXTreeSourceAsh::SerializeNode, just call node->Serialize(out_data) but > > then do some postprocessing there to handle web areas. > > > Yeah; that's breaking the abstraction of ax objects to View, but I > guess we can do that. Any other suggestions? It's ok as is I suppose. > > https://codereview.chromium.org/667713006/ > > > > On Wed, Oct 29, 2014 at 9:02 AM, <dtseng@chromium.org> wrote: > >> Also, added a test to demonstrate events work across trees. >> >> On Tue 28 Oct 2014 05:14:11 PM PDT, dmazzoni@chromium.org wrote: >> >> > Great first draft! More comments coming. >> > >> > >> > >> > >> https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... >> > File ui/views/accessibility/ax_view_obj_wrapper.cc (right): >> > >> > >> https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... >> > ui/views/accessibility/ax_view_obj_wrapper.cc:52: view_data.role = >> > ui::AX_ROLE_WEB_AREA; >> > Why not just fix this in WebView? >> >> There are possibly implications for other platforms; IIRC, VoiceOver >> expects a web area role to have some specific children (e.g. scrollbars). >> > >> > Is the WebView a web area, or is it a host to a web area? If it's a >> > host, maybe it should have a role like IFRAME or FRAME, or maybe even a >> > new role like WEB_VIEW? >> >> The original tree just has a client as a child with no other >> descendants, so I suppose it's a host? >> >> > >> > https://codereview.chromium.org/667713006/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
First thoughts. This is generally very exciting, but I wonder if we can do things a bit more simply on the JS side. https://codereview.chromium.org/667713006/diff/1/ui/accessibility/ax_enums.idl File ui/accessibility/ax_enums.idl (right): https://codereview.chromium.org/667713006/diff/1/ui/accessibility/ax_enums.id... ui/accessibility/ax_enums.idl:299: // Internal only; do not expose to platform APIs. On 2014/10/29 06:53:30, dmazzoni wrote: > I don't think we should put these here. > > Let's add ax_frame_id that specifies the frame id of an axtree, and > ax_child_frame_id that specifies the id of a child axtree contained within this > tree. I'm confused by this. These are being now used as process_id and routing_id but not named as such. Was that what you intended? https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... File ui/views/accessibility/ax_view_obj_wrapper.cc (right): https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... ui/views/accessibility/ax_view_obj_wrapper.cc:52: view_data.role = ui::AX_ROLE_WEB_AREA; On 2014/10/29 00:14:11, dmazzoni wrote: > Why not just fix this in WebView? > > Is the WebView a web area, or is it a host to a web area? If it's a host, maybe > it should have a role like IFRAME or FRAME, or maybe even a new role like > WEB_VIEW? Further to that - if we consider a WebView a host node, we could create a new AutomationRootNode on load() and add it as a child of the WebView, avoiding having to deal with WebArea nodes during unserialisation as per my other comment. https://codereview.chromium.org/667713006/diff/100001/chrome/browser/ui/ash/a... File chrome/browser/ui/ash/accessibility/ax_tree_source_ash.cc (right): https://codereview.chromium.org/667713006/diff/100001/chrome/browser/ui/ash/a... chrome/browser/ui/ash/accessibility/ax_tree_source_ash.cc:109: out_data->AddIntAttribute(ui::AX_ATTR_FRAME_ID, process_id); I'm confused as to why these two attributes are so named. https://codereview.chromium.org/667713006/diff/100001/chrome/common/extension... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/667713006/diff/100001/chrome/common/extension... chrome/common/extensions/api/automation.idl:245: AutomationNode root; Why did this change? An AutomationRootNode _is_ an AutomationNode - why would a root not be an AutomationRootNode? https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:136: automationUtil.onTreeLoaded(this.processID, this.routingID, function(root) { What if instead of creating the AutomationRootNode in automationUtil, we made load() a function on AutomationRootNode and created all webArea nodes as AutomationRootNodes as we were building the tree? Then we could pass the webArea (root) node into a function in automationUtil to cache it in the map, and it would receive events as normal, without having to do any extra accounting in this file. Do you think that could work? https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:137: if (callback) Perhaps onTreeLoaded should check for null/undefined callback instead; then we could just pass callback directly here. https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation_custom_bindings.js (right): https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation_custom_bindings.js:21: window.automationUtil = function() {}; Perhaps this should be in another file which is included by both this and automation_node, and exported via the utils.expose() function. (Understand if this was just the quickest way to do it, though.) https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation_custom_bindings.js:38: automationUtil.onTreeLoaded = function(pid, rid, callback) { I think this is a misnomer: the tree hasn't loaded at this point, right? It's more like onGotTreeIdentifier, or even storeTreeCallback or something.
https://codereview.chromium.org/667713006/diff/1/ui/accessibility/ax_enums.idl File ui/accessibility/ax_enums.idl (right): https://codereview.chromium.org/667713006/diff/1/ui/accessibility/ax_enums.id... ui/accessibility/ax_enums.idl:299: // Internal only; do not expose to platform APIs. On 2014/10/29 19:29:11, aboxhall wrote: > On 2014/10/29 06:53:30, dmazzoni wrote: > > I don't think we should put these here. > > > > Let's add ax_frame_id that specifies the frame id of an axtree, and > > ax_child_frame_id that specifies the id of a child axtree contained within > this > > tree. > > I'm confused by this. These are being now used as process_id and routing_id but > not named as such. Was that what you intended? It is; the intent is to have us ultimately use abstracted frame/child frame id's which we as accessibility keep track of. Added TODO. https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... File ui/views/accessibility/ax_view_obj_wrapper.cc (right): https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... ui/views/accessibility/ax_view_obj_wrapper.cc:52: view_data.role = ui::AX_ROLE_WEB_AREA; On 2014/10/29 19:29:11, aboxhall wrote: > On 2014/10/29 00:14:11, dmazzoni wrote: > > Why not just fix this in WebView? > > > > Is the WebView a web area, or is it a host to a web area? If it's a host, > maybe > > it should have a role like IFRAME or FRAME, or maybe even a new role like > > WEB_VIEW? > > Further to that - if we consider a WebView a host node, we could create a new > AutomationRootNode on load() and add it as a child of the WebView, avoiding > having to deal with WebArea nodes during unserialisation as per my other > comment. I'm not sure I entirely understand this. We need to populate the host node, whatever we call it, during unserialization because we need to populate its process id and routing id and make it available to be looked up. Also, consider the scenario where a caller calls getTree(). This should automatically "link" up the current tab to the desktop tree without actually calling load. https://codereview.chromium.org/667713006/diff/100001/chrome/common/extension... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/667713006/diff/100001/chrome/common/extension... chrome/common/extensions/api/automation.idl:245: AutomationNode root; On 2014/10/29 19:29:11, aboxhall wrote: > Why did this change? An AutomationRootNode _is_ an AutomationNode - why would a > root not be an AutomationRootNode? I removed the AutomationRootNode interface; can put it back, but it doesn't seem to serve any purpose now that load can be on a non-root node. Root node seems pretty confusing to me as well because it's not a root node in the case of the desktop tree. https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:136: automationUtil.onTreeLoaded(this.processID, this.routingID, function(root) { On 2014/10/29 19:29:11, aboxhall wrote: > What if instead of creating the AutomationRootNode in automationUtil, we made > load() a function on AutomationRootNode and created all webArea nodes as > AutomationRootNodes as we were building the tree? Then we could pass the webArea > (root) node into a function in automationUtil to cache it in the map, and it > would receive events as normal, without having to do any extra accounting in > this file. Do you think that could work? Could you explain further? The webArea node hosts the rootWebArea node. So, desktop tree contains a leaf webArea node which "points" to a tabs (or rfh) backed tree. A webArea node is _not_ a root node; it's process id and routing id only serve to point to a child; it also doesn't manage an ax object cache. Same problem here as stated earlier. The webArea node is a descendant of some desktop root node, so the creation of such a node occurs during createChildren_ (and not in automation_custom_bindings); since we make various passes to set the data, we don't have the actual role at creation time. This is perfectly fine, but means the way I'm doing it now is probably the only way unless we change the unserialization logic. https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:137: if (callback) On 2014/10/29 19:29:11, aboxhall wrote: > Perhaps onTreeLoaded should check for null/undefined callback instead; then we > could just pass callback directly here. Done. https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation_custom_bindings.js (right): https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation_custom_bindings.js:21: window.automationUtil = function() {}; On 2014/10/29 19:29:11, aboxhall wrote: > Perhaps this should be in another file which is included by both this and > automation_node, and exported via the utils.expose() function. (Understand if > this was just the quickest way to do it, though.) Yup that was it :). https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation_custom_bindings.js:38: automationUtil.onTreeLoaded = function(pid, rid, callback) { On 2014/10/29 19:29:11, aboxhall wrote: > I think this is a misnomer: the tree hasn't loaded at this point, right? It's > more like onGotTreeIdentifier, or even storeTreeCallback or something. Done.
https://codereview.chromium.org/667713006/diff/1/ui/accessibility/ax_enums.idl File ui/accessibility/ax_enums.idl (right): https://codereview.chromium.org/667713006/diff/1/ui/accessibility/ax_enums.id... ui/accessibility/ax_enums.idl:299: // Internal only; do not expose to platform APIs. On 2014/10/29 20:35:30, David Tseng wrote: > On 2014/10/29 19:29:11, aboxhall wrote: > > On 2014/10/29 06:53:30, dmazzoni wrote: > > > I don't think we should put these here. > > > > > > Let's add ax_frame_id that specifies the frame id of an axtree, and > > > ax_child_frame_id that specifies the id of a child axtree contained within > > this > > > tree. > > > > I'm confused by this. These are being now used as process_id and routing_id > but > > not named as such. Was that what you intended? > > It is; the intent is to have us ultimately use abstracted frame/child frame id's > which we as accessibility keep track of. Added TODO. I find this really confusing. Why not just put process_id and routing_id in here while we're still using them? (This question as much for Dominic.) https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... File ui/views/accessibility/ax_view_obj_wrapper.cc (right): https://codereview.chromium.org/667713006/diff/1/ui/views/accessibility/ax_vi... ui/views/accessibility/ax_view_obj_wrapper.cc:52: view_data.role = ui::AX_ROLE_WEB_AREA; On 2014/10/29 20:35:30, David Tseng wrote: > On 2014/10/29 19:29:11, aboxhall wrote: > > On 2014/10/29 00:14:11, dmazzoni wrote: > > > Why not just fix this in WebView? > > > > > > Is the WebView a web area, or is it a host to a web area? If it's a host, > > maybe > > > it should have a role like IFRAME or FRAME, or maybe even a new role like > > > WEB_VIEW? > > > > Further to that - if we consider a WebView a host node, we could create a new > > AutomationRootNode on load() and add it as a child of the WebView, avoiding > > having to deal with WebArea nodes during unserialisation as per my other > > comment. > > I'm not sure I entirely understand this. We need to populate the host node, > whatever we call it, during unserialization because we need to populate its > process id and routing id and make it available to be looked up. > > Also, consider the scenario where a caller calls getTree(). This should > automatically "link" up the current tab to the desktop tree without actually > calling load. I think my other comment now merges into this one, so we can deprecate this thread :) https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:136: automationUtil.onTreeLoaded(this.processID, this.routingID, function(root) { On 2014/10/29 20:35:30, David Tseng wrote: > On 2014/10/29 19:29:11, aboxhall wrote: > > What if instead of creating the AutomationRootNode in automationUtil, we made > > load() a function on AutomationRootNode and created all webArea nodes as > > AutomationRootNodes as we were building the tree? Then we could pass the > webArea > > (root) node into a function in automationUtil to cache it in the map, and it > > would receive events as normal, without having to do any extra accounting in > > this file. Do you think that could work? > > Could you explain further? > > The webArea node hosts the rootWebArea node. So, > > desktop tree contains a leaf webArea node which "points" to a tabs (or rfh) > backed tree. > > A webArea node is _not_ a root node; it's process id and routing id only serve > to point to a child; it also doesn't manage an ax object cache. > > Same problem here as stated earlier. The webArea node is a descendant of some > desktop root node, so the creation of such a node occurs during createChildren_ > (and not in automation_custom_bindings); since we make various passes to set the > data, we don't have the actual role at creation time. This is perfectly fine, > but means the way I'm doing it now is probably the only way unless we change the > unserialization logic. So if I understand correctly, the webArea node has as its only child the root node of the embedded tree? I was assuming (from this change) that we were treating the webArea node as the root of the new tree (although now I see that's not the case). So in that case, why not construct the new AutomationRootNode as the only child of the webArea node during unserialization, with its loaded attribute set to false, and then set that in the map in automationUtil, (since we have its pid and rid at unserialization time now) rather than constructing it later on? Then this method would only need to call enableFrame with the appropriate PID and RID (which could be private properties of the AutomationRootNode), and the existing bindings would take care of the rest. Am I missing something?
https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:136: automationUtil.onTreeLoaded(this.processID, this.routingID, function(root) { On 2014/10/29 20:53:29, aboxhall wrote: > On 2014/10/29 20:35:30, David Tseng wrote: > > On 2014/10/29 19:29:11, aboxhall wrote: > > > What if instead of creating the AutomationRootNode in automationUtil, we > made > > > load() a function on AutomationRootNode and created all webArea nodes as > > > AutomationRootNodes as we were building the tree? Then we could pass the > > webArea > > > (root) node into a function in automationUtil to cache it in the map, and it > > > would receive events as normal, without having to do any extra accounting in > > > this file. Do you think that could work? > > > > Could you explain further? > > > > The webArea node hosts the rootWebArea node. So, > > > > desktop tree contains a leaf webArea node which "points" to a tabs (or rfh) > > backed tree. > > > > A webArea node is _not_ a root node; it's process id and routing id only serve > > to point to a child; it also doesn't manage an ax object cache. > > > > Same problem here as stated earlier. The webArea node is a descendant of some > > desktop root node, so the creation of such a node occurs during > createChildren_ > > (and not in automation_custom_bindings); since we make various passes to set > the > > data, we don't have the actual role at creation time. This is perfectly fine, > > but means the way I'm doing it now is probably the only way unless we change > the > > unserialization logic. > > So if I understand correctly, the webArea node has as its only child the root > node of the embedded tree? I was assuming (from this change) that we were > treating the webArea node as the root of the new tree (although now I see that's > not the case). > > So in that case, why not construct the new AutomationRootNode as the only child > of the webArea node during unserialization, with its loaded attribute set to > false, and then set that in the map in automationUtil, (since we have its pid > and rid at unserialization time now) rather than constructing it later on? Then > this method would only need to call enableFrame with the appropriate PID and RID > (which could be private properties of the AutomationRootNode), and the existing > bindings would take care of the rest. Am I missing something? Good questions. Again, to do that during unserialization, we need to know we're dealing with a webArea node. We don't have that information because role gets set later; unless we do some kind of post processing. Also, the webArea node (backed by views::WebView) may not actually have a valid rfh. For example, in the test, you will see two webAreas. The first, for whatever reason, never actually maps to a valid rfh on the ash desktop. I understand the confusion about load() applying only to AutomationRootNode's, but I think we want to be flexible here and allowing load() to get called on a node that's hosting a root.
As a general comment, I'm not sure we actually need the loaded attribute to be exposed publically; seems like it's unnecessary and we're not really setting it properly for ordinary tabs trees currently. Would anyone object if I removed it from the idl?
Patchset #9 (id:160001) has been deleted
Patchset #9 (id:180001) has been deleted
Patchset #8 (id:140001) has been deleted
On 2014/10/29 21:25:05, David Tseng wrote: > As a general comment, I'm not sure we actually need the loaded attribute to be > exposed publically; seems like it's unnecessary and we're not really setting it > properly for ordinary tabs trees currently. Would anyone object if I removed it > from the idl? What's the workflow for knowing whether we need to call load() in that case? I guess we can just check for whether the node has any children...
https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:136: automationUtil.onTreeLoaded(this.processID, this.routingID, function(root) { On 2014/10/29 21:05:39, David Tseng wrote: > Again, to do that during unserialization, we need to know we're dealing with a > webArea node. We don't have that information because role gets set later; unless > we do some kind of post processing. I don't think it would be that hard to do the post-processing. It would just be a step in between setData_() and the event propagation during updateNode_() where we check the role and create a new AutomationRootNode if necessary. > Also, the webArea node (backed by views::WebView) may not actually have a valid > rfh. For example, in the test, you will see two webAreas. The first, for > whatever reason, never actually maps to a valid rfh on the ash desktop. So what happens if you call load() on that other webArea? How do we handle that case now? Is it significantly different to if we created an unloaded placeholder AutomationRootNode? Can we detect ahead of time that there's no valid RFH and avoid creating the placeholder in that case? > I understand the confusion about load() applying only to AutomationRootNode's, > but I think we want to be flexible here and allowing load() to get called on a > node that's hosting a root. I still don't see the advantage :\
On Wed, Oct 29, 2014 at 2:31 PM, <aboxhall@chromium.org> wrote: > On 2014/10/29 21:25:05, David Tseng wrote: > >> As a general comment, I'm not sure we actually need the loaded attribute >> to be >> exposed publically; seems like it's unnecessary and we're not really >> setting >> > it > >> properly for ordinary tabs trees currently. Would anyone object if I >> removed >> > it > >> from the idl? >> > > What's the workflow for knowing whether we need to call load() in that > case? I > guess we can just check for whether the node has any children... > > No; the presence of an AutomationRootNode means the tree is loaded. The absence means it's not. load() should only ever be callable on a webArea node. If that makes sense. > https://codereview.chromium.org/667713006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/667713006/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:136: automationUtil.onTreeLoaded(this.processID, this.routingID, function(root) { On 2014/10/29 21:33:08, aboxhall wrote: > On 2014/10/29 21:05:39, David Tseng wrote: > > Again, to do that during unserialization, we need to know we're dealing with a > > webArea node. We don't have that information because role gets set later; > unless > > we do some kind of post processing. > > I don't think it would be that hard to do the post-processing. It would just be > a step in between setData_() and the event propagation during updateNode_() > where we check the role and create a new AutomationRootNode if necessary. > > > Also, the webArea node (backed by views::WebView) may not actually have a > valid > > rfh. For example, in the test, you will see two webAreas. The first, for > > whatever reason, never actually maps to a valid rfh on the ash desktop. > > So what happens if you call load() on that other webArea? How do we handle that > case now? Is it significantly different to if we created an unloaded placeholder > AutomationRootNode? Can we detect ahead of time that there's no valid RFH and > avoid creating the placeholder in that case? > > > I understand the confusion about load() applying only to AutomationRootNode's, > > but I think we want to be flexible here and allowing load() to get called on a > > node that's hosting a root. > > I still don't see the advantage :\ Right, it wouldn't be that hard at all...but the node would have an invalid ax id and have no backing object back in the browser/renderer side.
Taking offline :). To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Wed, Oct 29, 2014 at 12:29 PM, <aboxhall@chromium.org> wrote: > https://codereview.chromium.org/667713006/diff/1/ui/ > accessibility/ax_enums.idl#newcode299 > ui/accessibility/ax_enums.idl:299: // Internal only; do not expose to > platform APIs. > On 2014/10/29 06:53:30, dmazzoni wrote: > >> I don't think we should put these here. >> > > Let's add ax_frame_id that specifies the frame id of an axtree, and >> ax_child_frame_id that specifies the id of a child axtree contained >> > within this > >> tree. >> > > I'm confused by this. These are being now used as process_id and > routing_id but not named as such. Was that what you intended? That's not what I had in mind - I think this should be a new unique id that we assign for every process_id / routing_id pair we encounter when sending messages to the automation client internals by building a hash map in the C++ code. Further to that - if we consider a WebView a host node, we could create > a new AutomationRootNode on load() and add it as a child of the WebView, > avoiding having to deal with WebArea nodes during unserialisation as per > my other comment. > That sounds fine. Is it okay if the IFRAME role works similarly, it should have a load() method on it and an AutomationRootNode. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Wed, Oct 29, 2014 at 1:53 PM, <aboxhall@chromium.org> wrote: > I find this really confusing. Why not just put process_id and routing_id > in here while we're still using them? (This question as much for > Dominic.) Yeah, my intent was that we should just bite the bullet and make a hash map from abstract ids to process_id/routing_ids now - you don't have to refactor the automation api completely, just to introduce the new id in this changelist and convert back to a process_id / routing_id every time you need it To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
David, thanks so much for exploring this. I like the look of this, but what do you both think? I was kind of hoping we could do away with idToWebArea_ and related logic, but I can definitely see the issue with IDs. We could assign some temporary (e.g. negative) IDs for child AutomationRootNodes and do some bookkeeping to update the parent's children list when the correct ID arrives (we might need to do that anyway, given the 'placeholder' issue, unless that has been fixed, Dominic?), but I can see how that is starting to look like just as much work as the current indirection... https://codereview.chromium.org/667713006/diff/280001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation_custom_bindings.js (right): https://codereview.chromium.org/667713006/diff/280001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation_custom_bindings.js:44: if (!targetTree) { Should probably check loaded here.
David, thanks so much for exploring this. I like the look of this, but what do you both think? I was kind of hoping we could do away with idToWebArea_ and related logic, but I can definitely see the issue with IDs. We could assign some temporary (e.g. negative) IDs for child AutomationRootNodes and do some bookkeeping to update the parent's children list when the correct ID arrives (we might need to do that anyway, given the 'placeholder' issue, unless that has been fixed, Dominic?), but I can see how that is starting to look like just as much work as the current indirection...
Just got the tests to pass. It's a bit messier than the other strategy but maintains the AutomationRootNode interface. Basically, a few issues: - webArea has a child client node backing it on the browser side, so we just ignore that inside of createNewChildren_, but we have to check the current node didn't already allocate a new root web area - we have to account now for the fact that sometimes we want our callback triggered when the load state changes and not when we create a new root node - can't avoid keeping track of id to web area because we need bi directional links from root web areas to their hosting web areas; id to web area gives us the child->parent link/relationship while the original id to root gives us the parent ->child. - we still need to manage the loaded state which is just another state to possibly get wrong On Wed, Oct 29, 2014 at 4:32 PM, <aboxhall@chromium.org> wrote: David, thanks so much for exploring this. I like the look of this, but what do you both think? I was kind of hoping we could do away with idToWebArea_ and related logic, but I can definitely see the issue with IDs. We could assign some temporary (e.g. negative) IDs for child AutomationRootNodes and do some bookkeeping to update the parent's children list when the correct ID arrives (we might need to do that anyway, given the 'placeholder' issue, unless that has been fixed, Dominic?), but I can see how that is starting to look like just as much work as the current indirection... https://codereview.chromium.org/667713006/diff/280001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation_custom_bindings.js (right): https://codereview.chromium.org/667713006/diff/280001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation_custom_bindings.js:44: if (!targetTree) { Should probably check loaded here. https://codereview.chromium.org/667713006/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/10/29 22:21:33, dmazzoni wrote: > On Wed, Oct 29, 2014 at 1:53 PM, <mailto:aboxhall@chromium.org> wrote: > > > I find this really confusing. Why not just put process_id and routing_id > > in here while we're still using them? (This question as much for > > Dominic.) > > > Yeah, my intent was that we should just bite the bullet and make a hash map > from abstract ids to process_id/routing_ids now - you don't have to > refactor the automation api completely, just to introduce the new id in > this changelist and convert back to a process_id / routing_id every time > you need it > Yeah; this is a big change as it affects basically every api (enableTab, enableFrame, actions, etc). It feels wrong because it really should be the responsibility of the underlying web contents to maintain some notion of an id and not to have us maintaining something like that. With that said, I did look into it and it amounts to keeping around a singleton that maps from a pair process id/routing id to an "AXTreeID" and a map in the reverse though maintaining such a structure accounting for oopif's isn't clear to me.
https://codereview.chromium.org/667713006/diff/300001/chrome/browser/ui/ash/a... File chrome/browser/ui/ash/accessibility/ax_tree_source_ash.cc (right): https://codereview.chromium.org/667713006/diff/300001/chrome/browser/ui/ash/a... chrome/browser/ui/ash/accessibility/ax_tree_source_ash.cc:99: if (out_data->role == ui::AX_ROLE_GROUP) { I still think it'd be best to just add a new WEB_VIEW role and set that in GetAccessibilityState for the WebView class, rather than using kViewClassName. It doesn't have to affect any other platforms because you can just map it to the same native role as AX_ROLE_GROUP on Windows. https://codereview.chromium.org/667713006/diff/300001/chrome/browser/ui/ash/a... chrome/browser/ui/ash/accessibility/ax_tree_source_ash.cc:101: if (view->GetClassName() == views::WebView::kViewClassName) { nit: indent everything inside this if https://codereview.chromium.org/667713006/diff/300001/chrome/browser/ui/ash/a... chrome/browser/ui/ash/accessibility/ax_tree_source_ash.cc:102: out_data->role = ui::AX_ROLE_WEB_AREA; As before, web area is not correct for this since this is a host https://codereview.chromium.org/667713006/diff/300001/chrome/common/extension... File chrome/common/extensions/api/automation_internal.idl (right): https://codereview.chromium.org/667713006/diff/300001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:102: //id. nit: space after // https://codereview.chromium.org/667713006/diff/300001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/667713006/diff/300001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:481: var pid = privates(node).impl.processID; I think it'd be more clear if this was childProcessID and childRoutingID https://codereview.chromium.org/667713006/diff/300001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:484: var targetTree = automationUtil.idToAutomationRootNode[id]; targetTree -> childTree https://codereview.chromium.org/667713006/diff/300001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:533: delete nodeData.intAttributes['processId']; this doesn't match, should be nodeData.intAttributes.frameId, or whatever we rename it
PTAL. Made major changes; please take a fresh look.
Great! Almost there! https://codereview.chromium.org/667713006/diff/450001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/automation/tests/unit/test.js (right): https://codereview.chromium.org/667713006/diff/450001/chrome/test/data/extens... chrome/test/data/extensions/api_test/automation/tests/unit/test.js:9: // Unused. Nit: indentation https://codereview.chromium.org/667713006/diff/450001/ui/accessibility/ax_tre... File ui/accessibility/ax_tree_id_registry.h (right): https://codereview.chromium.org/667713006/diff/450001/ui/accessibility/ax_tre... ui/accessibility/ax_tree_id_registry.h:21: class AX_EXPORT AXTreeIDRegistry { This class looks great, but it can't go in ui/accessibility. Some options: 1) Put the interface in content/public/browser. 2) It's only used from chrome/ for now, so put it in chrome/browser/accessibility; until in the future if native accessibility wants to share these ids https://codereview.chromium.org/667713006/diff/450001/ui/views/controls/webvi... File ui/views/controls/webview/webview.cc (right): https://codereview.chromium.org/667713006/diff/450001/ui/views/controls/webvi... ui/views/controls/webview/webview.cc:213: state->role = ui::AX_ROLE_WEB_VIEW; Please map this in NativeViewAccessibilityWin::MSAARole too
Patchset #19 (id:420001) has been deleted
Patchset #19 (id:440001) has been deleted
Patchset #18 (id:400001) has been deleted
Patchset #17 (id:380001) has been deleted
Patchset #16 (id:360001) has been deleted
Patchset #15 (id:340001) has been deleted
Patchset #14 (id:320001) has been deleted
Patchset #10 (id:240001) has been deleted
Patchset #9 (id:220001) has been deleted
Patchset #8 (id:200001) has been deleted
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/667713006/diff/450001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/automation/tests/unit/test.js (right): https://codereview.chromium.org/667713006/diff/450001/chrome/test/data/extens... chrome/test/data/extensions/api_test/automation/tests/unit/test.js:9: // Unused. On 2014/10/31 22:20:26, dmazzoni wrote: > Nit: indentation Done. https://codereview.chromium.org/667713006/diff/450001/ui/accessibility/ax_tre... File ui/accessibility/ax_tree_id_registry.h (right): https://codereview.chromium.org/667713006/diff/450001/ui/accessibility/ax_tre... ui/accessibility/ax_tree_id_registry.h:21: class AX_EXPORT AXTreeIDRegistry { On 2014/10/31 22:20:26, dmazzoni wrote: > This class looks great, but it can't go in ui/accessibility. > > Some options: > > 1) Put the interface in content/public/browser. > 2) It's only used from chrome/ for now, so put it in > chrome/browser/accessibility; until in the future if native accessibility wants > to share these ids Technically, no deps violations, but yeah, it should probably go under chrome/ or content/. Putting under chrome/ for now until we figure out the situation with other platforms. https://codereview.chromium.org/667713006/diff/450001/ui/views/controls/webvi... File ui/views/controls/webview/webview.cc (right): https://codereview.chromium.org/667713006/diff/450001/ui/views/controls/webvi... ui/views/controls/webview/webview.cc:213: state->role = ui::AX_ROLE_WEB_VIEW; On 2014/10/31 22:20:26, dmazzoni wrote: > Please map this in NativeViewAccessibilityWin::MSAARole too Done.
dtseng@chromium.org changed reviewers: + kalman@chromium.org, sky@chromium.org
+ kalman for automation*idl + sky for webview.cc
lgtm
webview LGTM
extension IDLs lgtm
Apologies for my slowness, and what may (hopefully won't) amount to another bikeshed on the composing trees issue. https://codereview.chromium.org/667713006/diff/490001/chrome/browser/extensio... File chrome/browser/extensions/api/automation/automation_apitest.cc (right): https://codereview.chromium.org/667713006/diff/490001/chrome/browser/extensio... chrome/browser/extensions/api/automation/automation_apitest.cc:344: // TODO(dtseng): Need to rewrite this test in terms of tree ids... You can assign this TODO to me if you like! https://codereview.chromium.org/667713006/diff/490001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/667713006/diff/490001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:174: // Limited to desktop tree for now. TODO for the circumstances under which it won't be limited to desktop tree? https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:308: role: 'rootWebArea', Why is this necessary? Won't this come down in the initial data? https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:371: idToWebView_[this.treeID] = undefined; delete idToWebView_[this.treeID] does the same thing and (to me) looks clearer. https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:507: if (nodeData.role == schema.RoleType.webView) { I think we should have a set of roles which can be host nodes, and check against that here, or at least a TODO to add it. This logic will be the same for iframes, right? Anything else? https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:509: nodeImpl.pendingChildFrame = true; Do we want to expose the pendingChildFrame property? https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:513: idToWebView_[nodeImpl.childTreeID] = node; So the id in idToWebView is the ID of the frame it's hosting? Perhaps it would be better named as frameIdToHostNode or similar? https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:515: automationUtil.storeTreeCallback(nodeImpl.childTreeID, function(root) { What's preventing us from setting root.id as the sole child of nodeImpl here, and nodeImpl.id as root's parent ID? Node IDs are globally unique, aren't they? It's entirely possible I'm (again) missing some reason why this wouldn't work, but it seems like that would be much simpler, and remove the need to keep idToWebView and lookUpWebChild_(), and thus the need to expose idToAutomationRootNode to this file. Please help me figure out what I'm missing here :) https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:633: var AutomationRootNode = utils.expose('AutomationRootNode', Do we still need to expose this type at all? https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation_custom_bindings.js (right): https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation_custom_bindings.js:21: window.automationUtil = function() {}; I'm really not sure about the implications of this. It might be fine, but it makes me nervous. What is |window| here? https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation_custom_bindings.js:27: automationUtil.DESKTOP_TREE_ID = 0; Why does DESKTOP_TREE_ID need to be on automationUtil? I don't see it used in automation_node.js. https://codereview.chromium.org/667713006/diff/550001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/automation/tests/desktop/load_tabs.js (right): https://codereview.chromium.org/667713006/diff/550001/chrome/test/data/extens... chrome/test/data/extensions/api_test/automation/tests/desktop/load_tabs.js:43: function testSubevents() { Great test :)
https://codereview.chromium.org/667713006/diff/490001/chrome/browser/extensio... File chrome/browser/extensions/api/automation/automation_apitest.cc (right): https://codereview.chromium.org/667713006/diff/490001/chrome/browser/extensio... chrome/browser/extensions/api/automation/automation_apitest.cc:344: // TODO(dtseng): Need to rewrite this test in terms of tree ids... On 2014/11/03 17:15:48, aboxhall wrote: > You can assign this TODO to me if you like! Done. https://codereview.chromium.org/667713006/diff/490001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/667713006/diff/490001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:174: // Limited to desktop tree for now. On 2014/11/03 17:15:48, aboxhall wrote: > TODO for the circumstances under which it won't be limited to desktop tree? Done. https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:308: role: 'rootWebArea', On 2014/11/03 17:15:49, aboxhall wrote: > Why is this necessary? Won't this come down in the initial data? It was (if we give callers the placeholder node) which comes before any data; no longer true so removed. https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:371: idToWebView_[this.treeID] = undefined; On 2014/11/03 17:15:49, aboxhall wrote: > delete idToWebView_[this.treeID] does the same thing and (to me) looks clearer. Obsolete. https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:507: if (nodeData.role == schema.RoleType.webView) { On 2014/11/03 17:15:49, aboxhall wrote: > I think we should have a set of roles which can be host nodes, and check against > that here, or at least a TODO to add it. This logic will be the same for > iframes, right? Anything else? Added TODO since that seems a bit early at this point. https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:509: nodeImpl.pendingChildFrame = true; On 2014/11/03 17:15:49, aboxhall wrote: > Do we want to expose the pendingChildFrame property? I'd prefer for the caller to check if there are children. https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:513: idToWebView_[nodeImpl.childTreeID] = node; On 2014/11/03 17:15:49, aboxhall wrote: > So the id in idToWebView is the ID of the frame it's hosting? Perhaps it would > be better named as frameIdToHostNode or similar? Obsolete with other change. https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:515: automationUtil.storeTreeCallback(nodeImpl.childTreeID, function(root) { On 2014/11/03 17:15:49, aboxhall wrote: > What's preventing us from setting root.id as the sole child of nodeImpl here, > and nodeImpl.id as root's parent ID? Node IDs are globally unique, aren't they? I'm pretty sure node ids are not globally unique across two different ax trees. AX tree idds are globally unique. I suppose we could keep a parentHostID and childRootID on nodeImpl and root respectively, but I'm not clear how to update those nodes when a destroyed event comes through. The idToWebView_ establishes the parent to child mapping while the idToAutomationRootNode maps the child to parent mapping where child is the subroot and parent is the host. > It's entirely possible I'm (again) missing some reason why this wouldn't work, > but it seems like that would be much simpler, and remove the need to keep > idToWebView and lookUpWebChild_(), and thus the need to expose > idToAutomationRootNode to this file. > > Please help me figure out what I'm missing here :) No worries; I just spent some time trying to do this; it does indeed make the code cleaner. The only reason to not do this and make things slightly more complex is nthat now JSON.stringify() won't work on some nodes (since they have circular references). Other than that, it does clean up things a bit. Thanks. https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:633: var AutomationRootNode = utils.expose('AutomationRootNode', On 2014/11/03 17:15:49, aboxhall wrote: > Do we still need to expose this type at all? Still seems useful at least for testing. https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation_custom_bindings.js (right): https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation_custom_bindings.js:21: window.automationUtil = function() {}; On 2014/11/03 17:15:49, aboxhall wrote: > I'm really not sure about the implications of this. It might be fine, but it > makes me nervous. What is |window| here? kalman can probably better comment, but window is the window objet running within the v8 context for the extension system; I don't think the extension's background page runs in the same context (i.e. it has a different window object). https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation_custom_bindings.js:27: automationUtil.DESKTOP_TREE_ID = 0; On 2014/11/03 17:15:49, aboxhall wrote: > Why does DESKTOP_TREE_ID need to be on automationUtil? I don't see it used in > automation_node.js. Removed.
lgtm https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/667713006/diff/490001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:515: automationUtil.storeTreeCallback(nodeImpl.childTreeID, function(root) { On 2014/11/03 19:31:52, David Tseng wrote: > On 2014/11/03 17:15:49, aboxhall wrote: > > What's preventing us from setting root.id as the sole child of nodeImpl here, > > and nodeImpl.id as root's parent ID? Node IDs are globally unique, aren't > they? > > I'm pretty sure node ids are not globally unique across two different ax trees. > AX tree idds are globally unique. Ah of course, they won't be unique if they're in different renderers, which is the whole point of this. > I suppose we could keep a parentHostID and childRootID on nodeImpl and root > respectively, but I'm not clear how to update those nodes when a destroyed event > comes through. > > The idToWebView_ establishes the parent to child mapping while the > idToAutomationRootNode maps the child to parent mapping where child is the > subroot and parent is the host. > > > It's entirely possible I'm (again) missing some reason why this wouldn't work, > > but it seems like that would be much simpler, and remove the need to keep > > idToWebView and lookUpWebChild_(), and thus the need to expose > > idToAutomationRootNode to this file. > > > > Please help me figure out what I'm missing here :) > > No worries; I just spent some time trying to do this; it does indeed make the > code cleaner. > > The only reason to not do this and make things slightly more complex is nthat > now JSON.stringify() won't work on some nodes (since they have circular > references). > > Other than that, it does clean up things a bit. Thanks. Oh, this looks great! Thanks for that. https://codereview.chromium.org/667713006/diff/490001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/automation/tests/desktop/load_tabs.js (right): https://codereview.chromium.org/667713006/diff/490001/chrome/test/data/extens... chrome/test/data/extensions/api_test/automation/tests/desktop/load_tabs.js:32: assertEq(evt.target, subroot.parent()); Do we not want to test this any more?
https://codereview.chromium.org/667713006/diff/490001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/automation/tests/desktop/load_tabs.js (right): https://codereview.chromium.org/667713006/diff/490001/chrome/test/data/extens... chrome/test/data/extensions/api_test/automation/tests/desktop/load_tabs.js:32: assertEq(evt.target, subroot.parent()); On 2014/11/03 19:40:18, aboxhall wrote: > Do we not want to test this any more? We can't re: JSON.stringify on that parent node results in a circular stringify. https://codereview.chromium.org/667713006/diff/490001/chrome/test/data/extens... chrome/test/data/extensions/api_test/automation/tests/desktop/load_tabs.js:32: assertEq(evt.target, subroot.parent()); On 2014/11/03 19:40:18, aboxhall wrote: > Do we not want to test this any more? Exported toJSON to get around circular stringification issue (since assertEq uses JSON.stringify).
Patchset #16 (id:600001) has been deleted
Patchset #17 (id:630001) has been deleted
Patchset #17 (id:630001) has been deleted
Patchset #16 (id:520025) has been deleted
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/667713006/650001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/667713006/670001
Message was sent while issue was closed.
Committed patchset #17 (id:670001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/ad8ae0f7f45d921efd8da77ffecbcd7253c87870 Cr-Commit-Position: refs/heads/master@{#302648} |
