Reland "Move most of FrameLoader::CheckCompleted() to Document"

Reland "Move most of FrameLoader::CheckCompleted() to Document"

Split out the parts that are specific to the committed Document, and
move those to a new Document::CheckCompleted(). Most current callers of
FrameLoader::CheckCompleted() will now call Document::CheckCompleted()
instead. Rename the remainder of FrameLoader::CheckCompleted() to
DidFinishNavigation(), and have it handle the logic that is tied to
setting Frame::loading_ to false and firing DidStopLoading() callbacks.

Originally reviewed in https://codereview.chromium.org/2809733003/

BUG=

Review-Url: https://codereview.chromium.org/2837763003
Cr-Commit-Position: refs/heads/master@{#468030}
Committed: https://chromium.googlesource.com/chromium/src/+/716a136332fbd8efc12a3892b6ae2a6c0be94ba7

[Nate Chapin, 2017-04-24 22:58:38]

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

[commit-bot: I haz the power, 2017-04-24 22:59: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-04-24 23:38:30]

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

[commit-bot: I haz the power, 2017-04-24 23:38:31]

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

[Nate Chapin, 2017-04-25 21:55:04]

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

[commit-bot: I haz the power, 2017-04-25 21:55:30]

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-04-25 22:46:38]

Description was changed from

==========
Reland "Move most of FrameLoader::CheckCompleted() to Document"

This reverts commit f88c2c4ef00eb6cfc5a8c3a7cfa686ccd80afae5.

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

to

==========
Reland "Move most of FrameLoader::CheckCompleted() to Document"

Split out the the parts that are specific to the committed Document, and
move those to a new Document::CheckCompleted(). Most current callers of
FrameLoader::CheckCompleted() will now call Document::CheckCompleted()
instead. Rename the remained of FrameLoader::CheckCompleted() to
DidFinishNavigation(), and have it handling the logic that is tied to
setting Frame::loading_ to false and firing DidStopLoading() callbacks.

Originally reviewed in https://codereview.chromium.org/2809733003/

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

[Nate Chapin, 2017-04-25 22:46:58]

Description was changed from

==========
Reland "Move most of FrameLoader::CheckCompleted() to Document"

Split out the the parts that are specific to the committed Document, and
move those to a new Document::CheckCompleted(). Most current callers of
FrameLoader::CheckCompleted() will now call Document::CheckCompleted()
instead. Rename the remained of FrameLoader::CheckCompleted() to
DidFinishNavigation(), and have it handling the logic that is tied to
setting Frame::loading_ to false and firing DidStopLoading() callbacks.

Originally reviewed in https://codereview.chromium.org/2809733003/

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

to

==========
Reland "Move most of FrameLoader::CheckCompleted() to Document"

Split out the parts that are specific to the committed Document, and
move those to a new Document::CheckCompleted(). Most current callers of
FrameLoader::CheckCompleted() will now call Document::CheckCompleted()
instead. Rename the remained of FrameLoader::CheckCompleted() to
DidFinishNavigation(), and have it handling the logic that is tied to
setting Frame::loading_ to false and firing DidStopLoading() callbacks.

Originally reviewed in https://codereview.chromium.org/2809733003/

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

[Nate Chapin, 2017-04-25 22:47:30]

Description was changed from

==========
Reland "Move most of FrameLoader::CheckCompleted() to Document"

Split out the parts that are specific to the committed Document, and
move those to a new Document::CheckCompleted(). Most current callers of
FrameLoader::CheckCompleted() will now call Document::CheckCompleted()
instead. Rename the remained of FrameLoader::CheckCompleted() to
DidFinishNavigation(), and have it handling the logic that is tied to
setting Frame::loading_ to false and firing DidStopLoading() callbacks.

Originally reviewed in https://codereview.chromium.org/2809733003/

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

to

==========
Reland "Move most of FrameLoader::CheckCompleted() to Document"

