Commit instant loader when the instant page navigates away from instant URL.

Commit instant loader when the instant page navigates away from instant URL.


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

[Shishir, 2012-11-26 23:22:25]

PTAL.

[Shishir, 2012-12-06 21:03:39]

PTAL.

[sky, 2012-12-06 22:37:29]

https://codereview.chromium.org/11416187/diff/3001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_client.cc (right):

https://codereview.chromium.org/11416187/diff/3001/chrome/browser/instant/ins...
chrome/browser/instant/instant_client.cc:108: LOG(ERROR) << "*****
DidCommitProvisionalLoadForFrame: " << url;
remove logging.

https://codereview.chromium.org/11416187/diff/3001/chrome/browser/instant/ins...
chrome/browser/instant/instant_client.cc:109:
delegate_->InstantClientAboutToNavigate(url);
Maybe name this InstantClientAboutToNativeMainFrame to make it clear its for the
main frame.

https://codereview.chromium.org/11416187/diff/3001/content/public/browser/web...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/11416187/diff/3001/content/public/browser/web...
content/public/browser/web_contents_observer.h:103: virtual void
AboutToOpenURL(const GURL& url) {}
Add description.

[Shishir, 2012-12-06 23:20:30]

PTAL.

https://codereview.chromium.org/11416187/diff/3001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_client.cc (right):

https://codereview.chromium.org/11416187/diff/3001/chrome/browser/instant/ins...
chrome/browser/instant/instant_client.cc:108: LOG(ERROR) << "*****
DidCommitProvisionalLoadForFrame: " << url;
On 2012/12/06 22:37:29, sky wrote:
> remove logging.

Done.

https://codereview.chromium.org/11416187/diff/3001/chrome/browser/instant/ins...
chrome/browser/instant/instant_client.cc:109:
delegate_->InstantClientAboutToNavigate(url);
On 2012/12/06 22:37:29, sky wrote:
> Maybe name this InstantClientAboutToNativeMainFrame to make it clear its for
the
> main frame.

Changed to AboutToNativeMainFrame as Samarth pointed out we do not use prefixes
here.

https://codereview.chromium.org/11416187/diff/3001/content/public/browser/web...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/11416187/diff/3001/content/public/browser/web...
content/public/browser/web_contents_observer.h:103: virtual void
AboutToOpenURL(const GURL& url) {}
On 2012/12/06 22:37:29, sky wrote:
> Add description.

Not sure how useful it is, but added.

[sky, 2012-12-07 00:39:34]

LGTM

[Shishir, 2012-12-07 00:41:48]

Adding jam for new API in content/..

[samarth, 2012-12-07 01:14:34]

lgtm

https://codereview.chromium.org/11416187/diff/8012/chrome/browser/instant/ins...
File chrome/browser/instant/instant_client.cc (right):

https://codereview.chromium.org/11416187/diff/8012/chrome/browser/instant/ins...
chrome/browser/instant/instant_client.cc:106: if (!is_main_frame)
nit: fix indent

https://codereview.chromium.org/11416187/diff/8012/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11416187/diff/8012/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:441: if (!IsCurrent() && type !=
INSTANT_COMMIT_NAVIGATED)
This goes against the name of this function :)

How about changing IsCurrent to take a reason?  And while we're at it, I would
rename it to something like CanCommit or CanCommitForReason.  (The only other
place it gets used is in OmniboxLostFocus I think.)

https://codereview.chromium.org/11416187/diff/8012/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:489: // TODO(shishir): Should we
commit the last navigation for
I'm a little confused by this.  Is the situation you're describing where I'm on
the NTP with instant URL A, it navigates to A' (also an instant URL) and then to
B?

https://codereview.chromium.org/11416187/diff/8012/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:781: if (url == instant_url)
Why is this check necessary given the check below?

[jam, 2012-12-07 06:18:14]

https://codereview.chromium.org/11416187/diff/8012/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11416187/diff/8012/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:1564:
AboutToOpenURL(params.url));
who's the WebContentsDelegate that gets called right below this? is it Browser?
i.e. can you get that one to call the instant code so that the content api
doesn't need to get another callback?

