Refactoring: Simplify DocumentWriter by reorganizing its lifetime.

Source/core/loader/DocumentLoader.cpp

Index: Source/core/loader/DocumentLoader.cpp
diff --git a/Source/core/loader/DocumentLoader.cpp b/Source/core/loader/DocumentLoader.cpp
index 650451786d46ccf01302669a1a1c1f74006602b8..c6c6802497fc5e77a687f82d1cd6fc750bc557d2 100644
--- a/Source/core/loader/DocumentLoader.cpp
+++ b/Source/core/loader/DocumentLoader.cpp
@@ -30,13 +30,7 @@
 #include "config.h"
 #include "core/loader/DocumentLoader.h"
 
-#include <wtf/Assertions.h>
-#include <wtf/MemoryInstrumentationHashMap.h>
-#include <wtf/MemoryInstrumentationHashSet.h>
-#include <wtf/MemoryInstrumentationVector.h>
-#include <wtf/text/CString.h>
-#include <wtf/text/WTFString.h>
-#include <wtf/unicode/Unicode.h>
+#include "core/dom/DOMImplementation.h"
 #include "core/dom/Document.h"
 #include "core/dom/DocumentParser.h"
 #include "core/dom/Event.h"
@@ -50,6 +44,7 @@
 #include "core/loader/FrameLoader.h"
 #include "core/loader/FrameLoaderClient.h"
 #include "core/loader/ResourceLoader.h"
+#include "core/loader/SinkDocument.h"
 #include "core/loader/TextResourceDecoder.h"
 #include "core/loader/UniqueIdentifier.h"
 #include "core/loader/appcache/ApplicationCacheHost.h"
@@ -66,6 +61,13 @@
 #include "core/platform/Logging.h"
 #include "weborigin/SchemeRegistry.h"
 #include "weborigin/SecurityPolicy.h"
