Bug 152694 - [WinCairo] Download should use header values from provided request object.
Summary: [WinCairo] Download should use header values from provided request object.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-04 09:58 PST by peavo
Modified: 2016-01-05 03:43 PST (History)
2 users (show)

See Also:

Attachments
Patch (2.09 KB, patch)
2016-01-04 10:03 PST, peavo 		no flags Details | Formatted Diff | Diff
Patch (1.76 KB, patch)
2016-01-04 12:51 PST, peavo 		achristensen: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description peavo 2016-01-04 09:58:52 PST 
We currently ignore the header values in the request object, but we shouldn't do that.
Comment 1 peavo 2016-01-04 10:03:10 PST 
Created attachment 268201 [details]
Patch
Comment 2 Alex Christensen 2016-01-04 10:11:02 PST 
Comment on attachment 268201 [details]
Patch

It would be better to just iterate the existing headers, not all possible headers.  What if there is an unrecognized header?
Comment 3 peavo 2016-01-04 11:18:55 PST 
(In reply to comment #2)
> Comment on attachment 268201 [details]
> Patch
> 
> It would be better to just iterate the existing headers, not all possible
> headers.  What if there is an unrecognized header?

Thanks for looking into this :) As far as I can see, the IWebURLRequest interface does not support iterating over existing headers, but maybe I've missed something? It is possible to get all headers as an IPropertyBag object, but this type only seems to support reading known header names.
Comment 4 Alex Christensen 2016-01-04 11:43:48 PST 
Comment on attachment 268201 [details]
Patch

I think this is a bad approach.  I think this should do something more like WebDownload::initWithRequest in WebDownloadCFNet.cpp instead
Comment 5 peavo 2016-01-04 11:55:44 PST 
(In reply to comment #4)
> Comment on attachment 268201 [details]
> Patch
> 
> I think this is a bad approach.  I think this should do something more like
> WebDownload::initWithRequest in WebDownloadCFNet.cpp instead

Thanks, looks good, I'll update the patch :)
Comment 6 peavo 2016-01-04 12:51:52 PST 
Created attachment 268222 [details]
Patch
Comment 7 Alex Christensen 2016-01-04 13:17:58 PST 
Comment on attachment 268222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268222&action=review

r=me

> Source/WebKit/win/WebDownloadCurl.cpp:105
> +    init(nullptr, resourceRequest, ResourceResponse(), delegate);

this ResourceResponse() might cause some problems.
Comment 8 peavo 2016-01-04 13:23:14 PST 
(In reply to comment #7)
> Comment on attachment 268222 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268222&action=review
> 
> r=me
> 
> > Source/WebKit/win/WebDownloadCurl.cpp:105
> > +    init(nullptr, resourceRequest, ResourceResponse(), delegate);
> 
> this ResourceResponse() might cause some problems.

Thanks for reviewing :) I don't think the ResourceResponse object is used, is that what you mean?
Comment 9 Alex Christensen 2016-01-04 13:25:01 PST 
We might run into some code somewhere where we want to use the ResourceResponse object and it's not a meaningful object.  If we run into this, we'll fix it in the future.
Comment 10 peavo 2016-01-04 13:27:48 PST 
(In reply to comment #9)
> We might run into some code somewhere where we want to use the
> ResourceResponse object and it's not a meaningful object.  If we run into
> this, we'll fix it in the future.

Sounds good.
Comment 11 peavo 2016-01-05 03:43:33 PST 
Committed r194581: <http://trac.webkit.org/changeset/194581>