LayoutTests:
authorggaren <ggaren@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Oct 2006 20:22:08 +0000 (20:22 +0000)
committerggaren <ggaren@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Oct 2006 20:22:08 +0000 (20:22 +0000)
        Tests for accessing renderer-dependent properties from a javascript: URL.

        * fast/frames/frame-js-url-clientWidth-expected.txt: Added.
        * fast/frames/frame-js-url-clientWidth.html: Added.
        * fast/frames/iframe-js-url-clientWidth-expected.txt: Added.
        * fast/frames/iframe-js-url-clientWidth.html: Added.

WebCore:

        Reviewed by Darin, John.

        - Merged more frame and iframe code
        - Fixed a bug where iframes returned incorrect values for renderer-dependent
        properties during javascript: loads because they didn't have renderers at
        load time

        PLT insists this is a small performance win. Don't believe its lies.

        * bindings/js/kjs_html.cpp:
        (KJS::JSHTMLElement::frameGetter): Renamed frameWidth and frameHeight to
        width and height, for consistency with the rest of the DOM.
        * dom/ContainerNode.cpp:
        (WebCore::ContainerNode::queuePostAttachCallback): Added a post-attach callback
        queue that gets drained after the render tree is fully constructed. Currently,
        this is only used for frame loading.
        (WebCore::ContainerNode::attach):
        * dom/ContainerNode.h:
        * html/HTMLFrameElement.cpp:
        (WebCore::HTMLFrameElement::insertedIntoDocument): Removed frame loading from
        attach() because loading iframes during attach() causes a crash.
        Moved frame loading logic into insertedIntoDocument(). That's a more
        logical place for it because  document insertion is what
        triggers frame loading. Made frame loading a post-attach callback, instead
        of an immediate action, to fix the incorrect values bug.
        (WebCore::HTMLFrameElement::attach): Added setWidget call that used to be
        in HTMLIFrameElement::attach. IFRAME requires this call. FRAME will soon
        require this call, once I remove the call from WebKit.
        (WebCore::HTMLFrameElement::setLocation):
        (WebCore::HTMLFrameElement::width):
        (WebCore::HTMLFrameElement::height):
        * html/HTMLFrameElement.h: Made openURL non-virtual to avoid the unnecessary
        killing of puppies.
        * html/HTMLIFrameElement.cpp: Merged code into HTMLFrameElement
        (WebCore::HTMLIFrameElement::rendererIsNeeded):
        (WebCore::HTMLIFrameElement::createRenderer):
        (WebCore::HTMLIFrameElement::insertedIntoDocument):
        (WebCore::HTMLIFrameElement::removedFromDocument):
        (WebCore::HTMLIFrameElement::attach):
        * page/FrameView.h: Removed unused method.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@17040 268f45cc-cd09-0410-ab3c-d52691b4dbfc

14 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/frames/frame-js-url-clientWidth-expected.txt [new file with mode: 0644]
LayoutTests/fast/frames/frame-js-url-clientWidth.html [new file with mode: 0644]
LayoutTests/fast/frames/iframe-js-url-clientWidth-expected.txt [new file with mode: 0644]
LayoutTests/fast/frames/iframe-js-url-clientWidth.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/bindings/js/kjs_html.cpp
WebCore/dom/ContainerNode.cpp
WebCore/dom/ContainerNode.h
WebCore/html/HTMLFrameElement.cpp
WebCore/html/HTMLFrameElement.h
WebCore/html/HTMLFrameSetElement.cpp
WebCore/html/HTMLIFrameElement.cpp
WebCore/page/FrameView.h

index f291e89..6057c38 100644 (file)
@@ -1,3 +1,12 @@
+2006-10-13  Geoffrey Garen  <ggaren@apple.com>
+
+        Tests for accessing renderer-dependent properties from a javascript: URL.
+        
+        * fast/frames/frame-js-url-clientWidth-expected.txt: Added.
+        * fast/frames/frame-js-url-clientWidth.html: Added.
+        * fast/frames/iframe-js-url-clientWidth-expected.txt: Added.
+        * fast/frames/iframe-js-url-clientWidth.html: Added.
+
 2006-10-13  David Harrison  <harrison@apple.com>
 
         Reviewed by Justin.