Split out the parts that are specific to the committed Document, and
move those to a new Document::CheckCompleted(). Most current callers of
FrameLoader::CheckCompleted() will now call Document::CheckCompleted()
instead. Rename the remainder of FrameLoader::CheckCompleted() to
DidFinishNavigation(), and have it handle the logic that is tied to
setting Frame::loading_ to false and firing DidStopLoading() callbacks.

Originally reviewed in https://codereview.chromium.org/2809733003/

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

[Nate Chapin, 2017-04-25 23:09:23]

japhet@chromium.org changed reviewers:
+ dfalcantara@chromium.org, nasko@chromium.org, yhirano@chromium.org

[Nate Chapin, 2017-04-25 23:09:24]

Patchset 3 is the CL as it was when it was reverted, just rebased. Diff 3->5 for
the substantive changes.

nasko: Please review the changes to WebFrame::swap() for OOPIF-correctness.

yhirano: The only non-rebaseline changes are in WebFrame.cpp and
IncrementLoadEventDelayCount.h

dfalcantara: Please re-rubberstamp the
SearchGeolocationDisclosureInfoBarTest.java test fix.

[gone, 2017-04-25 23:37:19]

Re-rubberstamping lgtm.

[nasko, 2017-04-25 23:46:37]

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

[nasko, 2017-04-25 23:46:37]

lfg@ is a better reviewer for WebFrame::Swap, I defer to him.

[lfg, 2017-04-27 17:08:25]

https://codereview.chromium.org/2837763003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/2837763003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrame.cpp:48:
std::unique_ptr<IncrementLoadEventDelayCount> delay_parent_load =
This looks good, but do you mind explaining why this wasn't needed before? It
seems that we are now calling CheckCompleted where we weren't before.

Also, by the time the detach happens, we already swapped the old frame from the
FrameTree, so why is the new frame allowed to declare itself complete?

[Nate Chapin, 2017-04-27 17:20:43]

On 2017/04/27 17:08:25, lfg wrote:
>
https://codereview.chromium.org/2837763003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/WebFrame.cpp (right):
> 
>
https://codereview.chromium.org/2837763003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/WebFrame.cpp:48:
> std::unique_ptr<IncrementLoadEventDelayCount> delay_parent_load =
> This looks good, but do you mind explaining why this wasn't needed before? It
> seems that we are now calling CheckCompleted where we weren't before.

Basically, there used to be a special path for completing a detached Document
(to enable onload to fire in a detached Document, specifically, see the direct
call to ImplicitClose() from Document::close() that this patch removes). This
allowed the main load completion path (shouldComplete() in FrameLoader.cpp) to
assume that it didn't need to handle a detached Document and early-exit, so
WebFrame::swap() Just Worked. Now that this CL merges those paths, we need to
handle this case.

> 
> Also, by the time the detach happens, we already swapped the old frame from
thea;ft
> FrameTree, so why is the new frame allowed to declare itself complete?

At least in the local->remote swap case (the only one I saw failing with the
original patch), The newly swapped-in RemoteFrame's loading_ bit would be false
inside WebFrame::Swap(). It doesn't get set to true until
RenderFrameImpl::OnSwapOut() calls RenderFrameProxy::OnDidStartLoading() after
Swap() completes.

Might it be safe to move the OnDidStartLoading() call before the Swap() call? If
so, that should remove the need to use IncrementLoadEventDelayCount.

[Nate Chapin, 2017-04-27 18:25:55]

