html_viewer/web_view: An app for running layout-tests.

html_viewer/web_view: An app for running layout-tests.

The standalone app (layout_test_runner) can be used as a driver with the
run-webkit-tests script in blink to run the layout tests. A brief explanation
of how it works:
. The app launches mojo:web_view_test_runner.
. In system-check mode, the mojo:web_view_test_runner app terminates when the
  view-tree host comes up successfully, instead of loading any pages.
. LayoutTestHTMLViewer installs a callback on WebTestInterfaces to run a callback
  when the test stops running.
. When a test stops running, LayoutTestHTMLViewer notifies the
  mojo:web_view_test_runner app about it through the LayoutTestRunner interface.

Command:
  ./third_party/WebKit/Tools/Scripts/run-webkit-tests \
      --driver-name=layout_test_runner \
      --additional-driver-flag=--content-handlers=text/html,mojo:layout_test_html_viewer \
      --debug \
      --target=Debug_gn \
      fast/events/touch/create-touch-event.html

Only text-only tests are supported right now. Pixel/audio output are yet to be
done. Some stats:
 . fast/dom: 1434 passes out of 1963 tests.
 . fast/events: 449 passes out of 738 tests.
 . fast/events/touch: 99 passes out of 131 tests.

BUG=478250
R=jochen@chromium.org, rsleevi@chromium.org, sky@chromium.org