diff --git a/LayoutTests/fast/frames/frame-js-url-clientWidth-expected.txt b/LayoutTests/fast/frames/frame-js-url-clientWidth-expected.txt
new file mode 100644 (file)
index 0000000..03bee60
--- /dev/null
@@ -0,0 +1,11 @@
+ALERT: This page tests whether an iframe correctly reports renderer-dependent values when executing a javascript: load. If the test passes, you'll see a series of 'PASS' messages below.
+
+ALERT: PASS: frame.frameElement.width should be 250 and is.
+
+ALERT: PASS: frame.frameElement.clientWidth should be 250 and is.
+
+ALERT: PASS: frame.frameElement.height should be 250 and is.
+
+ALERT: PASS: frame.frameElement.clientHeight should be 250 and is.
+
+
diff --git a/LayoutTests/fast/frames/frame-js-url-clientWidth.html b/LayoutTests/fast/frames/frame-js-url-clientWidth.html
new file mode 100644 (file)
index 0000000..a4fd365
--- /dev/null
@@ -0,0 +1,45 @@
+<script>
+if (window.layoutTestController)
+    layoutTestController.dumpAsText();
+    
+function log(s)
+{
+    if (window.layoutTestController)
+        alert(s);
+    else
+        logger.document.write('<p>' + s + '</p>');
+}
+
+function shouldBe(a, b)
+{
+    var evalA;
+    try {
+        evalA = eval(a)
+    } catch (e) {
+        evalA = 'Caught exception: ' + e;
+    }
+    
+    if (evalA == b)
+        log('PASS: ' + a + ' should be ' + b + ' and is.\n');
+    else
+        log('FAIL: ' + a + ' should be ' + b + ' but instead is ' + evalA + '.\n');
+}
+</script>
+<frameset rows="250, *">
+<frameset cols="*, 250">
+<frame name="logger">
+<frame
+    name="frame"
+    src="javascript:
+        top.log('This page tests whether an iframe correctly reports renderer-dependent values ' +
+                'when executing a javascript: load. If the test passes, you\'ll see a series of ' +
+                '\'PASS\' messages below.\n'
+                );
+        top.shouldBe('frame.frameElement.width', 250);
+        top.shouldBe('frame.frameElement.clientWidth', 250);
+        top.shouldBe('frame.frameElement.height', 250);
+        top.shouldBe('frame.frameElement.clientHeight', 250);
+    "
+>
+</frameset>
+</frameset>
diff --git a/LayoutTests/fast/frames/iframe-js-url-clientWidth-expected.txt b/LayoutTests/fast/frames/iframe-js-url-clientWidth-expected.txt
new file mode 100644 (file)
index 0000000..4dff998
--- /dev/null
@@ -0,0 +1,5 @@
+This page tests whether an iframe correctly reports renderer-dependent values when executing a javascript: load. If the test passes, you'll see a series of 'PASS' messages below.
+
+PASS: iframe.frameElement.clientWidth should be 250 and is.
+PASS: iframe.frameElement.clientHeight should be 250 and is.
+
diff --git a/LayoutTests/fast/frames/iframe-js-url-clientWidth.html b/LayoutTests/fast/frames/iframe-js-url-clientWidth.html
new file mode 100644 (file)
index 0000000..751fd2d
--- /dev/null
@@ -0,0 +1,39 @@
+<script>
+if (window.layoutTestController)
+    layoutTestController.dumpAsText();
+    
+function log(s)
+{
+    document.getElementById('console').appendChild(document.createTextNode(s));
+}
+
+function shouldBe(a, b)
+{
+    var evalA;
+    try {
+        evalA = eval(a)
+    } catch (e) {
+        evalA = 'Caught exception: ' + e;
+    }
+    
+    if (evalA == b)
+        log('PASS: ' + a + ' should be ' + b + ' and is.\n');
+    else
+        log('FAIL: ' + a + ' should be ' + b + ' but instead is ' + evalA + '.\n');
+}
+</script>
+<p>
+This page tests whether an iframe correctly reports renderer-dependent values
+when executing a javascript: load. If the test passes, you'll see a series of
+'PASS' messages below.
+</p>
+<hr>
+<pre id="console"></pre>
+<iframe
+    name="iframe"
+    style="width: 250px; height: 250px; border: 1px solid black; background-color:red;"
+    src="javascript:
+        top.shouldBe('iframe.frameElement.clientWidth', 250);
+        top.shouldBe('iframe.frameElement.clientHeight', 250);
+    "
+></iframe>
index 2aa99f1..2ba22e5 100644 (file)
@@ -1,3 +1,46 @@
+2006-10-13  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Darin, John.
+
+        - Merged more frame and iframe code
+        - Fixed a bug where iframes returned incorrect values for renderer-dependent 
+        properties during javascript: loads because they didn't have renderers at 
+        load time
+        
+        PLT insists this is a small performance win. Don't believe its lies.
+
+        * bindings/js/kjs_html.cpp:
+        (KJS::JSHTMLElement::frameGetter): Renamed frameWidth and frameHeight to
+        width and height, for consistency with the rest of the DOM.
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::queuePostAttachCallback): Added a post-attach callback
+        queue that gets drained after the render tree is fully constructed. Currently,
+        this is only used for frame loading.
+        (WebCore::ContainerNode::attach):
+        * dom/ContainerNode.h:
+        * html/HTMLFrameElement.cpp:
+        (WebCore::HTMLFrameElement::insertedIntoDocument): Removed frame loading from
+        attach() because loading iframes during attach() causes a crash. 
+        Moved frame loading logic into insertedIntoDocument(). That's a more 
+        logical place for it because  document insertion is what 
+        triggers frame loading. Made frame loading a post-attach callback, instead
+        of an immediate action, to fix the incorrect values bug.
+        (WebCore::HTMLFrameElement::attach): Added setWidget call that used to be
+        in HTMLIFrameElement::attach. IFRAME requires this call. FRAME will soon 
+        require this call, once I remove the call from WebKit.
+        (WebCore::HTMLFrameElement::setLocation):
+        (WebCore::HTMLFrameElement::width):
+        (WebCore::HTMLFrameElement::height):
+        * html/HTMLFrameElement.h: Made openURL non-virtual to avoid the unnecessary
+        killing of puppies.
+        * html/HTMLIFrameElement.cpp: Merged code into HTMLFrameElement
+        (WebCore::HTMLIFrameElement::rendererIsNeeded):
+        (WebCore::HTMLIFrameElement::createRenderer):
+        (WebCore::HTMLIFrameElement::insertedIntoDocument):
+        (WebCore::HTMLIFrameElement::removedFromDocument):
+        (WebCore::HTMLIFrameElement::attach):
+        * page/FrameView.h: Removed unused method.
+
 2006-10-13  David Harrison  <harrison@apple.com>
 
         Reviewed by Justin.
