Move attributes and methods from HTMLDocument to Document

Source/core/dom/Document.cpp

Index: Source/core/dom/Document.cpp
diff --git a/Source/core/dom/Document.cpp b/Source/core/dom/Document.cpp
index 770524fd52e9b5655e3be6c233914a6481137d15..947d41b331cd2797ab50bc868f63498f911d6e57 100644
--- a/Source/core/dom/Document.cpp
+++ b/Source/core/dom/Document.cpp
@@ -2281,19 +2281,8 @@ ScriptableDocumentParser* Document::scriptableDocumentParser() const
     return parser() ? parser()->asScriptableDocumentParser() : 0;
 }
 
-void Document::open(Document* ownerDocument, ExceptionState& exceptionState)
+void Document::open()
 {
-    if (importLoader()) {
-        exceptionState.throwDOMException(InvalidStateError, "Imported document doesn't support open().");
-        return;
-    }
-
-    if (ownerDocument) {
-        setURL(ownerDocument->url());
-        m_cookieURL = ownerDocument->cookieURL();
-        setSecurityOrigin(ownerDocument->securityOrigin());
-    }
-
     if (m_frame) {
         if (ScriptableDocumentParser* parser = scriptableDocumentParser()) {
             if (parser->isParsing()) {
@@ -2321,6 +2310,27 @@ void Document::open(Document* ownerDocument, ExceptionState& exceptionState)
         m_loadEventProgress = LoadEventNotRun;
 }
 
+void Document::open(Document* ownerDocument, ExceptionState& exceptionState)
+{
+    if (importLoader()) {
+        exceptionState.throwDOMException(InvalidStateError, "Imported document doesn't support open().");

[philipj_slow, 2015/06/24 11:52:19]

Is an import document also an HTML document? If not it could be included in the
error handling below and afterwards you could ASSERT(!importLoader()) if it's
important.

[Habib Virji, 2015/06/25 13:09:46]

Changed as suggested and it is do important added assert.

[philipj_slow, 2015/06/27 21:42:56]

Can you link to
http://w3c.github.io/webcomponents/spec/imports/#interface-document for these to
make it clear that there is actually a spec for this bit, which I assumed there
wasn't and was happily surprised to find.

[Habib Virji, 2015/06/29 12:17:41]

Done.

+        return;
+    }
+
+    if (isXHTMLDocument()) {

[tkent, 2015/06/23 23:42:17]

Why do you check isXHTMLDocument(), not isXMLDocument()?
What about SVGDocument, ImageDocument, PluginDocument, and MediaDocument?

[philipj_slow, 2015/06/24 11:52:19]

The spec says "If the Document object is not flagged as an HTML document, throw
an InvalidStateError exception and abort these steps." so this ought to be |if
(!isHTMLDocument())| with a message like "Only HTML documents support open()."

[Habib Virji, 2015/06/25 13:09:46]

I find spec in-consistent. 

What you mentioned is do specified in the steps but a note before this step
says:
"Throws an InvalidStateError exception if the Document is an XML document." 

Since !isHTMLDocument() != isXMLDocument() (as explained below), note and steps
are inconsistent. 

[philipj_slow, 2015/06/27 21:42:56]

The note is non-normative, so !isHTMLDocument() is what's actually required by
the spec for open().

[Habib Virji, 2015/06/29 12:17:42]

Done.

+        exceptionState.throwDOMException(InvalidStateError, "XHTML doesn't support open().");
+        return;
+    }
+
+    if (ownerDocument) {
+        setURL(ownerDocument->url());
+        m_cookieURL = ownerDocument->cookieURL();
+        setSecurityOrigin(ownerDocument->securityOrigin());
+    }
+
+    open();
+}
+
 void Document::detachParser()
 {
     if (!m_parser)
@@ -2444,6 +2454,14 @@ Element* Document::viewportDefiningElement(const ComputedStyle* rootStyle) const
     return rootElement;
 }
 
+void Document::close()
+{
+    if (!scriptableDocumentParser() || !scriptableDocumentParser()->wasCreatedByScript() || !scriptableDocumentParser()->isParsing())
+        return;
+
+    explicitClose();
+}
+
 void Document::close(ExceptionState& exceptionState)
 {
     // FIXME: We should follow the specification more closely:
@@ -2454,10 +2472,12 @@ void Document::close(ExceptionState& exceptionState)
         return;
     }
 
-    if (!scriptableDocumentParser() || !scriptableDocumentParser()->wasCreatedByScript() || !scriptableDocumentParser()->isParsing())
+    if (isXHTMLDocument()) {

[philipj_slow, 2015/06/24 11:52:19]

Spec says "If the Document object is not flagged as an HTML document, throw an
InvalidStateError exception and abort these steps."

[Habib Virji, 2015/06/25 13:09:47]

Since !isHTMLDocument() != isXMLDocument() (as explained below), note and steps
are inconsistent. 

+        exceptionState.throwDOMException(InvalidStateError, "XHTML doesn't support close().");
         return;
+    }
 
-    explicitClose();
+    close();
 }
 
 void Document::explicitClose()
@@ -2700,6 +2720,11 @@ void Document::write(const SegmentedString& text, Document* ownerDocument, Excep
         return;
     }
 
+    if (isXHTMLDocument()) {

[philipj_slow, 2015/06/24 11:52:19]

Here the spec actually says "If the method was invoked on an XML document, throw
an InvalidStateError exception and abort these steps." which would be |if
(isXMLDocument())|. If that's absolutely equivalent to !isHTMLDocument() I would
use that instead for consistency, and if there is some difference we should
probably have the spec changed.

[Habib Virji, 2015/06/25 13:09:46]

isXMLDocument() relies on value of DocumentClass, which is initialized as
XMLDocument when creating the XML document, XMLDocument | XHTMLDocument when
creating a XHTMLDocument and so forth. Same applies for isHTMLDocument(). 

This implies !isHTMLDocument sets DocumentClass only for HTMLDocument and will
imply not be set for XML, SVG, XHTML but also for Image, Plugin and Media. Which
is not the spec says, so have kept isXMLDocument(). 

Spec for write() is consistent with steps and note. So either note for open and
close should be updated or step for write should be updated. 

[philipj_slow, 2015/06/27 21:42:56]

Does write() need to be supported for Image, Plugin or Media documents?

I agree that the spec is inconsistent. What do you think it should say? To me it
seems most sensible to consistently throw for !isHTMLDocument.

[Habib Virji, 2015/06/29 12:17:42]

It should not. I agree it should throw for !isHTMLDocument.

+        exceptionState.throwDOMException(InvalidStateError, "XHTML doesn't support write().");
+        return;
+    }
+
     NestingLevelIncrementer nestingLevelIncrementer(m_writeRecursionDepth);
 
     m_writeRecursionIsTooDeep = (m_writeRecursionDepth > 1) && m_writeRecursionIsTooDeep;
@@ -2716,7 +2741,7 @@ void Document::write(const SegmentedString& text, Document* ownerDocument, Excep
     }
 
     if (!hasInsertionPoint)
-        open(ownerDocument);
+        open(ownerDocument, exceptionState);

[philipj_slow, 2015/06/24 11:52:19]

It looks like it's impossible for this to throw an exception, so
ASSERT_NO_EXCEPTION? If it were possible, it would also be necessary to handle
it.

[Habib Virji, 2015/06/25 13:09:46]

The reason for adding was it relied on ASSERT_NO_EXCEPTION value as this was
defaultValue for the open(document, exceptionState). With two method one without
parameter, needed to remove default values and this was the only call that was
without exception but with the Document parameter. I have back exceptionState
default in the open with parameter to avoid indicating this will generate an
error. 

 
     ASSERT(m_parser);
     m_parser->insert(text);
@@ -2735,6 +2760,24 @@ void Document::writeln(const String& text, Document* ownerDocument, ExceptionSta
     write("\n", ownerDocument);
 }
 
+void Document::write(LocalDOMWindow* callingWindow, const Vector<String>& text, ExceptionState& exceptionState)
+{
+    ASSERT(callingWindow);
+    StringBuilder builder;
+    for (const String& string : text)
+        builder.append(string);
+    write(builder.toString(), callingWindow->document(), exceptionState);
+}
+
+void Document::writeln(LocalDOMWindow* callingWindow, const Vector<String>& text, ExceptionState& exceptionState)
+{
+    ASSERT(callingWindow);
+    StringBuilder builder;
+    for (const String& string : text)
+        builder.append(string);
+    writeln(builder.toString(), callingWindow->document(), exceptionState);
+}
+
 const KURL& Document::virtualURL() const
 {
     return m_url;