Move loadType() to DocumentLoader

Move loadType() to DocumentLoader

BUG=

Review-Url: https://codereview.chromium.org/2653673006
Cr-Commit-Position: refs/heads/master@{#446831}
Committed: https://chromium.googlesource.com/chromium/src/+/36b47e73e35bcea80ed02f6da9f8b3652545726c

[Nate Chapin, 2017-01-23 22:24:13]

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

[commit-bot: I haz the power, 2017-01-23 22:24: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-01-23 23:45:54]

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

[commit-bot: I haz the power, 2017-01-23 23:45:54]

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

[Nate Chapin, 2017-01-24 18:44:17]

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

[commit-bot: I haz the power, 2017-01-24 18:45: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-01-24 20:31:45]

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

[commit-bot: I haz the power, 2017-01-24 20:31:46]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Nate Chapin, 2017-01-24 21:02:20]

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

[commit-bot: I haz the power, 2017-01-24 21:03: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-01-24 21:41:21]

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

[commit-bot: I haz the power, 2017-01-24 21:41:22]

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-01-24 21:42:17]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2017-01-24 21:42:20]

The CQ bit was unchecked by japhet@chromium.org

[Nate Chapin, 2017-01-24 21:42:26]

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

[commit-bot: I haz the power, 2017-01-24 21:42:33]

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 23:23:26]

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

[commit-bot: I haz the power, 2017-01-24 23:23:27]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-01-25 00:11:36]

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

[Nate Chapin, 2017-01-25 00:11:37]

FrameLoadType is a concept specific to a single navigation, but it currently
lives on FrameLoader. FrameLoader::m_loadType is the FrameLoadType for the most
recent navigation to start. This could hypothetically lead to bugs, but mostly
it's just unclear which it's referring to.

This patch attempts to clarify. It leads to a bit more complicated logic in a
few places, though, so I'm not 100% sure it's worth it. What do you think?

I think it might, with some additional refactoring, lead to a simplification of
the WebDataSource public API, and maybe enable us to remove the NavgationType
enum.

[yhirano, 2017-01-25 06:30:43]

https://codereview.chromium.org/2653673006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2653673006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:750: if (m_documentLoader)
Can you tell me when |m_documentLoader| is null?

https://codereview.chromium.org/2653673006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/2653673006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:532: .documentLoader()
This function can be called from DocumentLoader::redirectReceived, which means
previous FrameLoader::loadType() equals to
provisionalDocumentLoader()->loadType(), right?

This function can be called from FrameLoader::startLoad. In such a case
consulting documentLoader() looks fine (if it exists - see my question in
FrameLoader::checkCompleted).

[Nate Chapin, 2017-01-25 17:25:29]

https://codereview.chromium.org/2653673006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2653673006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:750: if (m_documentLoader)
On 2017/01/25 06:30:43, yhirano wrote:
> Can you tell me when |m_documentLoader| is null?

m_documentLoader is null:
* In a narrow window during FrameLoader::init(), which shouldn't be reachable
here.
* During frame detach. Most (maybe all?) of the m_documentLoader null checks in
FrameLoader are for this case.

It's possible there's a better place to defend against a null m_documentLoader,
but there are definitely layout tests that crashed without this null check.

https://codereview.chromium.org/2653673006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/2653673006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:532: .documentLoader()
On 2017/01/25 06:30:43, yhirano wrote:
> This function can be called from DocumentLoader::redirectReceived, which means
> previous FrameLoader::loadType() equals to
> provisionalDocumentLoader()->loadType(), right?
> 
> This function can be called from FrameLoader::startLoad. In such a case
> consulting documentLoader() looks fine (if it exists - see my question in
> FrameLoader::checkCompleted). 

This logic is not looking at this frame's load type, but the parent's load type.

For this frame, the provisionalDocumentLoader()->loadType() would be correct.
But when checking the parent, the committed documentLoader() is always right. If
an iframe's navigation needs to check its parent's state, it always wants the
DocumentLoader of it's parent's current Document, which is the committed
DocumentLoader.

Did that make sense?

[yhirano, 2017-01-27 08:34:13]

Thanks for the explanation. LGTM. Though I'm not sure this is worth doing by
itself, as you're planning further refactoring, I'm fine with it.

https://codereview.chromium.org/2653673006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.h (right):

https://codereview.chromium.org/2653673006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.h:128: void
setReplacesCurrentHistoryItem(bool replacesCurrentHistoryItem) {
Is it possible to remove this function by computing replacesCurrentHistoryItem()
directly from loadType()? If so, maybe moving replacesCurrentHistoryItem() to
WebDataSource is good, too.

[kouhei (in TOK), 2017-01-27 17:13:53]

On 2017/01/27 08:34:13, yhirano wrote:
> Thanks for the explanation. LGTM. Though I'm not sure this is worth doing by
> itself, as you're planning further refactoring, I'm fine with it.

core/ lgtm
Moving per-nav lifetime states to DocumentLoader makes sense to me.

[Nate Chapin, 2017-01-27 17:57:56]

https://codereview.chromium.org/2653673006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.h (right):

https://codereview.chromium.org/2653673006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.h:128: void
setReplacesCurrentHistoryItem(bool replacesCurrentHistoryItem) {
On 2017/01/27 08:34:12, yhirano wrote:
> Is it possible to remove this function by computing
replacesCurrentHistoryItem()
> directly from loadType()? If so, maybe moving replacesCurrentHistoryItem() to
> WebDataSource is good, too.

Yes, removing m_replacesCurrentHistoryItem and using loadType() directly is on
my followups.

[Nate Chapin, 2017-01-27 19:29:35]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2017-01-27 19:30:09]

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-27 19:33:47]

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

[commit-bot: I haz the power, 2017-01-27 19:33:48]

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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  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-01-27 20:39:28]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2017-01-27 20:39:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
yhirano@chromium.org, kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2653673006/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-27 20:40:05]

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-27 23:41:56]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485549568918830,
"parent_rev": "ebf15845426ed332bd1e2570c5a8488ad7e075ff", "commit_rev":
"36b47e73e35bcea80ed02f6da9f8b3652545726c"}

[commit-bot: I haz the power, 2017-01-27 23:42:33]

Description was changed from

==========
Move loadType() to DocumentLoader

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

to

==========
Move loadType() to DocumentLoader

BUG=

Review-Url: https://codereview.chromium.org/2653673006
Cr-Commit-Position: refs/heads/master@{#446831}
Committed:
https://chromium.googlesource.com/chromium/src/+/36b47e73e35bcea80ed02f6da9f8...
==========

[commit-bot: I haz the power, 2017-01-27 23:42:34]

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