You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by solomax <gi...@git.apache.org> on 2017/11/09 12:17:34 UTC
[GitHub] wicket pull request #244: [WICKET-6491] ajax download seems to work under bo...
GitHub user solomax opened a pull request:
https://github.com/apache/wicket/pull/244
[WICKET-6491] ajax download seems to work under both IE11 and Edge
I would appreciate if someone can review my changes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/wicket WICKET-6491-ajax-download-ms
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/wicket/pull/244.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #244
----
commit e1e6235e33156086fc3b7b13d49261ef10c2183d
Author: Maxim Solodovnik <so...@gmail.com>
Date: 2017-11-09T12:15:26Z
[WICKET-6491] ajax download seems to work under both IE11 and Edge
----
---
[GitHub] wicket pull request #244: [WICKET-6491] ajax download seems to work under bo...
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on a diff in the pull request:
https://github.com/apache/wicket/pull/244#discussion_r150014533
--- Diff: wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/AjaxDownloadBehavior.java ---
@@ -323,10 +323,30 @@ private String getName()
}
/**
+ * Controls if anti cache parameter should be added to the URL or not
+ *
+ * @return {@code true} to add the anti cache request parameter, {@code false} - otherwise
+ */
+ protected boolean shouldAddAntiCacheParameter() {
--- End diff --
What is this good for? The resource can control caching - I don't see the necessity for an antiCache parameter (as ResourceLink doesn't need have it either).
---
[GitHub] wicket pull request #244: [WICKET-6491] ajax download seems to work under bo...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/244#discussion_r149991409
--- Diff: wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/wicket-ajaxdownload.js ---
@@ -108,29 +108,32 @@
var disposition = xhr.getResponseHeader("Content-Disposition");
if (disposition) {
var matches = /filename[^;=\n]*=(([""]).*?\2|[^;\n]*)/.exec(disposition);
- if (matches != null && matches[1]) {
+ if (matches !== null && matches[1]) {
--- End diff --
Wasn't aware of this file
Just have checked `../../wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/wicket-ajaxdownload.js` is already added
---
[GitHub] wicket pull request #244: [WICKET-6491] ajax download seems to work under bo...
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on a diff in the pull request:
https://github.com/apache/wicket/pull/244#discussion_r150011145
--- Diff: wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/AjaxDownloadBehavior.java ---
@@ -323,10 +323,30 @@ private String getName()
}
/**
+ * Controls if anti cache parameter should be added to the URL or not
+ *
+ * @return {@code true} to add the anti cache request parameter, {@code false} - otherwise
+ */
+ protected boolean shouldAddAntiCacheParameter() {
--- End diff --
What is this needed for? The resource controls the caching.
---
[GitHub] wicket pull request #244: [WICKET-6491] ajax download seems to work under bo...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/244#discussion_r150018189
--- Diff: wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/AjaxDownloadBehavior.java ---
@@ -323,10 +323,30 @@ private String getName()
}
/**
+ * Controls if anti cache parameter should be added to the URL or not
+ *
+ * @return {@code true} to add the anti cache request parameter, {@code false} - otherwise
+ */
+ protected boolean shouldAddAntiCacheParameter() {
--- End diff --
I'll double-check (modify my tests) and remove it
I thought it would save some lines of code if anti-cache will be on by default
---
[GitHub] wicket pull request #244: [WICKET-6491] ajax download seems to work under bo...
Posted by martin-g <gi...@git.apache.org>.
Github user martin-g commented on a diff in the pull request:
https://github.com/apache/wicket/pull/244#discussion_r149995282
--- Diff: wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/wicket-ajaxdownload.js ---
@@ -108,29 +108,32 @@
var disposition = xhr.getResponseHeader("Content-Disposition");
if (disposition) {
var matches = /filename[^;=\n]*=(([""]).*?\2|[^;\n]*)/.exec(disposition);
- if (matches != null && matches[1]) {
+ if (matches !== null && matches[1]) {
--- End diff --
Strange that it didn't break the build due to `!=`.
---
[GitHub] wicket issue #244: [WICKET-6491] ajax download seems to work under both IE11...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on the issue:
https://github.com/apache/wicket/pull/244
@svenmeier I reverted my changes to AjaxDownloadBehavior.java, thanks for catching this :)
---
[GitHub] wicket pull request #244: [WICKET-6491] ajax download seems to work under bo...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/244#discussion_r150012308
--- Diff: wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/AjaxDownloadBehavior.java ---
@@ -323,10 +323,30 @@ private String getName()
}
/**
+ * Controls if anti cache parameter should be added to the URL or not
+ *
+ * @return {@code true} to add the anti cache request parameter, {@code false} - otherwise
+ */
+ protected boolean shouldAddAntiCacheParameter() {
--- End diff --
During my tests I found the issue: subsequent download was cached. I have implemented this anti-cache, same as in wicket Image.java
---
[GitHub] wicket pull request #244: [WICKET-6491] ajax download seems to work under bo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/wicket/pull/244
---
[GitHub] wicket pull request #244: [WICKET-6491] ajax download seems to work under bo...
Posted by martin-g <gi...@git.apache.org>.
Github user martin-g commented on a diff in the pull request:
https://github.com/apache/wicket/pull/244#discussion_r149970025
--- Diff: wicket-extensions/src/main/java/org/apache/wicket/extensions/ajax/wicket-ajaxdownload.js ---
@@ -108,29 +108,32 @@
var disposition = xhr.getResponseHeader("Content-Disposition");
if (disposition) {
var matches = /filename[^;=\n]*=(([""]).*?\2|[^;\n]*)/.exec(disposition);
- if (matches != null && matches[1]) {
+ if (matches !== null && matches[1]) {
--- End diff --
Let's add this .js file to the ones checked by JSLint in `testing/js-tests/Gruntfile.js
---
[GitHub] wicket issue #244: [WICKET-6491] ajax download seems to work under both IE11...
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on the issue:
https://github.com/apache/wicket/pull/244
Is it OK to merge this PR?
---