Beginnings of a unit test for downloads that occur in a new window....

Beginnings of a unit test for downloads that occur in a new window.
For now this works for IE regularly (no cf in the picture) and triggers a download in a new window.
It is disabled since support for this scenario is not yet ready in CF.

I'm adding a couple of things to WebBrowserEventSink:
- A way to gracefully close IE.  This way we allow IE to quit on its own and don't go the "kill" route unless necessary.
  This should help with reducing cases where we've killed IE and the next time it is started it presents the user with an "IE crashed" dialog which could cause subsequent unit tests to fail.
  Another thing this adds is support for the OnQuit event that can be useful to add to unit tests as an expected event.
- A new OnFileDownload event.  I also updated several tests to expect this event.

TEST=Adds a unit test.
BUG=none


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

[amit, 2010-02-11 23:23:21]

nice!

http://codereview.chromium.org/569011/diff/5002/4004
File chrome_frame/test/chrome_frame_test_utils.cc (right):

http://codereview.chromium.org/569011/diff/5002/4004#newcode761
chrome_frame/test/chrome_frame_test_utils.cc:761: ::WaitForSingleObject(process,
wait_for_process_on_uninitialize_timeout_);
Other tests could benefit by closing IE here.
we could call CloseWebBrowserWithOnQuitEvent here and wait a little but while
peeking for messages so that Quit event is delivered.

http://codereview.chromium.org/569011/diff/5002/4004#newcode796
chrome_frame/test/chrome_frame_test_utils.cc:796: // Always cancel file
downloads in tests.
this may not be sufficient, in case we really want to see if something got
downloaded or not.

http://codereview.chromium.org/569011/diff/5002/4008
File chrome_frame/test/test_mock_with_web_server.cc (right):

http://codereview.chromium.org/569011/diff/5002/4008#newcode169
chrome_frame/test/test_mock_with_web_server.cc:169: EXPECT_CALL(mock,
OnFileDownload(VARIANT_TRUE, _))
It's weird that this event is fired for any new popup.

http://codereview.chromium.org/569011/diff/5002/4008#newcode786
chrome_frame/test/test_mock_with_web_server.cc:786:
&MockWebBrowserEventSink::CloseWebBrowserWithOnQuitEvent))));
If we move this into MockWebBrowserEventSink::Uninitialize, then all the
existing tests will get the benefit of clean shutdown. We could handle QUIT_LOOP
as before (optionally  waiting for OnQuit).

[tommi (sloooow) - chröme, 2010-02-12 17:00:36]

http://codereview.chromium.org/569011/diff/5002/4004
File chrome_frame/test/chrome_frame_test_utils.cc (right):

http://codereview.chromium.org/569011/diff/5002/4004#newcode761
chrome_frame/test/chrome_frame_test_utils.cc:761: ::WaitForSingleObject(process,
wait_for_process_on_uninitialize_timeout_);
On 2010/02/11 23:23:21, amit wrote:
> Other tests could benefit by closing IE here.
> we could call CloseWebBrowserWithOnQuitEvent here and wait a little but while
> peeking for messages so that Quit event is delivered.

This should happen after the browser has quit and fired the quit event.

http://codereview.chromium.org/569011/diff/5002/4004#newcode796
chrome_frame/test/chrome_frame_test_utils.cc:796: // Always cancel file
downloads in tests.
On 2010/02/11 23:23:21, amit wrote:
> this may not be sufficient, in case we really want to see if something got
> downloaded or not.

that's true.  do you want me to add a setter to allow downloads?

http://codereview.chromium.org/569011/diff/5002/4008
File chrome_frame/test/test_mock_with_web_server.cc (right):

http://codereview.chromium.org/569011/diff/5002/4008#newcode169
chrome_frame/test/test_mock_with_web_server.cc:169: EXPECT_CALL(mock,
OnFileDownload(VARIANT_TRUE, _))
On 2010/02/11 23:23:21, amit wrote:
> It's weird that this event is fired for any new popup.

yes.  when the "active_document" flag is set, it's actually the html page that's
being downloaded.  It fires for non-popups too.

http://codereview.chromium.org/569011/diff/5002/4008#newcode786
chrome_frame/test/test_mock_with_web_server.cc:786:
&MockWebBrowserEventSink::CloseWebBrowserWithOnQuitEvent))));
On 2010/02/11 23:23:21, amit wrote:
> If we move this into MockWebBrowserEventSink::Uninitialize, then all the
> existing tests will get the benefit of clean shutdown. We could handle
QUIT_LOOP
> as before (optionally  waiting for OnQuit). 

