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

• Component: WebKit Qt
• Status: CLOSED FIXED

[Dawit A.]

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.

[Dawit A.]

Created attachment 51235 [details]
Alternate patch as described in the original report...

[Simon Hausmann]

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]

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]

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.]

(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.]

(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]

(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]

> 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.]

(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.]

(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]

Please do not set r+ but r? !

r+ means that it was reviewed positively, but you want to request a review.

[Dawit A.]

(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]

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]

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]

(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]

(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 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 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.]

(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.]

(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.]

(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]

(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.]

Created attachment 51391 [details]
Updated Content-Disposition patch...

Updated patch with requested changes including appropriate ChangeLog modifications.

[WebKit Review Bot]

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.]

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]

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.]

Created attachment 51510 [details]
Updated Content-Disposition path III

Perhaps third time is the charm...

[Kenneth Rohde Christiansen]

Simon, should we make the release block on this?

[Dawit A.]

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 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.]

(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.]

Created attachment 51873 [details]
Updated Content-Disposition patch V

Added the chromium source code changes as requested...

[WebKit Review Bot]

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.]

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 on attachment 51905 [details]
Updated Content-Disposition patch VI

Looks good to me!

[Eric Seidel (no email)]

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

All reviewed patches have been landed.  Closing bug.

[Brady Eidson]

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.]

(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]

(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.]

(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]

Revision r56750 cherry-picked into qtwebkit-2.0 with commit 46b4cfc4c16f59e402e21c11db057ac28fbd4b02

[Brady Eidson]

> > 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.  :)