Bug 73358 - Download page URL should be set by WebCore
Summary: Download page URL should be set by WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-29 13:28 PST by Alexey Proskuryakov
Modified: 2011-11-30 04:13 PST (History)
5 users (show)

See Also:


Attachments
proposed patch (19.26 KB, patch)
2011-11-29 13:35 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2011-11-29 13:28:00 PST
When a file is downloaded, we need to tell NSURLDownload where it came from to have correct quarantine metadata.

Currently, this is done in WebKit, which has several downsides:
- code duplication;
- need to use an SPI;
- it's difficult to pass this data over to WebKit2 UI process.

It would be much easier to just set request's main document URL - CFNetwork uses that for quarantine.
Comment 1 Alexey Proskuryakov 2011-11-29 13:35:07 PST
Created attachment 117036 [details]
proposed patch
Comment 2 Adam Barth 2011-11-29 13:59:16 PST
Comment on attachment 117036 [details]
proposed patch

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

> Source/WebCore/ChangeLog:8
> +        No change in behavior, just refactoring.

Even on non-Mac platforms?
Comment 3 Alexey Proskuryakov 2011-11-29 14:19:30 PST
Yes. Do you think that it changes behavior on other platforms?
Comment 4 Adam Barth 2011-11-29 15:17:41 PST
(In reply to comment #3)
> Yes. Do you think that it changes behavior on other platforms?

I haven't looked at it closely.  I just noticed that the code only existed previously on Mac.

The code itself looks pretty dodgy.  For example:

hostOnlyURLString = makeString(originalURL.protocol(), "://", originalURL.host(), ":", String::number(port));

is making a lot of assumptions about the format of the URL.
Comment 5 Alexey Proskuryakov 2011-11-29 15:39:55 PST
> I just noticed that the code only existed previously on Mac.

Chromium will likely want to use it too if it wants to set file quarantine data on Mac. It has no use for other platforms AFAICT, but also negligible cost.

> The code itself looks pretty dodgy.

Agreed - that's why I added many FIXME comments when moving it from WebKit(s) to WebCore. In this particular case, we shouldn't be constructing a host only URL string in the first place, but it's better than nothing.
Comment 6 Adam Barth 2011-11-29 16:00:45 PST
Thanks for explaining!
Comment 7 WebKit Review Bot 2011-11-30 04:13:48 PST
Comment on attachment 117036 [details]
proposed patch

Clearing flags on attachment: 117036

Committed r101486: <http://trac.webkit.org/changeset/101486>
Comment 8 WebKit Review Bot 2011-11-30 04:13:53 PST
All reviewed patches have been landed.  Closing bug.