Move HistoryItem handling to DocumentLoader

Move HistoryItem handling to DocumentLoader

Currently, FrameLoader stores 2 HistoryItems: m_provisionalItem and
m_currentItem. m_currentItem corresponds to the current committed
Document/DocumentLoader, while m_provisionalItem corresponds to the
provisional DocumentLoader if and only if the provisional navigation
is back/forward. Instead, have each DocumentLoader store its
associated HistoryItem: initialized at navigation start for
back/forward navigations, and at commit time for all others.

Move a bunch of commit-time history logic that doesn't need to
interact with FrameLoader over to DocumentLoader.

BUG=

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

[Nate Chapin, 2017-02-22 22:05:17]

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

[commit-bot: I haz the power, 2017-02-22 22:06:23]

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-02-22 22:24:59]

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

[commit-bot: I haz the power, 2017-02-22 22:25:00]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Nate Chapin, 2017-02-22 22:34:52]

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

[commit-bot: I haz the power, 2017-02-22 22:36:14]

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-02-22 22:39:05]

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

[commit-bot: I haz the power, 2017-02-22 22:39:06]

Dry run: 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...)

[Nate Chapin, 2017-02-23 18:38:22]

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

[commit-bot: I haz the power, 2017-02-23 18:38:49]

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-02-23 20:01:03]

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

[commit-bot: I haz the power, 2017-02-23 20:01:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Nate Chapin, 2017-02-23 22:50:40]

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

[commit-bot: I haz the power, 2017-02-23 22:51: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-02-23 23:58:41]

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

[commit-bot: I haz the power, 2017-02-23 23:58:42]

Dry run: 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...)

[Nate Chapin, 2017-03-08 18:03:27]

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

[commit-bot: I haz the power, 2017-03-08 18:04:19]

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-03-08 18:07:20]

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

[commit-bot: I haz the power, 2017-03-08 18:07:21]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[Nate Chapin, 2017-03-08 18:14:20]

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

[commit-bot: I haz the power, 2017-03-08 18:14:56]

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-03-08 18:17:50]

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

[commit-bot: I haz the power, 2017-03-08 18:17:51]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Nate Chapin, 2017-03-08 22:59:26]

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

[commit-bot: I haz the power, 2017-03-08 23:00:00]

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-03-09 00:17:41]

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

[commit-bot: I haz the power, 2017-03-09 00:17:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2017-03-09 22:56:39]

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

[commit-bot: I haz the power, 2017-03-09 22:57:04]

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-03-09 23:39:01]

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

[commit-bot: I haz the power, 2017-03-09 23:39:02]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Nate Chapin, 2017-03-11 00:38:57]

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

[commit-bot: I haz the power, 2017-03-11 00:39:23]

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-03-11 02:10:48]

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

[commit-bot: I haz the power, 2017-03-11 02:10:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2017-03-21 23:38:24]

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

[commit-bot: I haz the power, 2017-03-21 23:38:52]

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-03-22 00:08:58]

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

[commit-bot: I haz the power, 2017-03-22 00:08:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2017-03-22 19:18:31]

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

[commit-bot: I haz the power, 2017-03-22 19:19:16]

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-03-22 19:23:08]

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

[commit-bot: I haz the power, 2017-03-22 19:23:08]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Nate Chapin, 2017-03-22 20:20:00]

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

[commit-bot: I haz the power, 2017-03-22 20:20:55]

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-03-22 20:23:45]

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

[commit-bot: I haz the power, 2017-03-22 20:23:46]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Nate Chapin, 2017-03-22 22:10:37]

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

[commit-bot: I haz the power, 2017-03-22 22:11:41]

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-03-22 22:48:25]

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

[commit-bot: I haz the power, 2017-03-22 22:48:25]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Nate Chapin, 2017-03-23 21:05:27]

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

[commit-bot: I haz the power, 2017-03-23 21:06:09]

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-03-23 22:44:49]

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

[commit-bot: I haz the power, 2017-03-23 22:44:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2017-03-23 22:47:11]

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

[commit-bot: I haz the power, 2017-03-23 22:47:47]

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-03-24 00:14:18]

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

[commit-bot: I haz the power, 2017-03-24 00:14:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2017-03-27 16:59:19]

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

[commit-bot: I haz the power, 2017-03-27 17:00:29]

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-03-27 18:05:25]

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

[commit-bot: I haz the power, 2017-03-27 18:05:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2017-03-27 19:00:28]

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

