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

chrome/browser/instant/instant_overlay.cc

Index: chrome/browser/instant/instant_overlay.cc
diff --git a/chrome/browser/instant/instant_overlay.cc b/chrome/browser/instant/instant_overlay.cc
index 687b1f6b6587a77b8a88daf6f698f0ca93f62ee6..68029eb354fd4ab1422b9b36d2cfda1fd61eface 100644
--- a/chrome/browser/instant/instant_overlay.cc
+++ b/chrome/browser/instant/instant_overlay.cc
@@ -134,6 +134,22 @@ void InstantOverlay::OnMouseUp() {
 content::WebContents* InstantOverlay::OpenURLFromTab(
     content::WebContents* source,
     const content::OpenURLParams& params) {
+  if (!supports_instant()) {
+    // If the page doesn't yet support Instant, it hasn't fully loaded.

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

If such a utility doesn't exist already, it would be nice to factor this into a
separate function that maps OpenURLParams to LoadURLParams.

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

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).

+    // This is a redirect that we should allow. http://crbug.com/177948
+    content::NavigationController::LoadURLParams load_params(params.url);
+    load_params.transition_type = params.transition;
+    load_params.referrer = params.referrer;
+    load_params.extra_headers = params.extra_headers;
+    load_params.is_renderer_initiated = params.is_renderer_initiated;
+    load_params.transferred_global_request_id =
+        params.transferred_global_request_id;
+    load_params.is_cross_site_redirect = params.is_cross_site_redirect;

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

Shouldn't this always be true here? Maybe dcheck?

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

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).

+

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

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]

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

+    contents()->GetController().LoadURLWithParams(load_params);

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

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.)?

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

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

+    return contents();
+  }
+
   // We will allow the navigate to continue if we are able to commit the
   // overlay.
   //