[import] Cleanup: get rid of ad-hoc state machine.

[import] Get rid of ad-hoc state machine.

This change removes its error-prone state transition logic from HTMLImport.
Instead, it introduces invalidate-and-recalc state model like style calculation.

In this model,

 * Possible state change is notified by HTMLImport::stateWillChange(),
   which schedules tree-wide state recalculation, that is done by
   HTMLImport::recalcTreeState() static member function.

 * HTMLImport::recalcTreeState() traverses the import tree
   and recomputes the state of each import through recalcState().

 * If the state of an import has changed, it gets stateDidChange() notification.
   This single notification replaces old didXxx() notification functions.

 * Per-import state calculation is extracted as HTMLImportStateResolver class.
   This class does compute the state based on the loader status and
   the state of other imports whose states should be already updated.

Although this doesn't stop maintaining the explicit state for
performance reasons, this recalc model is much more easy to reason
thus less error prone than original state machine model.

As a bonus, crbug.com/336646 is gone.
A crash reported there happened due to incorrect m_state value
and this change fixes it by giving correct state value.

On import-events-inline.html: The change happened to reveal a timing problem (crbug.com/336113).
It should be fixed in coming changes

TEST=import-events-inline.html, import-shared-crash.html
R=dglazkov, dominicc
BUG=336646
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165624

[Hajime Morrita, 2014-01-17 12:08:17]

LGTM?

I'm sorry for that this is rather a big rewrite than 
incremental change. It's difficult to do this in that way.

All tests passed.

[Hajime Morrita, 2014-01-22 05:12:32]

@dominicc PTAL?

[dglazkov, 2014-01-22 17:45:54]

lgtm

https://codereview.chromium.org/141143006/diff/110001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImport.h (right):

https://codereview.chromium.org/141143006/diff/110001/Source/core/html/HTMLIm...
Source/core/html/HTMLImport.h:113: bool isStateBlockedFromRunningScript() const
{ return state() <= BlockedFromRunningScript; }
Why not just query state() directly at the callsites? Why add these helpers? I
was confused why "State" is in the name of the function at the callsite.

https://codereview.chromium.org/141143006/diff/110001/Source/core/html/HTMLIm...
Source/core/html/HTMLImport.h:168: return m_cachedState;
Why is this called cachedState and not just state?

Oh, I see. The value could be invalid and needed resolution. Hmm.. Can we maybe
expose a state machine, rather than state? This way, HTMLImport::state could
return a machine instance and all the helpers would live on it? Just thinking
outloud.

https://codereview.chromium.org/141143006/diff/110001/Source/core/html/HTMLIm...
Source/core/html/HTMLImport.h:177: virtual void scheduleRecalcState() = 0;
You have a dichotomy here between "recalculate" and "resolve" later in the name
of the actual class that does the work.

