Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(51)

Issue 246893006: [Downloads] Clear suggested filename on cross origin redirect. (Closed)

Created:
6 years, 8 months ago by asanka
Modified:
6 years, 7 months ago
CC:
chromium-reviews, benjhayden+dwatch_chromium.org, jam, darin-cc_chromium.org
Visibility:
Public.

Description

[Downloads] Clear suggested filename on cross origin redirect. The @download attribute can be used to specify a suggested filename for a download. However, this suggested name should only be used if the referring document is in the same origin as the downloaded resource. This CL clears the suggested name if the final download URL after redirects is in a different origin than the original download URL. The companion Blink CL at https://codereview.chromium.org/197033005/ ensures that the suggested name for a download initiated via a @download attribute would only be non-empty if the referring document is allowed to set a suggested name for the original download URL. BUG=346744 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266441

Patch Set 1 #

Total comments: 12

Patch Set 2 : Simplify use of EmbeddedTestServer #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+172 lines, -6 lines) Patch
M content/browser/download/download_browsertest.cc View 1 8 chunks +153 lines, -7 lines 2 comments Download
M content/browser/download/download_resource_handler.cc View 1 chunk +8 lines, -0 lines 0 comments Download
A content/test/data/download/download-attribute.html View 1 chunk +12 lines, -0 lines 0 comments Download
A + content/test/data/download/download-attribute.html.mock-http-headers View 0 chunks +-1 lines, --1 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
asanka
abarth: This is an attempt to revive https://codereview.chromium.org/197033005/. I've considered moving the entire responsibility of ...
6 years, 8 months ago (2014-04-22 15:41:14 UTC) #1
abarth-chromium
Sounds like a reasonable division. I'm not an OWNER here, so you'll probably need to ...
6 years, 8 months ago (2014-04-22 23:26:51 UTC) #2
asanka
On 2014/04/22 23:26:51, abarth wrote: > Sounds like a reasonable division. I'm not an OWNER ...
6 years, 8 months ago (2014-04-23 05:41:30 UTC) #3
Randy Smith (Not in Mondays)
Doesn't decrease my wish that we could refactor to allow DRH unit tests :-}. https://codereview.chromium.org/246893006/diff/1/content/browser/download/download_browsertest.cc ...
6 years, 8 months ago (2014-04-23 19:12:26 UTC) #4
asanka
https://codereview.chromium.org/246893006/diff/1/content/browser/download/download_browsertest.cc File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/246893006/diff/1/content/browser/download/download_browsertest.cc#newcode534 content/browser/download/download_browsertest.cc:534: // TODO(asanka): Consider migrating more tests to use the ...
6 years, 8 months ago (2014-04-23 22:59:27 UTC) #5
Randy Smith (Not in Mondays)
LGTM. https://codereview.chromium.org/246893006/diff/40001/content/browser/download/download_browsertest.cc File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/246893006/diff/40001/content/browser/download/download_browsertest.cc#newcode1746 content/browser/download/download_browsertest.cc:1746: EXPECT_EQ(FILE_PATH_LITERAL("download"), Suggestion: Comment that this is the key ...
6 years, 8 months ago (2014-04-24 22:08:43 UTC) #6
asanka
The CQ bit was checked by asanka@chromium.org
6 years, 8 months ago (2014-04-25 22:07:47 UTC) #7
asanka
Thanks! https://codereview.chromium.org/246893006/diff/40001/content/browser/download/download_browsertest.cc File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/246893006/diff/40001/content/browser/download/download_browsertest.cc#newcode1746 content/browser/download/download_browsertest.cc:1746: EXPECT_EQ(FILE_PATH_LITERAL("download"), On 2014/04/24 22:08:43, rdsmith wrote: > Suggestion: ...
6 years, 8 months ago (2014-04-25 22:08:07 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asanka@chromium.org/246893006/40001
6 years, 8 months ago (2014-04-25 22:28:53 UTC) #9
asanka
The CQ bit was unchecked by asanka@chromium.org
6 years, 7 months ago (2014-04-28 05:11:57 UTC) #10
asanka
The CQ bit was checked by asanka@chromium.org
6 years, 7 months ago (2014-04-28 05:12:06 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asanka@chromium.org/246893006/40001
6 years, 7 months ago (2014-04-28 05:12:43 UTC) #12
commit-bot: I haz the power
6 years, 7 months ago (2014-04-28 06:06:03 UTC) #13
Message was sent while issue was closed.
Change committed as 266441

Powered by Google App Engine
This is Rietveld 408576698