[webkit-reviews] review granted: [Bug 39695] FrameLoader cleanup: Merge some bool members into a state machine : [Attachment 57052] patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Nate Chapin <japhet at chromium.org>'s
request for review:
Bug 39695: FrameLoader cleanup: Merge some bool members into a state machine
https://bugs.webkit.org/show_bug.cgi?id=39695

Attachment 57052: patch
https://bugs.webkit.org/attachment.cgi?id=57052&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
Some comments below.  Generally looks great, but there's a little more polish
we can add.

WebCore/ChangeLog:6
 +	    (m_receivedData, m_cancellingWithLoadInProcess) and merge
Ha!

WebCore/ChangeLog:9
 +	    m_committedFirstRealDocumentLoad).
Oh good.  I tried this before and failed.  Glad you figured it out.

WebCore/loader/FrameLoader.cpp:705
 +	m_currentState = isDisplayingInitialEmptyDocument() ?
DisplayingInitialEmptyDocumentPostCommit : CommittedFirstRealLoad;
Ah!  This what I screwed up before.  Nice.

WebCore/loader/FrameLoader.cpp: 
 +	m_isDisplayingInitialEmptyDocument = m_creatingInitialEmptyDocument;
I'm surprised there's no change in the state machine here.

WebCore/loader/FrameLoader.cpp:2525
 +	m_currentState = isDisplayingInitialEmptyDocument() ?
DisplayingInitialEmptyDocumentPostCommit : CommittedFirstRealLoad;
This line of code appears twice.  Is there a way to abstract it into
"advancing" the state machine somehow?

WebCore/loader/FrameLoader.cpp:2978
 +	    m_currentState = FirstLayoutDone;
This logic also appears in a couple places...  Sometimes you write "if
(committedFirstRealDocumentLoad())" and sometimes "if (m_currentState ==
CommittedFirstRealLoad)".  Are these different?  Can we try for another
"advance the state machine" kind of method?

WebCore/loader/FrameLoader.h:97
 +	enum FrameLoaderState {
Do these always advance in this order?	Can we assert that the transitions
occur correctly?

WebCore/loader/FrameLoader.h:532
 +	FrameLoaderState m_currentState;
I'd just call this "m_state".  I guess it depends on whether you plan on
folding more bools into this machine or into separate machines.