[commit-bot: I haz the power, 2017-03-27 19:01:13]

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

[Nate Chapin, 2017-03-27 20:47:10]

Description was changed from

==========
Move HistoryItem handling to DocumentLoader

BUG=
==========

to

==========
Move HistoryItem handling to DocumentLoader

Currently, FrameLoader stores 2 HistoryItems: m_provisionalItem and
m_currentItem. m_currentItem corresponds to the current committed
Document/DocumentLOader, while m_provisionalItem corresponds to the provisional
DocumentLoader if and only if the provisional navigation is back/forward.
Instead, have each DocumentLoader store its associated HistoryItem: initialized
at navigation start for back/forward navigations, and at commit time for all
others.

Move a bunch of commit-type history logic that doesn't need to interact with
FrameLoader over to DocumentLoader.

BUG=
==========

[Nate Chapin, 2017-03-27 20:47:31]

Description was changed from

==========
Move HistoryItem handling to DocumentLoader

Currently, FrameLoader stores 2 HistoryItems: m_provisionalItem and
m_currentItem. m_currentItem corresponds to the current committed
Document/DocumentLOader, while m_provisionalItem corresponds to the provisional
DocumentLoader if and only if the provisional navigation is back/forward.
Instead, have each DocumentLoader store its associated HistoryItem: initialized
at navigation start for back/forward navigations, and at commit time for all
others.

Move a bunch of commit-type history logic that doesn't need to interact with
FrameLoader over to DocumentLoader.

BUG=
==========

to

==========
Move HistoryItem handling to DocumentLoader

Currently, FrameLoader stores 2 HistoryItems: m_provisionalItem and
m_currentItem. m_currentItem corresponds to the current committed
Document/DocumentLOader, while m_provisionalItem corresponds to the
provisional DocumentLoader if and only if the provisional navigation
is back/forward. Instead, have each DocumentLoader store its
associated HistoryItem: initialized at navigation start for
back/forward navigations, and at commit time for all others.

Move a bunch of commit-type history logic that doesn't need to
interact with FrameLoader over to DocumentLoader.

BUG=
==========

[commit-bot: I haz the power, 2017-03-27 20:52:02]

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

[commit-bot: I haz the power, 2017-03-27 20:52:03]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-03-27 21:01:23]

Description was changed from

==========
Move HistoryItem handling to DocumentLoader

Currently, FrameLoader stores 2 HistoryItems: m_provisionalItem and
m_currentItem. m_currentItem corresponds to the current committed
Document/DocumentLOader, while m_provisionalItem corresponds to the
provisional DocumentLoader if and only if the provisional navigation
is back/forward. Instead, have each DocumentLoader store its
associated HistoryItem: initialized at navigation start for
back/forward navigations, and at commit time for all others.

Move a bunch of commit-type history logic that doesn't need to
interact with FrameLoader over to DocumentLoader.

BUG=
==========

to

==========
Move HistoryItem handling to DocumentLoader

Currently, FrameLoader stores 2 HistoryItems: m_provisionalItem and
m_currentItem. m_currentItem corresponds to the current committed
Document/DocumentLoader, while m_provisionalItem corresponds to the
provisional DocumentLoader if and only if the provisional navigation
is back/forward. Instead, have each DocumentLoader store its
associated HistoryItem: initialized at navigation start for
back/forward navigations, and at commit time for all others.

Move a bunch of commit-time history logic that doesn't need to
interact with FrameLoader over to DocumentLoader.

BUG=
==========

[Nate Chapin, 2017-03-27 21:01:59]

japhet@chromium.org changed reviewers:
+ mkwst@chromium.org, yhirano@chromium.org

