Added unit tests for ChromeFrame IE full tab mode. These test the following c...

Added unit tests for ChromeFrame IE full tab mode. These test the following cases:-
1. A window.open call issued from a page within ChromeFrame in IE full tab mode makes it back
   to IE before being shunted into Chrome.
2. Brings up the chrome frame context menu in full tab mode and invokes the About Chrome frame 
   option.

While I was working on these tests based on a lot of help from Stoyan, we had these tests use gmock
and I also changed the disallowed URL test to use gmock.

The other changes are to always run the chrome frame tests in the STA apartment as the newly added
full tab mode tests cannot handle calls coming in from background RPC worker threads.

Bug=26066, 26172
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30520

[amit, 2009-10-29 05:53:11]

Looks excellent and thanks for making a nice start with tests. 
LGTM

http://codereview.chromium.org/340029/diff/3003/5001
File chrome_frame/test/chrome_frame_unittests.cc (right):

http://codereview.chromium.org/340029/diff/3003/5001#newcode1405
Line 1405: void ShowContextMenu() {
Is MockWebBrowserEventSink a generic mock? In that case ShowContextMenu and
SelectAboutChromeFrame does not belong here. They should be part of some test
specific class (derived from MockWebBrowserEventSink) along with
GetChromeRendererWindow() and related helpers.

http://codereview.chromium.org/340029/diff/3003/5001#newcode1411
Line 1411: PostMessage(renderer_window, WM_RBUTTONDOWN, MK_RBUTTON,
MAKELPARAM(50, 50));
nit: 80 and unnecessary newline above.

http://codereview.chromium.org/340029/diff/3003/5001#newcode1440
Line 1440: 
no need for extra empty lines here and everywhere else, just group similar
calls.

http://codereview.chromium.org/340029/diff/3003/5002
File chrome_frame/test/chrome_frame_unittests.h (right):

http://codereview.chromium.org/340029/diff/3003/5002#newcode166
Line 166: return IDispEventSimpleImpl<0, WebBrowserEventSink,
&DIID_DWebBrowserEvents2>::
typedef IDispEventSimpleImpl<0, WebBrowserEventSink, &DIID_DWebBrowserEvents2>
to something like DispEventsImpl. This will also help contain the expression
within 80 columns

http://codereview.chromium.org/340029/diff/3003/5003
File chrome_frame/test/data/chrome_frame_tester_helpers.js (right):

http://codereview.chromium.org/340029/diff/3003/5003#newcode144
Line 144: var is_chrome = /chrome/.test(navigator.userAgent.toLowerCase());
alternatively you could test for externalHost.

http://codereview.chromium.org/340029/diff/3003/5004
File chrome_frame/test/data/chrome_frame_window_open.html (right):

http://codereview.chromium.org/340029/diff/3003/5004#newcode3
Line 3: be followed by a CR/LF ending, so please do not change the line endings.
-->
Really? Is this needed?

http://codereview.chromium.org/340029/diff/3003/5005
File chrome_frame/test/data/chrome_frame_window_open_popup.html (right):

http://codereview.chromium.org/340029/diff/3003/5005#newcode3
Line 3: <meta http-equiv="x-ua-compatible" content="chrome=1" />
This shouldn't be necessary, right?

[stoyan, 2009-10-29 16:34:48]

http://codereview.chromium.org/340029/diff/3003/5001
File chrome_frame/test/chrome_frame_unittests.cc (right):

http://codereview.chromium.org/340029/diff/3003/5001#newcode1551
Line 1551: CreateFunctor(&loop.loop_, &MessageLoop::PostDelayedTask, FROM_HERE,
This will not compile - C2782. I made subtle change in gmock-mutant on your
machine :)
If the call was not satisfied, do we leak the Task?

http://codereview.chromium.org/340029/diff/3003/5001#newcode1562
Line 1562: EXPECT_TRUE(SUCCEEDED(hr));
ASSERT_SUCCEEDED

http://codereview.chromium.org/340029/diff/3003/5002
File chrome_frame/test/chrome_frame_unittests.h (right):

http://codereview.chromium.org/340029/diff/3003/5002#newcode157
Line 157: DWORD flags, BSTR url_context, BSTR url) {
more indent

[ananta, 2009-10-29 17:57:50]

http://codereview.chromium.org/340029/diff/3003/5001
File chrome_frame/test/chrome_frame_unittests.cc (right):

http://codereview.chromium.org/340029/diff/3003/5001#newcode1411
Line 1411: PostMessage(renderer_window, WM_RBUTTONDOWN, MK_RBUTTON,
MAKELPARAM(50, 50));
On 2009/10/29 05:53:11, amit wrote:
> nit: 80 and unnecessary newline above.
these functions are now in the chrome_frame_test namespace.

http://codereview.chromium.org/340029/diff/3003/5001#newcode1440
Line 1440: 
On 2009/10/29 05:53:11, amit wrote:
> no need for extra empty lines here and everywhere else, just group similar
> calls.
Removed some of the lines.

http://codereview.chromium.org/340029/diff/3003/5001#newcode1551
Line 1551: CreateFunctor(&loop.loop_, &MessageLoop::PostDelayedTask, FROM_HERE,
On 2009/10/29 16:34:48, stoyan wrote:
> This will not compile - C2782. I made subtle change in gmock-mutant on your
> machine :)
> If the call was not satisfied, do we leak the Task?

As per our discussion removed the CreateFunctor and moved all this logic into
the ShowChromeFrameContextMenu helper function.

http://codereview.chromium.org/340029/diff/3003/5001#newcode1562
Line 1562: EXPECT_TRUE(SUCCEEDED(hr));
On 2009/10/29 16:34:48, stoyan wrote:
> ASSERT_SUCCEEDED

Done.

http://codereview.chromium.org/340029/diff/3003/5002
File chrome_frame/test/chrome_frame_unittests.h (right):

http://codereview.chromium.org/340029/diff/3003/5002#newcode157
Line 157: DWORD flags, BSTR url_context, BSTR url) {
On 2009/10/29 16:34:48, stoyan wrote:
> more indent

Done.

http://codereview.chromium.org/340029/diff/3003/5002#newcode166
Line 166: return IDispEventSimpleImpl<0, WebBrowserEventSink,
&DIID_DWebBrowserEvents2>::
On 2009/10/29 05:53:11, amit wrote:
> typedef IDispEventSimpleImpl<0, WebBrowserEventSink, &DIID_DWebBrowserEvents2>
> to something like DispEventsImpl. This will also help contain the expression
> within 80 columns

Done.

http://codereview.chromium.org/340029/diff/3003/5004
File chrome_frame/test/data/chrome_frame_window_open.html (right):

http://codereview.chromium.org/340029/diff/3003/5004#newcode3
Line 3: be followed by a CR/LF ending, so please do not change the line endings.
-->
On 2009/10/29 05:53:11, amit wrote:
> Really? Is this needed?
For IE yes

http://codereview.chromium.org/340029/diff/3003/5005
File chrome_frame/test/data/chrome_frame_window_open_popup.html (right):

http://codereview.chromium.org/340029/diff/3003/5005#newcode3
Line 3: <meta http-equiv="x-ua-compatible" content="chrome=1" />
On 2009/10/29 05:53:11, amit wrote:
> This shouldn't be necessary, right?
For this test yes, as we rely on the popup to open up in chrome to determine
whether the whole test succeeded.

[stoyan, 2009-10-29 18:37:35]

lg