Adds testing infrastructure to web_ui_browsertest to support testing HtmlDialogs.

Adds testing infrastructure to web_ui_browsertest to support testing HtmlDialogs.

Dialogs shown using ShowHtmlDialog are not added to any tab strip or as a browser window. This CL adds to web_ui_browsertest functions to grab the newly created WebUI instance and run tests on it. This also adds WebUI certificate viewer tests.

BUG=None
TEST=Adds tests CertificateViewerUITest.testDialogURL, CertificateViewerUITest.testCN, CertificateViewerUITest.testDetails.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102310

[flackr, 2011-09-09 15:11:03]

I am waiting on http://codereview.chromium.org/7670041/ to be able to enable
WebUI dialogs at runtime and run this test unconditionally. I just want to get
the review process started to speed up submission afterwards.

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/certifi...
File chrome/browser/ui/webui/certificate_viewer_ui_browsertest.cc (right):

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/certifi...
chrome/browser/ui/webui/certificate_viewer_ui_browsertest.cc:9: // #include
"chrome/browser/ui/webui/chrome_web_ui.h"
This will be uncommented before submission. I am waiting on
http://codereview.chromium.org/7670041/.

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/certifi...
chrome/browser/ui/webui/certificate_viewer_ui_browsertest.cc:24: //
ChromeWebUI::SetMoreWebUI(true);
This will be uncommented before submission. I am waiting on
http://codereview.chromium.org/7670041/.

[Sheridan Rawlins, 2011-09-09 15:47:49]

First pass at comments before heading in to the office.