[Nate Chapin, 2017-03-27 21:02:02]

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (left):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1636:
m_frame->loader().currentItem()->isCurrentDocument(this))
This is the sole use of isCurrentDocument(), and dates back ~10 years to a time
when there wasn't as strong of a tie between Document and
FrameLoader::m_documentLoader and FrameLoader::m_currentItems. The guarantees
that they were changed simultaneously on commit were much weaker, this isn't
necessary now.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (left):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:337: commitIfReady();
The equivalent of commitIfReady() now happens in ensureWriter().

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:324:
m_historyItem->setDocumentState(m_frame->document()->formElementsState());
This was in FrameLoader::setHistoryItemStateForCommit(), but it's only needed
for same-document navigations (for different-document navigations,
m_frame->document() is being initialized and has no content).

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:144: ResourceRequest
FrameLoader::resourceRequestFromHistoryItem(
Moved to HistoryItem, since it actually depends on HistoryItem.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:716: if (!m_currentItem)
I couldn't find any evidence this clause is needed. Everything else in this
function moved to DocumentLoader::updateForSameDocumentNavigation.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:834:
m_provisionalDocumentLoader->loadType() == FrameLoadTypeBackForward)
This clause moved to WebLocalFrameImpl::loadData(), its only user, and where it
can easily get hold of the HistoryItem needed for the back/forward navigation.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1019: if
(isBackForwardLoadType(frameLoadType)) {
Equivalent moved to startLoad(), after the provisional DocumentLoader is
created.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:688: if (opener() &&
request.resourceRequest().url().isEmpty())
Was a special case in FrameLoader::receivedFirstData() to override the history
commit type.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:726:
request.substituteData().failingURL().isEmpty()) {
Was a special case in FrameLoader::receivedFirstData() to override the history
commit type.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1089:
m_documentLoader->markAsCommitted();
(1) m_documentLoader can be nullptr here, which is bizarre.
(2) Due to ordering issues, there isn't a good time to mark the DocumentLoader
as committed from DocumentLoader. However, this sets the bit in the next best
place: exactly when the committing DocumentLoader transitions from
m_provisionalDocumentLoader to m_documentLoader.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1246: void
FrameLoader::detachProvisionalDocumentLoader(DocumentLoader* loader) {
I don't like having this, but a DocumentLoader needs a way to self-detach from
loadFailed(), and this is the only interaction it needs with FrameLoader
(besides the very general checkCompleted() call).

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1748: bool
replaceCurrentItem = loadType == FrameLoadTypeReplaceCurrentItem &&
The browser process DCHECKS when it received a blank navigation in a brand new
window.open()ed window, if the navigation is marked as replacing it's current
entry. This hack gives the browser process what it expects, but I don't
understand the relevant change (https://codereview.chromium.org/1272453002) well
enough to be sure whether that invariant can be safely reworked or removed.

[Mike West, 2017-03-28 14:36:09]

I've read through it once, and I think it looks good overall. I need to read it
again to make sure I understood the change, though... I'll try to get to it
tonight; if not, tomorrow morning.

[Nate Chapin, 2017-03-28 16:35:51]

On 2017/03/28 14:36:09, Mike West wrote:
> I've read through it once, and I think it looks good overall. I need to read
it
> again to make sure I understood the change, though... I'll try to get to it
> tonight; if not, tomorrow morning.

Sounds good, no rush. It is kind of a beast; if you want me to try to find ways
to break it up, let me know.

[Mike West, 2017-03-29 08:36:20]

I'm good with this, LGTM. Thanks for the helpful comments along the way!

[yhirano, 2017-03-29 09:38:13]

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/History.cpp (right):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/History.cpp:81:
frame()->loader().documentLoader()->historyItem()) {
Can |frame()->loader().documentLoader()| be null? Ditto below.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:505: // If a
redirection happens during a back/forward navigation, don't restore
Is this comment still valid?

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:703: DCHECK_GE(m_state,
Committed);
There are several early returns in FrameLoader::prepareForCommit before calling
markAsCommitted. Does this assertion always hold?

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:975: if
(!m_frame->loader().stateMachine()->committedMultipleRealLoads() &&
Can you tell my why you changed the calling order from receivedFirstData?

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1460: (documentLoader() &&
Can you tell me why this can be removed safely?

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/HistoryItem.cpp (right):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/HistoryItem.cpp:28: #include
"core/dom/Document.h"
-include

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/HistoryItem.cpp:172:
request.setHTTPBody(m_formData.get());
get() is not needed.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2059: historyItem =
provisionalDocumentLoader->historyItem();
Where is this line (i.e., L2059) from?

[Nate Chapin, 2017-03-29 17:55:48]

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

[commit-bot: I haz the power, 2017-03-29 17:56:38]

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

[Nate Chapin, 2017-03-29 18:23:06]

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/History.cpp (right):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/History.cpp:81:
frame()->loader().documentLoader()->historyItem()) {
On 2017/03/29 09:38:13, yhirano wrote:
> Can |frame()->loader().documentLoader()| be null? Ditto below.

I *think* it can't. FrameLoader::documentLoader() should only be null during
frame detach, and if the frame is detaching, DOMWindowClient::frame() (i.e., the
frame() here) should be null.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:505: // If a
redirection happens during a back/forward navigation, don't restore
On 2017/03/29 09:38:13, yhirano wrote:
> Is this comment still valid?

Yes, this is the same behavior as before.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:703: DCHECK_GE(m_state,
Committed);
On 2017/03/29 09:38:13, yhirano wrote:
> There are several early returns in FrameLoader::prepareForCommit before
calling
> markAsCommitted. Does this assertion always hold?

The early exits are the reason it's DCHECK_GE() instead of DCHECK_EQ(). In the
early exit cases, this DocumentLoader will get detached, at which point m_state
will be SentDidFinishLoad.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:975: if
(!m_frame->loader().stateMachine()->committedMultipleRealLoads() &&
On 2017/03/29 09:38:13, yhirano wrote:
> Can you tell my why you changed the calling order from receivedFirstData?

It was accidental, but when we update FrameLoaderStateMachine shouldn't be
ordering-sensitive. Will switch.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1460: (documentLoader() &&
On 2017/03/29 09:38:13, yhirano wrote:
> Can you tell me why this can be removed safely?

The FrameView null-check at the top of processFragment should be triggered if
documentLoader() is null, and neither of the calls above should be able to
trigger detach.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/HistoryItem.cpp (right):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/HistoryItem.cpp:28: #include
"core/dom/Document.h"
On 2017/03/29 09:38:13, yhirano wrote:
> -include

Done.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/HistoryItem.cpp:172:
request.setHTTPBody(m_formData.get());
On 2017/03/29 09:38:13, yhirano wrote:
> get() is not needed.

Done.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2059: historyItem =
provisionalDocumentLoader->historyItem();
On 2017/03/29 09:38:13, yhirano wrote:
> Where is this line (i.e., L2059) from?

Yeah, I should have mentioned this, sorry. This line is why I felt I had to move
this logic out of determineFrameLoadType.

This block handles the case where a back/forward navigation fails and we want to
replace it with an error page. Previously, the history state portion of that
logic worked magically: FrameLoader::m_provisionalItem would be set by the
back/forward navgation, and it would *not* be cleared before the error page
navigation started. So the error page would just pick up the existing
m_provisionalItem and use it when it committed.

That doesn't work with this change, because the provisional HistoryItem lives on
the provisional DocumentLoader, and the error page navigation will replace the
provsional DocumentLoader. Therefore, we need to grab the provisional
DocumentLoader's HistoryItem before we create the error page's provisional
DocumentLoader. determineFrameLoadType() could grab it, but would need a special
case to return it to the caller. Since some of the error page replacement logic
was here already, I thought it best to move it all here, especially since we're
passing a HistoryItem here anyway.

[commit-bot: I haz the power, 2017-03-29 19:55:47]

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

[commit-bot: I haz the power, 2017-03-29 19:55:49]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-03-30 05:04:58]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2017-03-30 05:05:00]

(Wanted to learn the code around back/forward a bit more so trying to read this
change but man, this is tough)

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1748: bool
replaceCurrentItem = loadType == FrameLoadTypeReplaceCurrentItem &&
On 2017/03/27 21:02:01, Nate Chapin wrote:
> The browser process DCHECKS when it received a blank navigation in a brand new
> window.open()ed window, if the navigation is marked as replacing it's current
> entry. This hack gives the browser process what it expects, but I don't
> understand the relevant change (https://codereview.chromium.org/1272453002)
well
> enough to be sure whether that invariant can be safely reworked or removed.

Could we have some of this note as a code comment?  It'd be hard to reason about
otherwise.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2059: historyItem =
provisionalDocumentLoader->historyItem();
Could we note some of this as code comment, e.g. 'this is for error page
handling and we need the history item before provisional docloader is replaced'
?

https://codereview.chromium.org/2710983003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.h (right):

https://codereview.chromium.org/2710983003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.h:152: void
markAsCommitted() {
Is the DCHECK the reason you wanted to use different naming pattern from simple
setters like below?  (nit: Could we move the code into .cpp if it's not meant to
be a simplest setter (conceptually)?)

https://codereview.chromium.org/2710983003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.h:261: enum class
HistoryNavigationType { DifferentDocument, Fragment, HistoryApi };
nit: prefixing 'k' for new enums? (other enums in this class seem to have it)

[yhirano, 2017-03-30 11:03:18]

lgtm

[Nate Chapin, 2017-03-30 20:06:26]

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

[commit-bot: I haz the power, 2017-03-30 20:07:26]

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

[Nate Chapin, 2017-03-30 20:13:56]

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1748: bool
replaceCurrentItem = loadType == FrameLoadTypeReplaceCurrentItem &&
On 2017/03/30 05:04:59, kinuko wrote:
> On 2017/03/27 21:02:01, Nate Chapin wrote:
> > The browser process DCHECKS when it received a blank navigation in a brand
new
> > window.open()ed window, if the navigation is marked as replacing it's
current
> > entry. This hack gives the browser process what it expects, but I don't
> > understand the relevant change (https://codereview.chromium.org/1272453002)
> well
> > enough to be sure whether that invariant can be safely reworked or removed.
> 
> Could we have some of this note as a code comment?  It'd be hard to reason
about
> otherwise.

Added an explanation/TODO.

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2710983003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2059: historyItem =
provisionalDocumentLoader->historyItem();
On 2017/03/30 05:05:00, kinuko wrote:
> Could we note some of this as code comment, e.g. 'this is for error page
> handling and we need the history item before provisional docloader is
replaced'
> ?

Done.

https://codereview.chromium.org/2710983003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.h (right):

https://codereview.chromium.org/2710983003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.h:152: void
markAsCommitted() {
On 2017/03/30 05:05:00, kinuko wrote:
> Is the DCHECK the reason you wanted to use different naming pattern from
simple
> setters like below?  (nit: Could we move the code into .cpp if it's not meant
to
> be a simplest setter (conceptually)?)

The DCHECK is part of the reason. The other is that "setCommitted()" just didn't
feel right to me, for reasons I can't quite justify. :) If you feel strongly,
I'll switch it.

Moved to .cpp.

https://codereview.chromium.org/2710983003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.h:261: enum class
HistoryNavigationType { DifferentDocument, Fragment, HistoryApi };
On 2017/03/30 05:05:00, kinuko wrote:
> nit: prefixing 'k' for new enums? (other enums in this class seem to have it)

Aww...that makes it have to line wrap :(

Done.

[commit-bot: I haz the power, 2017-03-30 22:05:27]

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

[commit-bot: I haz the power, 2017-03-30 22:05:28]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-03-31 00:14:55]

Thanks! lgtm
 
>
https://codereview.chromium.org/2710983003/diff/280001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/loader/DocumentLoader.h:261: enum class
> HistoryNavigationType { DifferentDocument, Fragment, HistoryApi };
> On 2017/03/30 05:05:00, kinuko wrote:
> > nit: prefixing 'k' for new enums? (other enums in this class seem to have
it)
> 
> Aww...that makes it have to line wrap :(

Oops. :(

[Nate Chapin, 2017-03-31 16:15:13]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2017-03-31 16:15:13]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2710983003/#ps300001
(title: "Address kinuko's comments")

[commit-bot: I haz the power, 2017-03-31 16:15:54]

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-03-31 16:23:49]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1490976913224760,
"parent_rev": "e7aca0670b59b9062005a9b5a96cfb2d29743682", "commit_rev":
"f3bd4b3364770b777839da74c479b0ae9a063cb3"}

[commit-bot: I haz the power, 2017-03-31 16:24:45]

Description was changed from

==========
Move HistoryItem handling to DocumentLoader

Currently, FrameLoader stores 2 HistoryItems: m_provisionalItem and
m_currentItem. m_currentItem corresponds to the current committed
Document/DocumentLoader, while m_provisionalItem corresponds to the
provisional DocumentLoader if and only if the provisional navigation
is back/forward. Instead, have each DocumentLoader store its
associated HistoryItem: initialized at navigation start for
back/forward navigations, and at commit time for all others.

Move a bunch of commit-time history logic that doesn't need to
interact with FrameLoader over to DocumentLoader.

BUG=
==========

to

==========
Move HistoryItem handling to DocumentLoader

Currently, FrameLoader stores 2 HistoryItems: m_provisionalItem and
m_currentItem. m_currentItem corresponds to the current committed
Document/DocumentLoader, while m_provisionalItem corresponds to the
provisional DocumentLoader if and only if the provisional navigation
is back/forward. Instead, have each DocumentLoader store its
associated HistoryItem: initialized at navigation start for
back/forward navigations, and at commit time for all others.

Move a bunch of commit-time history logic that doesn't need to
interact with FrameLoader over to DocumentLoader.

BUG=

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

[commit-bot: I haz the power, 2017-03-31 16:24:47]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/f3bd4b3364770b777839da74c479...