DidNavigate refactor of doom

This is almost a complete rewrite of DidNavigate and the associated NavigationController logic. The approach is that the NavigationController should be responsible for the logic and memory management of navigation. Previously, half the logic and memory management lived in WebContents which made it very hard to figure out what was going on.

I split out the various navigation types into separate functions, which then copy and update any existing NavigationEntry as necessary. Previously, WebContents would make a new one which would be manually populated with random fields (I think some were forgotten, too), and then the NavigationController may or may not commit it.


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

[brettw, 2008-09-09 15:35:12]

This passes all unit tests but still breaks a few UI tests related to session
saving and SSL. There are also a few FIXMEs in there (for example, in
TabContents there is some unfinished stuff). This patch is already so large, I
thought it would be good to get a head start on reviewing it. Hopefully my
additional changes will be very easy to see on the diff.

[brettw, 2008-09-09 15:41:17]

Huan, you reviewed the last in this series of patches. You may want to give this
a look as well.

[brettw, 2008-09-09 16:24:21]

Jay, can you look at SSL handling?

[wtc, 2008-09-09 16:54:52]

I looked at ssl_manager.cc, ssl_policy.cc, and
ssl_blocking_page.cc.

The changes to ssl_manager.cc and ssl_policy.cc are
straightforward -- the NOTIFY_SSL_STATE_CHANGED
notifications have no details.

I don't fully understand the changes to
ssl_blocking_page.cc, so I'll let others review
them.  I have a minor comment below.

http://codereview.chromium.org/479/diff/403/645
File chrome/browser/ssl_blocking_page.cc (right):

http://codereview.chromium.org/479/diff/403/645#newcode149
Line 149: entry = tab_->controller()->GetPendingEntry();
This whole else block should be deleted, because
it is just assigning the current value of 'entry' to
'entry'.

[jcampan, 2008-09-09 20:32:52]

I looked at:
ssl_manager.cc, ssl_policy.cc, ssl_blocking_page.cc, web_contents.cc

LGTM

http://codereview.chromium.org/479/diff/403/646
File chrome/browser/navigation_controller.cc (right):

http://codereview.chromium.org/479/diff/403/646#newcode719
Line 719: // information (the rest was the default for a new entry). Why?
I think this was so that if the info bar is updated before the SSL manager was
notified the style is the old style as opposed to the default style. That would
avoid the location bar flashing (from secure to insecure).
But I am not sure this make sense since the notification to the SSL manager
would happen right after this (before the location bar had a chance to repaint)
so I could be wrong.
You can leave it out like this, I'll take a look once you have checked in.

[brettw, 2008-09-09 22:42:36]

http://codereview.chromium.org/479/diff/403/646
File chrome/browser/navigation_controller.cc (right):

http://codereview.chromium.org/479/diff/403/646#newcode719
Line 719: // information (the rest was the default for a new entry). Why?
Maybe you should file a bug on this so you don't forget. I'm not entirely sure
what you mean to do to check it.

[Charlie Reis, 2008-09-10 00:21:50]

The SiteInstance changes seem to look OK to me.  Mainly some comment nits below.

http://codereview.chromium.org/479/diff/685/413
File chrome/browser/native_ui_contents.cc (right):

http://codereview.chromium.org/479/diff/685/413#newcode221
Line 221: // This is not a WebContents, so we use a NULL SiteInstance.
Doesn't look like you need this comment anymore.

http://codereview.chromium.org/479/diff/685/413#newcode281
Line 281: // This is not a WebContents, so we use a NULL SiteInstance.
Ditto.

http://codereview.chromium.org/479/diff/685/412
File chrome/browser/navigation_controller.cc (right):

http://codereview.chromium.org/479/diff/685/412#newcode45
Line 45: // treat as a new navigation.
The first sentence of this paragraph doesn't make sense as is.

http://codereview.chromium.org/479/diff/685/408
File chrome/browser/navigation_controller.h (right):

http://codereview.chromium.org/479/diff/685/408#newcode243
Line 243: // given details structure and edbroadcast the
NOTIFY_NAV_ENTRY_COMMITT
nits: edbroadcast, _COMMITT

http://codereview.chromium.org/479/diff/685/421
File chrome/browser/navigation_controller_unittest.cc (left):