+#include "wtf/Assertions.h"
+#include "wtf/MemoryInstrumentationHashMap.h"
+#include "wtf/MemoryInstrumentationHashSet.h"
+#include "wtf/MemoryInstrumentationVector.h"
+#include "wtf/text/CString.h"
+#include "wtf/text/WTFString.h"
+#include "wtf/unicode/Unicode.h"
 
 namespace WebCore {
 
@@ -96,14 +98,12 @@ DocumentLoader::DocumentLoader(const ResourceRequest& req, const SubstituteData&
     : m_deferMainResourceDataLoad(true)
     , m_frame(0)
     , m_cachedResourceLoader(CachedResourceLoader::create(this))
-    , m_writer(m_frame)
     , m_originalRequest(req)
     , m_substituteData(substituteData)
     , m_originalRequestCopy(req)
     , m_request(req)
     , m_committed(false)
     , m_isStopping(false)
-    , m_gotFirstByte(false)
     , m_isClientRedirect(false)
     , m_wasOnloadHandled(false)
     , m_loadingMainResource(false)
@@ -298,7 +298,6 @@ void DocumentLoader::commitIfReady()
     if (!m_committed) {
         m_committed = true;
         frameLoader()->commitProvisionalLoad();
-        m_writer.setMIMEType(m_response.mimeType());
     }
 }
 
@@ -344,15 +343,19 @@ void DocumentLoader::finishedLoading(double finishTime)
     if (!frameLoader())
         return;
 
-    if (!maybeCreateArchive()) {
+    if (isArchiveMIMEType(m_response.mimeType())) {
+        createArchive();
+    } else {
         // If this is an empty document, it will not have actually been created yet. Commit dummy data so that
         // DocumentWriter::begin() gets called and creates the Document.
-        if (!m_gotFirstByte)
+        if (!m_writer)
             commitData(0, 0);
+
         frameLoader()->client()->finishedLoading(this);
     }
 
-    m_writer.end();
+    endWriting(m_writer.get());
+
     if (!m_mainDocumentError.isNull())
         return;
     clearMainResourceLoader();
@@ -629,34 +632,33 @@ void DocumentLoader::stopLoadingForPolicyChange()
     cancelMainResourceLoad(error);
 }
 
-void DocumentLoader::commitData(const char* bytes, size_t length)
+void DocumentLoader::ensureWriter()
 {
-    if (!m_gotFirstByte) {
-        m_gotFirstByte = true;
-        m_writer.begin(documentURL(), false);
-        m_writer.setDocumentWasLoadedAsPartOfNavigation();
+    ensureWriter(m_response.mimeType());
+}
 
-        if (frameLoader()->stateMachine()->creatingInitialEmptyDocument())
-            return;
+void DocumentLoader::ensureWriter(const String& mimeType)
+{
+    if (m_writer)
+        return;

[abarth-chromium, 2013/06/26 03:04:08]

Should we ASSERT that the mimeTypes match?

[Hajime Morrita, 2013/06/26 04:24:39]

We should. Will do.

+
+    String encoding = overrideEncoding().isNull() ? response().textEncodingName().impl() : overrideEncoding();
+    bool userChosen = !overrideEncoding().isNull();
+    m_writer = createWriterFor(m_frame, 0, documentURL(), mimeType, encoding, false, true);
+    m_writer->setDocumentWasLoadedAsPartOfNavigation();
+
+    if (frameLoader()->stateMachine()->creatingInitialEmptyDocument())
+        return;
         
-        // The origin is the MHTML file, we need to set the base URL to the document encoded in the MHTML so
-        // relative URLs are resolved properly.
-        if (m_archive)
-            m_frame->document()->setBaseURLOverride(m_archive->mainResource()->url());
-
-        // Call receivedFirstData() exactly once per load.
-        frameLoader()->receivedFirstData();
-
-        bool userChosen = true;
-        String encoding = overrideEncoding();
-        if (encoding.isNull()) {
-            userChosen = false;
-            encoding = response().textEncodingName();
-        }
-        m_writer.setEncoding(encoding, userChosen);
-    }
+    // Call receivedFirstData() exactly once per load.
+    frameLoader()->receivedFirstData();
+}
+
+void DocumentLoader::commitData(const char* bytes, size_t length)
+{
+    ensureWriter();
     ASSERT(m_frame->document()->parsing());
-    m_writer.addData(bytes, length);
+    m_writer->addData(bytes, length);
 }
 
 void DocumentLoader::reportMemoryUsage(MemoryObjectInfo* memoryObjectInfo) const
@@ -711,8 +713,8 @@ void DocumentLoader::setFrame(Frame* frame)
     if (m_frame == frame)
         return;
     ASSERT(frame && !m_frame);
+    ASSERT(!m_writer);
     m_frame = frame;
-    m_writer.setFrame(frame);
 }
 
 void DocumentLoader::detachFromFrame()
@@ -763,22 +765,21 @@ bool DocumentLoader::isLoadingInAPISense() const
     return frameLoader()->subframeIsLoading();
 }
 
-bool DocumentLoader::maybeCreateArchive()
+void DocumentLoader::createArchive()
 {
-    // Give the archive machinery a crack at this document. If the MIME type is not an archive type, it will return 0.
-    if (!isArchiveMIMEType(m_response.mimeType()))
-        return false;
-
     m_archive = MHTMLArchive::create(m_response.url(), mainResourceData().get());
     ASSERT(m_archive);
     
     addAllArchiveResources(m_archive.get());
     ArchiveResource* mainResource = m_archive->mainResource();
-    m_writer.setMIMEType(mainResource->mimeType());
-    
-    ASSERT(m_frame->document());
+
+    ensureWriter(mainResource->mimeType());
+
+    // The origin is the MHTML file, we need to set the base URL to the document encoded in the MHTML so
+    // relative URLs are resolved properly.
+    m_frame->document()->setBaseURLOverride(m_archive->mainResource()->url());
+
     commitData(mainResource->data()->data(), mainResource->data()->size());
-    return true;
 }
 
 void DocumentLoader::addAllArchiveResources(MHTMLArchive* archive)
@@ -901,9 +902,9 @@ void DocumentLoader::addResourceLoader(ResourceLoader* loader)
     // The main resource's underlying ResourceLoader will ask to be added here.
     // It is much simpler to handle special casing of main resource loads if we don't
     // let it be added. In the main resource load case, mainResourceLoader()
