Add a browsertest for magic iframe.

Add a browsertest for magic iframe.
The test verifies that in-progress XHR is still loading after transfer.
BUG=55200
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95676

[Dmitry Titov, 2011-08-03 01:04:55]

Pawel, could you please take a look at this new test? I'm not creating a lot of
them, so would appreciate your feedback.

The test is currently disabled and I'm working on a fix for the bug, but it's
somewhat involved so will come a bit later as separate patch.

I've put a comment into one place that you probably won't like - where I have a
1-second delay in the test. I was trying to figure out how to 'hold' a network
request inside browser until the DOM operations on iframe transfer complete but
don't see the way. The works seem to be a false positive which seems to be ok
(the test is not flaky this way).

[Paweł Hajdan Jr., 2011-08-03 16:27:27]

Please make sure to also get reviewed by someone more familiar with magic
iframes. I only reviewed it from the testing POV; thanks for submitting it.

http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
File chrome/browser/magic_iframe_browsertest.cc (right):

http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
chrome/browser/magic_iframe_browsertest.cc:25: GURL GetTestURL(const char* path)
{
nit: Why not just const std::string& ? Or StringPiece maybe...

http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
chrome/browser/magic_iframe_browsertest.cc:26: std::string url_path =
"files/magic_iframe/";
This seems to be assuming things about current working directory. Please use
PathService to get an absolute path.

http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
chrome/browser/magic_iframe_browsertest.cc:31:
DISALLOW_COPY_AND_ASSIGN(MagicIframeBrowserTest);
nit: No need for DISALLOW_... for test fixtures. Please just get rid of it.

http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
chrome/browser/magic_iframe_browsertest.cc:39: #define
MAYBE_TransferIframeCloseWindow DISABLED_TransferIframeCloseWindow
nit: No MAYBE here, just DISABLED below.

Also, I'm generally not a fan of adding tests that are disabled. Unless you're
landing a two-sided patch or something like that, I'd suggest waiting until it
really works.

http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
chrome/browser/magic_iframe_browsertest.cc:45: LOG(INFO) << "INFO: \n"
Don't spam the output please. It will not hang because browser_tests have an
out-of-process test runner, so hanging test will be forcibly terminated. It can
be just converted to a comment in the code.

http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
chrome/browser/magic_iframe_browsertest.cc:54: LOG(INFO) << "\nLog: " << result;
Do we always need to print the log or can we do that only in case of failure?

Tip: you can do things like ASSERT_NE(...) << result; and the result will only
be printed if the assertion fails.

http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
chrome/browser/magic_iframe_browsertest.cc:57: 
nit: redundant empty line?

http://codereview.chromium.org/7550021/diff/2001/chrome/test/data/magic_ifram...
File chrome/test/data/magic_iframe/iframe-reparenting-close-window-child.html
(right):

http://codereview.chromium.org/7550021/diff/2001/chrome/test/data/magic_ifram...
chrome/test/data/magic_iframe/iframe-reparenting-close-window-child.html:12: // 
 window.close(); // This is what caused the XHR load to silently abort.
Please remove any commented-out code before committing.

http://codereview.chromium.org/7550021/diff/2001/chrome/test/data/magic_ifram...
File chrome/test/data/magic_iframe/iframe-reparenting-close-window.html (right):

http://codereview.chromium.org/7550021/diff/2001/chrome/test/data/magic_ifram...
chrome/test/data/magic_iframe/iframe-reparenting-close-window.html:23:
window.childWindow.addEventListener("load", "log('PASS: Child window loaded.')",
false);
Does the C++ part check for this PASS in the log?

[Dmitry Titov, 2011-08-04 18:49:22]

Thanks for review!

On 2011/08/03 16:27:27, Paweł Hajdan Jr. wrote:
> Please make sure to also get reviewed by someone more familiar with magic
> iframes. I only reviewed it from the testing POV; thanks for submitting it.
> 
>
http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
> File chrome/browser/magic_iframe_browsertest.cc (right):
> 
>
http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
> chrome/browser/magic_iframe_browsertest.cc:25: GURL GetTestURL(const char*
path)
> {
> nit: Why not just const std::string& ? Or StringPiece maybe...

Done.

> 
>
http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
> chrome/browser/magic_iframe_browsertest.cc:26: std::string url_path =
> "files/magic_iframe/";
> This seems to be assuming things about current working directory. Please use
> PathService to get an absolute path.

The "/files" part of the path is not actually present in the file system - it
triggers 'file handler' in TestServer... The path here is already relative to
the chrome/test/data... 
TestServer has a dozen of 'handlers' that have their own fixed 'root
directories' - since they are all relative anyways, we don't have keys for
PathService for them...

> 
>
http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
> chrome/browser/magic_iframe_browsertest.cc:31:
> DISALLOW_COPY_AND_ASSIGN(MagicIframeBrowserTest);
> nit: No need for DISALLOW_... for test fixtures. Please just get rid of it.

Done.

> 
>
http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
> chrome/browser/magic_iframe_browsertest.cc:39: #define
> MAYBE_TransferIframeCloseWindow DISABLED_TransferIframeCloseWindow
> nit: No MAYBE here, just DISABLED below.
> 

Done, simply DISABLED_ now.

> Also, I'm generally not a fan of adding tests that are disabled. Unless you're
> landing a two-sided patch or something like that, I'd suggest waiting until it
> really works.

The fix (which I am working on now) is going to be 2-3 patches... At least one
in WebKit. I could add this patch to the last Chrome-side one or just land it
now, to make final patch easier to review. Since this is hi-pri bug for Quasar,
I'm going to fix/land the whole thing anyways, so let me know which way you
prefer in this case.

> 
>
http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
> chrome/browser/magic_iframe_browsertest.cc:45: LOG(INFO) << "INFO: \n"
> Don't spam the output please. It will not hang because browser_tests have an
> out-of-process test runner, so hanging test will be forcibly terminated. It
can
> be just converted to a comment in the code.

Done, converted to comment in the code.

> 
>
http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
> chrome/browser/magic_iframe_browsertest.cc:54: LOG(INFO) << "\nLog: " <<
result;
> Do we always need to print the log or can we do that only in case of failure?
> 
> Tip: you can do things like ASSERT_NE(...) << result; and the result will only
> be printed if the assertion fails.

Thanks for suggestion, now only printing on failure.

> 
>
http://codereview.chromium.org/7550021/diff/2001/chrome/browser/magic_iframe_...
> chrome/browser/magic_iframe_browsertest.cc:57: 
> nit: redundant empty line?

Removed.

> 
>
http://codereview.chromium.org/7550021/diff/2001/chrome/test/data/magic_ifram...
> File chrome/test/data/magic_iframe/iframe-reparenting-close-window-child.html
> (right):
> 
>
http://codereview.chromium.org/7550021/diff/2001/chrome/test/data/magic_ifram...
> chrome/test/data/magic_iframe/iframe-reparenting-close-window-child.html:12:
// 
>  window.close(); // This is what caused the XHR load to silently abort.
> Please remove any commented-out code before committing.

Thanks! Un-commented, it should be there.

> 
>
http://codereview.chromium.org/7550021/diff/2001/chrome/test/data/magic_ifram...
> File chrome/test/data/magic_iframe/iframe-reparenting-close-window.html
(right):
> 
>
http://codereview.chromium.org/7550021/diff/2001/chrome/test/data/magic_ifram...
> chrome/test/data/magic_iframe/iframe-reparenting-close-window.html:23:
> window.childWindow.addEventListener("load", "log('PASS: Child window
loaded.')",
> false);
> Does the C++ part check for this PASS in the log?

It doesn't, so removed "PASS" part.

[Dmitry Titov, 2011-08-04 21:43:39]

David, could you please take a look from magic-iframe side?

[levin, 2011-08-04 22:12:57]

On 2011/08/04 21:43:39, Dmitry Titov wrote:
> David, could you please take a look from magic-iframe side?

LGTM

Some minor nits (mostly about comment text) which are optional. I think the only
thing which must be done is the 80 columns fixes due to style guidelines.

[levin, 2011-08-04 22:27:36]

Here's my comments. I pressed the wrong link before.

http://codereview.chromium.org/7550021/diff/10006/chrome/browser/magic_iframe...
File chrome/browser/magic_iframe_browsertest.cc (right):

http://codereview.chromium.org/7550021/diff/10006/chrome/browser/magic_iframe...
chrome/browser/magic_iframe_browsertest.cc:25: GURL GetTestURL(const
std::string& path) {
Seems like it shoudn't be in class as there isn't a need/desire to inline this
method.

http://codereview.chromium.org/7550021/diff/10006/chrome/browser/magic_iframe...
chrome/browser/magic_iframe_browsertest.cc:32: // This browser test is verifying
that reparenting of iframe between different
Recommend removing "This browser test is" and start with "Verifies" instead.

http://codereview.chromium.org/7550021/diff/10006/chrome/browser/magic_iframe...
chrome/browser/magic_iframe_browsertest.cc:37: // the original page closes).
This causes the final navigation not complete and
s/not/to not/

http://codereview.chromium.org/7550021/diff/10006/chrome/browser/magic_iframe...
chrome/browser/magic_iframe_browsertest.cc:38: // test terminated by timeout.
s/terminated by/is terminated due to a/

http://codereview.chromium.org/7550021/diff/10006/chrome/test/data/magic_ifra...
File chrome/test/data/magic_iframe/iframe-reparenting-close-window-child.html
(right):

http://codereview.chromium.org/7550021/diff/10006/chrome/test/data/magic_ifra...
chrome/test/data/magic_iframe/iframe-reparenting-close-window-child.html:18:
<iframe id="iframe" src="iframe-reparenting-close-window-iframe.html"></iframe>
80 col.

http://codereview.chromium.org/7550021/diff/10006/chrome/test/data/magic_ifra...
File chrome/test/data/magic_iframe/iframe-reparenting-close-window-iframe.html
(right):

http://codereview.chromium.org/7550021/diff/10006/chrome/test/data/magic_ifra...
chrome/test/data/magic_iframe/iframe-reparenting-close-window-iframe.html:24: //
actually happens before the iframe is transferred and all the onreadystatechange
80 col. (several places)

[Dmitry Titov, 2011-08-04 23:47:39]

On 2011/08/04 22:27:36, levin wrote:
http://codereview.chromium.org/7550021/diff/10006/chrome/browser/magic_iframe...
> File chrome/browser/magic_iframe_browsertest.cc (right):
> 
>
http://codereview.chromium.org/7550021/diff/10006/chrome/browser/magic_iframe...
> chrome/browser/magic_iframe_browsertest.cc:25: GURL GetTestURL(const
> std::string& path) {
> Seems like it shoudn't be in class as there isn't a need/desire to inline this
> method.

Moved outside, made 'static'.

> 
>
http://codereview.chromium.org/7550021/diff/10006/chrome/browser/magic_iframe...
> chrome/browser/magic_iframe_browsertest.cc:32: // This browser test is
verifying
> that reparenting of iframe between different
> Recommend removing "This browser test is" and start with "Verifies" instead.

Done.

> 
>
http://codereview.chromium.org/7550021/diff/10006/chrome/browser/magic_iframe...
> chrome/browser/magic_iframe_browsertest.cc:37: // the original page closes).
> This causes the final navigation not complete and
> s/not/to not/

Done.

> 
>
http://codereview.chromium.org/7550021/diff/10006/chrome/browser/magic_iframe...
> chrome/browser/magic_iframe_browsertest.cc:38: // test terminated by timeout.
> s/terminated by/is terminated due to a/

Done.

> 
>
http://codereview.chromium.org/7550021/diff/10006/chrome/test/data/magic_ifra...
> File chrome/test/data/magic_iframe/iframe-reparenting-close-window-child.html
> (right):
> 
>
http://codereview.chromium.org/7550021/diff/10006/chrome/test/data/magic_ifra...
> chrome/test/data/magic_iframe/iframe-reparenting-close-window-child.html:18:
> <iframe id="iframe"
src="iframe-reparenting-close-window-iframe.html"></iframe>
> 80 col.

Fixed.

> 
>
http://codereview.chromium.org/7550021/diff/10006/chrome/test/data/magic_ifra...
> File chrome/test/data/magic_iframe/iframe-reparenting-close-window-iframe.html
> (right):
> 
>
http://codereview.chromium.org/7550021/diff/10006/chrome/test/data/magic_ifra...
> chrome/test/data/magic_iframe/iframe-reparenting-close-window-iframe.html:24:
//
> actually happens before the iframe is transferred and all the
onreadystatechange
> 80 col. (several places)

Fixed.

[Paweł Hajdan Jr., 2011-08-05 18:00:17]

LGTM

[commit-bot: I haz the power, 2011-08-05 22:56:03]

Change committed as 95676