Instant: Don't commit unless the page supports Instant.

Instant: Don't commit unless the page supports Instant.

BUG=177948
R=samarth@chromium.org
TBR=shishir@chromium.org
TEST=Follow these steps:
1. Install the Google search app.
2. Enable Instant from chrome://settings.
3. Make sure you are signed in so that http://goto.google.com/instant-redirect works.
4. Start chrome with --instant-url="http://goto.google.com/instant-redirect".
Before: Infinite loop of loading and committing google.com
After: No infinite loop.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184467

[sreeram, 2013-02-25 16:40:22]

Samarth, please review.

Shishir knows this stuff, but he isn't in yet. Since this is a release blocking
bug, I'd like to get this in sooner than later. Thanks.

[sreeram, 2013-02-25 17:05:25]

+shishir

[samarth, 2013-02-25 17:15:05]

A couple of questions below and a test, please?

But this lgtm to fix the immediate bug.

Thanks,
Samarth

https://codereview.chromium.org/12334072/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_overlay.cc (right):

https://codereview.chromium.org/12334072/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_overlay.cc:138: // If the page doesn't yet
support Instant, it hasn't fully loaded.
If such a utility doesn't exist already, it would be nice to factor this into a
separate function that maps OpenURLParams to LoadURLParams.

https://codereview.chromium.org/12334072/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_overlay.cc:149:
contents()->GetController().LoadURLWithParams(load_params);
Does this do the right thing with respect to the instant process (i.e. use a new
process if going from an instant URL -> non-instant URL, etc.)?

[Shishir, 2013-02-25 17:20:03]

lgtm

https://codereview.chromium.org/12334072/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_overlay.cc (right):

https://codereview.chromium.org/12334072/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_overlay.cc:147:
load_params.is_cross_site_redirect = params.is_cross_site_redirect;
Shouldn't this always be true here? Maybe dcheck?

https://codereview.chromium.org/12334072/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_overlay.cc:148: 
The WindowDisposition is not being carried over. Is it possible for the page to
try to open a new window/tab?

[sreeram, 2013-02-25 17:48:01]

On 2013/02/25 17:15:05, samarth wrote:
> a test, please?

Sure. I promise to add one soon :)

Sorry that I'm applying double standards here, it's just that I haven't figured
out a good way to test this yet: What we want is to test that CommitIfPossible
is NOT called, so waiting for a commit notification isn't going to work. Both
commit and non-commit paths return the same webcontents, so I can't tell based
on the return value either. I'll figure it out eventually, but for now, I'd like
to fix the P1 bug asap.

https://codereview.chromium.org/12334072/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_overlay.cc (right):

https://codereview.chromium.org/12334072/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_overlay.cc:138: // If the page doesn't yet
support Instant, it hasn't fully loaded.
On 2013/02/25 17:15:05, samarth wrote:
> If such a utility doesn't exist already, it would be nice to factor this into
a
> separate function that maps OpenURLParams to LoadURLParams.

I couldn't find a utility function. I doubt creating one would be useful, since
the only places I could find that did something similar use a third type
(chrome::NavigateParams) to do the conversion to and from, which is completely
unnecessary in our case: Browser::OpenURLFromTab() (browser.cc) and
LoadURLInContents() (browser_navigator.cc).

https://codereview.chromium.org/12334072/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_overlay.cc:147:
load_params.is_cross_site_redirect = params.is_cross_site_redirect;
On 2013/02/25 17:20:03, Shishir wrote:
> Shouldn't this always be true here? Maybe dcheck?

I am not entirely sure (I can't say that I've tested all possible forms of
redirect that would end up in this path), so I'm hesitant to add this DCHECK().
For example, I've not tested what happens if the redirect is on the same-site
(i.e., say google.com -> google.com), except that either the source or the
target (but not both) fall into an app's manifest list (app manifests can
specify both hostname patterns as well as explicit paths).

https://codereview.chromium.org/12334072/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_overlay.cc:148: 
On 2013/02/25 17:20:03, Shishir wrote:
> The WindowDisposition is not being carried over. Is it possible for the page
to
> try to open a new window/tab?

The page may try, but we won't honour it. So, we'll just open it in the same
WebContents.

https://codereview.chromium.org/12334072/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_overlay.cc:149:
contents()->GetController().LoadURLWithParams(load_params);
On 2013/02/25 17:15:05, samarth wrote:
> Does this do the right thing with respect to the instant process (i.e. use a
new
> process if going from an instant URL -> non-instant URL, etc.)?

Yes. The process stuff is handled at a lower level, when the actual OpenURL()
call does its work.

[commit-bot: I haz the power, 2013-02-25 18:43:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sreeram@chromium.org/12334072/1

[commit-bot: I haz the power, 2013-02-25 20:09:57]

Change committed as 184467