Remove FrameLoader's checkLoadComplete and checkLoadCompleteForThisFrame

Remove FrameLoader's checkLoadComplete and checkLoadCompleteForThisFrame

Merge the logic that is still necessary into checkCompleted()

BUG=426520
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188339

[Nate Chapin, 2015-01-08 18:39:17]

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

[dcheng, 2015-01-09 07:43:55]

https://codereview.chromium.org/806183005/diff/160001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/806183005/diff/160001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:509: if
(m_stateMachine.committedFirstRealDocumentLoad() && m_frame->isLoading() &&
client() && allChildrenAreComplete() && !m_provisionalDocumentLoader &&
m_frame->document()->loadEventFinished()) {
For readability, it might be nice to split this to a helper function. I'm not
sure what to call it, maybe shouldReallyFinishLoad? I'm sure you can think of
something better =)

In any case, we can take advantage of that and better group the if / associated
comments.

// Initial empty document doesn't generate a start notification.
if (!m_stateMachine.committedFirstRealDocumentLoad()) return false;
// Stop notifications might have already been sent and the load is
re-completing.
// FIXME: wtf?
if (!m_frame->isLoading()) return false;

etc.

[Nate Chapin, 2015-01-09 21:42:28]

On 2015/01/09 07:43:55, dcheng wrote:
>
https://codereview.chromium.org/806183005/diff/160001/Source/core/loader/Fram...
> File Source/core/loader/FrameLoader.cpp (right):
> 
>
https://codereview.chromium.org/806183005/diff/160001/Source/core/loader/Fram...
> Source/core/loader/FrameLoader.cpp:509: if
> (m_stateMachine.committedFirstRealDocumentLoad() && m_frame->isLoading() &&
> client() && allChildrenAreComplete() && !m_provisionalDocumentLoader &&
> m_frame->document()->loadEventFinished()) {
> For readability, it might be nice to split this to a helper function. I'm not
> sure what to call it, maybe shouldReallyFinishLoad? I'm sure you can think of
> something better =)
> 
> In any case, we can take advantage of that and better group the if /
associated
> comments.
> 
> // Initial empty document doesn't generate a start notification.
> if (!m_stateMachine.committedFirstRealDocumentLoad()) return false;
> // Stop notifications might have already been sent and the load is
> re-completing.
> // FIXME: wtf?
> if (!m_frame->isLoading()) return false;
> 
> etc.

Done. I made them static file-scope functions as a matter of personal taste, I'm
willing to make them FrameLoader-private instead if you think there's a reason.

[dcheng, 2015-01-09 22:52:26]

https://codereview.chromium.org/806183005/diff/180001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/806183005/diff/180001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:447: for (Frame* child =
frame->tree().firstChild(); child; child = child->tree().traverseNext(frame)) {
I assume you intentionally changed this from nextSibling() to traverseNext()? If
so, we should update this function to be called  allDescendentsAreComplete

[Nate Chapin, 2015-01-12 18:39:30]

https://codereview.chromium.org/806183005/diff/180001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/806183005/diff/180001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:253: m_progressTracker->progressStarted();
I should probably explicitly mention what this is doing. Traditionally, an
explicit open has returned the frame to a loading state based on state it
changes in Document. We are losing our former concept of "we started loading but
we didn't tell the embedder", which explicit open used. This means that a
document.open() or document.write() in a complete frame will send start
notifications to chromium. This may lead to the spinner in the browser UI
appearing more frequently.

https://codereview.chromium.org/806183005/diff/180001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:447: for (Frame* child =
frame->tree().firstChild(); child; child = child->tree().traverseNext(frame)) {
On 2015/01/09 22:52:25, dcheng wrote:
> I assume you intentionally changed this from nextSibling() to traverseNext()?
If
> so, we should update this function to be called  allDescendentsAreComplete

Done.

[dcheng, 2015-01-13 00:20:53]

https://codereview.chromium.org/806183005/diff/180001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/806183005/diff/180001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:253: m_progressTracker->progressStarted();
On 2015/01/12 18:39:30, Nate Chapin wrote:
> I should probably explicitly mention what this is doing. Traditionally, an
> explicit open has returned the frame to a loading state based on state it
> changes in Document. We are losing our former concept of "we started loading
but
> we didn't tell the embedder", which explicit open used. This means that a
> document.open() or document.write() in a complete frame will send start
> notifications to chromium. This may lead to the spinner in the browser UI
> appearing more frequently.

Does this mean a document.write() with no document.close() will result in an
infinite spinner?

[Nate Chapin, 2015-01-13 21:13:00]

https://codereview.chromium.org/806183005/diff/180001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/806183005/diff/180001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:253: m_progressTracker->progressStarted();
On 2015/01/13 00:20:52, dcheng wrote:
> On 2015/01/12 18:39:30, Nate Chapin wrote:
> > I should probably explicitly mention what this is doing. Traditionally, an
> > explicit open has returned the frame to a loading state based on state it
> > changes in Document. We are losing our former concept of "we started loading
> but
> > we didn't tell the embedder", which explicit open used. This means that a
> > document.open() or document.write() in a complete frame will send start
> > notifications to chromium. This may lead to the spinner in the browser UI
> > appearing more frequently.
> 
> Does this mean a document.write() with no document.close() will result in an
> infinite spinner?

I just tested it, and yes :(

Updated this patch to remove this hack and add a new one. Added
Frame::isLoadingInChild() and used it in allDescendantsAreComplete(), which
allows us to smuggle back in "document.write() blocks load events but doesn't
notify the embedder".

[dcheng, 2015-01-13 22:18:55]

On 2015/01/13 at 21:13:00, japhet wrote:
> Updated this patch to remove this hack and add a new one. Added
Frame::isLoadingInChild() and used it in allDescendantsAreComplete(), which
allows us to smuggle back in "document.write() blocks load events but doesn't
notify the embedder".

Yuck.

LGTM =)

https://codereview.chromium.org/806183005/diff/240001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/806183005/diff/240001/Source/core/frame/Frame...
Source/core/frame/Frame.h:117: virtual bool isLoadingAsChild() const { return
isLoading(); }
Hmm. I think in the long-term, we should hide these as protected/private
members, and use some variation of
http://en.wikibooks.org/wiki/More_C++_Idioms/Friendship_and_the_Attorney-Client
to expose them. This is OK for now though.

[Nate Chapin, 2015-01-13 22:30:57]

On 2015/01/13 22:18:55, dcheng wrote:
> On 2015/01/13 at 21:13:00, japhet wrote:
> > Updated this patch to remove this hack and add a new one. Added
> Frame::isLoadingInChild() and used it in allDescendantsAreComplete(), which
> allows us to smuggle back in "document.write() blocks load events but doesn't
> notify the embedder".
> 
> Yuck.
> 
> LGTM =)
> 
>
https://codereview.chromium.org/806183005/diff/240001/Source/core/frame/Frame.h
> File Source/core/frame/Frame.h (right):
> 
>
https://codereview.chromium.org/806183005/diff/240001/Source/core/frame/Frame...
> Source/core/frame/Frame.h:117: virtual bool isLoadingAsChild() const { return
> isLoading(); }
> Hmm. I think in the long-term, we should hide these as protected/private
> members, and use some variation of
>
http://en.wikibooks.org/wiki/More_C++_Idioms/Friendship_and_the_Attorney-Client
> to expose them. This is OK for now though.

Ok, I'll do that in a followup.

[Nate Chapin, 2015-01-13 22:31:05]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2015-01-13 22:32:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/806183005/240001

[commit-bot: I haz the power, 2015-01-13 23:46:21]

Committed patchset #13 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188339