Enable DOWNLOAD_ATTRIBUTE feature for GTK port, to support download attribute feature for an anchor tag <a download=filename ...>. The feature was introduced on Chromium with r91797 and enabled on BlackBerry with r116265.
Created attachment 175405 [details] Patch
This patch changes cross-platform code, removing [Gtk] from title. I'm not sure if we want to support this in WebKit2. Brady, Sam, what do you think?
I thought it's needed because it's a HTML5 feature: http://dev.w3.org/html5/spec/single-page.html#downloading-resources Is there a special reason for not supporting it in WebKit2?
FWIW, it seems that copy of the spec has all kinds of issues around it's definition of download="". The WHATWG spec has a more complete definition: http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#downloading-resources If there's a problem with the feature, though, please do let me know!
I don't quite feel like digging into w3c or whatwg archives to look at the discussion of why it was added. Why was it added? What is the use case? Most user agents give users a way to decide to download any arbitrary resource, so I'm not entirely sure what value is added by pre-flagging a downloadable resource. Also some user agents support showing "intended for downloading" resources inline and do, treating them as normal navigations. Finally some user agents don't support "downloading for later" at all. It seems weird that content providers need a second way to achieve the same thing that content-disposition does. Especially when content-disposition gets to override the values here.
This is the e-mail that accompanied its addition to the spec last year: http://lists.w3.org/Archives/Public/public-whatwg-archive/2011Jul/0319.html
(In reply to comment #5) > I don't quite feel like digging into w3c or whatwg archives to look at the discussion of why it was added. > > Why was it added? What is the use case? When the author intend to set a default action for a link as download. There are many cases for that such as attached files for blog, email,... > Most user agents give users a way to decide to download any arbitrary resource, so I'm not entirely sure what value is added by pre-flagging a downloadable resource. Difference is the default action for clicking the link is changed. Sometimes authors want to set a default action even for the "html" file like below: <a href="http://www.google.com/index.html" download="google.html">link</a> And, many web pages block the context menu with scripts for security or other reasons. (ex> http://blog.naver.com/agapeuni?Redirect=Log&logNo=60105728697) In that case, only the default action can be executed for a specific link. > Also some user agents support showing "intended for downloading" resources inline and do, treating them as normal navigations. > > Finally some user agents don't support "downloading for later" at all. > > It seems weird that content providers need a second way to achieve the same thing that content-disposition does. Especially when content-disposition gets to override the values here. I don't think it's a second way. It's to force to download resources that are usually not downloaded when the link is clicked.
(In reply to comment #7) > (In reply to comment #5) > > Most user agents give users a way to decide to download any arbitrary resource, so I'm not entirely sure what value is added by pre-flagging a downloadable resource. > > Difference is the default action for clicking the link is changed. The default action is now *implied* but it has not actually changed. > Sometimes authors want to set a default action even for the "html" file like below: > <a href="http://www.google.com/index.html" download="google.html">link</a> Which is advisory, not compulsory. > And, many web pages block the context menu with scripts for security or other reasons. > (ex> http://blog.naver.com/agapeuni?Redirect=Log&logNo=60105728697) > In that case, only the default action can be executed for a specific link. There is no way that I know of for a page to block a voluntary download action, *especially* in a world of extensions. > > Also some user agents support showing "intended for downloading" resources inline and do, treating them as normal navigations. This change neglects the fact that the user agent might want to show the resource inline even though the <a> was marked with the download attritbute. > > > > Finally some user agents don't support "downloading for later" at all. > > > > It seems weird that content providers need a second way to achieve the same thing that content-disposition does. Especially when content-disposition gets to override the values here. > > I don't think it's a second way. It's to force to download resources that are usually not downloaded when the link is clicked. I have two problems with this statement. 1 - It is absolutely not to *force* the download of a resource. Nothing in the spec *requires* the user agent to download. It is supposedly a "hint". 2 - It is a second way. There is already a way to hint to browsers that something should be a download and that is the content-disposition header. The spec even says that if an <a download> was clicked and the response includes a content-disposition header that the response header should take precedence. But anyways, these points aside: With the change you propose WebKit would not be up to spec because there's no code enforcing the "in cross-origin situations" clause of the spec. Also we shouldn't accept changes that purport to be about implementing the spec without layout tests. Historically downloads have been 100% a browser feature and not at all a browser-engine feature. Since this is HTML's first real attempt at defining download behavior it's the first time we've had the need to test the engine's download behavior... which means someone will need to augment layout test infrastructure to be able to catch regressions.
(In reply to comment #8) > Historically downloads have been 100% a browser feature and not at all a browser-engine feature. Since this is HTML's first real attempt at defining download behavior it's the first time we've had the need to test the engine's download behavior... which means someone will need to augment layout test infrastructure to be able to catch regressions. I see that there are some layout tests about detecting <a download>. I'm not convinced they are actually testing what they'd need to be testing to cover this. Looking at them more closely now.
I don't believe the existing layouttests test the right thing. I see nothing that actually tests whether or not the result is downloaded as opposed to navigated to. I see nothing relating to policy delegate callbacks that determine what will actually happen with the click. I see nothing related the cross-origin requirement in the spec.
Comment on attachment 175405 [details] Patch I don't think this feature is in good enough shape to be making changes to cross platform code, especially when that code isn't solely contained to the feature. As-is we treat the click on such a link as an *order* to the user agent to download. We need this to be a *REQUEST* for the user agent to download. We need to implement the cross-origin protection described in the spec, and *should* implement the "content-disposition header can overwrite the filename" part of the spec. Doing both of those means starting the load as a normal main resource load and waiting until the response headers come in. We should pipe it through the normal policy delegate callbacks at that point. The change there needs to be that the policy delegate knows this was *requested* to be a download with a certain filename, but the user agent still gets to decide. And all of those things said, we need layouttest coverage that monitors the policy delegate's actual decision instead of assuming that a click on one of these should automatically be a download.
With all of those changes in place, we'd turn this on in our port and I doubt any WK2 port would have a problem with it. Until then, we should not affect the "normal" download mechanism for the ports that don't like the feature in its current state.
1. "force" vs "hint" (In reply to comment #8) Ian Hickson, could you please reply what is right? I thought it's closer to "force" because the implementation of Chrome on the bug 64580 forces the link to download, and the mail thread Ian shared said <a download=filename.txt> would have the effect of adding (or overriding) the header "Content-Disposition: attachment;filename=filename.txt". https://bugs.webkit.org/show_bug.cgi?id=64580 http://lists.w3.org/Archives/Public/public-whatwg-archive/2011Jul/0319.html 2. Layout Test (In reply to comment #9) The 4 layout tests was introduced with the bug 64508, and I agree with it can't exactly test the download operation. But I think because it's the feature and layout tests that are used on the othre ports (Chrome, BlackBerry), it could be used for now, before the limitation of layout tests solved. 3. Cross-Origin issue (In reply to comment #10) It's already discussed and applied with bug 64580. Source code for that is on Source/WebCore/html/HTMLAnchorElement.cpp:511. I'll modify the 'StartDownload' message to pass 'ResourceRequest' to support that, and test about the coross-origin issue before uploading new patch. 4. Modifying the Patch (In reply to comment #11) I'll use the DOWNLOAD_ATTRIBUTE option for modified sources for not affect other ports, and upload it soon. Thank you.
(In reply to comment #13) > 1. "force" vs "hint" (In reply to comment #8) > Ian Hickson, could you please reply what is right? My point is that nothing should ever *REQUIRE* the user agent to save a file to the user's local file system (i.e. - download) > I thought it's closer to "force" because the implementation of Chrome on the bug 64580 forces the link to download, Chrome chose for a click on one of these links whose default was not prevented to always result in beginning a download. That's a choice Chrome can choose as a user agent, but not something the engine should impose. > and the mail thread Ian shared said <a download=filename.txt> would have the effect of adding (or overriding) the header "Content-Disposition: attachment;filename=filename.txt". In the spec it is precisely the other way around (and should be, IMHO): "The attribute can furthermore be given a value, to specify the filename that user agents are to use when storing the resource in a file system. This value can be overridden by the Content-Disposition HTTP header's filename parameters." > 2. Layout Test (In reply to comment #9) > The 4 layout tests was introduced with the bug 64508, and I agree with it can't exactly test the download operation. > But I think because it's the feature and layout tests that are used on the othre ports (Chrome, BlackBerry), > it could be used for now, before the limitation of layout tests solved. But you are changing cross platform downloads-related code, not only code hidden behind the enabler for this feature. > 3. Cross-Origin issue (In reply to comment #10) > It's already discussed and applied with bug 64580. > Source code for that is on Source/WebCore/html/HTMLAnchorElement.cpp:511. You're thinking of a different cross-origin issue... > I'll modify the 'StartDownload' message to pass 'ResourceRequest' to support that, > and test about the coross-origin issue before uploading new patch. But the cross-origin issue I'm referring to is explicitly called out in the spec: "In cross-origin situations, the download attribute has to be combined with the Content-Disposition HTTP header, specifically with the attachment disposition type, to avoid the user being warned of possibly nefarious activity." That requirement cannot possibly be met until after the connection has been made to the server, as we cannot possibly have received the Content-Disposition HTTP header until we're getting the reply from the server. All of the code in place to support this feature as-is is *pre-request* code, but resolving this issue requires the connection to be made in the first place before we know we can commit to a cross-origin download.
(In reply to comment #14) > But the cross-origin issue I'm referring to is explicitly called out in the spec: > > "In cross-origin situations, the download attribute has to be combined with the Content-Disposition HTTP header, specifically with the attachment disposition type, to avoid the user being warned of possibly nefarious activity." > > That requirement cannot possibly be met until after the connection has been made to the server, as we cannot possibly have received the Content-Disposition HTTP header until we're getting the reply from the server. The spec provides more clarity on this, too. Reading http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#downloading-resources there are steps "1 through 6" for the pre-request part of the algortihm, but steps "1 through 20" on the response part of this. I can't find where those steps are addressed in WebKit, and they are important to us.
Created attachment 175738 [details] Patch
Comment on attachment 175738 [details] Patch Attachment 175738 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14937619
(In reply to comment #13) > 1. "force" vs "hint" (In reply to comment #8) > Ian Hickson, could you please reply what is right? Please read the spec, as Brady has. Ignore the e-mail threads when it comes to determining the actual requirements on implementors, they are only useful for spec archeology to determine reasoning, they don't have any normative value. The spec, and only the spec, has the "official" answer. (And if the spec is bad, then e-mail the list to have it changed. :-) )
The current callers of FrameLoaderClient::startDownload do so in direct response to receiving a PolicyDownload callback from a platform PolicyDelegate. The current implementation of this in WebKit blindly starts a download by default without consulting the policy delegate. It also starts the download based on the request and doesn't take in to account the response headers meaning it cannot possibly verify the trustedness of the operation based on the content-disposition header. Those two reasons are why I think the current implementation is folly. Here's how I think this feature should work in WebKit: When the user clicks an <a download> link WebCore should start a main resource load as normal. The difference is that it remembers the contexts that the load was started from <a download> and including the specified filename. Then FrameLoaderClient::dispatchDecidePolicyForResponse() is augmented. It now includes the context of whether the load came from an <a download> click, what the <a download> suggested filename is, and whether the operation is trusted from the "select a filename" algorithm. While user agents will still freely be able to ignore any red flags and perform the download, I still think it should be determined by the engine and explicitly included since it is in the spec. If the user-agent decides to go forward with the download anyways, then fine. Qt, Blackberry, Chrome, whoever else - That's fine. The engine has done its job interpreting the spec. We probably won't choose to go ahead with it if the red flags are up.
(In reply to comment #19) > The current callers of FrameLoaderClient::startDownload do so in direct response to receiving a PolicyDownload callback from a platform PolicyDelegate. > > The current implementation of this in WebKit blindly starts a download by default without consulting the policy delegate. It also starts the download based on the request and doesn't take in to account the response headers meaning it cannot possibly verify the trustedness of the operation based on the content-disposition header. I thought the operation with <a download> is similar with the operation for "download linked file" and make the implementation similar with it. So, I think the implementation of "download linked file" has same issues now, but is it OK because it's done by user's action? > When the user clicks an <a download> link WebCore should start a main resource load as normal. The difference is that it remembers the contexts that the load was started from <a download> and including the specified filename. For this, the current implementation on WebCore with bug 64580 seems need to be changed. Is it what you want?
(In reply to comment #20) > (In reply to comment #19) > > The current callers of FrameLoaderClient::startDownload do so in direct response to receiving a PolicyDownload callback from a platform PolicyDelegate. > > > > The current implementation of this in WebKit blindly starts a download by default without consulting the policy delegate. It also starts the download based on the request and doesn't take in to account the response headers meaning it cannot possibly verify the trustedness of the operation based on the content-disposition header. > > I thought the operation with <a download> is similar with the operation for "download linked file" and make the implementation similar with it. > So, I think the implementation of "download linked file" has same issues now, but is it OK because it's done by user's action? Correct, "download linked file" is a user action. "clicking a link" is also a user action, but not one with the built in expectation that "this will download". > > > When the user clicks an <a download> link WebCore should start a main resource load as normal. The difference is that it remembers the contexts that the load was started from <a download> and including the specified filename. > > For this, the current implementation on WebCore with bug 64580 seems need to be changed. Is it what you want? Well certainly 64580 is related to what I'm suggesting here. This bug could still be resolved with just remembering the context that an <a download> was clicked. 64580 could add the attributed filename to that context.
Comment on attachment 175738 [details] Patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.
(In reply to comment #8) > 2 - It is a second way. There is already a way to hint to browsers that something should be a download and that is the content-disposition header. That's not possible with generated content (e.g. from new Blob(...)). What is the current recommended practice for saving JS generated content with a filename in Safari?
If I understand it correctly, throughout this 20-months-old discussion the focus was "why do we need this?" and "what do the specs say?" before it died (presumably because the first questions was not answered satisfactorily and a specification in itself is a low motivation factor) As comment #23 (previous) also tried to point out, noone seemed to address the download of Blob URLs - which don't have any Content-Disposition headers. I can imagine that one of the main reasons for various user agents to implement <a download> "on their own" is just this very use case (BTW, I guess that would be FF, Chrome, Opera, Android, and BB according to http://caniuse.com/#feat=download ). The only references to this use case I could find in this bug tracker at all are bug 96462, comment 10 and bug 114865 (which I'm not so sure I even understood). So, trying to understand the ins and outs, if I would like to see this feature implemented in Webkit (and thus Safari, I hope), where would I go if not right here? Did I not search well? Should I rather try to file something in the Apple Bug Reporter? :) Thank you for your time and your effort to create great software.
(In reply to comment #24) > If I understand it correctly, throughout this 20-months-old discussion the focus was "why do we need this?" and "what do the specs say?" before it died (presumably because the first questions was not answered satisfactorily and a specification in itself is a low motivation factor) Where the discussion left off was "If we're going to do this, we have to do this the right way", and the attached patch is definitely not the right way. I think comment #19 lays out the roadmap. > So, trying to understand the ins and outs, if I would like to see this feature implemented in Webkit (and thus Safari, I hope), where would I go if not right here? You can go right here. And if you really want to champion this feature, then take the patch and actually fix the problems with it. At this point, since it's a spec'd feature and other browsers support it and it obviously has at least one use case, we wouldn't say no to a good implementation.
Note that security considerations haven't disappeared, see e.g. <http://lcamtuf.blogspot.ru/2014/03/messing-around-with-download.html>. As far as saving Blobs goes, <a download> is a weird hacky way to support it, something like FileWriter may make more sense (but the spec hasn't been finished, <http://www.w3.org/TR/file-writer-api/>). Given how hopelessly broken blob URLs are in general, we should probably look for ways to further limit these, not to make them more usable at this point.
(In reply to comment #26) You can already do everything on that page using Content-Disposition: attachment and onmousedown handlers that change the URL of the link after clicking. The security impact of a same-origin-limited download attribute (as implemented by Firefox) is exactly the same as allowing Content-Disposition: attachment to trigger downloads instead of ignoring the header. If you consider this a security vulnerability, you should be advocating the removal of Content-Disposition support. > Given how hopelessly broken blob URLs are in general How so? I maintain the FileSaver.js repo, and of the hundreds of issues I've had over the course of multiple years, nobody has ever had any issues with blobs URLs on any browsers except Safari. The only thing that's broken about blob URLs is Safari's attempts at limiting it's usefulness for triggering downloads of generated content resulting in a bunch of issues[1]. Instead of in general, are you talking about Safari's specific implementation? I would agree with you there. [1]: https://github.com/eligrey/FileSaver.js/issues/12#issuecomment-47247096
> The security impact of a same-origin-limited download attribute (as implemented by Firefox) is exactly the same as allowing Content-Disposition: attachment to trigger downloads instead of ignoring the header. If you consider this a security vulnerability, you should be advocating the removal of Content-Disposition support. So we have a spec for the download attribute, and a non-standard implementation in Firefox, apparently because Mozilla folks think that the standard is not good enough to implement. That's an unsettled security consideration. The current version of the spec has language like "This last step would make it impossible to download executables, which might not be desirable. As always, implementors are forced to balance security and usability in this matter." That's an explicitly unsettled security consideration. I said that "that security considerations haven't disappeared". What exactly in this comment did you disagree with? Thank you for telling me what I should do, by the way. > How so? I maintain the FileSaver.js repo, and of the hundreds of issues I've had over the course of multiple years, nobody has ever had any issues with blobs URLs on any browsers except Safari. Blob URLs still don't have a good enough lifetime model, meaning that it's very difficult to write a web application that doesn't leak huge amounts of data through these, and yet doesn't hit stale URLs. The spec has introduced several incompatible ideas for auto-revocation over the years, but they all ignore the fact the the browser engine may re-fetch any resources multiple times. As a result, using auto-revoking blob URLs for resources is very fragile - and difficult to debug, as the reasons for re-fetching differ from browser to browser, and may include hard to control situations like memory pressure, visibility state etc. > Instead of in general, are you talking about Safari's specific implementation? I would agree with you there. Are you aware of any issues in WebKit implementation of Blobs URLs that are not directly mirroring conceptual problems with these, or just trolling?
(In reply to comment #28) > Are you aware of any issues in WebKit implementation of Blobs URLs that are not directly mirroring conceptual problems with these, or just trolling? Yes, I linked you[1] to a comment describing a bug that breaks blob URLs entirely in WebKit. Here is a testcase someone wrote: http://jsfiddle.net/24FhL/ [1]: https://github.com/eligrey/FileSaver.js/issues/12#issuecomment-47247096
OK, I thought that you had some brokenness in mind that was unrelated to downloading. It was a while since I last checked what the expected behavior for opening a new window with a blob URL was - but at that time, it was essentially undefined in the spec, whether it was for downloading or not. It's not obvious, because a lot of Blob URL functionality is tied to a global object that was used for creating it, and the new window has a new unrelated global object. Actually, looks like it's still undefined - the File API spec says that the definition is in Fetch spec, and the Fetch spec only says "It has been argued this should be handled outside of fetching." If non-normative text in File API spec is to be trusted, then opening a Blob URL in a new window must never work - it does not have an identifier in the Blob URL Store of the new blank global object, so a network error should be reported.
I don't think opening a new window is the actual issue here (Eli, correct me if I'm wrong). Attempting to open the Blob URL in a new window is a hack to try to get around the missing download attribute. Here's the situation as I see it: 1) Client-side generation of data is important now, and is going to become more so in the future. This is especially true now that we have things like Emscripten that can compile arbitrary applications -- even quite large ones -- into Javascript and have them run within the browser at somewhat usable speed. 2) To be maximally useful, there needs to be a way to let the users save their browser-generated data on their own machines. This is trivial if the browser supports the download attribute. Internet Explorer does not, but does have a (non-standard) msSaveBlob() method that lets you achieve a similar effect. Safari stands alone among the major browsers in providing no mechanism to save a Blob to the local file system using a file name. The only way (that I know of) to make such an application work on Safari is to add an otherwise useless server round-trip. This gains nothing whatsoever in terms of security (the Blob doesn't magically become safe just because it's been posted to the server and echoed back, right?). It just makes the application slow, eats bandwidth, and requires some type of active processing (and the associated load) on the server side. In a browser that does support the download attribute, the entire application can be served as static HTML/JS. Talking about the shortcomings of Blobs isn't really addressing the issue, IMO. For good or ill, Blobs are what we have. The question is whether Safari will, or will not, support real productivity applications that run in the browser. https://github.com/kripken/emscripten/wiki/Porting-Examples-and-Demos
+1. This issue is causing our client-side generated exports to not be downloaded on Safari. CSV content is visible in a new tab as raw text but users still have to manually save it. Javascript generated XSLX files show up as a blank tab and the user has to switch to Chrome or Firefox to export these. This one issue is causing many of our users to simply switch away from Safari altogether, towards Chrome and Firefox.
Same issue here. cycle.travel/map generates GPX and TCX format cycle routes in the client. Safari users find that the file displays rather than downloading. At present I'm faced with reimplementing a whole tier of clientside logic on the server just for Safari.
I'm not clear if the debate on if this should be implemented or not is still going on, but for my part as a library author, I will need to disable the ability to create and download files (CSV, XLSX) in Safari using HTML / JS methods and instead fallback to Flash. This will make Safari the only current browser that required this option.
(In reply to comment #34) > I'm not clear if the debate on if this should be implemented or not is still > going on, but for my part as a library author, I will need to disable the > ability to create and download files (CSV, XLSX) in Safari using HTML / JS > methods and instead fallback to Flash. This will make Safari the only > current browser that required this option. I think comment #25 pretty conclusively says there's no debate on whether or not we should do this. The reason we haven't taken this patch is because it is wrong. Again alluded to by comment #25 and comment #19.
(In reply to comment #35) Thanks for the clarification Brady. The current incorrect patch is from 2012. Do you have a sense of whether this issue might get some priority again? It is just that it is a real pain point for our users that use Safari. We are in the same boat as many on this thread wanting to enable client-side data generation of files like csv, xlsx etc. It also limits functionality when we go offline with caching. We use Eli Grey's FileSaver.js so his issues are our issues :)
(In reply to comment #36) > (In reply to comment #35) > > Do you have a sense of whether this issue might get some priority again? WebKit, as a project, doesn't have any "priorities", really. Any individual contributor could take this patch, take my suggestions, and add this feature. As long as the patch was correct, we'd have no reason to turn it down, and every reason to take it.
(In reply to comment #37) > (In reply to comment #36) > > (In reply to comment #35) > > > WebKit, as a project, doesn't have any "priorities", really. To clarify, there's obviously focus areas that some contributors spend their time on, and obviously overall directions for the project. But we'd never say "the core contributors are busy working on other stuff, therefore nobody can work on this" It's all up to whatever scratches your individual itch.
(In reply to comment #38) Thanks Brady, totally understand that. We would love to contribute ( and do on other projects ) but don't have the skill set here. Are there ways that we could support someone with the right skills to do this to the right specs? I hate people that complain from the sidelines, so could we get some skin in the game by offering a bounty or reward so we can show our commitment? I don't think any open source project should turn into "buy your feature here" but actions ( or gestures ) speak louder than words. This could be as simple as offering to buy someone some beer or more serious like some money in the kitty for someone's time. I'm sure others on this thread might also be willing to chip in, just asking :)
I have already lost a couple of days trying to solve this issue. Safari crashes and closes down. All work is lost. In the end, I have decided to disable PDF generation when using Safari and to put a message on my web that states: "Best viewed on Chrome and Firefox". I would really appreciate if you solve this bug. It is really annoying.
> Safari crashes and closes down Could you please file a separate bug about this? That's something else, there is no way a missing feature would cause the crashes that you observe.
Apple, this bug is 3 years old. Why can't safari follow the same user experience as other browsers, for downloading files?
Would you be willing to provide a fix? WebKit is an open source project, so everyone is welcome to make fixes and improvements.
I'm not familiar with webkit. But I'd be more than happy to help. Like Simon Lampen suggested, I would also be happy to donate money if possible.
<rdar://problem/13177492>
Created attachment 274372 [details] Patch
Attachment 274372 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:9: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
This obviously needs tests and other work. But I'd like to get feedback on whether I plumbed the stuff together in a reasonable way.
Comment on attachment 274372 [details] Patch Attachment 274372 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/997659 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html js/dom/dom-static-property-for-in-iteration.html imported/w3c/web-platform-tests/html/dom/reflection-text.html
Created attachment 274378 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 274372 [details] Patch Attachment 274372 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/997663 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html js/dom/dom-static-property-for-in-iteration.html imported/w3c/web-platform-tests/html/dom/reflection-text.html
Created attachment 274379 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 274372 [details] Patch Attachment 274372 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/997657 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html imported/w3c/web-platform-tests/html/dom/reflection-text.html
Created attachment 274380 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 274372 [details] Patch Attachment 274372 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/997695 New failing tests: js/dom/dom-static-property-for-in-iteration.html
Created attachment 274386 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 275125 [details] Patch
Attachment 275125 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:9: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 275125 [details] Patch Attachment 275125 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1065053 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html imported/w3c/web-platform-tests/html/dom/reflection-text.html
Created attachment 275134 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 275125 [details] Patch Attachment 275125 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1065059 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html imported/w3c/web-platform-tests/html/dom/reflection-text.html
Created attachment 275135 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 275125 [details] Patch Attachment 275125 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1065550 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html imported/w3c/web-platform-tests/html/dom/reflection-text.html
Created attachment 275144 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Created attachment 275154 [details] Patch (Tests Fixes)
Comment on attachment 275154 [details] Patch (Tests Fixes) Attachment 275154 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1066211 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html
Created attachment 275161 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 275154 [details] Patch (Tests Fixes) View in context: https://bugs.webkit.org/attachment.cgi?id=275154&action=review This can be made a bit simpler by using a “suggested filename” argument, but no separate “has download attribute” argument, and defaulting the suggested filename to the null string. The presence of the suggested filename can also mean “should download”. I think this can be done fairly cleanly and cut out quite a bit of code. If we really do need to carry the “has download attribute” boolean separately rather than having the presence of the filename imply the request to download, I think it should be called something like “should download” or “is download requested”, since it should be named based on what we want the code to do, not the particular way the DOM told us to do it. Looks like imported/w3c/web-platform-tests/html/dom/interfaces.html is failing on iOS simulator. > Source/WebCore/html/HTMLAnchorElement.cpp:566 > + frame->loader().urlSelected(kurl, target(), event, LockHistory::No, LockBackForwardList::No, hasRel(RelationNoReferrer) ? NeverSendReferrer : MaybeSendReferrer, document().shouldOpenExternalURLsPolicyToPropagate(), HasDownloadAttribute::Yes, fastGetAttribute(downloadAttr)); A more natural way to do this would be to use a null AtomicString to mean "no suggested filename". That’s how getAttribute itself works. Then we would not need HasDownloadAttribute::Yes at all nor would we need the hasAttribute(downloadAttr) check. > Source/WebCore/loader/NavigationAction.h:50 > + NavigationAction(const ResourceRequest&, NavigationType, Event*, ShouldOpenExternalURLsPolicy, HasDownloadAttribute, const String&); Not clear what the string is without an argument name, so should have an argument name. > Source/WebCore/loader/NavigationAction.h:54 > + NavigationAction(const ResourceRequest&, FrameLoadType, bool isFormSubmission, Event*, ShouldOpenExternalURLsPolicy, HasDownloadAttribute, const String&); Not clear what the string is without an argument name, so should have an argument name. > Source/WebCore/loader/PolicyChecker.h:87 > + void continueAfterNavigationPolicy(PolicyAction, const String& suggestedFilename = String()); I think that { } is slightly more attractive than String() as a way to give a default null string argument. Same suggestion for many other call sites below. > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:80 > + void startDownload(WebCore::SessionID, DownloadID, const WebCore::ResourceRequest&, const String& suggestedName = String()); Ditto. > Source/WebKit2/UIProcess/WebPageProxy.h:1813 > + bool m_hasDownloadAttribute { false }; A webpage does not have a download attribute, so the name is not clear. This is state about the a current policy decision that is in flight, not state of the webpage. Setting this state on the web page proxy to carry the argument from decidePolicyForNavigationAction to receivedPolicyDecision is an ugly way to do things. Can we put this state into some object with a lifetime that is scoped to the policy decision instead of storing it as global state? If not, then we at least need to name the data member in a way that makes it clearer what it means. I suppose the closest example is m_shouldSuppressAppLinksInNextNavigationPolicyDecision but it’s not even quite the same because it is cleared out as soon as we ask for the policy decision, not expected to survive while waiting for the decision, which could take an arbitrarily long time.
Comment on attachment 275154 [details] Patch (Tests Fixes) View in context: https://bugs.webkit.org/attachment.cgi?id=275154&action=review >> Source/WebCore/html/HTMLAnchorElement.cpp:566 >> + frame->loader().urlSelected(kurl, target(), event, LockHistory::No, LockBackForwardList::No, hasRel(RelationNoReferrer) ? NeverSendReferrer : MaybeSendReferrer, document().shouldOpenExternalURLsPolicyToPropagate(), HasDownloadAttribute::Yes, fastGetAttribute(downloadAttr)); > > A more natural way to do this would be to use a null AtomicString to mean "no suggested filename". That’s how getAttribute itself works. Then we would not need HasDownloadAttribute::Yes at all nor would we need the hasAttribute(downloadAttr) check. 'download' does not require an argument, so we may have the "download" attribute (signifying the link is meant to be downloaded) but no filename (signifying the author doesn't have a preferred name). We only want to take the "downloading" branch in the case the site author intended downloading to be the preferred interaction for the link. What I really want is a tri-state value for "fastGetAttribute" that is NULL, emptyString, someString. In that case, it could work the way your propose. Or was that your point? That 'fastGetAttribute' will work the way I am hoping here? >> Source/WebCore/loader/NavigationAction.h:50 >> + NavigationAction(const ResourceRequest&, NavigationType, Event*, ShouldOpenExternalURLsPolicy, HasDownloadAttribute, const String&); > > Not clear what the string is without an argument name, so should have an argument name. Will do. >> Source/WebCore/loader/NavigationAction.h:54 >> + NavigationAction(const ResourceRequest&, FrameLoadType, bool isFormSubmission, Event*, ShouldOpenExternalURLsPolicy, HasDownloadAttribute, const String&); > > Not clear what the string is without an argument name, so should have an argument name. Ditto. >> Source/WebCore/loader/PolicyChecker.h:87 >> + void continueAfterNavigationPolicy(PolicyAction, const String& suggestedFilename = String()); > > I think that { } is slightly more attractive than String() as a way to give a default null string argument. Same suggestion for many other call sites below. OK. >> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:80 >> + void startDownload(WebCore::SessionID, DownloadID, const WebCore::ResourceRequest&, const String& suggestedName = String()); > > Ditto. Ditto! :-) >> Source/WebKit2/UIProcess/WebPageProxy.h:1813 >> + bool m_hasDownloadAttribute { false }; > > A webpage does not have a download attribute, so the name is not clear. This is state about the a current policy decision that is in flight, not state of the webpage. Setting this state on the web page proxy to carry the argument from decidePolicyForNavigationAction to receivedPolicyDecision is an ugly way to do things. Can we put this state into some object with a lifetime that is scoped to the policy decision instead of storing it as global state? If not, then we at least need to name the data member in a way that makes it clearer what it means. I suppose the closest example is m_shouldSuppressAppLinksInNextNavigationPolicyDecision but it’s not even quite the same because it is cleared out as soon as we ask for the policy decision, not expected to survive while waiting for the decision, which could take an arbitrarily long time. Hmm. Let me think about this.
(In reply to comment #69) > Comment on attachment 275154 [details] > Patch (Tests Fixes) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275154&action=review > > >> Source/WebCore/html/HTMLAnchorElement.cpp:566 > > A more natural way to do this would be to use a null AtomicString to mean "no suggested filename". That’s how getAttribute itself works. ... > > What I really want is a tri-state value for "fastGetAttribute" that is NULL, > emptyString, someString. In that case, it could work the way your propose. > > Or was that your point? That 'fastGetAttribute' will work the way I am > hoping here? A little experimentation shows that it does work the way I wanted. So I'll make that change, too!
(In reply to comment #69) > Comment on attachment 275154 [details] > Patch (Tests Fixes) > >> Source/WebKit2/UIProcess/WebPageProxy.h:1813 > >> + bool m_hasDownloadAttribute { false }; > > > > A webpage does not have a download attribute, so the name is not clear. This is state about the a current policy decision that is in flight, not state of the webpage. Setting this state on the web page proxy to carry the argument from decidePolicyForNavigationAction to receivedPolicyDecision is an ugly way to do things. > > Hmm. Let me think about this. I realize now that I don't need to handle this here. It can be much more naturally handled in decidePolicyForNavigationAction, which already has all the information is needs to decide whether to "use" the link, or "download" it. So these changes make the patch simpler and smaller!
Created attachment 275251 [details] Patch (After Darin Review)
I've updated the patch to address Darin's suggestions, and updated the iOS test expectations.
Comment on attachment 275251 [details] Patch (After Darin Review) View in context: https://bugs.webkit.org/attachment.cgi?id=275251&action=review Not sure why we mix suggestedName with downloadAttribute. I wasn’t able to see a pattern in where the name shift occurs. > Source/WebCore/html/HTMLAnchorElement.cpp:567 > + auto downloadAttribute = nullAtom Missing semicolon here. I think that’s why the GTK build is failing. > Source/WebKit2/NetworkProcess/Downloads/Download.h:79 > + Download(DownloadManager&, DownloadID, NSURLSessionDownloadTask*, const String& = { }); Should add argument name here. > Source/WebKit2/Shared/NavigationActionData.h:49 > + WTF::AtomicString downloadAttribute; Seems a shame to turn this into an atomic string when shipping it across the process boundary, even though it is an AtomicString inside the web process. No real reason we need to make it atomic. > Source/WebKit2/UIProcess/Downloads/DownloadProxy.cpp:201 > + if (!m_downloadAttribute.isEmpty()) Here you use isEmpty rather than isNull. On the one hand that does seem fine since an empty string is not a good filename, but there are many other bad filenames too, like "/", and so it’s not clear why we need to handle empty string specially. Only need isNull to properly support the "no attribute" case. Unless maybe encoding and decoding a null string turns it into an empty string.
Comment on attachment 275251 [details] Patch (After Darin Review) View in context: https://bugs.webkit.org/attachment.cgi?id=275251&action=review >> Source/WebCore/html/HTMLAnchorElement.cpp:567 >> + auto downloadAttribute = nullAtom > > Missing semicolon here. I think that’s why the GTK build is failing. Oh for goodness' sake. :-( >> Source/WebKit2/NetworkProcess/Downloads/Download.h:79 >> + Download(DownloadManager&, DownloadID, NSURLSessionDownloadTask*, const String& = { }); > > Should add argument name here. Will do! >> Source/WebKit2/Shared/NavigationActionData.h:49 >> + WTF::AtomicString downloadAttribute; > > Seems a shame to turn this into an atomic string when shipping it across the process boundary, even though it is an AtomicString inside the web process. No real reason we need to make it atomic. OK! >> Source/WebKit2/UIProcess/Downloads/DownloadProxy.cpp:201 >> + if (!m_downloadAttribute.isEmpty()) > > Here you use isEmpty rather than isNull. On the one hand that does seem fine since an empty string is not a good filename, but there are many other bad filenames too, like "/", and so it’s not clear why we need to handle empty string specially. Only need isNull to properly support the "no attribute" case. Unless maybe encoding and decoding a null string turns it into an empty string. You are right -- this was a mistake. I'm double-checking that I didn't make this same mistake anywhere else. It seems like Encoding/decoding a null string into an empty string would be a bug.
Comment on attachment 275154 [details] Patch (Tests Fixes) View in context: https://bugs.webkit.org/attachment.cgi?id=275154&action=review >>>> Source/WebKit2/UIProcess/WebPageProxy.h:1813 >>>> + bool m_hasDownloadAttribute { false }; >>> >>> A webpage does not have a download attribute, so the name is not clear. This is state about the a current policy decision that is in flight, not state of the webpage. Setting this state on the web page proxy to carry the argument from decidePolicyForNavigationAction to receivedPolicyDecision is an ugly way to do things. Can we put this state into some object with a lifetime that is scoped to the policy decision instead of storing it as global state? If not, then we at least need to name the data member in a way that makes it clearer what it means. I suppose the closest example is m_shouldSuppressAppLinksInNextNavigationPolicyDecision but it’s not even quite the same because it is cleared out as soon as we ask for the policy decision, not expected to survive while waiting for the decision, which could take an arbitrarily long time. >> >> Hmm. Let me think about this. > > I realize now that I don't need to handle this here. It can be much more naturally handled in decidePolicyForNavigationAction, which already has all the information is needs to decide whether to "use" the link, or "download" it. > > So these changes make the patch simpler and smaller! Uh oh. I remember why I did this the original way; some clients of WebKit don't pass through 'decidePolicyForNavigation' and don't realize they are downloading. This needs a little more work.
Created attachment 275269 [details] Patch (Double-check tests)
Comment on attachment 275269 [details] Patch (Double-check tests) Revised patch a final (hopefully) time to make sure EWS is happy, and to re-introduce WebPageProxy state needed to work with some client applications. This is being handled similarly to the way as m_syncNavigationActionPolicyAction and m_syncNavigationActionPolicyDownloadID are, so I've named the value "m_syncNavigationActionHasDownloadAttribute".
Comment on attachment 275269 [details] Patch (Double-check tests) Tests all pass with this revision. Will land this final patch.
Committed r198893: <http://trac.webkit.org/changeset/198893>
I know its not really the done thing to comment after bugs are fixed, but this is fantastic to have this feature supported in WebKit. Taking the liberty to speak for the others who have commented on this bug as well as myself: thanks so much for your work on this Brent!
(In reply to comment #81) > I know its not really the done thing to comment after bugs are fixed, but > this is fantastic to have this feature supported in WebKit. Taking the > liberty to speak for the others who have commented on this bug as well as > myself: thanks so much for your work on this Brent! Before you get too excited, you should follow the work at Bug 156056. I'm pretty sure there's a lot of stuff that's not working per specification. Testing things out and filing relevant bugs would be a huge help. :-)
Comment on attachment 275269 [details] Patch (Double-check tests) View in context: https://bugs.webkit.org/attachment.cgi?id=275269&action=review Not sure when you chose suggestedName and when suggestedFilename. I think you should use suggestedFilename everywhere instead. > Source/WebKit2/UIProcess/WebPageProxy.h:1813 > + bool m_syncNavigationActionHasDownloadAttribute { false }; I know that I inspired the use of m_sync here, and you said that the semantics are like the other m_sync data members, but that’s not exactly right. Those other data members are used only when m_inDecidePolicyForResponseSync is true, and this is used in a different way. I think there is still a potential problem here. Is there really a guarantee that we only have one policy choice in flight at a time? > Source/WebKit2/UIProcess/Downloads/DownloadProxy.h:80 > + void didStart(const WebCore::ResourceRequest&, const AtomicString& suggestedFilename); Should just be String. On the web process side this was an atomic string, but now in the UI process it’s not. > Source/WebKit2/UIProcess/Downloads/DownloadProxy.messages.in:24 > + DidStart(WebCore::ResourceRequest request, AtomicString suggestedFilename) Should just be String, not AtomicString. We don’t need to keep this an atomic string when it crosses the process boundary.
Can’t believe we are still waiting for this and it is nearly 2018. It’s difficult to make a simple cross-browser mobile app that allows a user to backup their config on iOS. Is there any timeline for this or is it another feature reserved for apps that live in the app store?
I should qualify; I’m talking about a prompt to save off to icloud/dropbox etc. Not just serve the user a dirty raw text file with no option to save
(In reply to Ben Frain from comment #84) > Can’t believe we are still waiting for this and it is nearly 2018. > > It’s difficult to make a simple cross-browser mobile app that allows a user > to backup their config on iOS. > > Is there any timeline for this or is it another feature reserved for apps > that live in the app store? This bug is closed as resolved. The download attribute shipped in Safari 10.1 on macOS. https://webkit.org/blog/7477/new-web-features-in-safari-10-1/ If you are talking about iOS, the bug to track is bug 167341. Thanks for the interest.
Thanks Timothy. It was the iOS side of things I was unhappy with so left the comment there, thanks.