Uninitialize is always called after the loop has run and is expected to run
synchronously.  CloseWebBrowserWithOnQuitEvent closes the browser asynchronously
so I think that would make Uninitialize() more complex.  On the other hand, I
can modify the other tests that currently close via QUIT_LOOP[_SOON] or
&chrome_frame_test::CloseAllIEWindows (which suspect is prone to killing the
browser) to use this approach instead.

What do you think?

[amit, 2010-02-12 17:17:23]

In general, your new graceful Quit method seems like a better way of doing
things. So I was thinking about ideas to use it in rest of the tests. Can you
think of a way we could do that? Maybe use another helper in place of QUIT_LOOP
that does this and posts a quit message?

On 2010/02/12 17:00:36, tommi wrote:
> http://codereview.chromium.org/569011/diff/5002/4004
> File chrome_frame/test/chrome_frame_test_utils.cc (right):
> 
> http://codereview.chromium.org/569011/diff/5002/4004#newcode761
> chrome_frame/test/chrome_frame_test_utils.cc:761:
::WaitForSingleObject(process,
> wait_for_process_on_uninitialize_timeout_);
> On 2010/02/11 23:23:21, amit wrote:
> > Other tests could benefit by closing IE here.
> > we could call CloseWebBrowserWithOnQuitEvent here and wait a little but
while
> > peeking for messages so that Quit event is delivered.
> 
> This should happen after the browser has quit and fired the quit event.
> 
> http://codereview.chromium.org/569011/diff/5002/4004#newcode796
> chrome_frame/test/chrome_frame_test_utils.cc:796: // Always cancel file
> downloads in tests.
> On 2010/02/11 23:23:21, amit wrote:
> > this may not be sufficient, in case we really want to see if something got
> > downloaded or not.
> 
> that's true.  do you want me to add a setter to allow downloads?
> 
> http://codereview.chromium.org/569011/diff/5002/4008
> File chrome_frame/test/test_mock_with_web_server.cc (right):
> 
> http://codereview.chromium.org/569011/diff/5002/4008#newcode169
> chrome_frame/test/test_mock_with_web_server.cc:169: EXPECT_CALL(mock,
> OnFileDownload(VARIANT_TRUE, _))
> On 2010/02/11 23:23:21, amit wrote:
> > It's weird that this event is fired for any new popup.
> 
> yes.  when the "active_document" flag is set, it's actually the html page
that's
> being downloaded.  It fires for non-popups too.
> 
> http://codereview.chromium.org/569011/diff/5002/4008#newcode786
> chrome_frame/test/test_mock_with_web_server.cc:786:
> &MockWebBrowserEventSink::CloseWebBrowserWithOnQuitEvent))));
> On 2010/02/11 23:23:21, amit wrote:
> > If we move this into MockWebBrowserEventSink::Uninitialize, then all the
> > existing tests will get the benefit of clean shutdown. We could handle
> QUIT_LOOP
> > as before (optionally  waiting for OnQuit). 
> 
> Uninitialize is always called after the loop has run and is expected to run
> synchronously.  CloseWebBrowserWithOnQuitEvent closes the browser
asynchronously
> so I think that would make Uninitialize() more complex.  On the other hand, I
> can modify the other tests that currently close via QUIT_LOOP[_SOON] or
> &chrome_frame_test::CloseAllIEWindows (which suspect is prone to killing the
> browser) to use this approach instead.
> 
> What do you think?

[tommi (sloooow) - chröme, 2010-02-13 00:30:38]

Patch updated.  I updated all the tests to use CloseWebBrowserWithOnQuitEvent
and expect the OnQuit event.

[amit, 2010-02-16 20:03:37]

http://codereview.chromium.org/569011/diff/5003/4022
File chrome_frame/test/chrome_frame_test_utils.cc (right):

http://codereview.chromium.org/569011/diff/5003/4022#newcode313
chrome_frame/test/chrome_frame_test_utils.cc:313:
AllowSetForegroundWindow(ASFW_ANY);
nice :)

http://codereview.chromium.org/569011/diff/5003/4022#newcode460
chrome_frame/test/chrome_frame_test_utils.cc:460: DWORD elapsed =
::GetTickCount() - now;
Tick count wrap around issue? Better to use the base::Time.

http://codereview.chromium.org/569011/diff/5003/4026
File chrome_frame/test/test_mock_with_web_server.cc (right):

http://codereview.chromium.org/569011/diff/5003/4026#newcode1088
chrome_frame/test/test_mock_with_web_server.cc:1088: // Go forward using
Rt-Click + DOWN + DOWN + ENTER
My fault but will you please get rid of this comment?

