Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(374)

Issue 2674353003: Enable WebViewAccessiblityTest for OOPIF webview. (Closed)

Created:
3 years, 10 months ago by avallee
Modified:
3 years, 6 months ago
Reviewers:
wjmaclean, dmazzoni, alexmos
CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, lfg, wjmaclean
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Enable WebViewAccessiblityTest for OOPIF webview. + Enabled WebViewAccessibilityTests for oopif. + Hooked up the inner AX tree to point back to the AX tree for the outer web contents. Otherwise BrowserAccessibility::Platform{ChildCount,GetChild} functions fail on the [child->GetParent() == this] check. BUG=610795, 685739 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2674353003 Cr-Commit-Position: refs/heads/master@{#481326} Committed: https://chromium.googlesource.com/chromium/src/+/d68cdd1afdff55d24558ef88441d9ac950d46dd4

Patch Set 1 #

Patch Set 2 : Accessiblity tree will search for inner web contents. #

Total comments: 8

Patch Set 3 : Address dmazzoni comments. #

Total comments: 6

Patch Set 4 : Fixed comment and spacing. #

Total comments: 20

Patch Set 5 : Address alexmos comments. #

Patch Set 6 : remove change from unrelated cl. #

Total comments: 10

Patch Set 7 : Rebase #

Patch Set 8 : Instrument and fix #

Patch Set 9 : Remove false assert #

Patch Set 10 : Remove test logging. #

Patch Set 11 : Address alexmos final comments. #

Total comments: 3

Patch Set 12 : Fix typos. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+84 lines, -23 lines) Patch
M chrome/browser/apps/guest_view/web_view_browsertest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -6 lines 0 comments Download
M content/browser/frame_host/render_frame_host_delegate.h View 1 2 3 4 5 6 7 8 1 chunk +9 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_delegate.cc View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -14 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +5 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +57 lines, -2 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 84 (61 generated)
avallee
Is WebContentsImpl the right place to do this or would GuestView/GuestViewManager/RenderFrameImpl feel more natural? Dmazzoni: ...
3 years, 10 months ago (2017-02-06 18:55:15 UTC) #2
dmazzoni
lgtm This makes sense. Do you think we should rename browser_plugin_embedder_ax_tree_id to something more general ...
3 years, 10 months ago (2017-02-06 18:59:12 UTC) #3
dmazzoni
As for fixing focus, there are two possibilities: 1. As you noticed, RenderFrameHostImpl::AXContentTreeDataToAXTreeData is one ...
3 years, 10 months ago (2017-02-06 19:04:28 UTC) #4
avallee
On 2017/02/06 18:59:12, dmazzoni wrote: > lgtm > > This makes sense. > > Do ...
3 years, 10 months ago (2017-02-06 19:05:19 UTC) #5
avallee
Dmazzoni: ptal, I fixed the test that was hanging. Lfg: any comment on the approach? ...
3 years, 10 months ago (2017-02-15 23:26:46 UTC) #9
dmazzoni
https://codereview.chromium.org/2674353003/diff/20001/chrome/browser/apps/guest_view/web_view_browsertest.cc File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/2674353003/diff/20001/chrome/browser/apps/guest_view/web_view_browsertest.cc#newcode890 chrome/browser/apps/guest_view/web_view_browsertest.cc:890: testing::Bool()); Yay! I think you can update the change ...
3 years, 10 months ago (2017-02-16 00:19:02 UTC) #12
dmazzoni
3 years, 10 months ago (2017-02-16 00:19:04 UTC) #13
avallee
Done. https://codereview.chromium.org/2674353003/diff/20001/chrome/browser/apps/guest_view/web_view_browsertest.cc File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/2674353003/diff/20001/chrome/browser/apps/guest_view/web_view_browsertest.cc#newcode890 chrome/browser/apps/guest_view/web_view_browsertest.cc:890: testing::Bool()); On 2017/02/16 00:19:01, dmazzoni wrote: > Yay! ...
3 years, 10 months ago (2017-02-21 20:00:32 UTC) #18
avallee
wjmaclean@chromium.org: Please review changes in chrome/
3 years, 10 months ago (2017-02-21 20:29:45 UTC) #20
wjmaclean
lgtm
3 years, 10 months ago (2017-02-21 20:34:53 UTC) #21
dmazzoni
lgtm Very clear now! https://codereview.chromium.org/2674353003/diff/40001/content/browser/frame_host/render_frame_host_delegate.h File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/2674353003/diff/40001/content/browser/frame_host/render_frame_host_delegate.h#newcode232 content/browser/frame_host/render_frame_host_delegate.h:232: // frame, return the main ...
3 years, 10 months ago (2017-02-22 08:46:41 UTC) #24
avallee
https://codereview.chromium.org/2674353003/diff/40001/content/browser/frame_host/render_frame_host_delegate.h File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/2674353003/diff/40001/content/browser/frame_host/render_frame_host_delegate.h#newcode232 content/browser/frame_host/render_frame_host_delegate.h:232: // frame, return the main frame, since the attchment ...
3 years, 10 months ago (2017-02-22 16:58:49 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2674353003/60001
3 years, 10 months ago (2017-02-22 16:59:17 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/370203)
3 years, 10 months ago (2017-02-22 17:06:42 UTC) #30
avallee
alexmos: PTAL for owners.
3 years, 10 months ago (2017-02-22 18:32:48 UTC) #32
alexmos
Thanks, a few comments and clarification questions below. It's a bit concerning that the focus ...
3 years, 10 months ago (2017-02-23 20:07:05 UTC) #33
avallee
Alexmos: I'm looking at a follow up cl to delete the focused webcontents pointer and ...
3 years, 9 months ago (2017-02-28 20:13:01 UTC) #36
alexmos
Sorry for the delay! LGTM with nits and one more question about returning the main ...
3 years, 9 months ago (2017-03-03 20:07:57 UTC) #42
avallee
PTAL. Should be submittable now. https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_contents/web_contents_impl.cc File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2674353003/diff/60001/content/browser/web_contents/web_contents_impl.cc#newcode4815 content/browser/web_contents/web_contents_impl.cc:4815: return contents->GetMainFrame(); On 2017/03/03 ...
3 years, 6 months ago (2017-06-21 02:25:36 UTC) #62
dmazzoni
still lgtm, one quick log to remove! https://codereview.chromium.org/2674353003/diff/200001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2674353003/diff/200001/content/renderer/render_frame_impl.cc#newcode2279 content/renderer/render_frame_impl.cc:2279: LOG(ERROR) << ...
3 years, 6 months ago (2017-06-21 20:18:21 UTC) #73
alexmos
Still LGTM, thanks! https://codereview.chromium.org/2674353003/diff/200001/content/browser/web_contents/web_contents_impl.cc File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2674353003/diff/200001/content/browser/web_contents/web_contents_impl.cc#newcode4624 content/browser/web_contents/web_contents_impl.cc:4624: // process boundry in focus order, ...
3 years, 6 months ago (2017-06-21 20:29:26 UTC) #74
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2674353003/220001
3 years, 6 months ago (2017-06-21 22:00:01 UTC) #81
commit-bot: I haz the power
3 years, 6 months ago (2017-06-21 22:07:44 UTC) #84
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/d68cdd1afdff55d24558ef88441d...

Powered by Google App Engine
This is Rietveld 408576698