Simplify FrameLoader::Clear(), remove FrameLoader::check_timer_

Simplify FrameLoader::Clear(), remove FrameLoader::check_timer_

Clear() is full of strange, old logic. Several clauses are outright unnecessary, or have better-encapsulated homes.

check_timer_ appears to have two uses, one of which is vestigial, the other of which can be better-situated to not require a timer.

BUG=

Review-Url: https://codereview.chromium.org/2854853006
Cr-Commit-Position: refs/heads/master@{#469774}
Committed: https://chromium.googlesource.com/chromium/src/+/2081f48578f7d6ff49d468a76e471f2b90f78178

[Nate Chapin, 2017-05-03 23:19:17]

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

[commit-bot: I haz the power, 2017-05-03 23:20:12]

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-05-04 00:54:44]

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

[commit-bot: I haz the power, 2017-05-04 00:54:49]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-05-04 18:07:41]

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

[commit-bot: I haz the power, 2017-05-04 18:08:11]

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-05-04 19:51:19]

Description was changed from

==========
Drop ScheduleCheckCompleted

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

to

==========
Simplify FrameLoader::Clear(), remove FrameLoader::check_timer_

Clear() is full of strange, old logic. Several clauses are outright unnecessary,
or have better-encapsulated homes.

check_timer_ apperas to have two uses, one of which is vestigial, the other of
which can be better-situated to not require a timer.

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

[Nate Chapin, 2017-05-04 19:51:29]

Description was changed from

==========
Simplify FrameLoader::Clear(), remove FrameLoader::check_timer_

Clear() is full of strange, old logic. Several clauses are outright unnecessary,
or have better-encapsulated homes.

check_timer_ apperas to have two uses, one of which is vestigial, the other of
which can be better-situated to not require a timer.

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

to

==========
Simplify FrameLoader::Clear(), remove FrameLoader::check_timer_

Clear() is full of strange, old logic. Several clauses are outright unnecessary,
or have better-encapsulated homes.

check_timer_ appears to have two uses, one of which is vestigial, the other of
which can be better-situated to not require a timer.

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

[Nate Chapin, 2017-05-04 19:53:11]

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

[Nate Chapin, 2017-05-04 19:53:12]

dcheng, what do you think of this?

https://codereview.chromium.org/2854853006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2854853006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:308:
ScheduleCheckCompleted();
This dates back to
https://chromium.googlesource.com/chromium/src/+/8ec6d2367142e649bb9ef374c259...,
which claims it "might have been fired and ignored while defersLoading was
true." I'm pretty sure that's not true anymore.

Also, I can't think of a situation in which we would meet the last requirement
to complete during deferral. All the things that block completion should be
stopped during deferral.

https://codereview.chromium.org/2854853006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:377:
frame_->GetDocument()->RemoveFocusedElementOfSubtree(frame_->GetDocument());
Documnet::Shutdown() clears the focused element unconditionally, I don't think
this is necessary.

https://codereview.chromium.org/2854853006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:382:
frame_->GetScriptController().EnableEval();
Moved to ScriptController::UpdateDocument, which should be called shortly after
Clear() (except in the frame-detach case, in which case it's irrelevant).

https://codereview.chromium.org/2854853006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:384:
frame_->GetNavigationScheduler().Cancel();
In the frame-detach case, StopAllLoaders() will cover this. In the
standard-commit case, CommitProvisionalLoad() cover this. In the javascript: url
case, we check that a location change isn't pending before replacing the
document. Ergo, I think this is vestigial.

https://codereview.chromium.org/2854853006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1214:
ToLocalFrame(parent)->Loader().ScheduleCheckCompleted();
Instead of scheduling a CheckCompleted() here, have HTMLFrameOwnerElement
directly call CheckCompleted() when its ContentFrame() is cleared, as that is
one of the very last steps of frame detach.

[commit-bot: I haz the power, 2017-05-04 20:16:01]

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

[commit-bot: I haz the power, 2017-05-04 20:16:02]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-05-05 06:12:31]