http://codereview.chromium.org/569011/diff/5003/4026#newcode1201
chrome_frame/test/test_mock_with_web_server.cc:1201:
mock.set_wait_for_process_on_uninitialize(kDefaultWaitForIEToTerminateMs);
could use the enum to set what kind of mock it is as discussed offline.

[tommi (sloooow) - chröme, 2010-02-16 22:07:36]

changes uploaded

http://codereview.chromium.org/569011/diff/5003/4022
File chrome_frame/test/chrome_frame_test_utils.cc (right):

http://codereview.chromium.org/569011/diff/5003/4022#newcode460
chrome_frame/test/chrome_frame_test_utils.cc:460: DWORD elapsed =
::GetTickCount() - now;
On 2010/02/16 20:03:37, amit wrote:
> Tick count wrap around issue? Better to use the base::Time.

Done.

http://codereview.chromium.org/569011/diff/5003/4026
File chrome_frame/test/test_mock_with_web_server.cc (right):

http://codereview.chromium.org/569011/diff/5003/4026#newcode1088
chrome_frame/test/test_mock_with_web_server.cc:1088: // Go forward using
Rt-Click + DOWN + DOWN + ENTER
On 2010/02/16 20:03:37, amit wrote:
> My fault but will you please get rid of this comment?

Done.

http://codereview.chromium.org/569011/diff/5003/4026#newcode1201
chrome_frame/test/test_mock_with_web_server.cc:1201:
mock.set_wait_for_process_on_uninitialize(kDefaultWaitForIEToTerminateMs);
On 2010/02/16 20:03:37, amit wrote:
> could use the enum to set what kind of mock it is as discussed offline.
I was going to do that but instead realized that we only ever call
LaunchIEAndNavigate on the main mock object.  So, I modified the webbrowser
event sink class to recognize this.

I also did a couple of other changes:

I changed the way CloseAllIEWindows is called.  It's now called from the
destructor of a "smart" object.  This is to guarantee that it's called after the
destructors of the mock objects have run.

I overrode CComStackObjectEx in order to call the mock object's Uninitialize
method from the destructor.  This way we don't have to call Uninitialize() for
every mock object at the end of every test.  If I don't do this, Uninitialize
will still be called from WebBrowserEventSink's destructor but at that point it
is too late since it runs _after_ CComStackObjectEx' destructor runs and that
destructor will throw an assert if the object's refcount is != 0.

[amit, 2010-02-16 23:45:31]

LGTM

http://codereview.chromium.org/569011/diff/5004/5006
File chrome_frame/test/chrome_frame_test_utils.h (right):

http://codereview.chromium.org/569011/diff/5004/5006#newcode262
chrome_frame/test/chrome_frame_test_utils.h:262: HRESULT
CloseWebBrowserWithOnQuitEvent();
nit: Since this is a graceful shutdown (as opposed to Kill), should we use a
more compact form: CloseWebBrowser()?

http://codereview.chromium.org/569011/diff/5004/5009
File chrome_frame/test/test_mock_with_web_server.cc (right):

http://codereview.chromium.org/569011/diff/5004/5009#newcode34
chrome_frame/test/test_mock_with_web_server.cc:34: class CloseIeAtEndOfScope {
nice :)

http://codereview.chromium.org/569011/diff/5004/5009#newcode51
chrome_frame/test/test_mock_with_web_server.cc:51: class
ComStackObjectWithUninitialize : public CComObjectStackEx<Base> {
nice++

[tommi (sloooow) - chröme, 2010-02-17 04:18:32]

thanks.  all done.

On 2010/02/16 23:45:31, amit wrote:
> LGTM
> 
> http://codereview.chromium.org/569011/diff/5004/5006
> File chrome_frame/test/chrome_frame_test_utils.h (right):
> 
> http://codereview.chromium.org/569011/diff/5004/5006#newcode262
> chrome_frame/test/chrome_frame_test_utils.h:262: HRESULT
> CloseWebBrowserWithOnQuitEvent();
> nit: Since this is a graceful shutdown (as opposed to Kill), should we use a
> more compact form: CloseWebBrowser()?
> 
> http://codereview.chromium.org/569011/diff/5004/5009
> File chrome_frame/test/test_mock_with_web_server.cc (right):
> 
> http://codereview.chromium.org/569011/diff/5004/5009#newcode34
> chrome_frame/test/test_mock_with_web_server.cc:34: class CloseIeAtEndOfScope {
> nice :)
> 
> http://codereview.chromium.org/569011/diff/5004/5009#newcode51
> chrome_frame/test/test_mock_with_web_server.cc:51: class
> ComStackObjectWithUninitialize : public CComObjectStackEx<Base> {
> nice++