Remove BackForwardClient, cleanup starting a history navigation

Remove BackForwardClient, cleanup starting a history navigation

* Don't proxy history navigations through the main frame, there's no need
* Have Source/core interact with history state entirely through FrameLoaderClient. It doesn't need to know that FrameLoaderClientImpl is actually talking to WebViewClient (though that will need to change eventually)
* Remove NavigationScheduler::scheduleHistoryNavigation. History navigations are always asynchronous anyway, since they need to talk to the browser process.

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184989

[Nate Chapin, 2014-10-22 19:57:17]

japhet@chromium.org changed reviewers:
+ abarth@chromium.org

[Nate Chapin, 2014-10-22 19:57:18]

https://codereview.chromium.org/669013002/diff/20001/Source/core/loader/Navig...
File Source/core/loader/NavigationScheduler.cpp (left):

https://codereview.chromium.org/669013002/diff/20001/Source/core/loader/Navig...
Source/core/loader/NavigationScheduler.cpp:352: cancel();
Dropping this cancel() should be the only behavior change of this patch. This
dates back to http://src.chromium.org/viewvc/blink?view=revision&revision=50111,
where it was referred to as a side effect. I don't think it's a desirable or
necessary side effect, but I'm not absolutely certain.

https://codereview.chromium.org/669013002/diff/20001/Source/web/FrameLoaderCl...
File Source/web/FrameLoaderClientImpl.cpp (left):

https://codereview.chromium.org/669013002/diff/20001/Source/web/FrameLoaderCl...
Source/web/FrameLoaderClientImpl.cpp:544: offset = std::max(offset,
-webview->client()->historyBackListCount());
This min/max bounding is unnecessary. For backspace navigation, where the only
offsets are -1 and 1, bounding renders the navigation invalid by setting the
offset to 0. For normal back/forward navigations, we have always declared the
navigation invalid if the offset was out of bounds (in NavigationScheduler).

[abarth-chromium, 2014-10-23 04:20:12]

Would you be willing to find another reviewer?

[Nate Chapin, 2014-10-23 18:44:46]

japhet@chromium.org changed reviewers:
+ dcheng@chromium.org

[Nate Chapin, 2014-10-23 18:44:46]

dcheng, is this something you're comfortable reviewing?

[dcheng, 2014-11-04 19:07:40]

https://codereview.chromium.org/669013002/diff/20001/Source/core/loader/Navig...
File Source/core/loader/NavigationScheduler.cpp (left):

https://codereview.chromium.org/669013002/diff/20001/Source/core/loader/Navig...
Source/core/loader/NavigationScheduler.cpp:352: cancel();
On 2014/10/22 at 19:57:17, Nate Chapin wrote:
> Dropping this cancel() should be the only behavior change of this patch. This
dates back to http://src.chromium.org/viewvc/blink?view=revision&revision=50111,
where it was referred to as a side effect. I don't think it's a desirable or
necessary side effect, but I'm not absolutely certain.

Let's make sure we match the behavior of other browsers. If IE/FF don't cancel
load on an invalid history navigation, then we should definitely match them =)

[Nate Chapin, 2014-11-06 23:10:31]

On 2014/11/04 19:07:40, dcheng wrote:
>
https://codereview.chromium.org/669013002/diff/20001/Source/core/loader/Navig...
> File Source/core/loader/NavigationScheduler.cpp (left):
> 
>
https://codereview.chromium.org/669013002/diff/20001/Source/core/loader/Navig...
> Source/core/loader/NavigationScheduler.cpp:352: cancel();
> On 2014/10/22 at 19:57:17, Nate Chapin wrote:
> > Dropping this cancel() should be the only behavior change of this patch.
This
> dates back to
http://src.chromium.org/viewvc/blink?view=revision&revision=50111,
> where it was referred to as a side effect. I don't think it's a desirable or
> necessary side effect, but I'm not absolutely certain.
> 
> Let's make sure we match the behavior of other browsers. If IE/FF don't cancel
> load on an invalid history navigation, then we should definitely match them =)

FF doesn't cancel the previous load, will test IE when I get home tonight.

[Nate Chapin, 2014-11-07 01:16:40]

On 2014/11/06 23:10:31, Nate Chapin wrote:
> On 2014/11/04 19:07:40, dcheng wrote:
> >
>
https://codereview.chromium.org/669013002/diff/20001/Source/core/loader/Navig...
> > File Source/core/loader/NavigationScheduler.cpp (left):
> > 
> >
>
https://codereview.chromium.org/669013002/diff/20001/Source/core/loader/Navig...
> > Source/core/loader/NavigationScheduler.cpp:352: cancel();
> > On 2014/10/22 at 19:57:17, Nate Chapin wrote:
> > > Dropping this cancel() should be the only behavior change of this patch.
> This
> > dates back to
> http://src.chromium.org/viewvc/blink?view=revision&revision=50111,
> > where it was referred to as a side effect. I don't think it's a desirable or
> > necessary side effect, but I'm not absolutely certain.
> > 
> > Let's make sure we match the behavior of other browsers. If IE/FF don't
cancel
> > load on an invalid history navigation, then we should definitely match them
=)
> 
> FF doesn't cancel the previous load, will test IE when I get home tonight.

I

[Nate Chapin, 2014-11-07 01:17:05]

On 2014/11/06 23:10:31, Nate Chapin wrote:
> On 2014/11/04 19:07:40, dcheng wrote:
> >
>
https://codereview.chromium.org/669013002/diff/20001/Source/core/loader/Navig...
> > File Source/core/loader/NavigationScheduler.cpp (left):
> > 
> >
>
https://codereview.chromium.org/669013002/diff/20001/Source/core/loader/Navig...
> > Source/core/loader/NavigationScheduler.cpp:352: cancel();
> > On 2014/10/22 at 19:57:17, Nate Chapin wrote:
> > > Dropping this cancel() should be the only behavior change of this patch.
> This
> > dates back to
> http://src.chromium.org/viewvc/blink?view=revision&revision=50111,
> > where it was referred to as a side effect. I don't think it's a desirable or
> > necessary side effect, but I'm not absolutely certain.
> > 
> > Let's make sure we match the behavior of other browsers. If IE/FF don't
cancel
> > load on an invalid history navigation, then we should definitely match them
=)
> 
> FF doesn't cancel the previous load, will test IE when I get home tonight.

IE doesn't cancel the previous load either, so I think this behavior change
makes things more standardized.

[dcheng, 2014-11-07 07:17:23]

lgtm

[Nate Chapin, 2014-11-07 20:12:03]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-11-07 20:12:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/669013002/40001

[commit-bot: I haz the power, 2014-11-07 21:22:27]

Committed patchset #3 (id:40001) as 184989