[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 #42 from adawit at kde.org  2010-03-30 00:09:59 PST ---
(In reply to comment #41)
> (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.

No arguments from me on that point...

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

you just made it sound like I was arguing in front of some high court somewhere
and my presuasive commentary won the day... Perhaps I should say, thank your
honor :)

> 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;

Hmm... there seems to be another enum declared in that same file that does not
follow the style you suggest here. No matter, I implemented it as you
suggested, except I added an additional enum, 'None', to the list as the first
item...

> ContentDispositionType contentDispositionType(const String&);

Done...

> Thanks for following up on this.

No problems... Only thing left is should I generate a patch against the last
patch or create a complete patch with the new suggested fixes ? Should this new
patch then be posted to a new bug report or should I still post it here ?

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