Remove old session history logic.

Remove old session history logic.

Now that UseSubframeNavigationEntries is on by default, we don't need
the old session history logic anymore, including HistoryController.

BUG=236848, 472717
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2648053002
Cr-Commit-Position: refs/heads/master@{#446374}
Committed: https://chromium.googlesource.com/chromium/src/+/ce0ef3575be4a50acfc2f2682878a69f8ce93bf0

[Charlie Reis, 2017-01-21 01:03:48]

Description was changed from

==========
Remove old navigation logic, now that UseSubframeNavigationEntries is on.

BUG=236848
TEST=No behavior change.
==========

to

==========
Remove old navigation logic, now that UseSubframeNavigationEntries is on.

BUG=236848
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2017-01-21 01:04:23]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-21 01:04:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-21 03:21:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-21 03:21:12]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-01-24 01:56:28]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 01:57:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2017-01-24 01:58:57]

Description was changed from

==========
Remove old navigation logic, now that UseSubframeNavigationEntries is on.

BUG=236848
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove old navigation logic, now that UseSubframeNavigationEntries is on.

BUG=236848, 472717
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2017-01-24 02:15:33]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 02:16:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 03:54:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 03:54:12]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-01-24 17:34:24]

creis@chromium.org changed reviewers:
+ nasko@chromium.org

[Charlie Reis, 2017-01-24 17:34:25]

Nasko, can you take a look when you get a chance?  (No rush, since I know you're
busy today.)  This is the big one, removing HistoryController and the policy
function itself.

https://codereview.chromium.org/2648053002/diff/40001/content/renderer/histor...
File content/renderer/history_controller_browsertest.cc (left):

https://codereview.chromium.org/2648053002/diff/40001/content/renderer/histor...
content/renderer/history_controller_browsertest.cc:40:
TEST_F(HistoryControllerTest, MAYBE_InertCommitRemovesChildren) {
This one seems more like a unit test, and we have similar test coverage in
NavigationControllerBrowserTest's FrameNavigationEntry_BackWithRedirect.  Issue
472717 was marked as fixed anyway.  :)

https://codereview.chromium.org/2648053002/diff/40001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2648053002/diff/40001/content/renderer/render...
content/renderer/render_view_browsertest.cc:938: TEST_F(RenderViewImplTest, 
DISABLED_LastCommittedUpdateState) {
This passed for me locally with 100 iterations.  Maybe I'll try re-enabling in a
followup CL.

https://codereview.chromium.org/2648053002/diff/40001/content/test/layouttest...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/2648053002/diff/40001/content/test/layouttest...
content/test/layouttest_support.cc:460: // See https://crbug.com/477150.
There's lots of layout tests that use dumpBackForwardList and still pass (e.g.,
back-during-beforeunload.html, history-back-initial-vs-final-url.html, etc), so
I'm not sure if we need this code at all.  It hasn't been doing anything since
August, anyway, so I'll try removing it in a followup.

[nasko, 2017-01-26 00:56:20]

This is super exciting! I'd change the CL title/description to say "session
history logic" instead of "navigation logic", as I think of the old navigation
logic being non-PlzNavigate code and this CL removes the already unused session
history code.

That said, LGTM!

https://codereview.chromium.org/2648053002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2648053002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:4872: // Method and post id are set for
all frames.
Does this comment still add meaningful info? Why wouldn't we add the info on
some frames?

https://codereview.chromium.org/2648053002/diff/40001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2648053002/diff/40001/content/renderer/render...
content/renderer/render_view_browsertest.cc:938: TEST_F(RenderViewImplTest, 
DISABLED_LastCommittedUpdateState) {
On 2017/01/24 17:34:24, Charlie Reis wrote:
> This passed for me locally with 100 iterations.  Maybe I'll try re-enabling in
a
> followup CL.

I'd also say try it in ASan build.

https://codereview.chromium.org/2648053002/diff/40001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2648053002/diff/40001/content/renderer/render...
content/renderer/render_view_impl.cc:1355: void
RenderViewImpl::SendFrameStateUpdates() {
Completely separate, but I wonder if we can remove this too and drive sending
updates for each frame separately as a follow up.

https://codereview.chromium.org/2648053002/diff/40001/content/test/layouttest...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/2648053002/diff/40001/content/test/layouttest...
content/test/layouttest_support.cc:460: // See https://crbug.com/477150.
On 2017/01/24 17:34:24, Charlie Reis wrote:
> There's lots of layout tests that use dumpBackForwardList and still pass
(e.g.,
> back-during-beforeunload.html, history-back-initial-vs-final-url.html, etc),
so
> I'm not sure if we need this code at all.  It hasn't been doing anything since
> August, anyway, so I'll try removing it in a followup.

sgtm

[Charlie Reis, 2017-01-26 00:59:33]

Description was changed from

==========
Remove old navigation logic, now that UseSubframeNavigationEntries is on.

BUG=236848, 472717
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove old session history logic.

Now that UseSubframeNavigationEntries is on by default, we don't need
the old session history logic anymore, including HistoryController.

BUG=236848, 472717
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2017-01-26 01:07:47]

Thanks!

https://codereview.chromium.org/2648053002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2648053002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:4872: // Method and post id are set for
all frames.
On 2017/01/26 00:56:20, nasko wrote:
> Does this comment still add meaningful info? Why wouldn't we add the info on
> some frames?

Done.

https://codereview.chromium.org/2648053002/diff/40001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2648053002/diff/40001/content/renderer/render...
content/renderer/render_view_impl.cc:1355: void
RenderViewImpl::SendFrameStateUpdates() {
On 2017/01/26 00:56:20, nasko wrote:
> Completely separate, but I wonder if we can remove this too and drive sending
> updates for each frame separately as a follow up.

I spent some time thinking about this when writing SendFrameStateUpdates, with
the reasoning that it's better to have fewer timers and wakeups.  Some
discussion here:
https://codereview.chromium.org/1449353002#msg4

Happy to chat more about it separately, though.

[Charlie Reis, 2017-01-26 01:07:52]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2017-01-26 01:07:53]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2648053002/#ps60001
(title: "Remove comment.")

[commit-bot: I haz the power, 2017-01-26 01:08:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 04:16:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 04:16:22]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Charlie Reis, 2017-01-26 17:09:34]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2017-01-26 17:09:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 17:53:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485450574814880,
"parent_rev": "7ecd005e3a871cc9a4979c0a3ef822d9e7c0a787", "commit_rev":
"ce0ef3575be4a50acfc2f2682878a69f8ce93bf0"}

[commit-bot: I haz the power, 2017-01-26 17:53:47]

Description was changed from

==========
Remove old session history logic.

Now that UseSubframeNavigationEntries is on by default, we don't need
the old session history logic anymore, including HistoryController.

BUG=236848, 472717
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove old session history logic.

Now that UseSubframeNavigationEntries is on by default, we don't need
the old session history logic anymore, including HistoryController.

BUG=236848, 472717
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2648053002
Cr-Commit-Position: refs/heads/master@{#446374}
Committed:
https://chromium.googlesource.com/chromium/src/+/ce0ef3575be4a50acfc2f2682878...
==========

[commit-bot: I haz the power, 2017-01-26 17:53:48]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/ce0ef3575be4a50acfc2f2682878...

[Avi (use Gerrit), 2017-02-03 21:58:26]

On 2017/01/26 17:53:48, commit-bot: I haz the power wrote:
> Committed patchset #4 (id:60001) as
>
https://chromium.googlesource.com/chromium/src/+/ce0ef3575be4a50acfc2f2682878...

Drive-by yay!