Bug 37175 – Factor DocumentWriter out of FrameLoader

• Component: Page Loading
• Status: RESOLVED FIXED

[Adam Barth]

There's a bunch of code in FrameLoader that understands the begin/write/end cycle of decoding bytes from the network and shoving them into a Document.  In this bug, I'll separate that code from the rest of FrameLoader (because there's hardly any interaction with the core loading machine).

[Adam Barth]

Created attachment 52678 [details]
Patch

[Adam Barth]

BTW, I can land this in a separate file.  I just left it in the main file so it's easier to review.

[WebKit Review Bot]

Attachment 52678 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1600285

[WebKit Review Bot]

Attachment 52678 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1629298

[Darin Adler]

Comment on attachment 52678 [details]
Patch

Thanks for continuing your work to make frame loader smaller.

I assume that your plan is to move this class into a separate file in a subsequent patch.

I like, but don't love, the name "document writer". In particular, it seems that the caller of "write" is what you would call the writer. The object you call write *on* is not the writer. I'll try to think of another word for you.

> +    // FIXME: What about the encoding?
> +    writer()->setDecoder(document->decoder());

I think you need to write a slightly longer comment sentence. It took me a while to figure out what your question meant.

>      // Always try UTF-8. If that fails, try frame encoding (if any) and then the default.
>      // For a newly opened frame with an empty URL, encoding() should not be used, because this methods asks decoder, which uses ISO-8859-1.
> -    Settings* settings = m_frame->settings();
> -    request.setResponseContentDispositionEncodingFallbackArray("UTF-8", m_URL.isEmpty() ? m_encoding : encoding(), settings ? settings->defaultTextEncodingName() : String());
> +    String encoding1 = "UTF-8";
> +    String encoding2 = writer()->contentDispositionEncodingFallback2();
> +    String encoding3;
> +    if (Settings* settings = m_frame->settings())
> +        encoding3 = settings->defaultTextEncodingName();
> +    request.setResponseContentDispositionEncodingFallbackArray(encoding1, encoding2, encoding3);
>  }
>  
> +String DocumentWriter::contentDispositionEncodingFallback2() const

> +    // FIXME: It's really unforunate to need to expose this piece of state.
> +    // I suspect a better design is to disentangle user-provided encodings,
> +    // default encodings, and the decoding we're currently using.
> +    String contentDispositionEncodingFallback2() const;

To me this function name seems awful. I believe these encodings are the preferred encodings. Calling it a "fallback array" as the request object does puts the emphasis on the wrong thing. And calling this "fallback 2" is utterly mystifying. Taking confusing code and turning it into a confusing function name isn't good. I suggest calling this function "frameEncoding", because that's what the comment calls it. There will still be the mystery of why the document writer is the object to ask for the frame's encoding.

