Bug 37175 - Factor DocumentWriter out of FrameLoader
: Factor DocumentWriter out of FrameLoader
Status: RESOLVED FIXED
: WebKit
Page Loading
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 37433
: 29947
  Show dependency treegraph
 
Reported: 2010-04-06 16:01 PST by
Modified: 2010-04-20 15:34 PST (History)


Attachments
Patch (42.66 KB, patch)
2010-04-06 16:09 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Patch (52.20 KB, patch)
2010-04-08 17:40 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Patch (64.43 KB, patch)
2010-04-08 17:44 PST, Adam Barth
eric: review+
abarth: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-06 16:01:53 PST
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).
------- Comment #1 From 2010-04-06 16:09:10 PST -------
Created an attachment (id=52678) [details]
Patch
------- Comment #2 From 2010-04-06 16:52:25 PST -------
BTW, I can land this in a separate file.  I just left it in the main file so it's easier to review.
------- Comment #3 From 2010-04-06 17:33:44 PST -------
Attachment 52678 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1600285
------- Comment #4 From 2010-04-06 18:00:33 PST -------
Attachment 52678 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1629298
------- Comment #5 From 2010-04-06 18:11:51 PST -------
(From update of attachment 52678 [details])
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.
------- Comment #6 From 2010-04-06 18:24:59 PST -------
> 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.
------- Comment #7 From 2010-04-06 18:25:22 PST -------
(From update of attachment 52678 [details])
marking r- while I work on Darin's comments.
------- Comment #8 From 2010-04-08 16:26:37 PST -------
> 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);
------- Comment #9 From 2010-04-08 17:14:31 PST -------
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.
------- Comment #10 From 2010-04-08 17:40:17 PST -------
Created an attachment (id=52924) [details]
Patch
------- Comment #11 From 2010-04-08 17:44:34 PST -------
Created an attachment (id=52925) [details]
Patch
------- Comment #12 From 2010-04-08 17:45:55 PST -------
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.
------- Comment #13 From 2010-04-08 18:15:33 PST -------
Attachment 52925 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1653261
------- Comment #14 From 2010-04-08 18:37:52 PST -------
(From update of attachment 52925 [details])
I'll make it build before landing.  Style error is a false positive, sadly...
------- Comment #15 From 2010-04-09 14:26:28 PST -------
(From update of attachment 52925 [details])
OK.  Looks much better.

You can't spell though: depricatedFrameEncoding

Please fix when you land.
------- Comment #16 From 2010-04-11 21:20:35 PST -------
Committed r57468: <http://trac.webkit.org/changeset/57468>
------- Comment #17 From 2010-04-11 21:35:40 PST -------
http://trac.webkit.org/changeset/57468 might have broken Chromium Mac Release and Chromium Linux Release
------- Comment #18 From 2010-04-11 22:53:22 PST -------
Will re-land better.
------- Comment #19 From 2010-04-20 15:23:59 PST -------
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
------- Comment #20 From 2010-04-20 15:34:48 PST -------
Landed better, but not perfectly: http://trac.webkit.org/changeset/57927