Bug 36395

Summary: [Qt] Patch to add support for Content-Disposition...
Product: WebKit Reporter: Dawit A. <adawit>
Component: WebKit QtAssignee: Nobody <webkit-unassigned>
Status: CLOSED FIXED    
Severity: Enhancement CC: beidson, commit-queue, hausmann, kenneth, webkit.review.bot
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: Other   
OS: All   
Bug Depends on:    
Bug Blocks: 35784    
Attachments:
Description Flags
Content-Disposition support
hausmann: review-
Alternate patch as described in the original report...
hausmann: review-
Updated Content-Disposition patch...
none
Updated Content-Disposition patch II
none
Updated Content-Disposition path III
none
Updated Content-Disposition patch IV
none
Updated Content-Disposition patch V
none
Updated Content-Disposition patch VI none

Dawit A.
Reported 2010-03-19 16:08:29 PDT
Created attachment 51199 [details] Content-Disposition support The attached file is an attempt to fix missing support for the Content-Disposition header in QtWebKit. Currently, this HTTP header is blissfully ignored by QtWebKit and causes problems for all clients that rely on it as a rendering engine. The attached patch attempts to resolve this issue and is based on the way this issue is handled in the chromium source code. Basically the patch checks for the presence of a content-disposition header and changes the resource handling policy to PolicyDownload. This forces any content that is supposed to be downloaded, i.e. contains the "attachement" keyword in the header, to be reported as unsupportedContent to the client using this library. There is one cavet with the patch I provided here. Because the patch forces the request to be treated as a download request, the action the user took (e.g. clicking on a link), will be ignored by default unless forwarding unsupportedContent signal is enabled and handled. One possible solution to this would be to simply add a new signal, e.g. QWebPage::downloadResponse (QNetworkReply*) or QWebPage::downloadRequest(QNetworkReply*). Then "void FrameLoaderClientQt::download(...)" could be amended to emit the new signal when the "Content-Disposition" header is detected. This is only a couple of line of additional code in the afforementioned function. However, that depends on whether or not a.) the patch is accepted and b.) simply sending unsupportedContent is not acceptable inorder to support the content-disposition header. Finally here is a test site for checking "Content-Disposition" support in your rendering engine/browser of choice: http://greenbytes.de/tech/tc2231/ Regards, Dawit A.
Attachments
Content-Disposition support (1.80 KB, patch)
2010-03-19 16:08 PDT, Dawit A. 		hausmann: review-
DetailsFormatted DiffDiff
Alternate patch as described in the original report... (3.99 KB, patch)
2010-03-20 16:07 PDT, Dawit A. 		hausmann: review-
DetailsFormatted DiffDiff
Updated Content-Disposition patch... (4.98 KB, patch)
2010-03-22 21:37 PDT, Dawit A. 		no flags
DetailsFormatted DiffDiff
Updated Content-Disposition patch II (7.53 KB, patch)
2010-03-22 21:48 PDT, Dawit A. 		no flags
DetailsFormatted DiffDiff
Updated Content-Disposition path III (6.35 KB, patch)
2010-03-24 09:13 PDT, Dawit A. 		no flags
DetailsFormatted DiffDiff
Updated Content-Disposition patch IV (6.54 KB, patch)
2010-03-25 08:53 PDT, Dawit A. 		no flags
DetailsFormatted DiffDiff
Updated Content-Disposition patch V (9.53 KB, patch)
2010-03-28 18:08 PDT, Dawit A. 		no flags
DetailsFormatted DiffDiff
Updated Content-Disposition patch VI (10.25 KB, patch)
2010-03-29 06:55 PDT, Dawit A. 		no flags
DetailsFormatted DiffDiff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.
Dawit A.
Comment 1 2010-03-20 16:07:47 PDT
Created attachment 51235 [details] Alternate patch as described in the original report...
Simon Hausmann
Comment 2 2010-03-21 16:17:38 PDT
Nice! Any chance of providing a layout test for this, maybe using an HTTP layout test or a synthetic network-access manager in the unit tests?
Simon Hausmann
Comment 3 2010-03-21 16:19:31 PDT
From an API perspective the first solution sounds better to me than the second one, which I feel may be confusing. Is it just me? :)
Simon Hausmann
Comment 4 2010-03-21 16:20:22 PDT
BTW, either patch is missing a ChangeLog and if you'd like reviewers to take a look at the patches, please mark them as available for review with the "review?" flag.
Dawit A.
Comment 5 2010-03-21 17:59:01 PDT
(In reply to comment #2) > Nice! > > Any chance of providing a layout test for this, maybe using an HTTP layout test > or a synthetic network-access manager in the unit tests? I have no idea how to do this... Can you please give me a hint where such unit test is supposed to be added ?
Dawit A.
Comment 6 2010-03-21 18:02:22 PDT
(In reply to comment #4) > BTW, either patch is missing a ChangeLog and if you'd like reviewers to take a > look at the patches, please mark them as available for review with the > "review?" flag. Where does one mark the patches with the review flag ? Find the changelog file though ; so I will go ahead and provide an patch that updates that as well...
Kenneth Rohde Christiansen
Comment 7 2010-03-21 19:19:31 PDT
(In reply to comment #5) > (In reply to comment #2) > > Nice! > > > > Any chance of providing a layout test for this, maybe using an HTTP layout test > > or a synthetic network-access manager in the unit tests? > > I have no idea how to do this... Can you please give me a hint where such unit > test is supposed to be added ? Unit tests for our API goes into WebKit/qt/tests
Kenneth Rohde Christiansen
Comment 8 2010-03-21 19:21:15 PDT
> Where does one mark the patches with the review flag ? Find the changelog file > though ; so I will go ahead and provide an patch that updates that as well... Click on "details" next to your patch. You can also do it when you attach a new attachment. Please generate ChangeLog using WebKitTools/Script/prepare-ChangeLog Also, always check coding style using WebKitTools/Script/check-webkit-style That should get you going :-) Great patch btw!
Dawit A.
Comment 9 2010-03-21 20:01:54 PDT
(In reply to comment #3) > From an API perspective the first solution sounds better to me than the second > one, which I feel may be confusing. Is it just me? :) The problem with the first solution is that QWebPage::unsupportedContent is not emitted by default unless the client application explicitly enables it by invoking QWebPage::setForwardUnsupportedContent. To me this seems to be an impedement because the "Content-Disposition" header is a common way web servers indicate that a resource is supposed to be downloaded and not rendered by default. As such, the handling of request that contain the "content-disposition" header should not be left up to a signal that is only emitted whenever a client application explicitly enables it. Furthermore, client applications are likely to handle unsupported content and download requests differently and having a single signal to deal with these two different issues seems to go against standard Qt API practise, no ? Anyhow, regardless of the chosen approach, the signal that gets emitted as result of encountering this header should IMHO need to be ON by default.
Dawit A.
Comment 10 2010-03-21 20:17:31 PDT
(In reply to comment #8) > > Where does one mark the patches with the review flag ? Find the changelog file > > though ; so I will go ahead and provide an patch that updates that as well... > > Click on "details" next to your patch. You can also do it when you attach a new > attachment. > > Please generate ChangeLog using WebKitTools/Script/prepare-ChangeLog Cannot use this at all since I am working off of a weekly snapshot and not a git-clone of the qtwebkit respository. > Also, always check coding style using WebKitTools/Script/check-webkit-style Did this and fixed only a single problem sort of related to my patch. However, my changes seem to be in files that the script deems exempt from such checks. I am talking about the qwebpage.* files specifically. > That should get you going :-) Great patch btw! Thanks for both ;)
Kenneth Rohde Christiansen
Comment 11 2010-03-21 20:35:48 PDT
Please do not set r+ but r? ! r+ means that it was reviewed positively, but you want to request a review.
Dawit A.
Comment 12 2010-03-21 20:42:30 PDT
(In reply to comment #11) > Please do not set r+ but r? ! > > r+ means that it was reviewed positively, but you want to request a review. Got it. Done...
WebKit Review Bot
Comment 13 2010-03-21 20:43:41 PDT
Attachment 51199 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 14 2010-03-21 20:44:04 PDT
Attachment 51235 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Simon Hausmann
Comment 15 2010-03-22 14:44:57 PDT
(In reply to comment #10) > (In reply to comment #8) > > > Where does one mark the patches with the review flag ? Find the changelog file > > > though ; so I will go ahead and provide an patch that updates that as well... > > > > Click on "details" next to your patch. You can also do it when you attach a new > > attachment. > > > > Please generate ChangeLog using WebKitTools/Script/prepare-ChangeLog > > Cannot use this at all since I am working off of a weekly snapshot and not a > git-clone of the qtwebkit respository. Then you'll have to create the ChangeLog entry yourself. But why not get a git clone of WebKit or use svn directly?
Simon Hausmann
Comment 16 2010-03-22 14:49:29 PDT
(In reply to comment #9) > (In reply to comment #3) > > From an API perspective the first solution sounds better to me than the second > > one, which I feel may be confusing. Is it just me? :) > > The problem with the first solution is that QWebPage::unsupportedContent is not > emitted by default unless the client application explicitly enables it by > invoking QWebPage::setForwardUnsupportedContent. To me this seems to be an > impedement because the "Content-Disposition" header is a common way web servers > indicate that a resource is supposed to be downloaded and not rendered by > default. > As such, the handling of request that contain the "content-disposition" header > should not be left up to a signal that is only emitted whenever a client > application explicitly enables it. Furthermore, client applications are likely > to handle unsupported content and download requests differently and having a > single signal to deal with these two different issues seems to go against > standard Qt API practise, no ? Anyhow, regardless of the chosen approach, the > signal that gets emitted as result of encountering this header should IMHO need > to be ON by default. Either approach requires changes in the application. The approach of adding an overloaded signal requires us to do _magic_ in WebKit, i.e. detect if someone is connected to the signal and otherwise _cancel_ the QNetworkReply's download. We had _exactly_ that problem with the unsupportedContent signal during the API design and decided against magic, as it tends to get in the programmer's way. So I'm in favour of a solution that is consistent to the existing API, which is why I like your first patch. It can be documented - in the class or module overview - when unsupportedContent() is emitted.
Simon Hausmann
Comment 17 2010-03-22 14:53:24 PDT
Comment on attachment 51199 [details] Content-Disposition support I prefer this approach, but instead of duplicating the code from WebKit/chromium/src/FrameLoaderClientImpl.cpp it should be moved into a common method in WebCore, maybe WebCore/platform/network/HTTPParser.*. Also this patch is missing a ChangeLog.
Simon Hausmann
Comment 18 2010-03-22 14:55:25 PDT
Comment on attachment 51235 [details] Alternate patch as described in the original report... This patch is also missing a ChangeLog. My main issue with it is that the reply isn't cancelled if the application forgets to connect to the right overloaded(!) signal. Despite the goal of mapping the attachment to a download, I think the approach of is more confusing that the first patch.
Dawit A.
Comment 19 2010-03-22 15:36:07 PDT
(In reply to comment #16) > (In reply to comment #9) > > (In reply to comment #3) > > > From an API perspective the first solution sounds better to me than the second > > > one, which I feel may be confusing. Is it just me? :) > > > > The problem with the first solution is that QWebPage::unsupportedContent is not > > emitted by default unless the client application explicitly enables it by > > invoking QWebPage::setForwardUnsupportedContent. To me this seems to be an > > impedement because the "Content-Disposition" header is a common way web servers > > indicate that a resource is supposed to be downloaded and not rendered by > > default. > > As such, the handling of request that contain the "content-disposition" header > > should not be left up to a signal that is only emitted whenever a client > > application explicitly enables it. Furthermore, client applications are likely > > to handle unsupported content and download requests differently and having a > > single signal to deal with these two different issues seems to go against > > standard Qt API practise, no ? Anyhow, regardless of the chosen approach, the > > signal that gets emitted as result of encountering this header should IMHO need > > to be ON by default. > > Either approach requires changes in the application. > > The approach of adding an overloaded signal requires us to do _magic_ in > WebKit, i.e. detect if someone is connected to the signal and otherwise > _cancel_ the QNetworkReply's download. We had _exactly_ that problem with the > unsupportedContent signal during the API design and decided against magic, as > it tends to get in the programmer's way. > > So I'm in favour of a solution that is consistent to the existing API, which is > why I like your first patch. It can be documented - in the class or module > overview - when unsupportedContent() is emitted. Well that is fine by me... The issue can indeed be solved through documenting that unsupportedContent signal is emitted whenever a "content-disposition" header is encountered as well...
Dawit A.
Comment 20 2010-03-22 15:43:19 PDT
(In reply to comment #17) > (From update of attachment 51199 [details]) > I prefer this approach, but instead of duplicating the code from > WebKit/chromium/src/FrameLoaderClientImpl.cpp it should be moved into a common > method in WebCore, maybe WebCore/platform/network/HTTPParser.*. > > Also this patch is missing a ChangeLog. The code that deals with the detecting "content-disposition" can indeed be moved into a common location instead of being duplicated. I did it this way because a.) I did not know where to factor the code out to and more importantly I did not think it would be okay to modify the codebase of another client which we would have to if we factor the code out. Anyhow, I will move the code out to a common location WebCore/platform/network/HTTPParsers.* as you suggest and provide a new patch based on that. I will also provide a ChangeLog entry for my changes...
Dawit A.
Comment 21 2010-03-22 15:50:59 PDT
(In reply to comment #15) > (In reply to comment #10) > > (In reply to comment #8) > > > > Where does one mark the patches with the review flag ? Find the changelog file > > > > though ; so I will go ahead and provide an patch that updates that as well... > > > > > > Click on "details" next to your patch. You can also do it when you attach a new > > > attachment. > > > > > > Please generate ChangeLog using WebKitTools/Script/prepare-ChangeLog > > > > Cannot use this at all since I am working off of a weekly snapshot and not a > > git-clone of the qtwebkit respository. > > Then you'll have to create the ChangeLog entry yourself. > > But why not get a git clone of WebKit or use svn directly? Too slow a connection to clone 1GB repository compare to downloading a 21.4 MB tar ball :) I did not know you can use svn to obtain the sources though. Can you do that ? I did not see anything about that here: https://trac.webkit.org/wiki/QtWebKitContrib
Simon Hausmann
Comment 22 2010-03-22 16:07:16 PDT
(In reply to comment #21) > (In reply to comment #15) > > (In reply to comment #10) > > > (In reply to comment #8) > > > > > Where does one mark the patches with the review flag ? Find the changelog file > > > > > though ; so I will go ahead and provide an patch that updates that as well... > > > > > > > > Click on "details" next to your patch. You can also do it when you attach a new > > > > attachment. > > > > > > > > Please generate ChangeLog using WebKitTools/Script/prepare-ChangeLog > > > > > > Cannot use this at all since I am working off of a weekly snapshot and not a > > > git-clone of the qtwebkit respository. > > > > Then you'll have to create the ChangeLog entry yourself. > > > > But why not get a git clone of WebKit or use svn directly? > > Too slow a connection to clone 1GB repository compare to downloading a 21.4 MB > tar ball :) I did not know you can use svn to obtain the sources though. Can > you do that ? I did not see anything about that here: > > https://trac.webkit.org/wiki/QtWebKitContrib Absolutely. WebKit is still developed in SVN, it just happens that many developers prefer to use git(-svn) on top of it, especially the ones on slow connections :) The normal WebKit instructions apply, see http://webkit.org/building/checkout.html So you can also just get the tarball and continue svn up from there or do a fresh (slow) svn co. Most of the the scripts - prepare-ChangeLog, webkit-patch, etc. - work with svn and git.
Dawit A.
Comment 23 2010-03-22 21:37:04 PDT
Created attachment 51391 [details] Updated Content-Disposition patch... Updated patch with requested changes including appropriate ChangeLog modifications.
WebKit Review Bot
Comment 24 2010-03-22 21:40:46 PDT
Attachment 51391 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Dawit A.
Comment 25 2010-03-22 21:48:41 PDT
Created attachment 51392 [details] Updated Content-Disposition patch II The same patch as 51391 except this one contains changes to the chromium source tree that makes it use the factored out code as well...
WebKit Review Bot
Comment 26 2010-03-22 21:51:14 PDT
Attachment 51392 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Dawit A.
Comment 27 2010-03-24 09:13:56 PDT
Created attachment 51510 [details] Updated Content-Disposition path III Perhaps third time is the charm...
Kenneth Rohde Christiansen
Comment 28 2010-03-24 10:08:53 PDT
Simon, should we make the release block on this?
Dawit A.
Comment 29 2010-03-25 08:53:08 PDT
Created attachment 51644 [details] Updated Content-Disposition patch IV The previous patch 51510 was incorrectly generated against qtwebkit-4.6. This new patch is generated against the qtwebkit-2.0 branch and obsoletes the previous version. Note that nothing of substance was changes from the previous patch.
Simon Hausmann
Comment 30 2010-03-28 13:50:15 PDT
Comment on attachment 51644 [details] Updated Content-Disposition patch IV Patch looks good to me, but before landing we have to preserve Google's copyright when moving the function! Also we still don't have a test for this :-( Dawit, do you want to follow up with a patch that removes the original from WebKit/chromium and makes it use the new function from WebCore? The build bots may help to make sure that your patch actually builds on chromium.
Dawit A.
Comment 31 2010-03-28 17:36:06 PDT
(In reply to comment #30) > (From update of attachment 51644 [details]) > Patch looks good to me, but before landing we have to preserve Google's > copyright when moving the function! > > Also we still don't have a test for this :-( Looked into adding a test case for this, but do not know where to begin. I most definitely need help in adding a test case for this. My test of the patch was exclusively done using kwebkitpart which passes all the tests in the link I gave in my original report. Ofcourse that is not relevant for qtwebkit. Anyhow, I need help with creating the test case... > Dawit, do you want to follow up with a patch that removes the original from > WebKit/chromium and makes it use the new function from WebCore? Actually I provided a patch to chromium's source tree in the 51392 attachment but I retracted it thinking I was not supposed to do that... Anyhow, I most definitely can provide an updated patch with the chromium source tree change ; specially since I have finally relented and cloned the qtwebkit git repository.
Dawit A.
Comment 32 2010-03-28 18:08:38 PDT
Created attachment 51873 [details] Updated Content-Disposition patch V Added the chromium source code changes as requested...
WebKit Review Bot
Comment 33 2010-03-29 00:01:32 PDT
Attachment 51873 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/ChangeLog:13: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:14: Line contains tab character. [whitespace/tab] [5] WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp" WebKit/chromium/ChangeLog:5: Line contains tab character. [whitespace/tab] [5] WebKit/chromium/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] WebKit/chromium/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Total errors found: 5 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Dawit A.
Comment 34 2010-03-29 06:55:48 PDT
Created attachment 51905 [details] Updated Content-Disposition patch VI Guess the bot style checker script is smarter than WebKitTools/Scripts/check-webkit-style since the later did not find any style errors when checking the previous patch. Anyhow, here is the same patch with the style issues fixed and Google's copyright added to HTTPParsers.*
Simon Hausmann
Comment 35 2010-03-29 07:23:25 PDT
Comment on attachment 51905 [details] Updated Content-Disposition patch VI Looks good to me!
Eric Seidel (no email)
Comment 36 2010-03-29 11:37:03 PDT
Comment on attachment 51644 [details] Updated Content-Disposition patch IV Cleared Simon Hausmann's review+ from obsolete attachment 51644 [details] so that this bug does not appear in http://webkit.org/pending-commit.
WebKit Commit Bot
Comment 37 2010-03-29 16:58:09 PDT
Comment on attachment 51905 [details] Updated Content-Disposition patch VI Clearing flags on attachment: 51905 Committed r56750: <http://trac.webkit.org/changeset/56750>
WebKit Commit Bot
Comment 38 2010-03-29 16:58:16 PDT
All reviewed patches have been landed. Closing bug.
Brady Eidson
Comment 39 2010-03-29 18:20:14 PDT
I emailed webkit-dev the following: Since two platforms now share this same code in their WebKit layers, it seems okay that this was moved into some shared location. But I have a few gripes: HTTPParsers is about parsing HTTP, not implementing policy. A better place is probably ResourceResponse itself. 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; ...and then the clients made their own decision based on that. Safari actually does this, for example.
Dawit A.
Comment 40 2010-03-29 19:44:55 PDT
(In reply to comment #39) > I emailed webkit-dev the following: > > Since two platforms now share this same code in their WebKit layers, it seems > okay that this was moved into some shared location. But I have a few gripes: > > HTTPParsers is about parsing HTTP, not implementing policy. A better place is > probably ResourceResponse itself. > > 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 ? Perhaps changing how it is implemented would ease the gripes you have with it ? How about changing it to bool HTTPParsers::isAttachmentContentDisposition(const String&) ; OR enum ContentDispositionType { Inline, Attachment }; int HTTPParsers::contentDispositionType(const String&); instead ? Regards, Dawit A.
Brady Eidson
Comment 41 2010-03-29 20:48:37 PDT
(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.
Dawit A.
Comment 42 2010-03-30 00:09:59 PDT
(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 ?
Simon Hausmann
Comment 43 2010-03-30 04:55:25 PDT
Revision r56750 cherry-picked into qtwebkit-2.0 with commit 46b4cfc4c16f59e402e21c11db057ac28fbd4b02
Brady Eidson
Comment 44 2010-03-30 09:08:48 PDT
> > 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... Good call on the None. > 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 ? New bug, I guess. CC me and I'll quickly review. :)
Note You need to log in before you can comment on or make changes to this bug.