https://codereview.chromium.org/11416187/diff/8012/content/public/browser/web...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/11416187/diff/8012/content/public/browser/web...
content/public/browser/web_contents_observer.h:98: virtual void DidOpenURL(const
GURL& url,
nit: looks like this isn't used anymore, can you remove it?

[Shishir, 2012-12-07 21:01:25]

PTAL.

https://codereview.chromium.org/11416187/diff/8012/chrome/browser/instant/ins...
File chrome/browser/instant/instant_client.cc (right):

https://codereview.chromium.org/11416187/diff/8012/chrome/browser/instant/ins...
chrome/browser/instant/instant_client.cc:106: if (!is_main_frame)
On 2012/12/07 01:14:34, samarth wrote:
> nit: fix indent

Done.

https://codereview.chromium.org/11416187/diff/8012/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11416187/diff/8012/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:441: if (!IsCurrent() && type !=
INSTANT_COMMIT_NAVIGATED)
On 2012/12/07 01:14:34, samarth wrote:
> This goes against the name of this function :)
> 
> How about changing IsCurrent to take a reason?  And while we're at it, I would
> rename it to something like CanCommit or CanCommitForReason.  (The only other
> place it gets used is in OmniboxLostFocus I think.)

It seemed better to rename IsCurrent to IsCurrentSearch. CanCommit seems more
general that current status to me.Take a look.

https://codereview.chromium.org/11416187/diff/8012/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:489: // TODO(shishir): Should we
commit the last navigation for
On 2012/12/07 01:14:34, samarth wrote:
> I'm a little confused by this.  Is the situation you're describing where I'm
on
> the NTP with instant URL A, it navigates to A' (also an instant URL) and then
to
> B?

Yes its the case you describe.

https://codereview.chromium.org/11416187/diff/8012/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:781: if (url == instant_url)
On 2012/12/07 01:14:34, samarth wrote:
> Why is this check necessary given the check below?

Not really necessary, just makes it clear that navigating to the instant URL is
also intercepted here. Removing.

https://codereview.chromium.org/11416187/diff/8012/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11416187/diff/8012/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:1564:
AboutToOpenURL(params.url));
On 2012/12/07 06:18:15, John Abd-El-Malek wrote:
> who's the WebContentsDelegate that gets called right below this? is it
Browser?
> i.e. can you get that one to call the instant code so that the content api
> doesn't need to get another callback?

We would like to get this call before the new WebContents is created and the
navigation happens so that we can commit the instant page. After the new
WebContents is created, committing the instant page will lead to the tab having
the wrong Webcontents.

https://codereview.chromium.org/11416187/diff/8012/content/public/browser/web...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/11416187/diff/8012/content/public/browser/web...
content/public/browser/web_contents_observer.h:98: virtual void DidOpenURL(const
GURL& url,
On 2012/12/07 06:18:15, John Abd-El-Malek wrote:
> nit: looks like this isn't used anymore, can you remove it?

Done.

[jam, 2012-12-07 21:21:02]

https://codereview.chromium.org/11416187/diff/8012/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11416187/diff/8012/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:1564:
AboutToOpenURL(params.url));
On 2012/12/07 21:01:25, Shishir wrote:
> On 2012/12/07 06:18:15, John Abd-El-Malek wrote:
> > who's the WebContentsDelegate that gets called right below this? is it
> Browser?
> > i.e. can you get that one to call the instant code so that the content api
> > doesn't need to get another callback?
> 
> We would like to get this call before the new WebContents is created and the
> navigation happens so that we can commit the instant page. After the new
> WebContents is created, committing the instant page will lead to the tab
having
> the wrong Webcontents.

maybe I wasn't clear. I'm not talking about changing the order of when you get
notified. I'm asking if you can get this notification by code change in
src/chrome alone, without modifying WebContentsObserver. delegate_ is
implemented in chrome.

[Shishir, 2012-12-07 21:45:21]

