Bug 39695 – FrameLoader cleanup: Merge some bool members into a state machine

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

FrameLoader has something like 25 private boolean members.  This patch will remove a couple of them (unused), and merge several more into a single machine.

[Nate Chapin]

Created attachment 57052 [details]
patch

[Adam Barth]

Comment on attachment 57052 [details]
patch

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.

[Nate Chapin]

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

Whether right or wrong, I moved this state change to the end of FrameLoader::init(), just to try to make it a little clearer and an unconditional transition.

> 
> 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?

I can see about that.

> 
> 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?

They are logically different, I should double check whether I'm messing them up.  I can see about abstrcting also.

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

I should add some comments to clarify the order.  The summary is that things more or less advance in order until CommittedFirstRealLoad, then there's a loop.

> 
> 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.

In the category of "sad things", there's already an m_state variable on FrameLoader that handles the Policy->Provisional->Committed transitions, which seem at least semi-independent of the m_currentState transition (i.e., I don't know whether I can merge them in a sane way).  I agree that I need a better name though.

[Nate Chapin]

Hmm...actually, if I'm thinking of adding a bunch of methods to advance state, shall I just use a class instead of enum and have the comparison/advancing functions hang off of that instead of FrameLoader?

[Adam Barth]

Sure

[Nate Chapin]

Created attachment 57821 [details]
patch with state machine, FrameLoaderState

[WebKit Review Bot]

Attachment 57821 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3126005

[WebKit Review Bot]

Attachment 57821 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3068012

[Nate Chapin]

Created attachment 57891 [details]
Hopefully with all the build files this time

[WebKit Review Bot]

Attachment 57891 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3110038

[Nate Chapin]

Created attachment 57906 [details]
Fix chromium compile issue

[Eric Seidel (no email)]

Comment on attachment 57052 [details]
patch

Cleared Adam Barth's review+ from obsolete attachment 57052 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Adam Barth]

Comment on attachment 57906 [details]
Fix chromium compile issue

Just naming nits:

WebCore/loader/FrameLoader.cpp:211
 +      , m_loaderState(new FrameLoaderState())
We probably don't need this malloc here.

WebCore/loader/FrameLoaderState.h:38
 +  class FrameLoaderState : public Noncopyable {
Sorry to nit pick you on names, but how about FrameLoaderStateMachine ?

WebCore/loader/FrameLoader.h:313
 +      FrameLoaderState* loaderState() const { return m_loaderState.get(); }
Then you could name this method stateMachine().  The problem is that FrameLoaderState doesn't hold all the state of the FrameLoader.  It just holds the state machine state.

WebCore/loader/FrameLoader.h:519
 +      OwnPtr<FrameLoaderState> m_loaderState;
If you look elsewhere in this class, you'll see some mutable member variables.  That's better than holding these subobjects as pointers because we don't need to malloc them, etc.

[Nate Chapin]

Created attachment 58163 [details]
Renamed to FrameLoaderStateMachine, made a mutable member

[Adam Barth]

Comment on attachment 58163 [details]
Renamed to FrameLoaderStateMachine, made a mutable member

I'm glad to see these bool go.  Thanks for the patch.

WebCore/ChangeLog:41
 +          (WebCore::FrameLoader::loaderState):
These lines are out of date.  Ideally, we'd update them, but it's a pain.

WebCore/loader/FrameLoaderStateMachine.h:13
 +   * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
This should say "Google"

WebCore/loader/FrameLoaderStateMachine.h:49
 +      // State modifiers
I wish these were more of a flow diagram with something like an advance() method or something.

WebCore/loader/FrameLoaderStateMachine.cpp:75
 +      ASSERT(m_state == CreatingInitialEmptyDocument);
Oh, these asserts are nice.  Thanks.

[Nate Chapin]

Created attachment 58195 [details]
Now with FrameLoaderStateMachine::advanceTo()

[Early Warning System Bot]

Attachment 58195 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3215053

[Nate Chapin]

Created attachment 58197 [details]
Now with FrameLoaderStateMachine::advanceTo() AND compiling

[WebKit Review Bot]

Attachment 58195 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3223058

[Nate Chapin]

Comment on attachment 58197 [details]
Now with FrameLoaderStateMachine::advanceTo() AND compiling

This patch is causing a couple of debug crashes on layout tests on my local client.  Clearing r? until I can figure out why.

[Eric Seidel (no email)]

Comment on attachment 58163 [details]
Renamed to FrameLoaderStateMachine, made a mutable member

Cleared Adam Barth's review+ from obsolete attachment 58163 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Nate Chapin]

Created attachment 58282 [details]
No assert failures in layout tests

[Adam Barth]

Comment on attachment 58282 [details]
No assert failures in layout tests

WebCore/loader/FrameLoaderStateMachine.h:46
 +          Uninitialized = 0,
No need for = 0 here.

WebCore/loader/FrameLoaderStateMachine.cpp:69
 +      ASSERT(State(m_state + 1) == state || (firstLayoutDone() && state == CommittedFirstRealLoad));
Nice.

[WebKit Review Bot]

http://trac.webkit.org/changeset/61568 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, and Qt Linux ARMv7 Release

[Nate Chapin]

http://trac.webkit.org/changeset/61568 
http://trac.webkit.org/changeset/61569
http://trac.webkit.org/changeset/61572