index 38293bd..c66e1db 100644 (file)
@@ -1041,8 +1041,8 @@ JSValue *JSHTMLElement::frameGetter(ExecState* exec, int token) const
         case FrameMarginWidth:     return jsString(frameElement.marginWidth());
         case FrameName:            return jsString(frameElement.name());
         case FrameNoResize:        return jsBoolean(frameElement.noResize());
-        case FrameWidth:           return jsNumber(frameElement.frameWidth());
-        case FrameHeight:          return jsNumber(frameElement.frameHeight());
+        case FrameWidth:           return jsNumber(frameElement.width());
+        case FrameHeight:          return jsNumber(frameElement.height());
         case FrameScrolling:       return jsString(frameElement.scrolling());
         case FrameSrc:
         case FrameLocation:        return jsString(frameElement.src());
index ff5c0b2..971455d 100644 (file)
@@ -34,6 +34,7 @@
 #include "RenderTheme.h"
 #include "RootInlineBox.h"
 #include "SystemTime.h"
+#include <wtf/Vector.h>
 
 namespace WebCore {
 
@@ -42,6 +43,11 @@ using namespace EventNames;
 static void dispatchChildInsertionEvents(Node*, ExceptionCode&);
 static void dispatchChildRemovalEvents(Node*, ExceptionCode&);
 
+typedef Vector<std::pair<NodeCallback, Node*> > NodeCallbackQueue;
+static NodeCallbackQueue* s_postAttachCallbackQueue = 0;
+
+static size_t s_attachDepth = 0;
+
 ContainerNode::ContainerNode(Document* doc)
     : EventTargetNode(doc), m_firstChild(0), m_lastChild(0)
 {
@@ -569,11 +575,40 @@ ContainerNode* ContainerNode::addChild(PassRefPtr<Node> newChild)
     return this;
 }
 
+void ContainerNode::queuePostAttachCallback(NodeCallback callback, Node* node)
+{
+    // To keep things simple, we forbid queueing post-attach callbacks from inside attach.
+    ASSERT(s_attachDepth == 0);
+    
+    if (!s_postAttachCallbackQueue)
+        s_postAttachCallbackQueue = new NodeCallbackQueue;
+    
+    s_postAttachCallbackQueue->append(std::pair<NodeCallback, Node*>(callback, node));
+}
+
 void ContainerNode::attach()
 {
+    ++s_attachDepth;
+
     for (Node* child = m_firstChild; child; child = child->nextSibling())
         child->attach();
     EventTargetNode::attach();
+
+    if (s_attachDepth == 1) {
+        if (s_postAttachCallbackQueue) {
+            // We recalculate size() each time through the loop because a callback
+            // can add more callbacks to the end of the queue.
+            for (size_t i = 0; i < s_postAttachCallbackQueue->size(); ++i) {
+                std::pair<NodeCallback, Node*>& pair = (*s_postAttachCallbackQueue)[i];
+                NodeCallback callback = pair.first;
+                Node* node = pair.second;
+                
+                callback(node);
+            }
+            s_postAttachCallbackQueue->clear();
+        }
+    }    
+    --s_attachDepth;
 }
 
 void ContainerNode::detach()
index d6d4b21..c781b1e 100644 (file)
  *
  */
 
-#ifndef DOM_ContainerNodeImpl_h
-#define DOM_ContainerNodeImpl_h
+#ifndef ContainerNode_h
+#define ContainerNode_h
 
 #include "EventTargetNode.h"
 
 namespace WebCore {
+    
+typedef void (*NodeCallback)(Node*);
 
 class ContainerNode : public EventTargetNode
 {
@@ -68,6 +70,9 @@ public:
     void removeChildren();
     void cloneChildNodes(Node* clone);
 
+protected:
+    static void queuePostAttachCallback(NodeCallback, Node*);
+
 private:
     Node* m_firstChild;
     Node* m_lastChild;
@@ -76,6 +81,6 @@ private:
     bool getLowerRightCorner(int& x, int& y) const;
 };
 
-} //namespace
+} // namespace WebCore
 
