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

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Adam Barth <abarth at webkit.org>  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.

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