Enable DumpAccessibilityTree tests to use cross-process iframes.

Enable DumpAccessibilityTree tests to use cross-process iframes.

Makes it possible for a DumpAccessibilityTree test to load an iframe
in a separate process simply by adding the text @CROSS-SITE: to the url,
for example:

  <iframe src="@CROSS-SITE:iframe.html"></iframe>

The test framework handles the details automatically including blocking
until the accessibility tree for the child frame finishes loading.
This significantly reduces the difficulty and complexity of adding new
tests for accessibility of out-of-process iframes.

This change also tries to improve the robustness of existing iframe
accessibility tests and re-enables a previously flaky test.

BUG=368298, 224659, 551601
Committed: https://crrev.com/ff8a7383fdcfa996b2c673fdaafbab2eedd05d8c
Cr-Commit-Position: refs/heads/master@{#370192}

[dmazzoni, 2016-01-05 22:57:05]

dmazzoni@chromium.org changed reviewers:
+ dcheng@chromium.org, dtseng@chromium.org, nasko@chromium.org, sky@chromium.org

[dmazzoni, 2016-01-05 22:57:05]

dtseng: primary reviewer
sky: content/test

nasko, dcheng: please review the changes to
dump_accessibility_browsertest_base.cc that navigate frames to remote sites.

[sky, 2016-01-05 23:23:49]

I did not look at content/test/data. OWNERS for that is '*', so one of your
other reviewers should review those changes.

LGTM

https://codereview.chromium.org/1552683002/diff/160001/content/test/accessibi...
File content/test/accessibility_browser_test_utils.cc (right):

https://codereview.chromium.org/1552683002/diff/160001/content/test/accessibi...
content/test/accessibility_browser_test_utils.cc:90: else
nit: no else after a return (see style guide).

[David Tseng, 2016-01-07 00:02:29]

lgtm

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager.cc:619: if
(!GetRoot())
When does this happen?

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
File content/browser/accessibility/dump_accessibility_browsertest_base.cc
(right):

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:63: //
Searches recursively and returns true if an accessibility node
nit: lines can fit more characters.

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:79: return
true;
You could negate the above if clauses and return the result directly.

[dcheng, 2016-01-07 00:07:11]

Seems reasonable to me: I'll defer to nasko@ for approval. I just had a few
nits.

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
File content/browser/accessibility/dump_accessibility_browsertest_base.cc
(right):

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:58: if (url
!= url::kAboutBlankURL)
Just curious: any particular reason to filter out about:blank?

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:269:
std::string host = base::StringPrintf("site%d.com", static_cast<int>(i));
Per http://google.github.io/styleguide/cppguide.html#64-bit_Portability, use
PRIuS from
https://code.google.com/p/chromium/codesearch#chromium/src/base/format_macros.h

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:304: for
(auto& url : all_frame_urls) {
const auto&, since mutability isn't required

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:315: for
(auto& str : wait_for) {
const auto&

[nasko, 2016-01-07 00:07:37]

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
File content/browser/accessibility/dump_accessibility_browsertest_base.cc
(right):

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:50: return
true;
nit: This is a small enough function that it can be made into a Lambda.

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:72: // not
possible with same-process iframes until bug 532249 is
nit: https://crbug.com/532249 as it will likify and avoid need to copy/paste the
bug number.

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:278: }
This loop is unnecessary as we have enough facilities to load frames cross
origin. Why not use the /cross-site/foo.com/path syntax?

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
File content/browser/accessibility/dump_accessibility_browsertest_base.h
(right):

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.h:23: class
DumpAccessibilityTestBase : public SitePerProcessBrowserTest {
nit: It seems that this tests uses only the setup of the test class. It might be
easier to just copy those over.

[dmazzoni, 2016-01-07 19:19:08]

Thanks for the great feedback. The use of lambdas and
/cross-site/ really improved this code.

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager.cc:619: if
(!GetRoot())
On 2016/01/07 00:02:29, David Tseng wrote:
> When does this happen?

In the middle of a tree update when we call something like OnNodeWillBeDeleted.
I've thought about rewriting that so that it precomputes the nodes that would be
deleted so it doesn't ever have to call back when the tree is broken.

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
File content/browser/accessibility/dump_accessibility_browsertest_base.cc
(right):

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:50: return
true;
On 2016/01/07 00:07:37, nasko wrote:
> nit: This is a small enough function that it can be made into a Lambda.

Done, though unfortunately it doesn't seem to pass lambdas with capture to
base::Bind.

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:58: if (url
!= url::kAboutBlankURL)
On 2016/01/07 00:07:11, dcheng wrote:
> Just curious: any particular reason to filter out about:blank?

Removed. I added this while debugging but it shouldn't actually be necessary.

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:63: //
Searches recursively and returns true if an accessibility node
On 2016/01/07 00:02:29, David Tseng wrote:
> nit: lines can fit more characters.

Done.

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:72: // not
possible with same-process iframes until bug 532249 is
On 2016/01/07 00:07:37, nasko wrote:
> nit: https://crbug.com/532249 as it will likify and avoid need to copy/paste
the
> bug number.

Done.

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:79: return
true;
On 2016/01/07 00:02:29, David Tseng wrote:
> You could negate the above if clauses and return the result directly.

Done.

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:269:
std::string host = base::StringPrintf("site%d.com", static_cast<int>(i));
On 2016/01/07 00:07:11, dcheng wrote:
> Per http://google.github.io/styleguide/cppguide.html#64-bit_Portability, use
> PRIuS from
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/format_macros.h

Moot due to comment below.

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:278: }
On 2016/01/07 00:07:37, nasko wrote:
> This loop is unnecessary as we have enough facilities to load frames cross
> origin. Why not use the /cross-site/foo.com/path syntax?

Great idea! For some reason I didn't think I could use it directly but it works
great. Much cleaner this way.

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:304: for
(auto& url : all_frame_urls) {
On 2016/01/07 00:07:11, dcheng wrote:
> const auto&, since mutability isn't required

Done.

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:315: for
(auto& str : wait_for) {
On 2016/01/07 00:07:11, dcheng wrote:
> const auto&

Done.

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
File content/browser/accessibility/dump_accessibility_browsertest_base.h
(right):

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.h:23: class
DumpAccessibilityTestBase : public SitePerProcessBrowserTest {
On 2016/01/07 00:07:37, nasko wrote:
> nit: It seems that this tests uses only the setup of the test class. It might
be
> easier to just copy those over.

Done.

https://codereview.chromium.org/1552683002/diff/160001/content/test/accessibi...
File content/test/accessibility_browser_test_utils.cc (right):

https://codereview.chromium.org/1552683002/diff/160001/content/test/accessibi...
content/test/accessibility_browser_test_utils.cc:90: else
On 2016/01/05 23:23:49, sky wrote:
> nit: no else after a return (see style guide).

Done.

[dmazzoni, 2016-01-07 21:37:47]

Unfortunately I had to revert the use of lambdas due to a VC++ bug:

http://stackoverflow.com/questions/26220448/operator-on-lambda

It would be possible to get it to work if I explicitly casted the lambda to the
right function pointer type and manually specified a calling convention, but
that would be far more verbose and eliminates all of the advantage of using
lambdas in the first place.

If you can think of another syntax that would work, let me know.

[nasko, 2016-01-08 22:57:45]

LGTM with couple of nits.

https://codereview.chromium.org/1552683002/diff/220001/content/browser/access...
File content/browser/accessibility/dump_accessibility_browsertest_base.cc
(right):

https://codereview.chromium.org/1552683002/diff/220001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:73: // not
possible with same-process iframes until http://crbug.com/532249
nit: httpS ;)

https://codereview.chromium.org/1552683002/diff/220001/content/test/data/acce...
File content/test/data/accessibility/aria/aria-dropeffect.html (right):

https://codereview.chromium.org/1552683002/diff/220001/content/test/data/acce...
content/test/data/accessibility/aria/aria-dropeffect.html:13: </ul>
nit: This file and a few others only differ in whitespace. Can those changes be
avoided?

[dmazzoni, 2016-01-11 19:04:41]

https://codereview.chromium.org/1552683002/diff/220001/content/browser/access...
File content/browser/accessibility/dump_accessibility_browsertest_base.cc
(right):

https://codereview.chromium.org/1552683002/diff/220001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:73: // not
possible with same-process iframes until http://crbug.com/532249
On 2016/01/08 22:57:45, nasko wrote:
> nit: httpS ;)

Done.

https://codereview.chromium.org/1552683002/diff/220001/content/test/data/acce...
File content/test/data/accessibility/aria/aria-dropeffect.html (right):

https://codereview.chromium.org/1552683002/diff/220001/content/test/data/acce...
content/test/data/accessibility/aria/aria-dropeffect.html:13: </ul>
On 2016/01/08 22:57:45, nasko wrote:
> nit: This file and a few others only differ in whitespace. Can those changes
be
> avoided?

Done.

[dmazzoni, 2016-01-11 19:40:53]

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
File content/browser/accessibility/dump_accessibility_browsertest_base.cc
(right):

https://codereview.chromium.org/1552683002/diff/160001/content/browser/access...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:58: if (url
!= url::kAboutBlankURL)
On 2016/01/07 19:19:08, dmazzoni wrote:
> On 2016/01/07 00:07:11, dcheng wrote:
> > Just curious: any particular reason to filter out about:blank?
> 
> Removed. I added this while debugging but it shouldn't actually be necessary.

Turns out this was needed for one test.

Consider the case where a parent frame A has a child iframe B and it writes to
the document using contentDocument.open() on the child frame B.

In this scenario, B's contentWindow.location.href matches A's url, but B's url
in the browser frame tree is still "about:blank".

I added a comment explaining this special case.

[dmazzoni, 2016-01-11 20:15:01]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-01-11 20:15:02]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
sky@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1552683002/#ps260001
(title: "Fix Win tests by adding back about:blank special case")

[commit-bot: I haz the power, 2016-01-11 20:16:16]

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

[commit-bot: I haz the power, 2016-01-12 02:00:53]

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

[commit-bot: I haz the power, 2016-01-12 02:00:54]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dmazzoni, 2016-01-12 04:10:35]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-01-12 04:11:18]

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

