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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1049013004: Add some simple HTMLViewer apptests. (Closed)

Created:
5 years, 8 months ago by msw
Modified:
4 years, 5 months ago
Reviewers:
sky
CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add some simple HTMLViewer apptests. BUG=

Patch Set 1 #

Patch Set 2 : Add an HTMLViewerTest interface, working on tests. #

Patch Set 3 : A working test! #

Patch Set 4 : Cleanup; seeing flaky hangs, crashes, etc. #

Patch Set 5 : Continue cleanup. #

Total comments: 6

Patch Set 6 : Delay AxProvider binding until the HTMLDocument has been loaded. #

Patch Set 7 : Cleanup; toying with an HTMLDocumentTestAPI for layoutTreeAsText. #

Patch Set 8 : Use net::SpawnedTestServer instead of mojo:http_server. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+368 lines, -49 lines) Patch
M BUILD.gn View 1 2 3 4 5 2 chunks +3 lines, -7 lines 0 comments Download
M mojo/application/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M mojo/application/application_test_main_chromium.cc View 1 2 3 4 5 6 7 2 chunks +7 lines, -0 lines 0 comments Download
M mojo/services/html_viewer/BUILD.gn View 1 2 3 4 5 6 7 2 chunks +22 lines, -0 lines 0 comments Download
M mojo/services/html_viewer/DEPS View 1 2 3 4 5 6 7 2 chunks +3 lines, -0 lines 0 comments Download
M mojo/services/html_viewer/ax_provider_impl.cc View 1 2 3 4 5 1 chunk +8 lines, -3 lines 0 comments Download
M mojo/services/html_viewer/ax_provider_impl_unittest.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M mojo/services/html_viewer/html_document.h View 1 2 3 4 5 6 4 chunks +19 lines, -6 lines 0 comments Download
M mojo/services/html_viewer/html_document.cc View 1 2 3 4 5 6 7 chunks +47 lines, -12 lines 0 comments Download
M mojo/services/html_viewer/html_viewer.cc View 1 2 3 4 5 6 9 chunks +70 lines, -10 lines 0 comments Download
A mojo/services/html_viewer/html_viewer_apptest.cc View 1 2 3 4 5 6 7 1 chunk +170 lines, -0 lines 0 comments Download
A + mojo/services/html_viewer/public/interfaces/BUILD.gn View 1 2 3 4 1 chunk +4 lines, -4 lines 0 comments Download
A + mojo/services/html_viewer/public/interfaces/html_viewer.mojom View 1 2 3 4 5 6 1 chunk +8 lines, -4 lines 0 comments Download
M third_party/mojo_services/src/http_server/public/cpp/http_server_util.h View 1 chunk +3 lines, -1 line 0 comments Download
M third_party/mojo_services/src/http_server/public/cpp/lib/http_server_util.cc View 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 4 (2 generated)
msw
Hey Scott, can you do a high-level review of this WIP CL?
5 years, 8 months ago (2015-04-02 00:36:56 UTC) #2
sky
5 years, 8 months ago (2015-04-02 16:56:17 UTC) #3 
https://codereview.chromium.org/1049013004/diff/80001/BUILD.gn
File BUILD.gn (left):

https://codereview.chromium.org/1049013004/diff/80001/BUILD.gn#oldcode579
BUILD.gn:579: if (!is_debug) {
Wow, this is weird. I wonder why we had these two in only release mode?

https://codereview.chromium.org/1049013004/diff/80001/mojo/services/html_view...
File mojo/services/html_viewer/BUILD.gn (right):

https://codereview.chromium.org/1049013004/diff/80001/mojo/services/html_view...
mojo/services/html_viewer/BUILD.gn:190: mojo_native_application("apptests") {
Shouldn't this be more specific, like html_viewer_apptests?

https://codereview.chromium.org/1049013004/diff/80001/mojo/services/html_view...
File mojo/services/html_viewer/html_viewer.cc (right):

https://codereview.chromium.org/1049013004/diff/80001/mojo/services/html_view...
mojo/services/html_viewer/html_viewer.cc:100:
service_provider_.AddService<mojo::HTMLViewerTestAPI>(this);
We should only do this if a command line switch is provided.

https://codereview.chromium.org/1049013004/diff/80001/mojo/services/html_view...
mojo/services/html_viewer/html_viewer.cc:137: wait_for_load_callback_ =
callback;
Log or DCHECK if already waiting?

https://codereview.chromium.org/1049013004/diff/80001/mojo/services/html_view...
mojo/services/html_viewer/html_viewer.cc:153: loaded_ = true;
Is this really loaded, or just created document?

https://codereview.chromium.org/1049013004/diff/80001/mojo/services/html_view...
File mojo/services/html_viewer/html_viewer_apptest.cc (right):

https://codereview.chromium.org/1049013004/diff/80001/mojo/services/html_view...
mojo/services/html_viewer/html_viewer_apptest.cc:22: class TestHttpServer :
public http_server::HttpHandler {
Why do you need your own server instead of using the http server all the chrome
tests use?

Powered by Google App Engine
This is Rietveld 408576698