[Webkit-unassigned] [Bug 37175] Factor DocumentWriter out of FrameLoader

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37175





--- Comment #5 from Darin Adler <darin at apple.com>  2010-04-06 18:11:51 PST ---
(From update of attachment 52678)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.