[commit-bot: I haz the power, 2016-01-12 10:56:19]

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

[commit-bot: I haz the power, 2016-01-12 10:56:20]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dmazzoni, 2016-01-13 21:15:22]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-01-13 21:15:22]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
nasko@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1552683002/#ps280001
(title: "Don't assume browser accessibility manager exists initially due to
Android initialization path")

[commit-bot: I haz the power, 2016-01-13 21:17:32]

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

[dmazzoni, 2016-01-13 22:48:56]

The CQ bit was unchecked by dmazzoni@chromium.org

[dmazzoni, 2016-01-13 22:53:08]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-01-13 22:53:08]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
nasko@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1552683002/#ps300001
(title: "Rebase")

[commit-bot: I haz the power, 2016-01-13 22:56:44]

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

[commit-bot: I haz the power, 2016-01-14 01:09:59]

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

[commit-bot: I haz the power, 2016-01-14 01:10:00]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[dmazzoni, 2016-01-14 19:48:16]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-01-14 19:49:50]

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

[dmazzoni, 2016-01-16 00:37:52]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-01-16 00:37:52]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
nasko@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1552683002/#ps340001
(title: "Fix Android tests")

[commit-bot: I haz the power, 2016-01-16 00:38:22]

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

[commit-bot: I haz the power, 2016-01-16 01:22:11]

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

