Use the request's URL rather than the document's when deciding to swap.

Use the request's URL rather than the document's when deciding to swap.
This allows pages to use a meta-refresh when trying to fork a new renderer
process for a cross-site popup.

Also migrate some stale/flaky UI tests to browser tests.

BUG=93517
BUG=32799
TEST=Use a meta-refresh instead of assigning to w.document.location in
http://www.google.com/chrome/intl/en/webmasters-faq.html#newtab


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

[Charlie Reis, 2011-08-25 18:32:20]

Matt: can you review the change and the new tests, since you've seen
decidePolicyForNavigation before?  I checked with Adam Barth (who changed the
old_url line as part of http://codereview.chromium.org/7242014), and this seems
like the right fix.

Pawel: can you take a quick look at the new tests for signs of potential
flakiness?  I've ported them over from some rotting and flaky UI tests, so
hopefully this will be an improvement.

John: can you take a look for owner's approval?

Thanks all!

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/tests/browser...
File chrome/browser/ui/tests/browser_uitest.cc (left):

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/tests/browser...
chrome/browser/ui/tests/browser_uitest.cc:83: #ifdef CHROME_V8
This ifdef is apparently stale, so this test hasn't been running.  It doesn't
even compile anymore...

It's been a long time since we've supported KJS, so I'm gutting this and moving
it to a browsertest.

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/tests/browser...
chrome/browser/ui/tests/browser_uitest.cc:130: #endif
Hoping the move to browsertests resolves this flakiness.

[Paweł Hajdan Jr., 2011-08-25 18:34:56]

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/browser_brows...
File chrome/browser/ui/browser_browsertest.cc (right):

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/browser_brows...
chrome/browser/ui/browser_browsertest.cc:376:
ui_test_utils::WaitForNewTab(browser());
This is suspicious. Could you use a windowed observer and add a TODO for
WaitForNewTab to remove this inherently flaky function?

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/browser_brows...
chrome/browser/ui/browser_browsertest.cc:383:
ui_test_utils::WaitForNavigation(&newtab->controller());
Don't use it. It's inherently flaky. Windowed observer instead please.

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/tests/browser...
File chrome/browser/ui/tests/browser_uitest.cc (left):

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/tests/browser...
chrome/browser/ui/tests/browser_uitest.cc:130: #endif
On 2011/08/25 18:32:20, creis wrote:
> Hoping the move to browsertests resolves this flakiness.

Ha ha, it's not that it magically solves all problems.

[Charlie Reis, 2011-08-25 18:44:45]

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/browser_brows...
File chrome/browser/ui/browser_browsertest.cc (right):

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/browser_brows...
chrome/browser/ui/browser_browsertest.cc:376:
ui_test_utils::WaitForNewTab(browser());
On 2011/08/25 18:34:56, Paweł Hajdan Jr. wrote:
> This is suspicious. Could you use a windowed observer and add a TODO for
> WaitForNewTab to remove this inherently flaky function?

Sure.  I haven't seen windowed observers before-- can you point to an example in
a test?

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/tests/browser...
File chrome/browser/ui/tests/browser_uitest.cc (left):

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/tests/browser...
chrome/browser/ui/tests/browser_uitest.cc:130: #endif
On 2011/08/25 18:34:56, Paweł Hajdan Jr. wrote:
> On 2011/08/25 18:32:20, creis wrote:
> > Hoping the move to browsertests resolves this flakiness.
> 
> Ha ha, it's not that it magically solves all problems.

Well, at least it's a chance to get rid of things like this...  :)

base::PlatformThread::Sleep(TestTimeouts::action_timeout_ms());

[Mihai Parparita -not on Chrome, 2011-08-25 18:44:55]

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/tests/browser...
File chrome/browser/ui/tests/browser_uitest.cc (left):

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/tests/browser...
chrome/browser/ui/tests/browser_uitest.cc:83: #ifdef CHROME_V8
On 2011/08/25 18:32:20, creis wrote:
> This ifdef is apparently stale, so this test hasn't been running.  It doesn't
> even compile anymore...
> 
> It's been a long time since we've supported KJS, so I'm gutting this and
moving
> it to a browsertest.

Not necessarily in this CL, but were you planning on removing other usages of
CHROME_V8 and related things (the javascript_engine gyp variable)?

[Matt Perry, 2011-08-25 18:45:20]

lgtm

http://codereview.chromium.org/7740028/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7740028/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:2131: // Use the request's URL rather than the
document's URL for this check.  For
nit: say something like "the frame's original request's URL" to distinguish it
from the currently fetched request.

[Charlie Reis, 2011-08-25 18:56:53]

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/tests/browser...
File chrome/browser/ui/tests/browser_uitest.cc (left):

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/tests/browser...
chrome/browser/ui/tests/browser_uitest.cc:83: #ifdef CHROME_V8
On 2011/08/25 18:44:55, Mihai Parparita wrote:
> On 2011/08/25 18:32:20, creis wrote:
> > This ifdef is apparently stale, so this test hasn't been running.  It
doesn't
> > even compile anymore...
> > 
> > It's been a long time since we've supported KJS, so I'm gutting this and
> moving
> > it to a browsertest.
> 
> Not necessarily in this CL, but were you planning on removing other usages of
> CHROME_V8 and related things (the javascript_engine gyp variable)?

I actually don't know much about the history of CHROME_V8 and whether it's
supposed to be deprecated, but I agree that the rest of the references to it
should probably be removed if it's not doing what it used to.  I've filed
http://crbug.com/94282 to remove it.

[jam, 2011-08-25 19:19:02]

lgtm

[Charlie Reis, 2011-08-25 22:07:30]

Thanks guys.  Paweł, can you take another look at the test?

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/browser_brows...
File chrome/browser/ui/browser_browsertest.cc (right):

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/browser_brows...
chrome/browser/ui/browser_browsertest.cc:376:
ui_test_utils::WaitForNewTab(browser());
On 2011/08/25 18:44:45, creis wrote:
> On 2011/08/25 18:34:56, Paweł Hajdan Jr. wrote:
> > This is suspicious. Could you use a windowed observer and add a TODO for
> > WaitForNewTab to remove this inherently flaky function?
> 
> Sure.  I haven't seen windowed observers before-- can you point to an example
in
> a test?

Done.

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/browser_brows...
chrome/browser/ui/browser_browsertest.cc:383:
ui_test_utils::WaitForNavigation(&newtab->controller());
On 2011/08/25 18:34:56, Paweł Hajdan Jr. wrote:
> Don't use it. It's inherently flaky. Windowed observer instead please.

Done.

http://codereview.chromium.org/7740028/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7740028/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:2131: // Use the request's URL rather than the
document's URL for this check.  For
On 2011/08/25 18:45:20, Matt Perry wrote:
> nit: say something like "the frame's original request's URL" to distinguish it
> from the currently fetched request.

Done.

[Charlie Reis, 2011-08-25 22:18:47]

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/browser_brows...
File chrome/browser/ui/browser_browsertest.cc (right):

http://codereview.chromium.org/7740028/diff/1/chrome/browser/ui/browser_brows...
chrome/browser/ui/browser_browsertest.cc:376:
ui_test_utils::WaitForNewTab(browser());
On 2011/08/25 22:07:31, creis wrote:
> On 2011/08/25 18:44:45, creis wrote:
> > On 2011/08/25 18:34:56, Paweł Hajdan Jr. wrote:
> > > This is suspicious. Could you use a windowed observer and add a TODO for
> > > WaitForNewTab to remove this inherently flaky function?
> > 
> > Sure.  I haven't seen windowed observers before-- can you point to an
example
> in
> > a test?
> 
> Done.

Got a merge conflict in ui_test_utils.h.  Looks like Greg beat me to the TODO. 
:)

[Paweł Hajdan Jr., 2011-08-25 23:36:30]

LGTM with a comment. Thanks!

http://codereview.chromium.org/7740028/diff/6002/chrome/browser/ui/browser_br...
File chrome/browser/ui/browser_browsertest.cc (right):

http://codereview.chromium.org/7740028/diff/6002/chrome/browser/ui/browser_br...
chrome/browser/ui/browser_browsertest.cc:388: EXPECT_EQ(https_url.spec(),
Do you remember earlier crash you encountered here? ASSERT_TRUE that things are
not NULL.

[commit-bot: I haz the power, 2011-08-26 18:33:47]

Change committed as 98457