Committed: https://crrev.com/60cb54f6837ab9b7f0377b36fa7e33cd076ba088
Cr-Commit-Position: refs/heads/master@{#348053}

[sadrul, 2015-09-03 17:08:10]

sadrul@chromium.org changed reviewers:
+ jam@chromium.org, sky@chromium.org

[sky, 2015-09-03 19:25:29]

Is there a reason you want to use mandoline? Why not a dedicated app?

[sky, 2015-09-03 19:31:59]

https://codereview.chromium.org/1308973005/diff/140001/components/html_viewer...
File components/html_viewer/public/interfaces/layout_test_runner.mojom (right):

https://codereview.chromium.org/1308973005/diff/140001/components/html_viewer...
components/html_viewer/public/interfaces/layout_test_runner.mojom:7: interface
LayoutTestRunner {
Generally we put interfaces near the implementation, not the call site. As
LayoutTestRunner is implemented in test_runner, it shouldn't be here. Putting an
interface here makes sense if html_viewer is going to implement it.

https://codereview.chromium.org/1308973005/diff/140001/components/view_manage...
File components/view_manager/display_manager.cc (right):

https://codereview.chromium.org/1308973005/diff/140001/components/view_manage...
components/view_manager/display_manager.cc:161: dirty_rect_ = gfx::Rect();
Why do you need to reset the dirty_rect_ here? Assuming you need this, add a
comment.

[sadrul, 2015-09-03 19:47:44]

On 2015/09/03 19:25:29, sky wrote:
> Is there a reason you want to use mandoline? Why not a dedicated app?

Mostly for expediency, I suppose. I briefly considered a dedicated app
completely separate from mandoline, but I felt mandoline already does some
set-up that would be useful for the test-runner too. Maybe that's not the case?
I can take a closer look at that if you think that'd be useful. I suppose we
would still want to use web_view for this, and so perhaps this dedicated
layout-test-runner app can live in //components/web_view/ (or maybe even a new
//components/layout_test_runner ... would that be confusing with the existing
//components/test_runner?)

https://codereview.chromium.org/1308973005/diff/140001/components/html_viewer...
File components/html_viewer/public/interfaces/layout_test_runner.mojom (right):

https://codereview.chromium.org/1308973005/diff/140001/components/html_viewer...
components/html_viewer/public/interfaces/layout_test_runner.mojom:7: interface
LayoutTestRunner {
On 2015/09/03 19:31:59, sky wrote:
> Generally we put interfaces near the implementation, not the call site. As
> LayoutTestRunner is implemented in test_runner, it shouldn't be here. Putting
an
> interface here makes sense if html_viewer is going to implement it.

I added it here since LayoutTestHTMLViewer in html_viewer talks to it to notify
it when the test finished. Or would it be OK to introduce a dependency on the
test-runner (whether mandoline, or another dedicated app) to
layout_test_html_viewer?

[sky, 2015-09-03 19:52:24]

While the phone_ui is an easy place now to add that logic now, I suspect with
time the phone ui will evolve and not be as useful as a layout test runner. I
think it better to have a specific test_runner app, similar to how we have
content_shell and chrome now.

https://codereview.chromium.org/1308973005/diff/140001/components/html_viewer...
File components/html_viewer/public/interfaces/layout_test_runner.mojom (right):

https://codereview.chromium.org/1308973005/diff/140001/components/html_viewer...
components/html_viewer/public/interfaces/layout_test_runner.mojom:7: interface
LayoutTestRunner {
On 2015/09/03 19:47:44, sadrul wrote:
> On 2015/09/03 19:31:59, sky wrote:
> > Generally we put interfaces near the implementation, not the call site. As
> > LayoutTestRunner is implemented in test_runner, it shouldn't be here.
Putting
> an
> > interface here makes sense if html_viewer is going to implement it.
> 
> I added it here since LayoutTestHTMLViewer in html_viewer talks to it to
notify
> it when the test finished. Or would it be OK to introduce a dependency on the
> test-runner (whether mandoline, or another dedicated app) to
> layout_test_html_viewer?

I think the interface should belong with the test_runner.

[jam, 2015-09-03 20:47:22]

On 2015/09/03 19:52:24, sky wrote:
> While the phone_ui is an easy place now to add that logic now, I suspect with
> time the phone ui will evolve and not be as useful as a layout test runner. I
> think it better to have a specific test_runner app, similar to how we have
> content_shell and chrome now.

+1 for not putting this in mandoline. mandoline is the equivalent of chrome and
will have UI and other apps. no need to spin all of that up for layout tests.

[sadrul, 2015-09-03 21:49:00]

On 2015/09/03 20:47:22, jam wrote:
> On 2015/09/03 19:52:24, sky wrote:
> > While the phone_ui is an easy place now to add that logic now, I suspect
with
> > time the phone ui will evolve and not be as useful as a layout test runner.
I
> > think it better to have a specific test_runner app, similar to how we have
> > content_shell and chrome now.
> 
> +1 for not putting this in mandoline. mandoline is the equivalent of chrome
and
> will have UI and other apps. no need to spin all of that up for layout tests.

After discussing with sky@, moved the app into
//components/web_view/test_runner/. Should be easy to relocate if we want to in
the future.

https://codereview.chromium.org/1308973005/diff/140001/components/view_manage...
File components/view_manager/display_manager.cc (right):

https://codereview.chromium.org/1308973005/diff/140001/components/view_manage...
components/view_manager/display_manager.cc:161: dirty_rect_ = gfx::Rect();
On 2015/09/03 19:31:59, sky wrote:
> Why do you need to reset the dirty_rect_ here? Assuming you need this, add a
> comment.

Looks like I don't need this anymore. Removed.

[sky, 2015-09-03 23:01:02]

https://codereview.chromium.org/1308973005/diff/180001/components/web_view/te...
File components/web_view/test_runner/BUILD.gn (right):

https://codereview.chromium.org/1308973005/diff/180001/components/web_view/te...
components/web_view/test_runner/BUILD.gn:8: executable("test_runner") {
Add this as a DEP to /mandoline/BUILD.gn's tests deps. Otherwise you have to
explicitly build, which means it'll get broken.

https://codereview.chromium.org/1308973005/diff/180001/components/web_view/te...
File components/web_view/test_runner/test_runner_application_delegate.cc
(right):

https://codereview.chromium.org/1308973005/diff/180001/components/web_view/te...
components/web_view/test_runner/test_runner_application_delegate.cc:136:
web_view_inited_(false),
I would wrap web_view_ in a scoped_ptr and create in LaunchURL so that you don't
need the awkward inited_. But I suppose either work.

https://codereview.chromium.org/1308973005/diff/180001/components/web_view/te...
components/web_view/test_runner/test_runner_application_delegate.cc:137:
cmdline_position_(0) {}
0u

https://codereview.chromium.org/1308973005/diff/180001/components/web_view/te...
components/web_view/test_runner/test_runner_application_delegate.cc:155: if
(root_)
As the destructor also deals with root_, could you just as well delete this
here?

[sadrul, 2015-09-04 03:48:58]

https://codereview.chromium.org/1308973005/diff/180001/components/web_view/te...
File components/web_view/test_runner/BUILD.gn (right):

https://codereview.chromium.org/1308973005/diff/180001/components/web_view/te...
components/web_view/test_runner/BUILD.gn:8: executable("test_runner") {
On 2015/09/03 23:01:02, sky wrote:
> Add this as a DEP to /mandoline/BUILD.gn's tests deps. Otherwise you have to
> explicitly build, which means it'll get broken.

Done.

https://codereview.chromium.org/1308973005/diff/180001/components/web_view/te...
File components/web_view/test_runner/test_runner_application_delegate.cc
(right):

https://codereview.chromium.org/1308973005/diff/180001/components/web_view/te...
components/web_view/test_runner/test_runner_application_delegate.cc:136:
web_view_inited_(false),
On 2015/09/03 23:01:02, sky wrote:
> I would wrap web_view_ in a scoped_ptr and create in LaunchURL so that you
don't
> need the awkward inited_. But I suppose either work.

Agree. Done.

https://codereview.chromium.org/1308973005/diff/180001/components/web_view/te...
components/web_view/test_runner/test_runner_application_delegate.cc:137:
cmdline_position_(0) {}
On 2015/09/03 23:01:02, sky wrote:
> 0u

Done.

https://codereview.chromium.org/1308973005/diff/180001/components/web_view/te...
components/web_view/test_runner/test_runner_application_delegate.cc:155: if
(root_)
On 2015/09/03 23:01:02, sky wrote:
> As the destructor also deals with root_, could you just as well delete this
> here?

Unfortunately not. As an mojo::ApplicationDelegate, the
TestRunnerApplicationDelegate is owned by the mojo::ApplicationRunner. So
deleting this here causes things to explode.

[sky, 2015-09-04 14:56:04]

LGTM

https://codereview.chromium.org/1308973005/diff/210001/components/web_view/fr...
File components/web_view/frame_connection.cc (right):

https://codereview.chromium.org/1308973005/diff/210001/components/web_view/fr...
components/web_view/frame_connection.cc:98: mojo::Array<mojo::String>
test_runner_interfaces;
Do you think we should only do this in certain situations? Say a command line
switch?

[sky, 2015-09-04 18:54:07]

https://codereview.chromium.org/1308973005/diff/230001/components/web_view/te...
File components/web_view/test_runner/test_runner_application_delegate.cc
(right):

https://codereview.chromium.org/1308973005/diff/230001/components/web_view/te...
components/web_view/test_runner/test_runner_application_delegate.cc:145:
web_view_->Init(app_, content_);
I just changed around SetEmbedRoot(). You'll want to nuke the call to
SetEmbedRoot() and before you call Init() do:

content_->SetAccessPolicy(mojo::ViewTree::ACCESS_POLICY_EMBED_ROOT);

Although now that I type this I think the call should move to webview. I can
sort that out later though. Details are in this patch:
https://codereview.chromium.org/1317713006/

[sadrul, 2015-09-04 21:08:37]

Patchset #14 (id:250001) has been deleted

[sadrul, 2015-09-04 21:16:03]

https://codereview.chromium.org/1308973005/diff/210001/components/web_view/fr...
File components/web_view/frame_connection.cc (right):

https://codereview.chromium.org/1308973005/diff/210001/components/web_view/fr...
components/web_view/frame_connection.cc:98: mojo::Array<mojo::String>
test_runner_interfaces;
On 2015/09/04 14:56:04, sky wrote:
> Do you think we should only do this in certain situations? Say a command line
> switch?

Not sure ... probably not? I can't think of a case where that would be
useful/necessary.

https://codereview.chromium.org/1308973005/diff/230001/components/web_view/te...
File components/web_view/test_runner/test_runner_application_delegate.cc
(right):

https://codereview.chromium.org/1308973005/diff/230001/components/web_view/te...
components/web_view/test_runner/test_runner_application_delegate.cc:145:
web_view_->Init(app_, content_);
On 2015/09/04 18:54:07, sky wrote:
> I just changed around SetEmbedRoot(). You'll want to nuke the call to
> SetEmbedRoot() and before you call Init() do:
> 
> content_->SetAccessPolicy(mojo::ViewTree::ACCESS_POLICY_EMBED_ROOT);
> 
> Although now that I type this I think the call should move to webview. I can
> sort that out later though. Details are in this patch:
> https://codereview.chromium.org/1317713006/

Done, thanks.

[sadrul, 2015-09-08 16:48:41]

sadrul@chromium.org changed reviewers:
+ jochen@chromium.org

[sadrul, 2015-09-08 16:48:41]

+jochen@

Still need review from a test_runner owner. Sending this one to jochen@ too

[jochen (gone - plz use gerrit), 2015-09-09 14:34:46]

i'd prefer if the LayoutTestHTMLRunner would register the callback in
WebTestDelegateImpl instead of making the test runner library manage it, is that
possible?

[sadrul, 2015-09-09 17:17:48]

On 2015/09/09 14:34:46, jochen wrote:
> i'd prefer if the LayoutTestHTMLRunner would register the callback in
> WebTestDelegateImpl instead of making the test runner library manage it, is
that
> possible?

I am not sure I completely understood what you mean (I couldn't find a
LayoutTestHTMLRunner). But I have moved the changes out of test-runner, and
added the completion callback in html_viewer::WebTestDelegateImpl. So now, the
test-runner lib doesn't manage the callback, and it all happens in html_viewer
and web_view. I have also added data_deps on imagediff, dump_syms, and
minidump_stackwalk for web_view/test_runner to satiate run-webkit-tests. PTAL

[jochen (gone - plz use gerrit), 2015-09-09 20:40:38]

yes, sorry, that's what I meant

lgtm

[sadrul, 2015-09-09 23:40:33]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-09-09 23:40:34]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1308973005/#ps370001
(title: "tot/merge")

[commit-bot: I haz the power, 2015-09-09 23:40:58]

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

[commit-bot: I haz the power, 2015-09-09 23:52:35]

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

[commit-bot: I haz the power, 2015-09-09 23:52:36]

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...)

[sadrul, 2015-09-09 23:56:14]

sadrul@chromium.org changed reviewers:
+ rsleevi@chromium.org

[sadrul, 2015-09-09 23:56:15]

+rsleevi@ for +net in components/web_view/test_runner/DEPS

[Ryan Sleevi, 2015-09-10 00:23:50]

lgtm

[sadrul, 2015-09-10 00:41:12]

Committed patchset #19 (id:370001) manually as
60cb54f6837ab9b7f0377b36fa7e33cd076ba088.

[commit-bot: I haz the power, 2015-09-23 12:05:30]

Patchset 19 (id:??) landed as
https://crrev.com/60cb54f6837ab9b7f0377b36fa7e33cd076ba088
Cr-Commit-Position: refs/heads/master@{#348053}