Navigation transitions: Added notifyTransitionsShown and setupTransitionsView to WebLocalFrame

Source/web/WebLocalFrameImpl.cpp

Index: Source/web/WebLocalFrameImpl.cpp
diff --git a/Source/web/WebLocalFrameImpl.cpp b/Source/web/WebLocalFrameImpl.cpp
index 0f7d09def92a947eb0c685d170f8c73e68a76815..0fb4249dace951bbd3a2d50e7fc359a7a1caa4e0 100644
--- a/Source/web/WebLocalFrameImpl.cpp
+++ b/Source/web/WebLocalFrameImpl.cpp
@@ -1295,6 +1295,58 @@ void WebLocalFrameImpl::addStyleSheetByURL(const WebString& url)
     frame()->document()->head()->appendChild(styleElement.release(), IGNORE_EXCEPTION);
 }
 
+void WebLocalFrameImpl::notifyTransitionShown(int elementsToHide)

[abarth-chromium, 2014/06/13 17:29:01]

This is way too much logic for WebLocalFrameImpl.  Please move the logic that
interacts with core objects into core.

+{
+    RefPtr<HTMLCollection> metaElements = frame()->document()->getElementsByTagName(HTMLNames::metaTag.localName());

[esprehn, 2014/06/13 08:55:20]

We don't use node lists of querySelector inside Source/ like this. For example
your code below makes us walk the entire document twice looking for <meta>
elements. You need to keep track the meta elements for transitions in a Vector
in document using ::insertedInto and attributeChanged notifications in
HTMLMetaElement.

[oystein (OOO til 10th of July), 2014/06/18 23:37:41]

Somewhat improved now. Is it worth using ::insertedInto and attributeChanged to
keep track a runtime list of the meta elements though, when we at most do this
twice per document (the ones which have navigation transitions defined?

+    if (elementsToHide >= 0) {
+        int currentValidTransitionElementsTag = 0;
+        unsigned length = metaElements->length();

[esprehn, 2014/06/13 08:55:20]

Touching length() walks the entire document, then you walk it again below.

[oystein (OOO til 10th of July), 2014/06/18 23:37:42]

Done.

+        for (unsigned i = 0; i < length; ++i) {
+            ASSERT(metaElements->item(i)->isHTMLElement());
+            HTMLMetaElement* metaElement = toHTMLMetaElement(metaElements->item(i));
+            if (metaElement->name() != "transition-elements")
+                continue;
+
+            Vector<String> tokens;
+            String(metaElement->content().impl()).split(';', tokens);

[esprehn, 2014/06/13 08:55:20]

metaElement->content().string().split(...)

[oystein (OOO til 10th of July), 2014/06/18 23:37:42]

Done.

+            if (tokens.size() != 2)
+                continue;
+
+            TrackExceptionState exceptionState;
+            RefPtr<NodeList> nodeList = frame()->document()->querySelectorAll(AtomicString(tokens[0]), exceptionState);
+            if (nodeList && !exceptionState.hadException()) {
+                unsigned nodeListLength = nodeList->length();
+                if (nodeListLength && elementsToHide != currentValidTransitionElementsTag++)
+                    continue;
+
+                for (unsigned nodeIdx = 0; nodeIdx < nodeListLength; ++nodeIdx) {
+                    Node* node = nodeList->item(nodeIdx);
+                    toElement(node)->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone);

[esprehn, 2014/06/13 08:55:19]

Modifying parts of the page like this doesn't seem right.

[oystein (OOO til 10th of July), 2014/06/18 23:37:41]

Is there a better way of hiding these elements?

+                }
+            }
+        }
+    }
+
+    frame()->loader().closeURL();
+    frame()->document()->setIsTransitionDocument();
+    frame()->document()->styleEngine()->enableExitTransitionStylesheets();

[esprehn, 2014/06/13 08:55:19]

You can't do this, closeURL() shuts the FrameLoader down and dispatches unload
events. I don't think you can then make us do recalcStyle and such later on the
same document.

[oystein (OOO til 10th of July), 2014/06/18 23:37:41]

I believe dispatching the unload events first was actually the idea here, so
that we wouldn't disrupt the normal page flow when we hide the transition
elements and apply any exit animations (from any transition-exiting
stylesheets). That still works, so I assume recalcStyle still works :).

+}
+
+void WebLocalFrameImpl::setupTransitionView(const WebString& markup)
+{
+    RefPtr<Document> ownerDocument(frame()->document());
+
+    // Need this so that the transition UA stylesheet gets applied.
+    frame()->document()->setIsTransitionDocument();
+    frame()->document()->enforceSandboxFlags(SandboxAll);

[abarth-chromium, 2014/06/13 17:29:01]

Can you assert that document()->securityOrigin()->isUnique() here?  I'm not sure
you're successfully gotten a unique security origin.

[oystein (OOO til 10th of July), 2014/06/18 23:37:41]

Done, though I'm not quite sure what I'm doing here. The page URL is basically
just about:blank, should we be using some other URL here with the domain we're
navigating to?

+    frame()->page()->settings().setScriptEnabled(false);

[abarth-chromium, 2014/06/13 17:29:01]

This line is redundant with enforceSandboxFlags(SandboxAll);

[oystein (OOO til 10th of July), 2014/06/18 23:37:41]

Done.

+
+    // FIXME(oysteine): Is there a better way to set this? Or should this HTML be an embedded resource?
+    // It could alternatively be generated in markup.cpp and included in |markup|.
+    String tempMarkup = String("<!DOCTYPE html><meta name=\"viewport\" content=\"width=device-width, user-scalable=0\">") + String(markup);

[esprehn, 2014/06/13 08:55:20]

You should talk to abarth@ about this.

[abarth-chromium, 2014/06/13 17:29:01]

This should be included in |markup|.  I'm not sure where |markup| is generated,
but whoever generates it should generate the complete string, including the
doctype and the viewport.

[oystein (OOO til 10th of July), 2014/06/18 23:37:42]

Done.

+    frame()->document()->loader()->replaceDocument(tempMarkup, ownerDocument.get());
+}

[abarth-chromium, 2014/06/13 17:29:01]

All this logic should be in core also.

+
 void WebLocalFrameImpl::setCaretVisible(bool visible)
 {
     frame()->selection().setCaretVisible(visible);