On 2012/12/07 21:21:02, John Abd-El-Malek wrote:
>
https://codereview.chromium.org/11416187/diff/8012/content/browser/web_conten...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/11416187/diff/8012/content/browser/web_conten...
> content/browser/web_contents/web_contents_impl.cc:1564:
> AboutToOpenURL(params.url));
> On 2012/12/07 21:01:25, Shishir wrote:
> > On 2012/12/07 06:18:15, John Abd-El-Malek wrote:
> > > who's the WebContentsDelegate that gets called right below this? is it
> > Browser?
> > > i.e. can you get that one to call the instant code so that the content api
> > > doesn't need to get another callback?
> > 
> > We would like to get this call before the new WebContents is created and the
> > navigation happens so that we can commit the instant page. After the new
> > WebContents is created, committing the instant page will lead to the tab
> having
> > the wrong Webcontents.
> 
> maybe I wasn't clear. I'm not talking about changing the order of when you get
> notified. I'm asking if you can get this notification by code change in
> src/chrome alone, without modifying WebContentsObserver. delegate_ is
> implemented in chrome.

Changed to not make changes to the content API. Not sure if this is the better
approach or not. PTAL.

[samarth, 2012-12-07 21:47:29]

FYI, sreeram renamed IsCurrent in 11476018, so you'll need to rebase on top of
his change.

[Shishir, 2012-12-07 22:00:05]

On 2012/12/07 21:47:29, samarth wrote:
> FYI, sreeram renamed IsCurrent in 11476018, so you'll need to rebase on top of
> his change.

Thanks, will do.

[sky, 2012-12-07 22:01:45]

https://codereview.chromium.org/11416187/diff/9007/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11416187/diff/9007/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1329: if (instant_controller_)
Does this really do what you want? Seems like you want
WebContentsObserver::NavigateToPendingEntry

[jam, 2012-12-07 22:11:53]

> > maybe I wasn't clear. I'm not talking about changing the order of when you
get
> > notified. I'm asking if you can get this notification by code change in
> > src/chrome alone, without modifying WebContentsObserver. delegate_ is
> > implemented in chrome.
> 
> Changed to not make changes to the content API. Not sure if this is the better
> approach or not. PTAL.

in general, we only want to add stuff to the content api if there's no other way
to do this. the api is meant to be minimal.

[Shishir, 2012-12-08 00:14:53]

PTAL.

https://codereview.chromium.org/11416187/diff/9007/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11416187/diff/9007/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1329: if (instant_controller_)
On 2012/12/07 22:01:46, sky wrote:
> Does this really do what you want? Seems like you want
> WebContentsObserver::NavigateToPendingEntry

Ok I got this  wrong. The change does not belong here but in the
InstantLoader::WebContentsDelegate which is the delegate for the InstantPage and
not the browser. I think this change should make it clear why we need to make
the change in OpenURLFromTab.

[sky, 2012-12-10 14:47:30]

LGTM with the following change

https://codereview.chromium.org/11416187/diff/16019/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11416187/diff/16019/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:165: if
(loader_->controller_->CommitIfCurrent(INSTANT_COMMIT_NAVIGATED))
Style guide says you want one of the following here:

  if (a)
    return b;
  return c;

or:
  return a ? b : c;

[Shishir, 2012-12-10 18:35:52]

https://codereview.chromium.org/11416187/diff/16019/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11416187/diff/16019/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:165: if
(loader_->controller_->CommitIfCurrent(INSTANT_COMMIT_NAVIGATED))
On 2012/12/10 14:47:30, sky wrote:
> Style guide says you want one of the following here:
> 
>   if (a)
>     return b;
>   return c;
> 
> or:
>   return a ? b : c;
> 

Done.

[commit-bot: I haz the power, 2012-12-10 18:37:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/11416187/21001

[commit-bot: I haz the power, 2012-12-10 18:40:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/11416187/21001

[Shishir, 2012-12-10 18:50:58]

On 2012/12/10 18:40:53, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/shishir%40chromium.org/11416187/21001

Stopping the commit as I just noticed a possibly related crash.

[Shishir, 2012-12-10 19:53:14]

Fixed the crash that was caused due to removal of TabContents from the
InstantLoader.

Adding Avi for chrome/browser/instant/instant_loader.cc

[Avi (use Gerrit), 2012-12-10 19:55:51]

LGTM

[commit-bot: I haz the power, 2012-12-10 20:07:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/11416187/21006

[commit-bot: I haz the power, 2012-12-10 22:56:55]

Change committed as 172168