On 2017/04/27 17:20:43, Nate Chapin wrote:
> On 2017/04/27 17:08:25, lfg wrote:
> >
>
https://codereview.chromium.org/2837763003/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/web/WebFrame.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2837763003/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/web/WebFrame.cpp:48:
> > std::unique_ptr<IncrementLoadEventDelayCount> delay_parent_load =
> > This looks good, but do you mind explaining why this wasn't needed before?
It
> > seems that we are now calling CheckCompleted where we weren't before.
> 
> Basically, there used to be a special path for completing a detached Document
> (to enable onload to fire in a detached Document, specifically, see the direct
> call to ImplicitClose() from Document::close() that this patch removes). This
> allowed the main load completion path (shouldComplete() in FrameLoader.cpp) to
> assume that it didn't need to handle a detached Document and early-exit, so
> WebFrame::swap() Just Worked. Now that this CL merges those paths, we need to
> handle this case.
> 
> > 
> > Also, by the time the detach happens, we already swapped the old frame from
> thea;ft
> > FrameTree, so why is the new frame allowed to declare itself complete?
> 
> At least in the local->remote swap case (the only one I saw failing with the
> original patch), The newly swapped-in RemoteFrame's loading_ bit would be
false
> inside WebFrame::Swap(). It doesn't get set to true until
> RenderFrameImpl::OnSwapOut() calls RenderFrameProxy::OnDidStartLoading() after
> Swap() completes.
> 
> Might it be safe to move the OnDidStartLoading() call before the Swap() call?
If
> so, that should remove the need to use IncrementLoadEventDelayCount.

Ah, no, that doesn't work: The pointers in the frame tree are updated before
Detach() is called, but HTMLFrameOwnerElement::SetContentFrame() can't be called
until after detach.

[lfg, 2017-04-27 18:28:07]

On 2017/04/27 18:25:55, Nate Chapin wrote:
> Ah, no, that doesn't work: The pointers in the frame tree are updated before
> Detach() is called, but HTMLFrameOwnerElement::SetContentFrame() can't be
called
> until after detach.

Yes, the core frame doesn't exist at that point, it's created later in
WebFrame::swap().

Ok, let's just go with your current implementation. lgtm.

[Nate Chapin, 2017-04-27 18:33:15]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2017-04-27 18:34:08]

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-04-27 18:58:29]

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

[commit-bot: I haz the power, 2017-04-27 18:58:31]

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

[yhirano, 2017-04-28 10:17:24]

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

[commit-bot: I haz the power, 2017-04-28 10:17:39]

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-04-28 12:20:58]

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

[commit-bot: I haz the power, 2017-04-28 12:20:59]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-04-28 16:54:00]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2017-04-28 16:54:29]

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-04-28 17:00:14]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493398440862840,
"parent_rev": "f95d508c15009e62252c38701982042740289d84", "commit_rev":
"716a136332fbd8efc12a3892b6ae2a6c0be94ba7"}

[commit-bot: I haz the power, 2017-04-28 17:00:29]

Description was changed from

==========
Reland "Move most of FrameLoader::CheckCompleted() to Document"

Split out the parts that are specific to the committed Document, and
move those to a new Document::CheckCompleted(). Most current callers of
FrameLoader::CheckCompleted() will now call Document::CheckCompleted()
instead. Rename the remainder of FrameLoader::CheckCompleted() to
DidFinishNavigation(), and have it handle the logic that is tied to
setting Frame::loading_ to false and firing DidStopLoading() callbacks.

Originally reviewed in https://codereview.chromium.org/2809733003/

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

to

==========
Reland "Move most of FrameLoader::CheckCompleted() to Document"

Split out the parts that are specific to the committed Document, and
move those to a new Document::CheckCompleted(). Most current callers of
FrameLoader::CheckCompleted() will now call Document::CheckCompleted()
instead. Rename the remainder of FrameLoader::CheckCompleted() to
DidFinishNavigation(), and have it handle the logic that is tied to
setting Frame::loading_ to false and firing DidStopLoading() callbacks.

Originally reviewed in https://codereview.chromium.org/2809733003/

BUG=

Review-Url: https://codereview.chromium.org/2837763003
Cr-Commit-Position: refs/heads/master@{#468030}
Committed:
https://chromium.googlesource.com/chromium/src/+/716a136332fbd8efc12a3892b6ae...
==========

[commit-bot: I haz the power, 2017-04-28 17:00:30]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/716a136332fbd8efc12a3892b6ae...