Fix WebView accessibility

Fix WebView accessibility

This makes the chrome://chrome-signin page accessible too.

BUG=557429, 587187, 453157
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/fdfa2e079cd79d0308211b0ad3b350bf0350d500
Cr-Commit-Position: refs/heads/master@{#383885}

[dmazzoni, 2016-03-19 06:48:50]

Description was changed from

==========
Fix WebView accessibility

This makes the chrome://chrome-signin page accessible too.

BUG=557429,587187,453157
==========

to

==========
Fix WebView accessibility

This makes the chrome://chrome-signin page accessible too.

BUG=557429,587187,453157
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[dmazzoni, 2016-03-21 04:21:20]

dmazzoni@chromium.org changed reviewers:
+ dcheng@chromium.org

[dmazzoni, 2016-03-21 04:21:20]

Not sure how to best write a test for this, will discuss online.

[dcheng, 2016-03-22 17:23:15]

code changes seem good. were you going to try implementing approach #2 for
testing?

[dmazzoni, 2016-03-24 04:35:43]

OK, test added.

The test is pretty basic because most of it is covered by
site isolation accessibility tests. If we can get focus events
inside of a WebView, the rest of WebView probably works too.

[dcheng, 2016-03-24 18:15:12]

https://codereview.chromium.org/1815933002/diff/40001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/1815933002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_manager.cc:190: if
(!g_focus_change_callback_for_testing.Pointer()) {
I wonder if it'd be possible to snoop IPC messages in the browser test itself,
and listen for, say, AccessibilityHostMsg_Events to figure out when to check for
focus changes. Is there similar precedent for adding test-only callbacks like
this to test accessibility code?

https://codereview.chromium.org/1815933002/diff/40001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/1815933002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_manager.h:197: static void
SetFocusChangeCallbackForTesting(base::Closure callback);
const base::Closure&

(base::Closure is movable now, but I think in general, we still prefer to pass
by const T& unless there's some clear win for passing by value and using move
semantics)

[dmazzoni, 2016-03-25 05:02:22]

https://codereview.chromium.org/1815933002/diff/40001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/1815933002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_manager.cc:190: if
(!g_focus_change_callback_for_testing.Pointer()) {
On 2016/03/24 18:15:11, dcheng wrote:
> I wonder if it'd be possible to snoop IPC messages in the browser test itself,
> and listen for, say, AccessibilityHostMsg_Events to figure out when to check
for
> focus changes. Is there similar precedent for adding test-only callbacks like
> this to test accessibility code?

That's how most of the tests in content/ work, we use
AccessibilityNotificationWaiter,
but I didn't want to pull a ton of interfaces into content/public just to test
this one
thing. I posted some other alternatives to site-isolation-discuss, this seemed
like
the most logical first step.

I'm also worried that snooping on particular IPCs might be a bit fragile, since
we rely on the hook in FrameTree::SetFocusedFrame that depents on a RFH
IPC and may come totally independently of accessibility IPCs.

Either way, I think a test-only callback really does make sense in this case
because
there aren't supposed to be any other consumers of BrowserAccessibilityManager
in the Chrome codebase. It's the part of the code that directly implements
native APIs, and this test just really needs to make sure it's got the whole
frame
tree when it does it.

https://codereview.chromium.org/1815933002/diff/40001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/1815933002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_manager.h:197: static void
SetFocusChangeCallbackForTesting(base::Closure callback);
On 2016/03/24 18:15:11, dcheng wrote:
> const base::Closure&
> 
> (base::Closure is movable now, but I think in general, we still prefer to pass
> by const T& unless there's some clear win for passing by value and using move
> semantics)

Done.

[dcheng, 2016-03-25 05:13:50]

lgtm

[dmazzoni, 2016-03-28 04:14:27]

dmazzoni@chromium.org changed reviewers:
+ fsamuel@chromium.org, phajdan.jr@chromium.org

[dmazzoni, 2016-03-28 04:14:28]

+phajdan.jr for content/public/test/
+fsamuel for chrome/browser/apps/guest_view/

[Fady Samuel, 2016-03-28 05:06:06]

lgtm

[Paweł Hajdan Jr., 2016-03-29 20:35:20]

LGTM w/comment (just checking)

https://codereview.chromium.org/1815933002/diff/100001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/1815933002/diff/100001/content/public/test/br...
content/public/test/browser_test_utils.cc:890:
BrowserAccessibilityManager::SetFocusChangeCallbackForTesting(
Is there a possibility for a race when the focus change happens before we reach
this line?

It depends on whether they both execute on the same thread or not, and I just
don't know.

[dmazzoni, 2016-03-29 20:37:24]

https://codereview.chromium.org/1815933002/diff/100001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/1815933002/diff/100001/content/public/test/br...
content/public/test/browser_test_utils.cc:890:
BrowserAccessibilityManager::SetFocusChangeCallbackForTesting(
On 2016/03/29 20:35:20, Paweł Hajdan Jr. wrote:
> Is there a possibility for a race when the focus change happens before we
reach
> this line?
> 
> It depends on whether they both execute on the same thread or not, and I just
> don't know.

Good question - the focus change also only happens on the
main thread so there's no worry about a race.

[dmazzoni, 2016-03-29 20:37:53]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-03-29 20:37:53]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/1815933002/#ps100001
(title: "Fix build dependencies")

[commit-bot: I haz the power, 2016-03-29 20:38:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1815933002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1815933002/100001

[commit-bot: I haz the power, 2016-03-29 20:52:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-29 20:52:54]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dmazzoni, 2016-03-29 21:29:11]

dmazzoni@chromium.org changed reviewers:
+ nasko@chromium.org

[dmazzoni, 2016-03-29 21:29:12]

+nasko for content/browser/frame_host

Oops, I forgot that dcheng@ doesn't own that directory.
(Why not?)

[nasko, 2016-03-29 21:35:44]

Mostly rubberstamp LGTM with one nit.

https://codereview.chromium.org/1815933002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1815933002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:459: void
SetBrowserPluginEmbedderAXTreeID(AXTreeIDRegistry::AXTreeID ax_tree_id);
nit: This is a simple setter method that is not part of the content/public API,
so use hacker_case.

[dmazzoni, 2016-03-29 21:42:20]

https://codereview.chromium.org/1815933002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1815933002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:459: void
SetBrowserPluginEmbedderAXTreeID(AXTreeIDRegistry::AXTreeID ax_tree_id);
On 2016/03/29 21:35:44, nasko (slow) wrote:
> nit: This is a simple setter method that is not part of the content/public
API,
> so use hacker_case.

Done.

[dmazzoni, 2016-03-29 21:42:52]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-03-29 21:42:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, dcheng@chromium.org, nasko@chromium.org,
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1815933002/#ps120001
(title: "Rename set_browser_plugin_embedder_ax_tree_id")

[commit-bot: I haz the power, 2016-03-29 21:43:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1815933002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1815933002/120001

[commit-bot: I haz the power, 2016-03-30 01:15:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-30 01:17:02]

Description was changed from

==========
Fix WebView accessibility

This makes the chrome://chrome-signin page accessible too.

BUG=557429,587187,453157
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix WebView accessibility

This makes the chrome://chrome-signin page accessible too.

BUG=557429,587187,453157
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/fdfa2e079cd79d0308211b0ad3b350bf0350d500
Cr-Commit-Position: refs/heads/master@{#383885}
==========

[commit-bot: I haz the power, 2016-03-30 01:17:03]

Patchset 7 (id:??) landed as
https://crrev.com/fdfa2e079cd79d0308211b0ad3b350bf0350d500
Cr-Commit-Position: refs/heads/master@{#383885}