Allow WebUI Tests to use preLoad in HtmlDialogUI.

Allow WebUI Tests to use preLoad in HtmlDialogUI.

- Refactor JsInjectionReadyObserver to its own header/impl files.
- Call JsInjectionReadyObserver::OnJsInjectionReady when the dialog
  is shown.
- Move preamble generation to after the AddLibrary calls & generate setting
  of preload_* variables rather than passing in browsePreload and
  browsePrintPreload.
- Modify tests that show HtmlDialogUI to show the dialogs using preamble
  rather than SetUpOnMainThread so AddLibrary & set_preload_* calls precede
  showing the dialog (so preload is called against the appropriate fixture).

R=flackr@chromium.org, jhawkins@chromium.org
BUG=104371
TEST=browser_tests --gtest_filter=Hung*.* -AND- CertificateViewerUITest*.*


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

[Sheridan Rawlins, 2011-11-17 00:26:30]

The trick here was allowing the dialog to be shown after the AddLibrary and test
fixture/name were set up.  While in there, also moved orphan
chrome/test/test_navigation_observer.* files into chrome/test/base/.

[Paweł Hajdan Jr., 2011-11-17 09:47:10]

Drive-by with a testing comment (by someone who has done chrome/test/base and
content/test as you probably know).

http://codereview.chromium.org/8586009/diff/3024/chrome/test/base/test_naviga...
File chrome/test/base/test_navigation_observer.h (right):

