[webkit-reviews] review granted: [Bug 50489] Move DocumentWriter to DocumentLoader : [Attachment 75549] Attempt #1

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Nate Chapin <japhet at chromium.org>'s
request for review:
Bug 50489: Move DocumentWriter to DocumentLoader
https://bugs.webkit.org/show_bug.cgi?id=50489

Attachment 75549: Attempt #1
https://bugs.webkit.org/attachment.cgi?id=75549&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=75549&action=review

This patch is great.  Thanks for doing this!  Some comments below to think
about before landing.

> WebKit/wx/WebKitSupport/FrameLoaderClientWx.cpp:437
> -	       FrameLoader* fl = loader->frameLoader();
> -	       fl->writer()->setEncoding(m_response.textEncodingName(), false);

> +	       loader->writer()->setEncoding(m_response.textEncodingName(),
false);

:)

> WebKit/wince/WebCoreSupport/FrameLoaderClientWinCE.cpp:453
> -	   FrameLoader* loader = documentLoader->frameLoader();
> -	   loader->writer()->setEncoding(m_response.textEncodingName(), false);

> +	   documentLoader->writer()->setEncoding(m_response.textEncodingName(),
false);

:)

> WebCore/dom/Document.cpp:432
> +	   m_documentLoader->writer()->setFrame(frame);

Hum...	This is a small sadness.

> WebCore/dom/Document.cpp:4449
> -	   DocumentLoader* documentLoader =
m_frame->loader()->documentLoader();
> -	   if (documentLoader && documentLoader->substituteData().isValid())
> +	   if (m_documentLoader->substituteData().isValid())

I really like that Document has a pointer to DocumentLoader.  Going through the
Frame / FrameLoader to get the DocumentLoader is just asking for trouble.

> WebCore/dom/Document.cpp:4528
>      f->loader()->setURL(url);

I dream that someday you'll get your patch to remove this working.  :)

> WebCore/html/HTMLLinkElement.cpp:219
> -	   if (charset.isEmpty() && document()->frame())
> -	       charset = document()->frame()->loader()->writer()->encoding();
> +	   if (charset.isEmpty())
> +	       charset = document()->loader()->writer()->encoding();

Nice.

> WebCore/loader/FrameLoader.cpp:235
> -    writer()->begin(KURL(), false);
> -    writer()->end();
> +    m_documentLoader->writer()->begin(KURL(), false);
> +    m_documentLoader->writer()->end();

This doesn't need to be m_provisionalDocumentLoader ?  I guess nothing would
work if it was wrong, so it must be right.

> WebCore/loader/FrameLoader.cpp:595
>      m_frame->setDocument(0);
> -    writer()->clear();
> +    if (activeDocumentLoader())
> +	   activeDocumentLoader()->writer()->clear();

Does it make sense to get the document loader from the document here instead?

> WebCore/loader/FrameLoader.cpp:2676
>      Settings* settings = m_frame->settings();
> -    request.setResponseContentDispositionEncodingFallbackArray("UTF-8",
writer()->deprecatedFrameEncoding(), settings ?
settings->defaultTextEncodingName() : String());
> +    request.setResponseContentDispositionEncodingFallbackArray("UTF-8",
activeDocumentLoader()->writer()->deprecatedFrameEncoding(), settings ?
settings->defaultTextEncodingName() : String());

Sigh.  This code is so evil, but not really your fault.

> WebCore/loader/DocumentLoader.cpp:378
> +    m_writer.setFrame(frame);

Why do we need to call setFrame in two places?	I would have expected this one
to be sufficient.

By the way, another approach is to give DocumentWriter a back-pointer to
DocumentLoader, which then has this back-pointer to Frame.  I'm not sure that
really buys you anything over the approach you have here though.