[Webkit-unassigned] [Bug 36395] [Qt] Patch to add support for Content-Disposition...

[bugzilla-daemon at webkit.org]

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





--- Comment #41 from Brady Eidson <beidson at apple.com>  2010-03-29 20:48:37 PST ---
(In reply to comment #40)
> (In reply to comment #39)
> 
> >
> > But as written the method implements a client policy.  This reeks of breaking
> > the layering between WebCore and WebKit.  I would feel a lot better about it if
> > the method was:
> > String ResourceResponseBase::contentDispositionType() const;
> 
> Though I have no particular objection to where this functionality ends up, I do
> not see how determining the type of content disposition is any different from
> determining the filename from the content-disposition header. In fact the
> filename portion of this header might be completely useless depending on the
> type attribute (inline or attachment or nothing). And since the code that
> determines the filename 
> 
> String filenameFromHTTPContentDisposition(const String&);
> 
> already exists in this very same file, would it not make sense to have the
> other one here too ? 

Indeed.

The more I look at this and try to put in words why this patch irritated me,
the more I see that the entirety of HTTPParsers.cpp irritates me.

The consistency of the methods is almost nonexistent.  The whole file seems to
be a dumping ground for random stuff coded in a random style. 
filenameFromHTTPContentDisposition(), for example, seems to belong on
ResourceResponse.

But since there's precedent, I revoke my blame and strong dissent.  :)

It's best to match the others that take a header value, like the companion
filenameFromHTTPContentDisposition(const String&);

> Perhaps changing how it is implemented would ease the
> gripes you have with it ? How about changing it to
> 
> bool HTTPParsers::isAttachmentContentDisposition(const String&) ;

This still reeks of "WebCore determining policy".  Better to make it more
general purpose.  Like:

> OR
> 
> enum ContentDispositionType
> {
>      Inline,
>      Attachment
> };
> 
> int HTTPParsers::contentDispositionType(const String&);
> 
> instead ?

Yes.  For completeness and matching our style, it'd be:

typedef enum {
    Inline,
    Attachment,
    Other
} ContentDispositionType;

ContentDispositionType contentDispositionType(const String&);

Thanks for following up on this.

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