-    // will still be null at this point, but m_gotFirstByte should be false here if and only
+    // will still be null at this point, but document() should be null here if and only

[abarth-chromium, 2013/06/26 03:04:08]

null -> zero

[Hajime Morrita, 2013/06/26 04:24:39]

Done.

     // if we are just starting the main resource load.
-    if (!m_gotFirstByte)
+    if (!document())
         return;
     ASSERT(!m_resourceLoaders.contains(loader));
     ASSERT(!mainResourceLoader() || mainResourceLoader() != loader);
@@ -1019,20 +1020,68 @@ void DocumentLoader::handledOnloadEvents()
 
 DocumentWriter* DocumentLoader::beginWriting(const String& mimeType, const String& encoding, const KURL& url)
 {
-    m_writer.setMIMEType(mimeType);
-    m_writer.setEncoding(encoding, false);
-    m_writer.begin(url);
-    return &m_writer;
+    m_writer = createWriterFor(m_frame, 0, url, mimeType, encoding, false, true);
+    return m_writer.get();
+}
+
+void DocumentLoader::endWriting(DocumentWriter* writer)
+{
+    ASSERT_UNUSED(writer, m_writer == writer);
+    m_writer->end();
+    m_writer.clear();
+}
+
+
+PassOwnPtr<DocumentWriter> DocumentLoader::createWriterFor(Frame* frame, const Document* ownerDocument, const KURL& url, const String& mimeType, const String& encoding, bool userChosen, bool dispatch)
+{
+    // Create a new document before clearing the frame, because it may need to
+    // inherit an aliased security context.
+    RefPtr<Document> document = DOMImplementation::createDocument(mimeType, frame, url, frame->inViewSourceMode());
+    if (document->isPluginDocument() && document->isSandboxed(SandboxPlugins))
+        document = SinkDocument::create(frame, url);
+    bool shouldReuseDefaultView = frame->loader()->stateMachine()->isDisplayingInitialEmptyDocument() && frame->document()->isSecureTransitionTo(url);
+
+    RefPtr<DOMWindow> originalDOMWindow;
+    if (shouldReuseDefaultView)
+        originalDOMWindow = frame->domWindow();
+    frame->loader()->clear(!shouldReuseDefaultView, !shouldReuseDefaultView);
+
+    if (!shouldReuseDefaultView) {
+        frame->setDOMWindow(DOMWindow::create(frame));
+    } else {
+        // Note that the old Document is still attached to the DOMWindow; the
+        // setDocument() call below will detach the old Document.
+        ASSERT(originalDOMWindow);
+        frame->setDOMWindow(originalDOMWindow);
+    }
+
+    frame->loader()->setOutgoingReferrer(url);
+    frame->domWindow()->setDocument(document);
+
+    if (ownerDocument) {
+        document->setCookieURL(ownerDocument->cookieURL());
+        document->setSecurityOrigin(ownerDocument->securityOrigin());
+    }
+
+    frame->loader()->didBeginDocument(dispatch);
+
+    return DocumentWriter::create(document.get(), mimeType, encoding, userChosen);
 }
 
 String DocumentLoader::mimeType() const
 {
-    return m_writer.mimeType();
+    return m_writer ? m_writer->mimeType() : m_response.mimeType().impl();

[abarth-chromium, 2013/06/26 03:04:08]

impl()   ?

[Hajime Morrita, 2013/06/26 04:24:39]

I shouldn't have used the ternary operator :-|

 }
 
+// This is only called by ScriptController::executeScriptIfJavaScriptURL
+// and always contains the result of evaluating a javascript: url.
+// This is the <iframe src="javascript:'html'"> case.
 void DocumentLoader::replaceDocument(const String& source, Document* ownerDocument)
 {
-    m_writer.replaceDocument(source, ownerDocument);
+    m_writer = createWriterFor(m_frame, ownerDocument, m_frame->document()->url(), "", "", false, true);
+    if (!source.isNull())
+        m_writer->appendReplacingData(source);
+    endWriting(m_writer.get());
 }
 
 } // namespace WebCore