Bug 37175 - Factor DocumentWriter out of FrameLoader
: Factor DocumentWriter out of FrameLoader
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Page Loading
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on: 37433
Blocks: 29947
  Show dependency treegraph
 
Reported: 2010-04-06 16:01 PDT by Adam Barth
Modified: 2010-04-20 15:34 PDT (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-04-06 16:01:53 PDT
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 Adam Barth 2010-04-06 16:09:10 PDT
Created attachment 52678 [details]
Patch
Comment 2 Adam Barth 2010-04-06 16:52:25 PDT
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 WebKit Review Bot 2010-04-06 17:33:44 PDT
Attachment 52678 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1600285
Comment 4 WebKit Review Bot 2010-04-06 18:00:33 PDT
Attachment 52678 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1629298
Comment 5 Darin Adler 2010-04-06 18:11:51 PDT
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.
Comment 6 Adam Barth 2010-04-06 18:24:59 PDT
> 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 Adam Barth 2010-04-06 18:25:22 PDT
Comment on attachment 52678 [details]
Patch

marking r- while I work on Darin's comments.
Comment 8 Adam Barth 2010-04-08 16:26:37 PDT
> 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 Adam Barth 2010-04-08 17:14:31 PDT
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 Adam Barth 2010-04-08 17:40:17 PDT
Created attachment 52924 [details]
Patch
Comment 11 Adam Barth 2010-04-08 17:44:34 PDT
Created attachment 52925 [details]
Patch
Comment 12 WebKit Review Bot 2010-04-08 17:45:55 PDT
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 WebKit Review Bot 2010-04-08 18:15:33 PDT
Attachment 52925 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1653261
Comment 14 Adam Barth 2010-04-08 18:37:52 PDT
Comment on attachment 52925 [details]
Patch

I'll make it build before landing.  Style error is a false positive, sadly...
Comment 15 Eric Seidel 2010-04-09 14:26:28 PDT
Comment on attachment 52925 [details]
Patch

OK.  Looks much better.

You can't spell though: depricatedFrameEncoding

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