> +class DocumentWriter : public Noncopyable {
> +public:
> +    DocumentWriter(Frame*);
> +    ~DocumentWriter();

Is there some reason you included a destructor? I think the compiler-generated one will suffice.

> +    void setDecoder(TextResourceDecoder* decoder);

No argument name needed here. This should take a PassRefPtr.

> +    // Callbacks from DocumentWriter
> +    void didBeginDocument(bool dispatch);

> -    void begin(const KURL&, bool dispatchWindowObjectAvailable = true, SecurityOrigin* forcedSecurityOrigin = 0);

You shortened the name of this boolean, perhaps because inside the function the shorter name was used. Please use the longer name.

Private members becoming public is always a disappointment.

[Adam Barth]

> I assume that your plan is to move this class into a separate file in a
> subsequent patch.

Yes.  I can also do that when I land the patch if you like.  It seemed easier to review this way.

> I like, but don't love, the name "document writer". In particular, it seems
> that the caller of "write" is what you would call the writer. The object you
> call write *on* is not the writer. I'll try to think of another word for you.

I see.  I was thinking of this class as part of the controller, but you're right that it's more of a model or a model adaptor.  Suggestions welcome.

> > +    // FIXME: What about the encoding?
> > +    writer()->setDecoder(document->decoder());
> 
> I think you need to write a slightly longer comment sentence. It took me a
> while to figure out what your question meant.

This was actually a note to me in working on the code.  I usually use "xyzzy" to mark internal comments (and then I grep for that string before posting), but I missed it in this case.  I think this line of code (the missing one about encoding) is a bug in the original code, but I haven't looked into it carefully enough.

> > +    // FIXME: It's really unforunate to need to expose this piece of state.
> > +    // I suspect a better design is to disentangle user-provided encodings,
> > +    // default encodings, and the decoding we're currently using.
> > +    String contentDispositionEncodingFallback2() const;
> 
> To me this function name seems awful. I believe these encodings are the
> preferred encodings. Calling it a "fallback array" as the request object does
> puts the emphasis on the wrong thing. And calling this "fallback 2" is utterly
> mystifying. Taking confusing code and turning it into a confusing function name
> isn't good. I suggest calling this function "frameEncoding", because that's
> what the comment calls it. There will still be the mystery of why the document
> writer is the object to ask for the frame's encoding.

Yeah, this function is a wart that I struggled with for a while.  The problem is that there seem to be three encodings that we need to worry about.  I think the right thing to do is have all three available off this object so the FrameLoader can decide what to do with them instead of putting that knowledge into this object.  I looked a bit into disentangling them, but it's somewhat complicated and I think it's better to do separately from the rest of this patch, which is mostly just moving stuff around.  In the mean time, calling it frameEncoding seems ok.  I had it as outgoingEncoding for a while, but that didn't make much sense either.

> > +class DocumentWriter : public Noncopyable {
> > +public:
> > +    DocumentWriter(Frame*);
> > +    ~DocumentWriter();
> 
> Is there some reason you included a destructor? I think the compiler-generated
> one will suffice.

The problem is the RefPtr of an incomplete type.  The destructor needs to be in a complication unit with the complete type.

> > +    void setDecoder(TextResourceDecoder* decoder);
> 
> No argument name needed here. This should take a PassRefPtr.

Oops.  I this snuck back in while I was looking at various other options for how to handle the encoding.

> > +    // Callbacks from DocumentWriter
> > +    void didBeginDocument(bool dispatch);
> 
> > -    void begin(const KURL&, bool dispatchWindowObjectAvailable = true, SecurityOrigin* forcedSecurityOrigin = 0);
> 
> You shortened the name of this boolean, perhaps because inside the function the
> shorter name was used. Please use the longer name.

Will fix.

> Private members becoming public is always a disappointment.

Another option is to move clear() into the new object, but it seems to be actuating a bunch of Frame-level state, and the new object wanted to be more about Document-related state.

My eventual goal is to make FrameLoader more of a hub object that coordinates larger interacting objects.  That process leads to more public members.

Thanks for the review.

[Adam Barth]

Comment on attachment 52678 [details]
Patch

marking r- while I work on Darin's comments.

[Adam Barth]

> I like, but don't love, the name "document writer". In particular, it seems
> that the caller of "write" is what you would call the writer. The object you
> call write *on* is not the writer. I'll try to think of another word for you.

Another point of interest on this subject is that DocumentWriter does call write() on the tokenizer:

+         tokenizer->write(decoded, true);

[Adam Barth]

I've followed all your recommendations.  I've also renamed DocumentWriter::write to DocumentWriter::addData so its clear that the "Write" in its name refers to the operation it performs on the tokenize/document.

[Adam Barth]

Created attachment 52924 [details]
Patch

[Adam Barth]

Created attachment 52925 [details]
Patch

[WebKit Review Bot]

Attachment 52925 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/loader/DocumentWriter.cpp:121:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 52925 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1653261

[Adam Barth]

Comment on attachment 52925 [details]
Patch

I'll make it build before landing.  Style error is a false positive, sadly...

[Eric Seidel (no email)]

Comment on attachment 52925 [details]
Patch

OK.  Looks much better.

You can't spell though: depricatedFrameEncoding

Please fix when you land.

[Adam Barth]

Committed r57468: <http://trac.webkit.org/changeset/57468>

[WebKit Review Bot]

http://trac.webkit.org/changeset/57468 might have broken Chromium Mac Release and Chromium Linux Release

[Adam Barth]

Will re-land better.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57927 might have broken Qt Linux ARMv5 Release and Chromium Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/57926
http://trac.webkit.org/changeset/57927

[Adam Barth]

Landed better, but not perfectly: http://trac.webkit.org/changeset/57927