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

chrome/browser/instant/instant_overlay.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/instant/instant_overlay.h"
 
 #include "base/auto_reset.h"
 #include "base/supports_user_data.h"
 #include "chrome/browser/ui/search/search.h"
 #include "content/public/browser/web_contents.h"
@@ skipping 116 matching lines @@
 void InstantOverlay::OnMouseUp() {
   if (is_pointer_down_from_activate_) {
     is_pointer_down_from_activate_ = false;
     instant_controller()->CommitIfPossible(INSTANT_COMMIT_FOCUS_LOST);
   }
 }
 
 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.
   //
   // First, cache the overlay contents since committing it will cause the
   // contents to be released (and be set to NULL).
   content::WebContents* overlay = contents();
   if (instant_controller()->CommitIfPossible(INSTANT_COMMIT_NAVIGATED)) {
     // If the commit was successful, the overlay's delegate should be the tab
     // strip, which will be able to handle the navigation.
     DCHECK_NE(&loader_, overlay->GetDelegate());
     return overlay->GetDelegate()->OpenURLFromTab(source, params);
   }
   return NULL;
 }
 
 void InstantOverlay::HandleStalePage() {
   is_stale_ = true;
   instant_controller()->ReloadOverlayIfStale();
 }