Bug 39695

Summary: FrameLoader cleanup: Merge some bool members into a state machine
Product: WebKit Reporter: Nate Chapin <japhet>
Component: WebCore Misc.Assignee: Nate Chapin <japhet>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, gns, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch
none
patch with state machine, FrameLoaderState
none
Hopefully with all the build files this time
none
Fix chromium compile issue
abarth: review-
Renamed to FrameLoaderStateMachine, made a mutable member
none
Now with FrameLoaderStateMachine::advanceTo()
none
Now with FrameLoaderStateMachine::advanceTo() AND compiling
none
No assert failures in layout tests abarth: review+

Description Nate Chapin 2010-05-25 15:57:39 PDT
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.
Comment 1 Nate Chapin 2010-05-25 16:05:25 PDT
Created attachment 57052 [details]
patch
Comment 2 Adam Barth 2010-05-26 10:18:51 PDT
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.
Comment 3 Nate Chapin 2010-05-26 16:19:37 PDT
> 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.
Comment 4 Nate Chapin 2010-05-26 17:02:58 PDT
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?
Comment 5 Adam Barth 2010-05-26 17:39:02 PDT
Sure
Comment 6 Nate Chapin 2010-06-03 15:25:33 PDT
Created attachment 57821 [details]
patch with state machine, FrameLoaderState
Comment 7 WebKit Review Bot 2010-06-03 19:47:59 PDT
Attachment 57821 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3126005
Comment 8 WebKit Review Bot 2010-06-03 21:39:27 PDT
Attachment 57821 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3068012
Comment 9 Nate Chapin 2010-06-04 10:07:44 PDT
Created attachment 57891 [details]
Hopefully with all the build files this time
Comment 10 WebKit Review Bot 2010-06-04 10:56:12 PDT
Attachment 57891 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3110038
Comment 11 Nate Chapin 2010-06-04 12:46:59 PDT
Created attachment 57906 [details]
Fix chromium compile issue
Comment 12 Eric Seidel (no email) 2010-06-07 09:43:22 PDT
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.
Comment 13 Adam Barth 2010-06-07 10:16:41 PDT
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.
Comment 14 Nate Chapin 2010-06-08 11:30:28 PDT
Created attachment 58163 [details]
Renamed to FrameLoaderStateMachine, made a mutable member
Comment 15 Adam Barth 2010-06-08 11:37:14 PDT
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.
Comment 16 Nate Chapin 2010-06-08 16:44:12 PDT
Created attachment 58195 [details]
Now with FrameLoaderStateMachine::advanceTo()
Comment 17 Early Warning System Bot 2010-06-08 16:53:53 PDT
Attachment 58195 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3215053
Comment 18 Nate Chapin 2010-06-08 16:54:27 PDT
Created attachment 58197 [details]
Now with FrameLoaderStateMachine::advanceTo() AND compiling
Comment 19 WebKit Review Bot 2010-06-08 17:14:09 PDT
Attachment 58195 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3223058
Comment 20 Nate Chapin 2010-06-08 17:17:31 PDT
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.
Comment 21 Eric Seidel (no email) 2010-06-09 11:16:35 PDT
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.
Comment 22 Nate Chapin 2010-06-09 12:57:36 PDT
Created attachment 58282 [details]
No assert failures in layout tests
Comment 23 Adam Barth 2010-06-18 15:58:12 PDT
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.
Comment 24 WebKit Review Bot 2010-06-21 13:16:56 PDT
http://trac.webkit.org/changeset/61568 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, and Qt Linux ARMv7 Release