http://codereview.chromium.org/8586009/diff/3024/chrome/test/base/test_naviga...
chrome/test/base/test_navigation_observer.h:5: #ifndef
CHROME_TEST_BASE_TEST_NAVIGATION_OBSERVER_H_
If you're moving this, could you rather move it to content/test, and move any
possible chrome dependencies along (there shouldn't be too many) ?

[flackr, 2011-11-17 14:47:27]

LGTM otherwise.

[Sheridan Rawlins, 2011-11-17 16:46:25]

http://codereview.chromium.org/8586009/diff/3024/chrome/test/base/test_naviga...
File chrome/test/base/test_navigation_observer.h (right):

http://codereview.chromium.org/8586009/diff/3024/chrome/test/base/test_naviga...
chrome/test/base/test_navigation_observer.h:5: #ifndef
CHROME_TEST_BASE_TEST_NAVIGATION_OBSERVER_H_
I don't feel comfortable adding that to this scope & here's why: The major
chrome dependency is on ui_test_utils::RunMessageLoop().  While this _can_ be
moved over, it will affect many files and should be a CL of its own.  If you
feel strongly about moving more test framework to content, please file a bug.

On 2011/11/17 09:47:10, Paweł Hajdan Jr. wrote:
> If you're moving this, could you rather move it to content/test, and move any
> possible chrome dependencies along (there shouldn't be too many) ?

[Paweł Hajdan Jr., 2011-11-17 17:11:42]

http://codereview.chromium.org/8586009/diff/3024/chrome/test/base/test_naviga...
File chrome/test/base/test_navigation_observer.h (right):

http://codereview.chromium.org/8586009/diff/3024/chrome/test/base/test_naviga...
chrome/test/base/test_navigation_observer.h:5: #ifndef
CHROME_TEST_BASE_TEST_NAVIGATION_OBSERVER_H_
On 2011/11/17 16:46:25, Sheridan Rawlins wrote:
> I don't feel comfortable adding that to this scope & here's why: The major
> chrome dependency is on ui_test_utils::RunMessageLoop().  While this _can_ be
> moved over, it will affect many files and should be a CL of its own.  If you
> feel strongly about moving more test framework to content, please file a bug.

You're moving this anyway, so it really makes sense to move it to the right
place.

You could solve the "too amny places depend on RunMessageLoop" by making the
chrome RunMessageLoop call the content RunMessageLoop until all call places are
transitioned.

[Sheridan Rawlins, 2011-11-17 23:20:35]

http://codereview.chromium.org/8586009/diff/3024/chrome/test/base/test_naviga...
File chrome/test/base/test_navigation_observer.h (right):

http://codereview.chromium.org/8586009/diff/3024/chrome/test/base/test_naviga...
chrome/test/base/test_navigation_observer.h:5: #ifndef
CHROME_TEST_BASE_TEST_NAVIGATION_OBSERVER_H_
On 2011/11/17 17:11:42, Paweł Hajdan Jr. wrote:
> On 2011/11/17 16:46:25, Sheridan Rawlins wrote:
> > I don't feel comfortable adding that to this scope & here's why: The major
> > chrome dependency is on ui_test_utils::RunMessageLoop().  While this _can_
be
> > moved over, it will affect many files and should be a CL of its own.  If you
> > feel strongly about moving more test framework to content, please file a
bug.
> 
> You're moving this anyway, so it really makes sense to move it to the right
> place.
> 
> You could solve the "too amny places depend on RunMessageLoop" by making the
> chrome RunMessageLoop call the content RunMessageLoop until all call places
are
> transitioned.

Thanks for the suggestion.  Filed http://crbug.com/104677 for the migration

Done.

[Sheridan Rawlins, 2011-11-18 05:17:34]

http://codereview.chromium.org/8586009/diff/3024/chrome/test/base/test_naviga...
File chrome/test/base/test_navigation_observer.h (right):

http://codereview.chromium.org/8586009/diff/3024/chrome/test/base/test_naviga...
chrome/test/base/test_navigation_observer.h:5: #ifndef
CHROME_TEST_BASE_TEST_NAVIGATION_OBSERVER_H_
Argh...  Now I'm hitting check_deps because of the dependency on views...

I really don't think this is worth the trouble for this bug.  I will roll back
this move.

On 2011/11/17 23:20:35, Sheridan Rawlins wrote:
> On 2011/11/17 17:11:42, Paweł Hajdan Jr. wrote:
> > On 2011/11/17 16:46:25, Sheridan Rawlins wrote:
> > > I don't feel comfortable adding that to this scope & here's why: The major
> > > chrome dependency is on ui_test_utils::RunMessageLoop().  While this _can_
> be
> > > moved over, it will affect many files and should be a CL of its own.  If
you
> > > feel strongly about moving more test framework to content, please file a
> bug.
> > 
> > You're moving this anyway, so it really makes sense to move it to the right
> > place.
> > 
> > You could solve the "too amny places depend on RunMessageLoop" by making the
> > chrome RunMessageLoop call the content RunMessageLoop until all call places
> are
> > transitioned.
> 
> Thanks for the suggestion.  Filed http://crbug.com/104677 for the migration
> 
> Done.

[Paweł Hajdan Jr., 2011-11-21 08:57:25]

http://codereview.chromium.org/8586009/diff/3024/chrome/test/base/test_naviga...
File chrome/test/base/test_navigation_observer.h (right):

http://codereview.chromium.org/8586009/diff/3024/chrome/test/base/test_naviga...
chrome/test/base/test_navigation_observer.h:5: #ifndef
CHROME_TEST_BASE_TEST_NAVIGATION_OBSERVER_H_
On 2011/11/18 05:17:34, Sheridan Rawlins wrote:
> Argh...  Now I'm hitting check_deps because of the dependency on views...
> 
> I really don't think this is worth the trouble for this bug.  I will roll back
> this move.

Okay, feel free to commit without this move (LGTM). However, I'm really
surprised by this. Can't content use views? Maybe it'd be OK to add that
dependency.

> On 2011/11/17 23:20:35, Sheridan Rawlins wrote:
> > On 2011/11/17 17:11:42, Paweł Hajdan Jr. wrote:
> > > On 2011/11/17 16:46:25, Sheridan Rawlins wrote:
> > > > I don't feel comfortable adding that to this scope & here's why: The
major
> > > > chrome dependency is on ui_test_utils::RunMessageLoop().  While this
_can_
> > be
> > > > moved over, it will affect many files and should be a CL of its own.  If
> you
> > > > feel strongly about moving more test framework to content, please file a
> > bug.
> > > 
> > > You're moving this anyway, so it really makes sense to move it to the
right
> > > place.
> > > 
> > > You could solve the "too amny places depend on RunMessageLoop" by making
the
> > > chrome RunMessageLoop call the content RunMessageLoop until all call
places
> > are
> > > transitioned.
> > 
> > Thanks for the suggestion.  Filed http://crbug.com/104677 for the migration
> > 
> > Done.
>

[James Hawkins, 2011-11-22 23:03:29]

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

http://codereview.chromium.org/8586009/diff/9001/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.h:121: preload_test_fixture_ =
preload_test_fixture;
Move the implementation the .cc file.

http://codereview.chromium.org/8586009/diff/9001/chrome/test/base/js2gtest.js
File chrome/test/base/js2gtest.js (right):

http://codereview.chromium.org/8586009/diff/9001/chrome/test/base/js2gtest.js...
chrome/test/base/js2gtest.js:70: * When true, add calls to
set_preload_test_(fixture|name).
Document the rationale.

http://codereview.chromium.org/8586009/diff/9001/chrome/test/base/js_injectio...
File chrome/test/base/js_injection_ready_observer.h (right):

http://codereview.chromium.org/8586009/diff/9001/chrome/test/base/js_injectio...
chrome/test/base/js_injection_ready_observer.h:19: virtual void
OnJsInjectionReady(RenderViewHost* render_view_host) = 0;
Document the parameter.

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

http://codereview.chromium.org/8586009/diff/9001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:25: GEN(' 
ShowCertificateViewer();');
What's with the extra whitespace at the beginning?

http://codereview.chromium.org/8586009/diff/9001/chrome/test/data/webui/certi...
File chrome/test/data/webui/certificate_viewer_ui_test-inl.h (right):

http://codereview.chromium.org/8586009/diff/9001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_ui_test-inl.h:26: virtual void
ShowCertificateViewer();
Why is this virtual?

[Sheridan Rawlins, 2011-11-23 20:10:06]

http://codereview.chromium.org/8586009/diff/3024/chrome/test/base/test_naviga...
File chrome/test/base/test_navigation_observer.h (right):

http://codereview.chromium.org/8586009/diff/3024/chrome/test/base/test_naviga...
chrome/test/base/test_navigation_observer.h:5: #ifndef
CHROME_TEST_BASE_TEST_NAVIGATION_OBSERVER_H_
FYI, filed http://crbug.com/105213 for followup.  content can NOT use views, but
we may have a pattern to allow the ui_test_utils::RunMessageLoop to remain in
chrome/test but integrate with the content/test observation utilities, which
I'll investigate (not in the scope of this CL).

On 2011/11/21 08:57:25, Paweł Hajdan Jr. wrote:
> On 2011/11/18 05:17:34, Sheridan Rawlins wrote:
> > Argh...  Now I'm hitting check_deps because of the dependency on views...
> > 
> > I really don't think this is worth the trouble for this bug.  I will roll
back
> > this move.
> 
> Okay, feel free to commit without this move (LGTM). However, I'm really
> surprised by this. Can't content use views? Maybe it'd be OK to add that
> dependency.
> 
> > On 2011/11/17 23:20:35, Sheridan Rawlins wrote:
> > > On 2011/11/17 17:11:42, Paweł Hajdan Jr. wrote:
> > > > On 2011/11/17 16:46:25, Sheridan Rawlins wrote:
> > > > > I don't feel comfortable adding that to this scope & here's why: The
> major
> > > > > chrome dependency is on ui_test_utils::RunMessageLoop().  While this
> _can_
> > > be
> > > > > moved over, it will affect many files and should be a CL of its own. 
If
> > you
> > > > > feel strongly about moving more test framework to content, please file
a
> > > bug.
> > > > 
> > > > You're moving this anyway, so it really makes sense to move it to the
> right
> > > > place.
> > > > 
> > > > You could solve the "too amny places depend on RunMessageLoop" by making
> the
> > > > chrome RunMessageLoop call the content RunMessageLoop until all call
> places
> > > are
> > > > transitioned.
> > > 
> > > Thanks for the suggestion.  Filed http://crbug.com/104677 for the
migration
> > > 
> > > Done.
> > 
>

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

http://codereview.chromium.org/8586009/diff/9001/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.h:121: preload_test_fixture_ =
preload_test_fixture;
On 2011/11/22 23:03:29, James Hawkins wrote:
> Move the implementation the .cc file.

Done.

http://codereview.chromium.org/8586009/diff/9001/chrome/test/base/js2gtest.js
File chrome/test/base/js2gtest.js (right):

http://codereview.chromium.org/8586009/diff/9001/chrome/test/base/js2gtest.js...
chrome/test/base/js2gtest.js:70: * When true, add calls to
set_preload_test_(fixture|name).
On 2011/11/22 23:03:29, James Hawkins wrote:
> Document the rationale.

Done.

http://codereview.chromium.org/8586009/diff/9001/chrome/test/base/js_injectio...
File chrome/test/base/js_injection_ready_observer.h (right):

http://codereview.chromium.org/8586009/diff/9001/chrome/test/base/js_injectio...
chrome/test/base/js_injection_ready_observer.h:19: virtual void
OnJsInjectionReady(RenderViewHost* render_view_host) = 0;
On 2011/11/22 23:03:29, James Hawkins wrote:
> Document the parameter.

Done.

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

http://codereview.chromium.org/8586009/diff/9001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:25: GEN(' 
ShowCertificateViewer();');
On 2011/11/22 23:03:29, James Hawkins wrote:
> What's with the extra whitespace at the beginning?

It outputs with proper indentation for the C++.  I'm not sure we need the
temporary C++ to show proper style, only valid C++ syntax.  Let me know if you
want me to scrap the indentation.

http://codereview.chromium.org/8586009/diff/9001/chrome/test/data/webui/certi...
File chrome/test/data/webui/certificate_viewer_ui_test-inl.h (right):

http://codereview.chromium.org/8586009/diff/9001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_ui_test-inl.h:26: virtual void
ShowCertificateViewer();
On 2011/11/22 23:03:29, James Hawkins wrote:
> Why is this virtual?

No reason - it was left over from when it overrode SetUpOnMainThread().

Done.

[James Hawkins, 2011-11-23 20:29:22]

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

http://codereview.chromium.org/8586009/diff/9001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:25: GEN(' 
ShowCertificateViewer();');
On 2011/11/23 20:10:07, Sheridan Rawlins wrote:
> On 2011/11/22 23:03:29, James Hawkins wrote:
> > What's with the extra whitespace at the beginning?
> 
> It outputs with proper indentation for the C++.  I'm not sure we need the
> temporary C++ to show proper style, only valid C++ syntax.  Let me know if you
> want me to scrap the indentation.

Yes, get rid of the indentation here and elsewhere.

[Sheridan Rawlins, 2011-11-24 01:17:46]

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

http://codereview.chromium.org/8586009/diff/9001/chrome/test/data/webui/certi...
chrome/test/data/webui/certificate_viewer_dialog_test.js:25: GEN(' 
ShowCertificateViewer();');
On 2011/11/23 20:29:22, James Hawkins wrote:
> On 2011/11/23 20:10:07, Sheridan Rawlins wrote:
> > On 2011/11/22 23:03:29, James Hawkins wrote:
> > > What's with the extra whitespace at the beginning?
> > 
> > It outputs with proper indentation for the C++.  I'm not sure we need the
> > temporary C++ to show proper style, only valid C++ syntax.  Let me know if
you
> > want me to scrap the indentation.
> 
> Yes, get rid of the indentation here and elsewhere.

Done.

[James Hawkins, 2011-11-27 19:28:26]

http://codereview.chromium.org/8586009/diff/23001/chrome/test/base/js_injecti...
File chrome/test/base/js_injection_ready_observer.h (right):

http://codereview.chromium.org/8586009/diff/23001/chrome/test/base/js_injecti...
chrome/test/base/js_injection_ready_observer.h:15: virtual
~JsInjectionReadyObserver();
Move destructor to protected.

http://codereview.chromium.org/8586009/diff/23001/chrome/test/data/webui/asyn...
File chrome/test/data/webui/async_gen.cc (right):

http://codereview.chromium.org/8586009/diff/23001/chrome/test/data/webui/asyn...
chrome/test/data/webui/async_gen.cc:33: "tornDown",
base::Bind(&AsyncWebUIMessageHandler::HandleTornDown,
"tornDown" This should probably be something similar to "onTornDown" to clarify
that it's a notification.

Or it should be "tearDown" to match the voice of "callJS".

http://codereview.chromium.org/8586009/diff/23001/chrome/test/data/webui/asyn...
File chrome/test/data/webui/async_gen.h (right):

http://codereview.chromium.org/8586009/diff/23001/chrome/test/data/webui/asyn...
chrome/test/data/webui/async_gen.h:12: 
Remove blank lines around the forward declaration.

http://codereview.chromium.org/8586009/diff/23001/chrome/test/data/webui/asyn...
chrome/test/data/webui/async_gen.h:34: // Implements WebUIMessageHandler.
// WebUIMessageHandler implementation.

http://codereview.chromium.org/8586009/diff/23001/chrome/test/data/webui/prin...
File chrome/test/data/webui/print_preview.h (right):

http://codereview.chromium.org/8586009/diff/23001/chrome/test/data/webui/prin...
chrome/test/data/webui/print_preview.h:20: // WebUIBrowserTest override.
// WebUIBrowserTest implementation.

[Sheridan Rawlins, 2011-11-29 06:57:16]

http://codereview.chromium.org/8586009/diff/23001/chrome/test/base/js_injecti...
File chrome/test/base/js_injection_ready_observer.h (right):

http://codereview.chromium.org/8586009/diff/23001/chrome/test/base/js_injecti...
chrome/test/base/js_injection_ready_observer.h:15: virtual
~JsInjectionReadyObserver();
Also moved constructor to adhere to interface requirements.

Done.

On 2011/11/27 19:28:26, James Hawkins wrote:
> Move destructor to protected.

http://codereview.chromium.org/8586009/diff/23001/chrome/test/data/webui/asyn...
File chrome/test/data/webui/async_gen.cc (right):

http://codereview.chromium.org/8586009/diff/23001/chrome/test/data/webui/asyn...
chrome/test/data/webui/async_gen.cc:33: "tornDown",
base::Bind(&AsyncWebUIMessageHandler::HandleTornDown,
I chose "tearDown".

Done.

On 2011/11/27 19:28:26, James Hawkins wrote:
> "tornDown" This should probably be something similar to "onTornDown" to
clarify
> that it's a notification.
> 
> Or it should be "tearDown" to match the voice of "callJS".

http://codereview.chromium.org/8586009/diff/23001/chrome/test/data/webui/asyn...
File chrome/test/data/webui/async_gen.h (right):

http://codereview.chromium.org/8586009/diff/23001/chrome/test/data/webui/asyn...
chrome/test/data/webui/async_gen.h:12: 
On 2011/11/27 19:28:26, James Hawkins wrote:
> Remove blank lines around the forward declaration.

Done.

http://codereview.chromium.org/8586009/diff/23001/chrome/test/data/webui/asyn...
chrome/test/data/webui/async_gen.h:34: // Implements WebUIMessageHandler.
On 2011/11/27 19:28:26, James Hawkins wrote:
> // WebUIMessageHandler implementation.

Done.

http://codereview.chromium.org/8586009/diff/23001/chrome/test/data/webui/prin...
File chrome/test/data/webui/print_preview.h (right):

http://codereview.chromium.org/8586009/diff/23001/chrome/test/data/webui/prin...
chrome/test/data/webui/print_preview.h:20: // WebUIBrowserTest override.
On 2011/11/27 19:28:26, James Hawkins wrote:
> // WebUIBrowserTest implementation.

Done.

[James Hawkins, 2011-11-29 18:20:54]

LGTM with nits.

http://codereview.chromium.org/8586009/diff/30001/chrome/test/base/js_injecti...
File chrome/test/base/js_injection_ready_observer.cc (right):

http://codereview.chromium.org/8586009/diff/30001/chrome/test/base/js_injecti...
chrome/test/base/js_injection_ready_observer.cc:10:
JsInjectionReadyObserver::~JsInjectionReadyObserver() {
Inline the destructor and remove this implementation file.

http://codereview.chromium.org/8586009/diff/30001/chrome/test/base/js_injecti...
File chrome/test/base/js_injection_ready_observer.h (right):

http://codereview.chromium.org/8586009/diff/30001/chrome/test/base/js_injecti...
chrome/test/base/js_injection_ready_observer.h:20: JsInjectionReadyObserver();
Remove the constructor. We don't add constructors for interfaces.

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

http://codereview.chromium.org/8586009/diff/30001/chrome/test/base/test_html_...
chrome/test/base/test_html_dialog_observer.h:39: JsInjectionReadyObserver*
js_injection_ready_observer_;
Document this var.

http://codereview.chromium.org/8586009/diff/30001/chrome/test/data/webui/prin...
File chrome/test/data/webui/print_preview.h (right):

http://codereview.chromium.org/8586009/diff/30001/chrome/test/data/webui/prin...
chrome/test/data/webui/print_preview.h:13: // Print Preview gtest fixture.
Remove redundant comment or make it actually descriptive.

[Sheridan Rawlins, 2011-11-29 23:56:49]

http://codereview.chromium.org/8586009/diff/30001/chrome/test/base/js_injecti...
File chrome/test/base/js_injection_ready_observer.cc (right):

http://codereview.chromium.org/8586009/diff/30001/chrome/test/base/js_injecti...
chrome/test/base/js_injection_ready_observer.cc:10:
JsInjectionReadyObserver::~JsInjectionReadyObserver() {
On 2011/11/29 18:20:54, James Hawkins wrote:
> Inline the destructor and remove this implementation file.

Done.

http://codereview.chromium.org/8586009/diff/30001/chrome/test/base/js_injecti...
File chrome/test/base/js_injection_ready_observer.h (right):

http://codereview.chromium.org/8586009/diff/30001/chrome/test/base/js_injecti...
chrome/test/base/js_injection_ready_observer.h:20: JsInjectionReadyObserver();
On 2011/11/29 18:20:54, James Hawkins wrote:
> Remove the constructor. We don't add constructors for interfaces.

Done.

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

http://codereview.chromium.org/8586009/diff/30001/chrome/test/base/test_html_...
chrome/test/base/test_html_dialog_observer.h:39: JsInjectionReadyObserver*
js_injection_ready_observer_;
On 2011/11/29 18:20:54, James Hawkins wrote:
> Document this var.

Done.

http://codereview.chromium.org/8586009/diff/30001/chrome/test/data/webui/prin...
File chrome/test/data/webui/print_preview.h (right):

http://codereview.chromium.org/8586009/diff/30001/chrome/test/data/webui/prin...
chrome/test/data/webui/print_preview.h:13: // Print Preview gtest fixture.
On 2011/11/29 18:20:54, James Hawkins wrote:
> Remove redundant comment or make it actually descriptive.

Done.

[commit-bot: I haz the power, 2011-11-30 02:02:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scr@chromium.org/8586009/35001

[commit-bot: I haz the power, 2011-11-30 06:28:53]

Change committed as 112146