https://codereview.chromium.org/2854853006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2854853006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1214:
ToLocalFrame(parent)->Loader().ScheduleCheckCompleted();
On 2017/05/04 19:53:12, Nate Chapin wrote:
> Instead of scheduling a CheckCompleted() here, have HTMLFrameOwnerElement
> directly call CheckCompleted() when its ContentFrame() is cleared, as that is
> one of the very last steps of frame detach.

It's unclear to me if there are edge cases that this might miss. For example, if
we have main frame A - child B - grandchild A, then detaching grandchild A
wouldn't call CheckCompleted(), because grandchild A's owner is not a
HTMLFrameOwnerElement.

I guess maybe this is already an existing problem though?

[Nate Chapin, 2017-05-05 16:49:14]

On 2017/05/05 06:12:31, dcheng wrote:
>
https://codereview.chromium.org/2854853006/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):
> 
>
https://codereview.chromium.org/2854853006/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/FrameLoader.cpp:1214:
> ToLocalFrame(parent)->Loader().ScheduleCheckCompleted();
> On 2017/05/04 19:53:12, Nate Chapin wrote:
> > Instead of scheduling a CheckCompleted() here, have HTMLFrameOwnerElement
> > directly call CheckCompleted() when its ContentFrame() is cleared, as that
is
> > one of the very last steps of frame detach.
> 
> It's unclear to me if there are edge cases that this might miss. For example,
if
> we have main frame A - child B - grandchild A, then detaching grandchild A
> wouldn't call CheckCompleted(), because grandchild A's owner is not a
> HTMLFrameOwnerElement.
> 
> I guess maybe this is already an existing problem though?

Yeah, that would be an existing problem.

Though I guess there is a slight behavior difference: in the case of main frame
foo.com/a - child foo.com/b - grandchild foo.com/c, if b gets detached, we
detach c then b, and b's CheckCompleted() can get called as a result of c's
detach, where before that would have happened on a timer. I think that's benign,
but it might be "extra work".

[dcheng, 2017-05-05 18:20:34]

LGTM

We might want to fix the completion check logic in a followup though.

[Nate Chapin, 2017-05-05 18:43:24]

On 2017/05/05 18:20:34, dcheng wrote:
> LGTM
> 
> We might want to fix the completion check logic in a followup though.

[Nate Chapin, 2017-05-05 18:44:10]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2017-05-05 18:44:57]

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-05-05 22:09:14]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494009850576840,
"parent_rev": "df1ffb753c87b529bca2cc1f42dbd42977955e8f", "commit_rev":
"2081f48578f7d6ff49d468a76e471f2b90f78178"}

[commit-bot: I haz the power, 2017-05-05 22:09:26]

Description was changed from

==========
Simplify FrameLoader::Clear(), remove FrameLoader::check_timer_

Clear() is full of strange, old logic. Several clauses are outright unnecessary,
or have better-encapsulated homes.

check_timer_ appears to have two uses, one of which is vestigial, the other of
which can be better-situated to not require a timer.

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

to

==========
Simplify FrameLoader::Clear(), remove FrameLoader::check_timer_

Clear() is full of strange, old logic. Several clauses are outright unnecessary,
or have better-encapsulated homes.

check_timer_ appears to have two uses, one of which is vestigial, the other of
which can be better-situated to not require a timer.

BUG=

Review-Url: https://codereview.chromium.org/2854853006
Cr-Commit-Position: refs/heads/master@{#469774}
Committed:
https://chromium.googlesource.com/chromium/src/+/2081f48578f7d6ff49d468a76e47...
==========

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

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/2081f48578f7d6ff49d468a76e47...

[Nate Chapin, 2017-05-08 18:05:59]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2871703002/ by japhet@chromium.org.

The reason for reverting is: Moving CheckCompleted() call to
HTMLFrameOwnerElement::ClearContentFrame() appears to be causing script to run
in a ScriptForbiddenScope. See crbug.com/719397.