Added support for running reliability tests for ChromeFrame on similar lines ...

Added support for running reliability tests for ChromeFrame on similar lines as Chrome. We only run these
tests for IE at this point. The reliability test code for Chrome has been copied and modified accordingly.

Other related changes in this CL include the following:-
1. If ChromeFrame is running in headless mode determined by a registry value in HKCU\Software\Google\ChromeFrame
   we initialize ChromeFrame crash reporting and connect to the Chrome crash server. This would enable us to
   gather crash dumps from the reliability test runs and report the same.

2. The LowIntegrity fixes for the WebBrowser which Stoyan had done a while back are only needed for IE7 on Vista.
   For this CL though we just do the requisite hacks if the OS is Vista. For Windows7 the returned IWebBrowser
   interface pointer works fine.

3. I moved the WebBrowserEventSink to chrome_frame_test_utils as this class is now shared.

Fixes portions of http://code.google.com/p/chromium/issues/detail?id=29451

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

[robertshield, 2009-12-07 19:48:49]

Looks awesome :-) Some comments below:

http://codereview.chromium.org/465074/diff/4027/4032
File chrome_frame/test/chrome_frame_test_utils.cc (right):

http://codereview.chromium.org/465074/diff/4027/4032#newcode634
chrome_frame/test/chrome_frame_test_utils.cc:634: if (win_util::GetWinVersion()
== win_util::WINVERSION_VISTA) {
Could you add a comment that this is only needed on Vista, not Win7? Without the
comment it looks like it should be a >= instead

http://codereview.chromium.org/465074/diff/4027/4032#newcode812
chrome_frame/test/chrome_frame_test_utils.cc:812: HWND
WebBrowserEventSink::GetChromeRendererWindow() {
could you rename this to GetAttachedChromeRendererWindow() or something? There
is already another method with the same name in this file which makes browsing
confusing.

http://codereview.chromium.org/465074/diff/4027/4032#newcode837
chrome_frame/test/chrome_frame_test_utils.cc:837:
web_browser2_.Attach(web_browser2);
this could be written more simply as 

web_browser2_ = web_browser2;

http://codereview.chromium.org/465074/diff/4027/4033
File chrome_frame/test/chrome_frame_test_utils.h (right):

http://codereview.chromium.org/465074/diff/4027/4033#newcode110
chrome_frame/test/chrome_frame_test_utils.h:110: struct TimedMsgLoop {
struct -> class?

http://codereview.chromium.org/465074/diff/4027/4033#newcode187
chrome_frame/test/chrome_frame_test_utils.h:187: OnBeforeNavigate2Internal,
&kBeforeNavigate2Info)
indent here and below

http://codereview.chromium.org/465074/diff/4027/4038
File chrome_frame/test/reliability/page_load_test.cc (right):

http://codereview.chromium.org/465074/diff/4027/4038#newcode80
chrome_frame/test/reliability/page_load_test.cc:80: const wchar_t
kChromeFrameHeadlessMode[] = L"ChromeFrameHeadlessMode";
Please add a comment explaining the purpose of this string. Also, this constant
is defined twice, here and in utils. Can it be defined only in one place?

http://codereview.chromium.org/465074/diff/4027/4038#newcode120
chrome_frame/test/reliability/page_load_test.cc:120: if
(!is_chrome_frame_navigation_) {
Please add a comment here explaining that cf: urls are special cased because
shut down is initiated by whoever is listening for Chrome Frame events. Also, if
the previous sentence is wrong, please disregard it and instead add a correct
comment :-)

http://codereview.chromium.org/465074/diff/4027/4038#newcode135
chrome_frame/test/reliability/page_load_test.cc:135: if
(StartsWith(navigate_url, L"cf:", true)) {
is the intent that all urls passed to the test start with cf:?

http://codereview.chromium.org/465074/diff/4027/4038#newcode375
chrome_frame/test/reliability/page_load_test.cc:375:
SetConfigBool(kChromeFrameHeadlessMode, true);
Is there any danger that if one run of a test crashes, it could leave behind the
headless mode registry value resulting in this being picked up for all
subsequent runs of Chrome Frame on that machine? It looks like we would need to
set the GTest flag "catch_exceptions" to guarantee that TearDown() always gets
run, but I could be reading the GTest code incorrectly.

[ananta, 2009-12-07 20:16:22]

http://codereview.chromium.org/465074/diff/4027/4032
File chrome_frame/test/chrome_frame_test_utils.cc (right):

http://codereview.chromium.org/465074/diff/4027/4032#newcode634
chrome_frame/test/chrome_frame_test_utils.cc:634: if (win_util::GetWinVersion()
== win_util::WINVERSION_VISTA) {
On 2009/12/07 19:48:49, robertshield wrote:
> Could you add a comment that this is only needed on Vista, not Win7? Without
the
> comment it looks like it should be a >= instead

Done.

http://codereview.chromium.org/465074/diff/4027/4032#newcode812
chrome_frame/test/chrome_frame_test_utils.cc:812: HWND
WebBrowserEventSink::GetChromeRendererWindow() {
On 2009/12/07 19:48:49, robertshield wrote:
> could you rename this to GetAttachedChromeRendererWindow() or something? There
> is already another method with the same name in this file which makes browsing
> confusing.

Done.

http://codereview.chromium.org/465074/diff/4027/4032#newcode837
chrome_frame/test/chrome_frame_test_utils.cc:837:
web_browser2_.Attach(web_browser2);
On 2009/12/07 19:48:49, robertshield wrote:
> this could be written more simply as 
> 
> web_browser2_ = web_browser2;
> 

Thanks. I must have been sleeping when I wrote this :(

http://codereview.chromium.org/465074/diff/4027/4033
File chrome_frame/test/chrome_frame_test_utils.h (right):

http://codereview.chromium.org/465074/diff/4027/4033#newcode110
chrome_frame/test/chrome_frame_test_utils.h:110: struct TimedMsgLoop {
On 2009/12/07 19:48:49, robertshield wrote:
> struct -> class?

Done.

http://codereview.chromium.org/465074/diff/4027/4033#newcode187
chrome_frame/test/chrome_frame_test_utils.h:187: OnBeforeNavigate2Internal,
&kBeforeNavigate2Info)
On 2009/12/07 19:48:49, robertshield wrote:
> indent here and below

Done.

http://codereview.chromium.org/465074/diff/4027/4038
File chrome_frame/test/reliability/page_load_test.cc (right):

http://codereview.chromium.org/465074/diff/4027/4038#newcode80
chrome_frame/test/reliability/page_load_test.cc:80: const wchar_t
kChromeFrameHeadlessMode[] = L"ChromeFrameHeadlessMode";
On 2009/12/07 19:48:49, robertshield wrote:
> Please add a comment explaining the purpose of this string. Also, this
constant
> is defined twice, here and in utils. Can it be defined only in one place?

Done.

http://codereview.chromium.org/465074/diff/4027/4038#newcode120
chrome_frame/test/reliability/page_load_test.cc:120: if
(!is_chrome_frame_navigation_) {
On 2009/12/07 19:48:49, robertshield wrote:
> Please add a comment here explaining that cf: urls are special cased because
> shut down is initiated by whoever is listening for Chrome Frame events. Also,
if
> the previous sentence is wrong, please disregard it and instead add a correct
> comment :-)

Done.

http://codereview.chromium.org/465074/diff/4027/4038#newcode135
chrome_frame/test/reliability/page_load_test.cc:135: if
(StartsWith(navigate_url, L"cf:", true)) {
On 2009/12/07 19:48:49, robertshield wrote:
> is the intent that all urls passed to the test start with cf:?
That is something which needs to be clarified. We can do the following:-
1. Force everything into chrome frame.
2. Launch the URL once into IE and once it chrome frame.
3. Some random order where every second URL or something goes into chromeframe?

Let me know what you think.

http://codereview.chromium.org/465074/diff/4027/4038#newcode375
chrome_frame/test/reliability/page_load_test.cc:375:
SetConfigBool(kChromeFrameHeadlessMode, true);
On 2009/12/07 19:48:49, robertshield wrote:
> Is there any danger that if one run of a test crashes, it could leave behind
the
> headless mode registry value resulting in this being picked up for all
> subsequent runs of Chrome Frame on that machine? It looks like we would need
to
> set the GTest flag "catch_exceptions" to guarantee that TearDown() always gets
> run, but I could be reading the GTest code incorrectly.

I will take a look at this.

[robertshield, 2009-12-07 20:58:02]

http://codereview.chromium.org/465074/diff/4027/4038
File chrome_frame/test/reliability/page_load_test.cc (right):

http://codereview.chromium.org/465074/diff/4027/4038#newcode135
chrome_frame/test/reliability/page_load_test.cc:135: if
(StartsWith(navigate_url, L"cf:", true)) {
On 2009/12/07 20:16:23, ananta wrote:
> On 2009/12/07 19:48:49, robertshield wrote:
> > is the intent that all urls passed to the test start with cf:?
> That is something which needs to be clarified. We can do the following:-
> 1. Force everything into chrome frame.
> 2. Launch the URL once into IE and once it chrome frame.
> 3. Some random order where every second URL or something goes into
chromeframe?

I think we should have 3. without the random part :-) I believe we want to test:

1) Loading a ChromeFrame page from a non-ChromeFrame page.
2) Navigating ChromeFrame from a ChromeFrame page.
3) Loading a non-ChromeFrame page from a ChromeFrame page.

Could we use an ordering like 

CF-CF-NonCF- CF-CF-NonCF- ...

which would cover the 3 cases? Is there a case I'm forgetting?

> 
> Let me know what you think.
> 
> 
>

[ananta, 2009-12-07 21:27:05]

On 2009/12/07 20:58:02, robertshield wrote:
> http://codereview.chromium.org/465074/diff/4027/4038
> File chrome_frame/test/reliability/page_load_test.cc (right):
> 
> http://codereview.chromium.org/465074/diff/4027/4038#newcode135
> chrome_frame/test/reliability/page_load_test.cc:135: if
> (StartsWith(navigate_url, L"cf:", true)) {
> On 2009/12/07 20:16:23, ananta wrote:
> > On 2009/12/07 19:48:49, robertshield wrote:
> > > is the intent that all urls passed to the test start with cf:?
> > That is something which needs to be clarified. We can do the following:-
> > 1. Force everything into chrome frame.
> > 2. Launch the URL once into IE and once it chrome frame.
> > 3. Some random order where every second URL or something goes into
> chromeframe?
> 
> I think we should have 3. without the random part :-) I believe we want to
test:
> 
> 1) Loading a ChromeFrame page from a non-ChromeFrame page.
> 2) Navigating ChromeFrame from a ChromeFrame page.
> 3) Loading a non-ChromeFrame page from a ChromeFrame page.
> 
> Could we use an ordering like 
> 
> CF-CF-NonCF- CF-CF-NonCF- ...
> 
> which would cover the 3 cases? Is there a case I'm forgetting?
> 
> > 
> > Let me know what you think.
> > 
> > 
> >

Done. Please take another look.

[robertshield, 2009-12-07 21:51:47]

LGTM

http://codereview.chromium.org/465074/diff/103/114
File chrome_frame/test/reliability/page_load_test.cc (right):

http://codereview.chromium.org/465074/diff/103/114#newcode356
chrome_frame/test/reliability/page_load_test.cc:356: if (file.fail())
nit: should we log an error here?

[ananta, 2009-12-07 23:12:04]

I also simplified the target control logic which was wrong :(
To ensure that we have the ordering like CF -> CF -> host and so on I changed
the code to send every 3rd navigation into the host, which works correctly.

http://codereview.chromium.org/465074/diff/103/114
File chrome_frame/test/reliability/page_load_test.cc (right):

http://codereview.chromium.org/465074/diff/103/114#newcode356
chrome_frame/test/reliability/page_load_test.cc:356: if (file.fail())
On 2009/12/07 21:51:48, robertshield wrote:
> nit: should we log an error here?
I think this is a valid case and occurs when we reach the end of the url list.
For e.g. if the url list has a newline at the end, etc.