http://codereview.chromium.org/479/diff/685/421#oldcode440
Line 440: TEST_F(NavigationControllerTest, LoadURL_NoPending) {
Is there a reason we can't test this anymore?  (This corresponds to link clicks,
right?)

http://codereview.chromium.org/479/diff/685/421
File chrome/browser/navigation_controller_unittest.cc (right):

http://codereview.chromium.org/479/diff/685/421#newcode600
Line 600: //contents->controller()->LoadURL(url1, PageTransition::TYPED);
Don't need these commented out lines.

http://codereview.chromium.org/479/diff/685/423
File chrome/browser/session_service.cc (right):

http://codereview.chromium.org/479/diff/685/423#newcode435
Line 435: controller->GetCurrentEntryIndex()));
Could grab current_entry_index to a local, since we compute it 3 times.  Not
expensive, though.

[brettw, 2008-09-10 19:32:25]

New snap is up. This is final as far as I know. All the tests pass and there are
no more FIXMEs.

For this patch, I reorganized how interstitials work. There is a special
function for adding and removing magic navigation entries for interstitials.
This patch also fixes an assertion in the old code where the navigation buttons
didn't update after you went back from an interstitial.

I updated how WebContents::Navigate works, renaming it to NavigateToPendingEntry
which cleans it up and avoids some problems. The tab contents can now just call
commit on the pending entry rather than making new entries themselves.

http://codereview.chromium.org/479/diff/685/421
File chrome/browser/navigation_controller_unittest.cc (left):

http://codereview.chromium.org/479/diff/685/421#oldcode440
Line 440: TEST_F(NavigationControllerTest, LoadURL_NoPending) {
No. I removed this test since it was redundant because I changed almost all
other tests to use "unrequested" navigations. But then I changed the other tests
back. I'll put this test back as well.

[jcampan, 2008-09-10 22:38:12]

LGTM

with nits below

http://codereview.chromium.org/479/diff/792/461
File chrome/browser/navigation_controller.cc (right):

http://codereview.chromium.org/479/diff/792/461#newcode631
Line 631: if (existing_entry_index == -1)
What case would that be? Should we DCHECK?

http://codereview.chromium.org/479/diff/792/470
File chrome/browser/navigation_controller_unittest.cc (right):

http://codereview.chromium.org/479/diff/792/470#newcode458
Line 458: // Tests navigations that come in unrequested. This happens when the
user
Remove extra white spaces

http://codereview.chromium.org/479/diff/792/470#newcode459
Line 459: // navigates from the web page, and here we test that there is no
pending entry.
Same here

http://codereview.chromium.org/479/diff/792/470#newcode481
Line 481: 
Remove extra white spaces.

http://codereview.chromium.org/479/diff/792/470#newcode708
Line 708: //contents->controller()->LoadURL(kUrl1, PageTransition::TYPED);
Why is this commented out?

[Charlie Reis, 2008-09-10 22:58:19]

LGTM, apart from one question below.

http://codereview.chromium.org/479/diff/792/461
File chrome/browser/navigation_controller.cc (right):

http://codereview.chromium.org/479/diff/792/461#newcode707
Line 707: entry->set_site_instance(active_contents_->GetSiteInstance());
Hmm, I missed this one before.  Did this correspond to an existing call to
set_site_instance somewhere else?

I don't think redirects affect the SiteInstance of an entry, since the
SiteInstance is created or selected during browser-initiated navigations and
doesn't change after renderer-initiated navigations.  That would mean this line
would be a no-op, though, which is fine.

[brettw, 2008-09-11 02:46:36]

http://codereview.chromium.org/479/diff/792/461
File chrome/browser/navigation_controller.cc (right):

http://codereview.chromium.org/479/diff/792/461#newcode707
Line 707: entry->set_site_instance(active_contents_->GetSiteInstance());
This gets hit during session restore. I clarified this and added a DCHECK for
the exact circumstances.

[Charlie Reis, 2008-09-11 03:59:27]

http://codereview.chromium.org/479/diff/792/461
File chrome/browser/navigation_controller.cc (right):

http://codereview.chromium.org/479/diff/792/461#newcode707
Line 707: entry->set_site_instance(active_contents_->GetSiteInstance());
On 2008/09/11 02:46:36, brettw wrote:
> This gets hit during session restore. I clarified this and added a DCHECK for
> the exact circumstances.

OK