[commit-bot: I haz the power, 2014-01-22 20:31:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/141143006/110001

[commit-bot: I haz the power, 2014-01-22 22:40:56]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2014-01-23 00:54:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/141143006/420001

[commit-bot: I haz the power, 2014-01-23 03:34:27]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2014-01-23 04:55:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/141143006/150002

[commit-bot: I haz the power, 2014-01-23 04:55:40]

Failed to apply patch for Source/core/html/HTMLImport.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/html/HTMLImport.h
  Hunk #2 FAILED at 109.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/core/html/HTMLImport.h.rej

Patch:       Source/core/html/HTMLImport.h
Index: Source/core/html/HTMLImport.h
diff --git a/Source/core/html/HTMLImport.h b/Source/core/html/HTMLImport.h
index
9ecc0254a14eabe3860251270d4f3dba7fd10807..07dfa46e60e7bde8d364a688d23ce7340629fefa
100644
--- a/Source/core/html/HTMLImport.h
+++ b/Source/core/html/HTMLImport.h
@@ -87,50 +87,19 @@ class KURL;
 //
 // # Script Blocking
 //
-// The HTML parser blocks on <script> when preceding <link>s aren't finish
loading imports.
-// Each HTMLImport instance tracks such a blocking state, that is called
"script-blocked"
-// or HTMLImport::isBlockedFromRunningScript().
+// - An import blocks the HTML parser of its own imported document from running
<script>
+//   until all of its children are loaded.
+//   Note that dynamically added import won't block the parser.
 //
-// ## Blocking Imports
-//
-// Each imports can become being script-blocked when new imports are added to
the import tree.
-// For example, the parser of a parent import is blocked when new child import
is given.
-// See HTMLImport::appendChild() to see how it is handled. Note that this
blocking-ness is
-// transitive. HTMLImport::blockPredecessorsOf() flips the flags iteratively to
fullfill this transitivity.
-//
-// ## Unblocking Imports
-//
-// The blocking state can change when one of the imports finish loading. The
Document notices it through
-// HTMLImportRoot::blockerGone(). The blockerGone() triggers
HTMLImport::unblockFromRunningScript(), which traverses
-// whole import tree and find unblock-able imports and unblock them.
-// Ready imported documents are notified through Document::didLoadAllImports()
so that
-// it can resume its parser.
-//
-// # Document Blocking
-//
-// There is another type of blocking state that is called
-// "document-blocked". If the import is document-blocked, it cannot
-// create its own document object because sharable imported document
-// can appear later. The spec defines the loading order of the
-// import: The earlier one in the import-tree order should win and
-// later ones should share earlier one.
-//
-// The "document-blocked" state keeps the tree node from loading its
-// import until all preceding imports are ready t respect the
-// order. Note that the node may fetch the bytes from the URL
-// speculatively, even though it doesn't process it.
-//
-// The node is "document-unblocked" when there are unfinished,
-// preceeding import loading. Unblocking attempt for
-// "document-blocked" happens at the same timing as unblocking
-// "script-blocked".
+// - An import under loading also blocks imported documents that follow from
being created.
+//   This is because an import can include another import that has same URLs of
following ones.
+//   In such case, the preceding import should be loaded and following ones
should be de-duped.
 //
 
 // The superclass of HTMLImportsController and HTMLImportChild
 // This represents the import tree data structure.
 class HTMLImport : public TreeNode<HTMLImport> {
 public:
-    static bool unblock(HTMLImport*);
     static bool isMaster(Document*);
 
     virtual ~HTMLImport() { }
@@ -140,65 +109,72 @@ public:
     HTMLImportsController* controller();
 
     bool isCreatedByParser() const { return m_createdByParser; }
-    bool isBlockedFromRunningScript() const { return m_state <=
BlockedFromRunningScript; }
-    bool isBlocked() const { return m_state < Ready; }
 
-    bool isBlockedFromCreatingDocument() const;
+    bool isStateBlockedFromRunningScript() const { return state() <=
BlockedFromRunningScript; }
+    bool isStateBlockedFromCreatingDocument() const { return state() <=
BlockedFromCreatingDocument; }
+    bool isStateReady() const { return state() == Ready; }
 
     void appendChild(HTMLImport*);
 
     virtual HTMLImportRoot* root() = 0;
     virtual Document* document() const = 0;
     virtual void wasDetachedFromDocument() = 0;
-    virtual void didFinishParsing() = 0;
+    virtual void didFinishParsing() { };
     virtual bool isDone() const = 0; // FIXME: Should be renamed to
haveFinishedLoading()
     virtual bool hasLoader() const = 0;
     virtual bool ownsLoader() const { return false; }
     virtual CustomElementMicrotaskImportStep* customElementMicrotaskStep()
const { return 0; }
+    virtual void stateDidChange();
 
-protected:
     enum State {
+        BlockedFromCreatingDocument = 0,
         BlockedFromRunningScript,
-        WaitingLoaderOrChildren,
-        Ready
+        Active,
+        Ready,
+        Invalid
     };
 
-    explicit HTMLImport(State state, bool createdByParser = false)
-        : m_state(state)
+protected:
+    // Stating from most conservative state.
+    // It will be corrected through state update flow.
+    explicit HTMLImport(bool createdByParser = false)
+        : m_cachedState(BlockedFromCreatingDocument)
         , m_createdByParser(createdByParser)
     { }
 
-    virtual void didUnblockFromCreatingDocument();
-    virtual void didUnblockFromRunningScript();
-    virtual void didBecomeReady();
+    void stateWillChange();
+    State state() const;
+    static void recalcTreeState(HTMLImport* root);
 
-    void loaderWasResolved();
-    void loaderDidFinish();
+#if !defined(NDEBUG)
+    void show();
+    void showTree(HTMLImport* highlight, unsigned depth);
+    virtual void showThis();
+#endif
 
 private:
-    static void block(HTMLImport*);
-
-    void blockPredecessorsOf(HTMLImport* child);
-    void blockFromRunningScript();
-    void waitLoaderOrChildren();
-    void unblockFromRunningScript();
-    void becomeReady();
-
-    bool isBlockedFromRunningScriptByPredecessors() const;
-    bool isBlockingFollowersFromRunningScript() const;
-    bool isBlockingFollowersFromCreatingDocument() const;
-
-    State m_state;
+    void recalcState();
+    void forceBlock();
+    void invalidateCachedState() { m_cachedState = Invalid; }
+    bool isStateCacheValid() const { return m_cachedState != Invalid; }
 
+    State m_cachedState;
     bool m_createdByParser : 1;
 };
 
+inline HTMLImport::State HTMLImport::state() const
+{
+    ASSERT(isStateCacheValid());
+    return m_cachedState;
+}
+
+
 // An abstract class to decouple its sublcass HTMLImportsController.
 class HTMLImportRoot : public HTMLImport {
 public:
-    HTMLImportRoot() : HTMLImport(Ready) { }
+    HTMLImportRoot() { }
 
-    virtual void blockerGone() = 0;
+    virtual void scheduleRecalcState() = 0;
     virtual HTMLImportsController* toController() = 0;
     virtual HTMLImportChild* findLinkFor(const KURL&, HTMLImport* excluding =
0) const = 0;
 };

[commit-bot: I haz the power, 2014-01-23 06:19:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/141143006/560001

[commit-bot: I haz the power, 2014-01-23 09:47:48]

Change committed as 165624