-#endif
+#endif // ContainerNode_h
index 4674340..640f9d6 100644 (file)
@@ -168,7 +168,7 @@ void HTMLFrameElement::parseMappedAttribute(MappedAttribute *attr)
 
 bool HTMLFrameElement::rendererIsNeeded(RenderStyle *style)
 {
-    // Ignore display: none.
+    // For compatibility, frames render even when display: none is set.
     return isURLAllowed(m_URL);
 }
 
@@ -177,6 +177,11 @@ RenderObject *HTMLFrameElement::createRenderer(RenderArena *arena, RenderStyle *
     return new (arena) RenderFrame(this);
 }
 
+void HTMLFrameElement::openURLCallback(Node* n)
+{
+    static_cast<HTMLFrameElement*>(n)->openURL();
+}
+
 void HTMLFrameElement::insertedIntoDocument()
 {
     HTMLElement::insertedIntoDocument();
@@ -187,12 +192,20 @@ void HTMLFrameElement::insertedIntoDocument()
 
     if (Frame* parentFrame = document()->frame())
         m_name = parentFrame->tree()->uniqueChildName(m_name);
+
+    // We delay frame loading until after the render tree is fully constructed.
+    // Othewise, a synchronous load that executed JavaScript would see incorrect 
+    // (0) values for the frame's renderer-dependent properties, like width.
+    queuePostAttachCallback(&HTMLFrameElement::openURLCallback, this);
 }
 
 void HTMLFrameElement::attach()
 {
     HTMLElement::attach();
     
+    // FIXME: Violates the Liskov Substitution Principle. FRAME elements attach 
+    // differently than FRAME element subclasses, even though FRAME element 
+    // subclasses nominally "are" FRAME elements.
     if (hasTagName(frameTag)) {
         if (HTMLFrameSetElement* frameSetElement = containingFrameSetElement()) {
             if (!m_frameBorderSet)
@@ -201,9 +214,10 @@ void HTMLFrameElement::attach()
                 m_noResize = frameSetElement->noResize();
         }
     }
-        
-    if (!contentFrame())
-        openURL();
+
+    if (RenderPart* renderPart = static_cast<RenderPart*>(renderer()))
+        if (Frame* frame = contentFrame())
+            renderPart->setWidget(frame->view());
 }
 
 void HTMLFrameElement::willRemove()
@@ -220,7 +234,7 @@ void HTMLFrameElement::setLocation(const String& str)
 {
     m_URL = AtomicString(str);
 
-    if (attached())
+    if (inDocument())
         openURL();
 }
 
@@ -349,7 +363,7 @@ void HTMLFrameElement::setSrc(const String &value)
     setAttribute(srcAttr, value);
 }
 
