[chrome_frame] Refactor/merge IE no interference tests with other mock event ...

[chrome_frame] Refactor/merge IE no interference tests with other mock event sink tests. Have the test mock contain, not inherit the IE event sink. Use the new test server to verify the right requests are being sent.
BUG=none
TEST=none

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

[kkania, 2010-06-16 22:41:19]

Let me know if you want me to break this into smaller pieces. I will be out on
vacation all next week, so if this is blocking anyone I can make it smaller.

CHANGES:
1) Deleted test_mock_with_web_server.cc and no_interference_test.cc and moved
the tests into navigation_test.cc and ui_test.cc

2) Generalized some tests to use a parameterized gtest fixture, so that the
tests run for IE, CF with meta tag, CF with headers.

3) Copied [Mock]WebBrowserEventSink to ie_event_sink.h/cc, renamed it to
[Mock]IEEventSink and made it use a listener (as you saw before). I left the
other version in for now because the reliability page load tests use it and I
felt like I was already doing too much to start with.

Feel free to stop by my desk or chat with me if you have quick questions.

[stoyan, 2010-06-17 15:18:17]

http://codereview.chromium.org/2822016/diff/1/25
File chrome_frame/test/test_with_web_server.h (right):

http://codereview.chromium.org/2822016/diff/1/25#newcode279
chrome_frame/test/test_with_web_server.h:279: void
ExpectAndServeRequest(CFInvocation invocation, const std::wstring& url);
I don't like these Expect* methods.
I would prefer to keep the webserver as dumb as possible and have explicit and
visible handling of the headers and content of the response in the test body. 
The modification of response in GetFileForPath method could be moved to
CFInvocation class.
for example:
headers = GetParam().Headers("200\r\n");
content = GetParam().Body("....);
EXPECT_CALL("/blah").WillOnce(SendFast(headers, body));

I'm also thinking about adding other parameters to CFInvocation - orthogonal to
META_TAG/HEADERS/NO_CF. Perhaps file size (insert bunch of spaces) or sending
speed (to exercise all variants).

Tell me what do you think.

http://codereview.chromium.org/2822016/diff/1/26
File chrome_frame/test/ui_test.cc (right):

http://codereview.chromium.org/2822016/diff/1/26#newcode27
chrome_frame/test/ui_test.cc:27: void SetUp() {
explicitly declare the function virtual as per style guide.

[kkania, 2010-06-17 16:41:13]

http://codereview.chromium.org/2822016/diff/1/25
File chrome_frame/test/test_with_web_server.h (right):

http://codereview.chromium.org/2822016/diff/1/25#newcode279
chrome_frame/test/test_with_web_server.h:279: void
ExpectAndServeRequest(CFInvocation invocation, const std::wstring& url);
I think some of the Expect* methods are useful for a lot of tests in which it is
not necessary to manipulate the headers and content so specifically. So I do not
see a good reason to delete them. 
However, I can see that some of the navigation tests would be clearer if they
were more explicit in how they are handling the headers and body.
My thought would be just to leave most of these Expect* methods here but to use
the mock directly for tests that want more control over the headers and the body
instead of adding additional Expect*s.

About adding stuff to CFInvocation, I am not so sure. Perhaps the stuff about
file size and transmission details belong in a different class. I am curious for
what tests you would want to run all these different variants. Just for a subset
of net-related tests or also others?

http://codereview.chromium.org/2822016/diff/1/26
File chrome_frame/test/ui_test.cc (right):

http://codereview.chromium.org/2822016/diff/1/26#newcode27
chrome_frame/test/ui_test.cc:27: void SetUp() {
On 2010/06/17 15:18:17, stoyan wrote:
> explicitly declare the function virtual as per style guide.

Done.

[stoyan, 2010-06-17 16:48:41]

LGTM

Yet, consider extracting the content and header manipulation methods out of
MockWebServer class.

[amit, 2010-06-17 22:23:53]

Looks quite impressive.

http://codereview.chromium.org/2822016/diff/1/2
File chrome_frame/chrome_frame.gyp (right):

http://codereview.chromium.org/2822016/diff/1/2#newcode232
chrome_frame/chrome_frame.gyp:232: 'test/ie_event_sink.cc',
I like the new structure and names, thanks for paying attention to lots of
details :)

http://codereview.chromium.org/2822016/diff/1/11
File chrome_frame/test/ie_event_sink.cc (right):

http://codereview.chromium.org/2822016/diff/1/11#newcode172
chrome_frame/test/ie_event_sink.cc:172: AutoLock lock(listener_lock_);
The lock seems awkward around every call to the listener. My fear is that the
listener's methods are oblivious to this lock being held and could execute
arbitrary code causing various combinations of recursion, reentrancy and
deadlocks.

I think a better way would be to ref-count the listner.

http://codereview.chromium.org/2822016/diff/1/12
File chrome_frame/test/ie_event_sink.h (right):

http://codereview.chromium.org/2822016/diff/1/12#newcode89
chrome_frame/test/ie_event_sink.h:89: bool HasInitializedBrowser() {
nit: const, also consider a lowercase name for this lightweight helper. Is it
possible to use existing web_browser() helper instead of adding this?

http://codereview.chromium.org/2822016/diff/1/12#newcode93
chrome_frame/test/ie_event_sink.h:93: HRESULT SetWebBrowser(IWebBrowser2*
web_browser2);
nit: comment and a newline after would be nice and consistent with the rest.

http://codereview.chromium.org/2822016/diff/1/14
File chrome_frame/test/mock_ie_event_sink_test.cc (right):

http://codereview.chromium.org/2822016/diff/1/14#newcode26
chrome_frame/test/mock_ie_event_sink_test.cc:26: ExpectationSet
MockIEEventSink::ExpectNavigationCardinality(
nice, thanks for tightening down expectations.

http://codereview.chromium.org/2822016/diff/1/16
File chrome_frame/test/navigation_test.cc (right):

http://codereview.chromium.org/2822016/diff/1/16#newcode24
chrome_frame/test/navigation_test.cc:24: class FullTabNavigationTest
Awesome! :)

http://codereview.chromium.org/2822016/diff/1/16#newcode76
chrome_frame/test/navigation_test.cc:76: TEST_P(FullTabNavigationTest,
RefreshNoCachedCopy) {
nice test.

http://codereview.chromium.org/2822016/diff/1/24
File chrome_frame/test/test_with_web_server.cc (right):

http://codereview.chromium.org/2822016/diff/1/24#newcode282
chrome_frame/test/test_with_web_server.cc:282: bool
MockWebServer::GetFileForPath(const std::wstring& path,
This could be a key point of failure and make it quite hard to debug tests. Any
failure here should not be silent.

http://codereview.chromium.org/2822016/diff/1/24#newcode292
chrome_frame/test/test_with_web_server.cc:292:
EXPECT_TRUE(net::GetMimeTypeFromFile(file_path, content_type));
A simple log with the file name being fetched and its mime type would be quite
helpful.

http://codereview.chromium.org/2822016/diff/1/24#newcode294
chrome_frame/test/test_with_web_server.cc:294: return false;
LOG error and ASSERT false in this case?

http://codereview.chromium.org/2822016/diff/1/24#newcode297
chrome_frame/test/test_with_web_server.cc:297: StartsWithASCII(*content_type,
"text/html", false)) {
Log/ASSERT if we encounter this inconsistency?

http://codereview.chromium.org/2822016/diff/1/25
File chrome_frame/test/test_with_web_server.h (right):

http://codereview.chromium.org/2822016/diff/1/25#newcode231
chrome_frame/test/test_with_web_server.h:231: bool InvokesCF() const {
nit: consider lowercase for low cost getters.

[stoyan, 2010-06-18 16:44:01]

http://codereview.chromium.org/2822016/diff/1/11
File chrome_frame/test/ie_event_sink.cc (right):

http://codereview.chromium.org/2822016/diff/1/11#newcode172
chrome_frame/test/ie_event_sink.cc:172: AutoLock lock(listener_lock_);
On 2010/06/17 22:23:53, amit wrote:
> The lock seems awkward around every call to the listener. My fear is that the
> listener's methods are oblivious to this lock being held and could execute
> arbitrary code causing various combinations of recursion, reentrancy and
> deadlocks.
> 
> I think a better way would be to ref-count the listner.

It's a MTA, right? You need lock.

[amit, 2010-06-18 17:38:58]

[ananta, for STA/MTA related history]

http://codereview.chromium.org/2822016/diff/1/11
File chrome_frame/test/ie_event_sink.cc (right):

http://codereview.chromium.org/2822016/diff/1/11#newcode172
chrome_frame/test/ie_event_sink.cc:172: AutoLock lock(listener_lock_);
On 2010/06/18 16:44:01, stoyan wrote:
> On 2010/06/17 22:23:53, amit wrote:
> > The lock seems awkward around every call to the listener. My fear is that
the
> > listener's methods are oblivious to this lock being held and could execute
> > arbitrary code causing various combinations of recursion, reentrancy and
> > deadlocks.
> > 
> > I think a better way would be to ref-count the listner.
> 
> It's a MTA, right? You need lock.

Is there a good reason for this to be in MTA?

[kkania, 2010-07-01 17:57:34]

ping, ananta, do you remember the reason this is MTA?

[ananta, 2010-07-01 19:05:29]

On 2010/07/01 17:57:34, kkania wrote:
> ping, ananta, do you remember the reason this is MTA?

The chrome frame tests run in a STA. I had tried changing it to a MTA which did
not work. 

The lock is not needed if we continue running the tests in a STA as the
callbacks would happen on the same thread. 

-Ananta

[kkania, 2010-07-02 01:41:08]

Got rid of lock, fixed comments, and fixed test problems on different platforms.

http://codereview.chromium.org/2822016/diff/1/11
File chrome_frame/test/ie_event_sink.cc (right):

http://codereview.chromium.org/2822016/diff/1/11#newcode172
chrome_frame/test/ie_event_sink.cc:172: AutoLock lock(listener_lock_);
On 2010/06/17 22:23:53, amit wrote:
> The lock seems awkward around every call to the listener. My fear is that the
> listener's methods are oblivious to this lock being held and could execute
> arbitrary code causing various combinations of recursion, reentrancy and
> deadlocks.
> 
> I think a better way would be to ref-count the listner.

Got rid of the lock and converted class to SingleThreadModel, as ananta
suggested

http://codereview.chromium.org/2822016/diff/1/12
File chrome_frame/test/ie_event_sink.h (right):

http://codereview.chromium.org/2822016/diff/1/12#newcode89
chrome_frame/test/ie_event_sink.h:89: bool HasInitializedBrowser() {
On 2010/06/17 22:23:53, amit wrote:
> nit: const, also consider a lowercase name for this lightweight helper. Is it
> possible to use existing web_browser() helper instead of adding this?

Done.

http://codereview.chromium.org/2822016/diff/1/12#newcode93
chrome_frame/test/ie_event_sink.h:93: HRESULT SetWebBrowser(IWebBrowser2*
web_browser2);
On 2010/06/17 22:23:53, amit wrote:
> nit: comment and a newline after would be nice and consistent with the rest.

Done.

http://codereview.chromium.org/2822016/diff/1/24
File chrome_frame/test/test_with_web_server.cc (right):

http://codereview.chromium.org/2822016/diff/1/24#newcode282
chrome_frame/test/test_with_web_server.cc:282: bool
MockWebServer::GetFileForPath(const std::wstring& path,
On 2010/06/17 22:23:53, amit wrote:
> This could be a key point of failure and make it quite hard to debug tests.
Any
> failure here should not be silent.

Done.

http://codereview.chromium.org/2822016/diff/1/24#newcode292
chrome_frame/test/test_with_web_server.cc:292:
EXPECT_TRUE(net::GetMimeTypeFromFile(file_path, content_type));
On 2010/06/17 22:23:53, amit wrote:
> A simple log with the file name being fetched and its mime type would be quite
> helpful.

Done.

http://codereview.chromium.org/2822016/diff/1/24#newcode294
chrome_frame/test/test_with_web_server.cc:294: return false;
On 2010/06/17 22:23:53, amit wrote:
> LOG error and ASSERT false in this case?

used EXPECT

http://codereview.chromium.org/2822016/diff/1/24#newcode297
chrome_frame/test/test_with_web_server.cc:297: StartsWithASCII(*content_type,
"text/html", false)) {
On 2010/06/17 22:23:53, amit wrote:
> Log/ASSERT if we encounter this inconsistency?

Not sure what you mean by inconsistency here.

http://codereview.chromium.org/2822016/diff/1/25
File chrome_frame/test/test_with_web_server.h (right):

http://codereview.chromium.org/2822016/diff/1/25#newcode231
chrome_frame/test/test_with_web_server.h:231: bool InvokesCF() const {
On 2010/06/17 22:23:53, amit wrote:
> nit: consider lowercase for low cost getters.

Done.

http://codereview.chromium.org/2822016/diff/1/26
File chrome_frame/test/ui_test.cc (right):

http://codereview.chromium.org/2822016/diff/1/26#newcode27
chrome_frame/test/ui_test.cc:27: void SetUp() {
On 2010/06/17 15:18:17, stoyan wrote:
> explicitly declare the function virtual as per style guide.

Done.

[amit, 2010-07-02 18:06:22]

lgtm++

http://codereview.chromium.org/2822016/diff/22001/23011
File chrome_frame/test/ie_event_sink.cc (right):

http://codereview.chromium.org/2822016/diff/22001/23011#newcode183
chrome_frame/test/ie_event_sink.cc:183: if (renderer_window) {
nit: IsWindow(renderer_window) here and elsewhere for window handle validity.

[kkania, 2013-02-28 21:51:09]

https://codereview.chromium.org/2822016/diff/22001/chrome_frame/test/ie_event...
File chrome_frame/test/ie_event_sink.cc (right):

https://codereview.chromium.org/2822016/diff/22001/chrome_frame/test/ie_event...
chrome_frame/test/ie_event_sink.cc:183: if (renderer_window) {
On 2010/07/02 18:06:22, amit wrote:
> nit: IsWindow(renderer_window) here and elsewhere for window handle validity.

Done.