We can chat later if you'd like - I can help remove the additional WEB_UI macros
and get your tests started from your Javascript test file.

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/web_ui_...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/web_ui_...
chrome/browser/ui/webui/web_ui_browsertest.cc:286: void
WebUIBrowserTest::Observe(int type,
I would prefer to see this and other logic broken out to its own class like
TestNavigationObserver was.

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/web_ui_...
chrome/browser/ui/webui/web_ui_browsertest.cc:289: if (type ==
chrome::NOTIFICATION_HTML_DIALOG_SHOWN) {
Even though only 2 cases, this looks like a switch.  Default can have
NOTREACHED().

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/web_ui_...
chrome/browser/ui/webui/web_ui_browsertest.cc:308: NOTREACHED() << "Unhandled
notification type";
Don't bloat with message that doesn't add info (you can tell why from the
line-no).

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/web_ui_...
File chrome/browser/ui/webui/web_ui_browsertest.h (right):

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/web_ui_...
chrome/browser/ui/webui/web_ui_browsertest.h:24: // These macros simplify the
declaration of simple javascript unit tests.
We shouldn't need any WEB_UI macros.

These macros should be torn down in favor of kicking off the tests from
javascript.  See chrome/test/data/print_preview.js for a complex example or
others such as .../async_gen.js

http://codereview.chromium.org/7861024/diff/1/chrome/test/data/webui/certific...
File chrome/test/data/webui/certificate_viewer_dialog_test.js (right):

http://codereview.chromium.org/7861024/diff/1/chrome/test/data/webui/certific...
chrome/test/data/webui/certificate_viewer_dialog_test.js:62: // Support
functions
I don't think you should need these - I would hope there's enough support as
long as you kick off the test from Javascript with TEST_F.

[Paweł Hajdan Jr., 2011-09-09 22:13:55]

Drive-by with testing comments.

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/certifi...
File chrome/browser/ui/webui/certificate_viewer_ui_browsertest.cc (right):

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/certifi...
chrome/browser/ui/webui/certificate_viewer_ui_browsertest.cc:16: #include
<iostream>
Do not #include <iostream>. This results in some static initializers.

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/web_ui_...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/web_ui_...
chrome/browser/ui/webui/web_ui_browsertest.cc:295: registrar_.Add(this,
content::NOTIFICATION_LOAD_STOP,
Is this possibly racy?

[flackr, 2011-09-15 18:41:52]

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/certifi...
File chrome/browser/ui/webui/certificate_viewer_ui_browsertest.cc (right):

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/certifi...
chrome/browser/ui/webui/certificate_viewer_ui_browsertest.cc:16: #include
<iostream>
On 2011/09/09 22:13:55, Paweł Hajdan Jr. wrote:
> Do not #include <iostream>. This results in some static initializers.

Oops, was using some cerr statements to help debug. Done.

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/web_ui_...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/web_ui_...
chrome/browser/ui/webui/web_ui_browsertest.cc:286: void
WebUIBrowserTest::Observe(int type,
On 2011/09/09 15:47:50, Sheridan Rawlins wrote:
> I would prefer to see this and other logic broken out to its own class like
> TestNavigationObserver was.

Done.

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/web_ui_...
chrome/browser/ui/webui/web_ui_browsertest.cc:289: if (type ==
chrome::NOTIFICATION_HTML_DIALOG_SHOWN) {
On 2011/09/09 15:47:50, Sheridan Rawlins wrote:
> Even though only 2 cases, this looks like a switch.  Default can have
> NOTREACHED().

Done.

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/web_ui_...
chrome/browser/ui/webui/web_ui_browsertest.cc:295: registrar_.Add(this,
content::NOTIFICATION_LOAD_STOP,
On 2011/09/09 22:13:55, Paweł Hajdan Jr. wrote:
> Is this possibly racy?

I don't so, but I'm not certain. The first notification,
NOTIFICATION_HTML_DIALOG_SHOWN is delivered as soon as the render view is
created. I will try to check if navigation is blocked on the render view
creation. I could listen to both events on construction but then I have to track
which NavigationControllers have completed loading until I know which one
belongs to the new WebUI instance. Do you think it would work to assume that the
next navigation during a dialog creation will be for the dialog and have a
DCHECK to fail if that's not the case? Alternately I could check for a dialog
WebUI instance (dynamic_cast?) on all completed navigations.

Btw, the code for this has been moved to test_html_dialog_observer.cc.

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/web_ui_...
chrome/browser/ui/webui/web_ui_browsertest.cc:308: NOTREACHED() << "Unhandled
notification type";
On 2011/09/09 15:47:50, Sheridan Rawlins wrote:
> Don't bloat with message that doesn't add info (you can tell why from the
> line-no).

Done.

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/web_ui_...
File chrome/browser/ui/webui/web_ui_browsertest.h (right):

http://codereview.chromium.org/7861024/diff/1/chrome/browser/ui/webui/web_ui_...
chrome/browser/ui/webui/web_ui_browsertest.h:24: // These macros simplify the
declaration of simple javascript unit tests.
On 2011/09/09 15:47:50, Sheridan Rawlins wrote:
> We shouldn't need any WEB_UI macros.
> 
> These macros should be torn down in favor of kicking off the tests from
> javascript.  See chrome/test/data/print_preview.js for a complex example or
> others such as .../async_gen.js

Done.

http://codereview.chromium.org/7861024/diff/1/chrome/test/data/webui/certific...
File chrome/test/data/webui/certificate_viewer_dialog_test.js (right):

http://codereview.chromium.org/7861024/diff/1/chrome/test/data/webui/certific...
chrome/test/data/webui/certificate_viewer_dialog_test.js:62: // Support
functions
On 2011/09/09 15:47:50, Sheridan Rawlins wrote:
> I don't think you should need these - I would hope there's enough support as
> long as you kick off the test from Javascript with TEST_F.

The issue I'm having is that the callback functions aren't running from a
context in which thrown errors are caught. The error is reported on the console
and the test waits indefinitely for testDone to be called. I suspect I am just
missing something though.

[Sheridan Rawlins, 2011-09-15 19:11:41]

http://codereview.chromium.org/7861024/diff/5001/chrome/browser/ui/webui/cert...
File chrome/browser/ui/webui/certificate_viewer_ui_browsertest.cc (right):

http://codereview.chromium.org/7861024/diff/5001/chrome/browser/ui/webui/cert...
chrome/browser/ui/webui/certificate_viewer_ui_browsertest.cc:32:
DCHECK(browser());
If these are tests, they should be ASSERT_TRUE; not DCHECK, so the test runner
doesn't fail.

http://codereview.chromium.org/7861024/diff/5001/chrome/test/data/webui/certi...
File chrome/test/data/webui/certificate_viewer_dialog_test.js (right):

http://codereview.chromium.org/7861024/diff/5001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:26: GEN('#include
"chrome/browser/ui/webui/certificate_viewer_ui_browsertest.cc"');
if this is included, please make it certificate_viewer_ui_browsertest-inl.h and
put it in the chrome_test.gypi.  When you do this, however, you'll need to
define any constructor/destructor here (for *_clang builds which will complain
of constructor/destructor in a header file.  I realize this current incarnation
works, but I like having EVERY file in the .gypi so that every file is available
in IDE's.

http://codereview.chromium.org/7861024/diff/5001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:34:
runAssertions('testDialogURL', function() {
I'm sorry, but why do you feel you need this?  TEST_F is already protected and
calls testDone().

This and the other TEST_F's should be as simple as:

TEST_F('CertificateViewerUITest', 'testDialogURL', function() {
  assertEquals(chrome.expectedUrl, window.location.href);
});

Ah, reading code below, I realize that these tests are not async, but the others
are.  The way I've handled this in the same file is to subclass.  Your subclass
should only override the isAsync and your tests should specify the correct
fixture for the async-ness of the test.

http://codereview.chromium.org/7861024/diff/5001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:45:
runAssertions('testCN', function() {
ditch runAssertions.

http://codereview.chromium.org/7861024/diff/5001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:70:
cert_viewer.getCertificateFields = function(certFieldDetails) {
You can use this.continueTest to wrap and ditch runAssertions below.

cert_viewer.getCertificateFields = this.continueTest(
    WhenTestDone.DEFAULT, function() {
      // ...
    });

http://codereview.chromium.org/7861024/diff/5001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:100: function
runAssertions(testName, f) {
Why is this needed?  Please remove this function.

http://codereview.chromium.org/7861024/diff/5001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:109: * @return
{Element} The first found element with a value, or null if not found.
{?Element}

http://codereview.chromium.org/7861024/diff/5001/chrome/test/test_html_dialog...
File chrome/test/test_html_dialog_observer.h (right):

http://codereview.chromium.org/7861024/diff/5001/chrome/test/test_html_dialog...
chrome/test/test_html_dialog_observer.h:37: };
DISALLOW_COPY_AND_ASSIGN(TestHtmlDialogObserver);

http://codereview.chromium.org/7861024/diff/5003/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_browsertest.h (right):

http://codereview.chromium.org/7861024/diff/5003/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.h:129: virtual void
SetWebUIInstance(WebUI* web_ui);
Do we need virtual here?

[flackr, 2011-09-15 21:02:36]

Thanks for all the help! The only thing that's a bit awkward is that the async
test has a different base test name, but I suppose it doesn't matter.

http://codereview.chromium.org/7861024/diff/5001/chrome/browser/ui/webui/cert...
File chrome/browser/ui/webui/certificate_viewer_ui_browsertest.cc (right):

http://codereview.chromium.org/7861024/diff/5001/chrome/browser/ui/webui/cert...
chrome/browser/ui/webui/certificate_viewer_ui_browsertest.cc:32:
DCHECK(browser());
On 2011/09/15 19:11:41, Sheridan Rawlins wrote:
> If these are tests, they should be ASSERT_TRUE; not DCHECK, so the test runner
> doesn't fail.

Done.

http://codereview.chromium.org/7861024/diff/5001/chrome/test/data/webui/certi...
File chrome/test/data/webui/certificate_viewer_dialog_test.js (right):

http://codereview.chromium.org/7861024/diff/5001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:26: GEN('#include
"chrome/browser/ui/webui/certificate_viewer_ui_browsertest.cc"');
On 2011/09/15 19:11:41, Sheridan Rawlins wrote:
> if this is included, please make it certificate_viewer_ui_browsertest-inl.h
and
> put it in the chrome_test.gypi.  When you do this, however, you'll need to
> define any constructor/destructor here (for *_clang builds which will complain
> of constructor/destructor in a header file.  I realize this current
incarnation
> works, but I like having EVERY file in the .gypi so that every file is
available
> in IDE's.

Done.

http://codereview.chromium.org/7861024/diff/5001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:34:
runAssertions('testDialogURL', function() {
On 2011/09/15 19:11:41, Sheridan Rawlins wrote:
> I'm sorry, but why do you feel you need this?  TEST_F is already protected and
> calls testDone().
> 
> This and the other TEST_F's should be as simple as:
> 
> TEST_F('CertificateViewerUITest', 'testDialogURL', function() {
>   assertEquals(chrome.expectedUrl, window.location.href);
> });
> 
> Ah, reading code below, I realize that these tests are not async, but the
others
> are.  The way I've handled this in the same file is to subclass.  Your
subclass
> should only override the isAsync and your tests should specify the correct
> fixture for the async-ness of the test.

Done.

http://codereview.chromium.org/7861024/diff/5001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:45:
runAssertions('testCN', function() {
On 2011/09/15 19:11:41, Sheridan Rawlins wrote:
> ditch runAssertions.

Done.

http://codereview.chromium.org/7861024/diff/5001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:70:
cert_viewer.getCertificateFields = function(certFieldDetails) {
On 2011/09/15 19:11:41, Sheridan Rawlins wrote:
> You can use this.continueTest to wrap and ditch runAssertions below.
> 
> cert_viewer.getCertificateFields = this.continueTest(
>     WhenTestDone.DEFAULT, function() {
>       // ...
>     });

Done.

http://codereview.chromium.org/7861024/diff/5001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:100: function
runAssertions(testName, f) {
On 2011/09/15 19:11:41, Sheridan Rawlins wrote:
> Why is this needed?  Please remove this function.

Done.

http://codereview.chromium.org/7861024/diff/5001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:109: * @return
{Element} The first found element with a value, or null if not found.
On 2011/09/15 19:11:41, Sheridan Rawlins wrote:
> {?Element}

Done.

http://codereview.chromium.org/7861024/diff/5001/chrome/test/test_html_dialog...
File chrome/test/test_html_dialog_observer.h (right):

http://codereview.chromium.org/7861024/diff/5001/chrome/test/test_html_dialog...
chrome/test/test_html_dialog_observer.h:37: };
On 2011/09/15 19:11:41, Sheridan Rawlins wrote:
> DISALLOW_COPY_AND_ASSIGN(TestHtmlDialogObserver);

Done.

http://codereview.chromium.org/7861024/diff/5003/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_browsertest.h (right):

http://codereview.chromium.org/7861024/diff/5003/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.h:129: virtual void
SetWebUIInstance(WebUI* web_ui);
On 2011/09/15 19:11:41, Sheridan Rawlins wrote:
> Do we need virtual here?

Unlikely. Done.

[Paweł Hajdan Jr., 2011-09-15 22:11:31]

http://codereview.chromium.org/7861024/diff/5004/chrome/test/test_html_dialog...
File chrome/test/test_html_dialog_observer.cc (right):

http://codereview.chromium.org/7861024/diff/5004/chrome/test/test_html_dialog...
chrome/test/test_html_dialog_observer.cc:34: // Wait for navigation on the new
WebUI instance to complete.
This might be racy as commented on before. Please do either a convincing
analysis of the code or register for the notification at some earlier point.

By the way, I think it should be pretty easy to just register info about all
NavigationControllers and then match it to the WebUI instance. Watch out for ABA
issues though.

http://codereview.chromium.org/7861024/diff/5004/chrome/test/test_html_dialog...
File chrome/test/test_html_dialog_observer.h (right):

http://codereview.chromium.org/7861024/diff/5004/chrome/test/test_html_dialog...
chrome/test/test_html_dialog_observer.h:5: #ifndef
CHROME_TEST_TEST_HTML_DIALOG_OBSERVER_H_
Do not add this directly under test. Use test/base.

[flackr, 2011-09-16 15:44:00]

http://codereview.chromium.org/7861024/diff/5004/chrome/test/test_html_dialog...
File chrome/test/test_html_dialog_observer.cc (right):

http://codereview.chromium.org/7861024/diff/5004/chrome/test/test_html_dialog...
chrome/test/test_html_dialog_observer.cc:34: // Wait for navigation on the new
WebUI instance to complete.
On 2011/09/15 22:11:31, Paweł Hajdan Jr. wrote:
> This might be racy as commented on before. Please do either a convincing
> analysis of the code or register for the notification at some earlier point.
> 
> By the way, I think it should be pretty easy to just register info about all
> NavigationControllers and then match it to the WebUI instance. Watch out for
ABA
> issues though.

If I register for all NavigationControllers then I would have to keep a set of
which NavigationControllers I've observed as it is possible that a navigation
occurs for another tab and until NOTIFICATION_HTML_DIALOG_SHOWN is issued I
don't know which NavigationController to watch for.

I did an analysis of the code and found that the render view is created (and
notification delivered resulting in NOTIFICATION_HTML_DIALOG_SHOWN) before
navigation takes place so there is no race condition. Since the navigate method
is called on the new render view host this should continue to be the case in the
future.

NOTIFICATION_HTML_DIALOG_SHOWN is issued on creation of the RenderView which is
the first step taken in TabContents::NavigateToEntry:
http://codesearch.google.com/#OAMlx_jo-ck/src/content/browser/tab_contents/ta...

Later in that method the newly created RenderViewHost is told to navigate:
http://codesearch.google.com/#OAMlx_jo-ck/src/content/browser/tab_contents/ta...

http://codereview.chromium.org/7861024/diff/5004/chrome/test/test_html_dialog...
File chrome/test/test_html_dialog_observer.h (right):

http://codereview.chromium.org/7861024/diff/5004/chrome/test/test_html_dialog...
chrome/test/test_html_dialog_observer.h:5: #ifndef
CHROME_TEST_TEST_HTML_DIALOG_OBSERVER_H_
On 2011/09/15 22:11:31, Paweł Hajdan Jr. wrote:
> Do not add this directly under test. Use test/base.

Done.

[Paweł Hajdan Jr., 2011-09-19 17:48:12]

http://codereview.chromium.org/7861024/diff/5004/chrome/test/test_html_dialog...
File chrome/test/test_html_dialog_observer.cc (right):

http://codereview.chromium.org/7861024/diff/5004/chrome/test/test_html_dialog...
chrome/test/test_html_dialog_observer.cc:34: // Wait for navigation on the new
WebUI instance to complete.
On 2011/09/16 15:44:00, flackr wrote:
> I did an analysis of the code and found that the render view is created (and
> notification delivered resulting in NOTIFICATION_HTML_DIALOG_SHOWN) before
> navigation takes place so there is no race condition.

Excellent. Could you please add the most important parts of that analysis to the
comment in the code? It would be very helpful when debugging in case the
behavior of TabContents changes.

[flackr, 2011-09-20 14:16:17]

Just added the comment to test_html_dialog_observer, other changes resulted from
merging.

http://codereview.chromium.org/7861024/diff/5004/chrome/test/test_html_dialog...
File chrome/test/test_html_dialog_observer.cc (right):

http://codereview.chromium.org/7861024/diff/5004/chrome/test/test_html_dialog...
chrome/test/test_html_dialog_observer.cc:34: // Wait for navigation on the new
WebUI instance to complete.
On 2011/09/19 17:48:12, Paweł Hajdan Jr. wrote:
> On 2011/09/16 15:44:00, flackr wrote:
> > I did an analysis of the code and found that the render view is created (and
> > notification delivered resulting in NOTIFICATION_HTML_DIALOG_SHOWN) before
> > navigation takes place so there is no race condition.
> 
> Excellent. Could you please add the most important parts of that analysis to
the
> comment in the code? It would be very helpful when debugging in case the
> behavior of TabContents changes.

Done. Hopefully it's not too wordy.

[Sheridan Rawlins, 2011-09-20 16:04:19]

http://codereview.chromium.org/7861024/diff/23001/chrome/browser/ui/webui/web...
File chrome/browser/ui/webui/web_ui_browsertest.h (right):

http://codereview.chromium.org/7861024/diff/23001/chrome/browser/ui/webui/web...
chrome/browser/ui/webui/web_ui_browsertest.h:186: WebUI* web_ui_instance_;
The comment should indicate that when it is non-NULL, then it is used, otherwise
the selected tab's web_ui is used.  Maybe it should be called something like
override_selected_web_ui_?

http://codereview.chromium.org/7861024/diff/23001/chrome/test/base/test_html_...
File chrome/test/base/test_html_dialog_observer.h (right):

http://codereview.chromium.org/7861024/diff/23001/chrome/test/base/test_html_...
chrome/test/base/test_html_dialog_observer.h:30: const NotificationDetails&
details);
OVERRIDE

http://codereview.chromium.org/7861024/diff/23001/chrome/test/data/webui/cert...
File chrome/test/data/webui/certificate_viewer_dialog_test.js (right):

http://codereview.chromium.org/7861024/diff/23001/chrome/test/data/webui/cert...
chrome/test/data/webui/certificate_viewer_dialog_test.js:47: function() {
Does this fit on previous line?

http://codereview.chromium.org/7861024/diff/23001/chrome/test/data/webui/cert...
chrome/test/data/webui/certificate_viewer_dialog_test.js:55: function() {
Does this fit on previous line?

http://codereview.chromium.org/7861024/diff/23001/chrome/test/data/webui/cert...
chrome/test/data/webui/certificate_viewer_dialog_test.js:65: function() {
Does this fit on previous line?

http://codereview.chromium.org/7861024/diff/23001/chrome/test/data/webui/cert...
chrome/test/data/webui/certificate_viewer_dialog_test.js:70:
this.continueTest(WhenTestDone.DEFAULT, function() {
Hmm...  I'm sad to see that you need to do continueTest(...)() within a TEST_F,
which I would expect to be protected and behave correctly on errors.

I think it would be more readable if TEST_F was always protected and failed on
errors.  That said, it would make sense to only fail on assertions; not expect
failures, and I think it would need to use the same code as in the continueTest
and deferRunTest, which was added after runTest and runTestFunction were
created.

Can you put a 
// TODO(scr) fix TEST_F to call testDone when assertions fail in async mode
here and in test_api.js above the runTest function?

Thanks.

http://codereview.chromium.org/7861024/diff/23001/chrome/test/data/webui/cert...
File chrome/test/data/webui/certificate_viewer_ui_test-inl.h (right):

http://codereview.chromium.org/7861024/diff/23001/chrome/test/data/webui/cert...
chrome/test/data/webui/certificate_viewer_ui_test-inl.h:25: virtual void
SetUpOnMainThread() {
private?  (I think since the superclass is public, this can/should be private).

Also add OVERRIDE .

[Paweł Hajdan Jr., 2011-09-20 17:41:16]

Code I commented in the drive-by LGTM, thanks.

[flackr, 2011-09-20 18:24:51]

http://codereview.chromium.org/7861024/diff/23001/chrome/browser/ui/webui/web...
File chrome/browser/ui/webui/web_ui_browsertest.h (right):

http://codereview.chromium.org/7861024/diff/23001/chrome/browser/ui/webui/web...
chrome/browser/ui/webui/web_ui_browsertest.h:186: WebUI* web_ui_instance_;
On 2011/09/20 16:04:19, Sheridan Rawlins wrote:
> The comment should indicate that when it is non-NULL, then it is used,
otherwise
> the selected tab's web_ui is used.  Maybe it should be called something like
> override_selected_web_ui_?

Done.

http://codereview.chromium.org/7861024/diff/23001/chrome/test/base/test_html_...
File chrome/test/base/test_html_dialog_observer.h (right):

http://codereview.chromium.org/7861024/diff/23001/chrome/test/base/test_html_...
chrome/test/base/test_html_dialog_observer.h:30: const NotificationDetails&
details);
On 2011/09/20 16:04:19, Sheridan Rawlins wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/7861024/diff/23001/chrome/test/data/webui/cert...
File chrome/test/data/webui/certificate_viewer_dialog_test.js (right):

http://codereview.chromium.org/7861024/diff/23001/chrome/test/data/webui/cert...
chrome/test/data/webui/certificate_viewer_dialog_test.js:47: function() {
On 2011/09/20 16:04:19, Sheridan Rawlins wrote:
> Does this fit on previous line?

Done.

http://codereview.chromium.org/7861024/diff/23001/chrome/test/data/webui/cert...
chrome/test/data/webui/certificate_viewer_dialog_test.js:55: function() {
On 2011/09/20 16:04:19, Sheridan Rawlins wrote:
> Does this fit on previous line?

Done.

http://codereview.chromium.org/7861024/diff/23001/chrome/test/data/webui/cert...
chrome/test/data/webui/certificate_viewer_dialog_test.js:65: function() {
On 2011/09/20 16:04:19, Sheridan Rawlins wrote:
> Does this fit on previous line?

Done.

http://codereview.chromium.org/7861024/diff/23001/chrome/test/data/webui/cert...
chrome/test/data/webui/certificate_viewer_dialog_test.js:70:
this.continueTest(WhenTestDone.DEFAULT, function() {
On 2011/09/20 16:04:19, Sheridan Rawlins wrote:
> Hmm...  I'm sad to see that you need to do continueTest(...)() within a
TEST_F,
> which I would expect to be protected and behave correctly on errors.
> 
> I think it would be more readable if TEST_F was always protected and failed on
> errors.  That said, it would make sense to only fail on assertions; not expect
> failures, and I think it would need to use the same code as in the
continueTest
> and deferRunTest, which was added after runTest and runTestFunction were
> created.
> 
> Can you put a 
> // TODO(scr) fix TEST_F to call testDone when assertions fail in async mode
> here and in test_api.js above the runTest function?
> 
> Thanks.

I agree, it would make sense to fail gracefully on assertions. I added the two
TODO's. Done.

http://codereview.chromium.org/7861024/diff/23001/chrome/test/data/webui/cert...
File chrome/test/data/webui/certificate_viewer_ui_test-inl.h (right):

http://codereview.chromium.org/7861024/diff/23001/chrome/test/data/webui/cert...
chrome/test/data/webui/certificate_viewer_ui_test-inl.h:25: virtual void
SetUpOnMainThread() {
On 2011/09/20 16:04:19, Sheridan Rawlins wrote:
> private?  (I think since the superclass is public, this can/should be
private).
> 
> Also add OVERRIDE .

Done.

[Sheridan Rawlins, 2011-09-20 19:23:18]

http://codereview.chromium.org/7861024/diff/29001/chrome/test/data/webui/test...
File chrome/test/data/webui/test_api.js (right):

http://codereview.chromium.org/7861024/diff/29001/chrome/test/data/webui/test...
chrome/test/data/webui/test_api.js:133: *            mode.
Please move comment below.

http://codereview.chromium.org/7861024/diff/29001/chrome/test/data/webui/test...
chrome/test/data/webui/test_api.js:749: function runTest(isAsync, testFunction,
testArguments) {
LGTM if you move the TODO(scr) to _this_ runTest.

Thanks.

[flackr, 2011-09-21 23:12:10]

I have to disable the certificate viewer tests for not posix or mac (these
platforms have OS certificate viewers). I did it in the same style as the dev
guide suggests handling failing tests for specific platforms. scr, let me know
if you think there's a better way to do this.

http://codereview.chromium.org/7861024/diff/29001/chrome/test/data/webui/test...
File chrome/test/data/webui/test_api.js (right):

http://codereview.chromium.org/7861024/diff/29001/chrome/test/data/webui/test...
chrome/test/data/webui/test_api.js:133: *            mode.
On 2011/09/20 19:23:19, Sheridan Rawlins wrote:
> Please move comment below.

Done.

http://codereview.chromium.org/7861024/diff/29001/chrome/test/data/webui/test...
chrome/test/data/webui/test_api.js:749: function runTest(isAsync, testFunction,
testArguments) {
On 2011/09/20 19:23:19, Sheridan Rawlins wrote:
> LGTM if you move the TODO(scr) to _this_ runTest.
> 
> Thanks.

Done.

[Sheridan Rawlins, 2011-09-22 01:27:42]

I can't think of a better way without pulling macro values into javascript,
which would be maintenance overhead.

Still LGTM.

-SCR

On 2011/09/21 23:12:10, flackr wrote:
> I have to disable the certificate viewer tests for not posix or mac (these
> platforms have OS certificate viewers). I did it in the same style as the dev
> guide suggests handling failing tests for specific platforms. scr, let me know
> if you think there's a better way to do this.
> 
>
http://codereview.chromium.org/7861024/diff/29001/chrome/test/data/webui/test...
> File chrome/test/data/webui/test_api.js (right):
> 
>
http://codereview.chromium.org/7861024/diff/29001/chrome/test/data/webui/test...
> chrome/test/data/webui/test_api.js:133: *            mode.
> On 2011/09/20 19:23:19, Sheridan Rawlins wrote:
> > Please move comment below.
> 
> Done.
> 
>
http://codereview.chromium.org/7861024/diff/29001/chrome/test/data/webui/test...
> chrome/test/data/webui/test_api.js:749: function runTest(isAsync,
testFunction,
> testArguments) {
> On 2011/09/20 19:23:19, Sheridan Rawlins wrote:
> > LGTM if you move the TODO(scr) to _this_ runTest.
> > 
> > Thanks.
> 
> Done.

[commit-bot: I haz the power, 2011-09-22 16:39:45]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/flackr%40chromium.org/7861024/29038

[commit-bot: I haz the power, 2011-09-22 18:06:32]

Change committed as 102310