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.
Created attachment 57052 [details] patch
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.
> 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.
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?
Sure
Created attachment 57821 [details] patch with state machine, FrameLoaderState
Attachment 57821 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3126005
Attachment 57821 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3068012
Created attachment 57891 [details] Hopefully with all the build files this time
Attachment 57891 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3110038
Created attachment 57906 [details] Fix chromium compile issue
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 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.
Created attachment 58163 [details] Renamed to FrameLoaderStateMachine, made a mutable member
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.
Created attachment 58195 [details] Now with FrameLoaderStateMachine::advanceTo()
Attachment 58195 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3215053
Created attachment 58197 [details] Now with FrameLoaderStateMachine::advanceTo() AND compiling
Attachment 58195 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3223058
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 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.
Created attachment 58282 [details] No assert failures in layout tests
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.
http://trac.webkit.org/changeset/61568 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, and Qt Linux ARMv7 Release
http://trac.webkit.org/changeset/61568 http://trac.webkit.org/changeset/61569 http://trac.webkit.org/changeset/61572