[commit-bot: I haz the power, 2016-01-16 01:22:12]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dmazzoni, 2016-01-19 21:27:14]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-01-19 21:27:15]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
nasko@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1552683002/#ps380001
(title: "Rebaseline last Android test")

[commit-bot: I haz the power, 2016-01-19 21:29:12]

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

[commit-bot: I haz the power, 2016-01-19 21:36:29]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-01-19 21:37:52]

Description was changed from

==========
Enable DumpAccessibilityTree tests to use cross-process iframes.

Makes it possible for a DumpAccessibilityTree test to load an iframe
in a separate process simply by adding the text @CROSS-SITE: to the url,
for example:

  <iframe src="@CROSS-SITE:iframe.html"></iframe>

The test framework handles the details automatically including blocking
until the accessibility tree for the child frame finishes loading.
This significantly reduces the difficulty and complexity of adding new
tests for accessibility of out-of-process iframes.

This change also tries to improve the robustness of existing iframe
accessibility tests and re-enables a previously flaky test.

BUG=368298, 224659, 551601
==========

to

==========
Enable DumpAccessibilityTree tests to use cross-process iframes.

Makes it possible for a DumpAccessibilityTree test to load an iframe
in a separate process simply by adding the text @CROSS-SITE: to the url,
for example:

  <iframe src="@CROSS-SITE:iframe.html"></iframe>

The test framework handles the details automatically including blocking
until the accessibility tree for the child frame finishes loading.
This significantly reduces the difficulty and complexity of adding new
tests for accessibility of out-of-process iframes.

This change also tries to improve the robustness of existing iframe
accessibility tests and re-enables a previously flaky test.

BUG=368298, 224659, 551601

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

[commit-bot: I haz the power, 2016-01-19 21:37:53]

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