-int HTMLFrameElement::frameWidth() const
+int HTMLFrameElement::width() const
 {
     if (!renderer())
         return 0;
@@ -358,7 +372,7 @@ int HTMLFrameElement::frameWidth() const
     return renderer()->width();
 }
 
-int HTMLFrameElement::frameHeight() const
+int HTMLFrameElement::height() const
 {
     if (!renderer())
         return 0;
@@ -367,4 +381,4 @@ int HTMLFrameElement::frameHeight() const
     return renderer()->height();
 }
 
-}
+} // namespace WebCore
index 93bc53b..2f14a8f 100644 (file)
@@ -99,14 +99,16 @@ public:
     virtual String src() const;
     void setSrc(const String&);
 
-    int frameWidth() const;
-    int frameHeight() const;
+    int width() const;
+    int height() const;
 
     bool viewSourceMode() const { return m_viewSource; }
 
 protected:
     bool isURLAllowed(const AtomicString&) const;
-    virtual void openURL();
+    void openURL();
+
+    static void openURLCallback(Node*);
 
     AtomicString m_URL;
     AtomicString m_name;
index 24cd43d..0de72fd 100644 (file)
@@ -106,7 +106,8 @@ void HTMLFrameSetElement::parseMappedAttribute(MappedAttribute *attr)
 
 bool HTMLFrameSetElement::rendererIsNeeded(RenderStyle *style)
 {
-    // Ignore display: none but do pay attention if a stylesheet has caused us to delay our loading.
+    // For compatibility, frames render even when display: none is set.
+    // However, we delay creating a renderer until stylesheets have loaded. 
     return style->isStyleAvailable();
 }
 
index 9bdb4b7..845af4c 100644 (file)
@@ -82,49 +82,42 @@ void HTMLIFrameElement::parseMappedAttribute(MappedAttribute *attr)
         HTMLFrameElement::parseMappedAttribute(attr);
 }
 
+bool HTMLIFrameElement::rendererIsNeeded(RenderStyle* style)
+{
+    return isURLAllowed(m_URL) && style->display() != NONE;
+}
+
+RenderObject* HTMLIFrameElement::createRenderer(RenderArena* arena, RenderStyle*)
+{
+    return new (arena) RenderPartObject(this);
+}
+
 void HTMLIFrameElement::insertedIntoDocument()
 {
-    HTMLFrameElement::insertedIntoDocument();
-    
     if (document()->isHTMLDocument()) {
-        HTMLDocument *doc = static_cast<HTMLDocument *>(document());
+        HTMLDocument* doc = static_cast<HTMLDocument*>(document());
         doc->addDocExtraNamedItem(oldNameAttr);
     }
 
-    openURL();
+    HTMLFrameElement::insertedIntoDocument();
 }
 
 void HTMLIFrameElement::removedFromDocument()
 {
     if (document()->isHTMLDocument()) {
-        HTMLDocument *doc = static_cast<HTMLDocument *>(document());
+        HTMLDocument* doc = static_cast<HTMLDocument* >(document());
         doc->removeDocExtraNamedItem(oldNameAttr);
     }
 
     HTMLFrameElement::removedFromDocument();
 }
 
-bool HTMLIFrameElement::rendererIsNeeded(RenderStyle *style)
-{
-    // Don't ignore display: none the way frame does.
-    return isURLAllowed(m_URL) && style->display() != NONE;
-}
-
-RenderObject *HTMLIFrameElement::createRenderer(RenderArena *arena, RenderStyle *style)
-{
-    return new (arena) RenderPartObject(this);
-}
-
 void HTMLIFrameElement::attach()
 {
     HTMLFrameElement::attach();
 
-    if (RenderPartObject* renderPartObject = static_cast<RenderPartObject*>(renderer())) {        
-        if (contentFrame()) {
-            renderPartObject->setWidget(contentFrame()->view());
-            renderPartObject->updateWidget();
-        }
-    }
+    if (RenderPartObject* renderPartObject = static_cast<RenderPartObject*>(renderer()))
+        renderPartObject->updateWidget();
 }
 
 bool HTMLIFrameElement::isURLAttribute(Attribute *attr) const
index b4566e3..14b41b5 100644 (file)
@@ -100,8 +100,6 @@ public:
 
     Frame* frame() const { return m_frame.get(); }
 
-    int frameWidth() const { return m_size.width(); }
-
     /**
      * Gets/Sets the margin width/height
      *