In the jsdownloads API, it should be possible to keep track of the page from which the download was started, if any.

Comment on attachment 745748 [details] [diff] [review] v1 Review of attachment 745748 [details] [diff] [review]: ----------------------------------------------------------------- Many of the considerations for the isPrivate case also apply here, we must set nsIHttpChannel.referrer and have a test that checks that the correct Referrer header is sent at the HTTP level. ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +422,5 @@ > + > + /** > + * The referrer for the download source. > + */ > + referrer: null, "The nsIURI for the referrer of the download source, or null if no referrer should be sent or the download source is not HTTP." ::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js @@ +27,5 @@ > > // Checks the generated DownloadSource and DownloadTarget properties. > do_check_true(download.source.uri.equals(TEST_SOURCE_URI)); > do_check_eq(download.target.file, targetFile); > + do_check_true(!download.source.referrer); do_check_true(download.source.referrer === null);

This patch depends on patch for bug 836439

Comment on attachment 749770 [details] [diff] [review] v3 Review of attachment 749770 [details] [diff] [review]: ----------------------------------------------------------------- Considerations from bug 836439 apply to the test case for this patch also, plus the additional comments below. ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +621,1 @@ > } I think sending the referrer in private requests is perfectly valid, this should be changed to a separate instanceof check like the one for nsIPrivateBrowsingChannel. We should also add a test verifying that a download from a non-HTTP channel (like a channel created from a short "data:" URI) still works even when specifying a referrer. ::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js @@ +27,5 @@ > > // Checks the generated DownloadSource and DownloadTarget properties. > do_check_true(download.source.uri.equals(TEST_SOURCE_URI)); > do_check_eq(download.target.file, targetFile); > + do_check_true(download.source.referrer == null); The "===" operator should be used here. @@ +38,5 @@ > > /** > + * Executes a download, started by constructing a Download object with referrer. > + */ > +add_task(function test_download_with_valid_referrer() This test case and the following one may be merged in a single test case named test_download_referrer. @@ +54,5 @@ > + > + // Starts the download and waits for completion. > + yield download.start(); > + > + // Checks the referrer property remains unchanged after download. This test is probably unnecessary, even in the merged test case.

(In reply to Paolo Amadini [:paolo] from comment #5) > We should also add a test verifying that a download from a non-HTTP channel > (like a channel created from a short "data:" URI) still works even when > specifying a referrer. Added a test for that. > > ::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js > @@ +27,5 @@ > > > > // Checks the generated DownloadSource and DownloadTarget properties. > > do_check_true(download.source.uri.equals(TEST_SOURCE_URI)); > > do_check_eq(download.target.file, targetFile); > > + do_check_true(download.source.referrer == null); > > The "===" operator should be used here. The issue is that we don't do any validation before assigning to download.source.referrer in Downloads.jsm. Therefore, the value would be undefined if no referrer is defined. Shall we add an validation to set it to null or let the test as it is?

Comment on attachment 753156 [details] [diff] [review] v4 Review of attachment 753156 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Raymond Lee [:raymondlee] from comment #6) > The issue is that we don't do any validation before assigning to > download.source.referrer in Downloads.jsm. Therefore, the value would be > undefined if no referrer is defined. Ah, I see. So, since omitting the parameter when createDownload is called is a valid input for us, I think the most correct solution is... ::: toolkit/components/jsdownloads/src/Downloads.jsm @@ +79,5 @@ > > download.source = new DownloadSource(); > download.source.uri = aProperties.source.uri; > download.source.isPrivate = aProperties.source.isPrivate; > + download.source.referrer = aProperties.source.referrer; ...to guard this assignment: if ("referrer" in aProperties.source) { download.source.referrer = aProperties.source.referrer; } So, if the referrer is undefined, we fall back to "null" from the DownloadSource prototype, and the "===" test should work. This should also avoid a JavaScript strict warning, if I remember correctly. And, while here, the same should be done for "isPrivate". ::: toolkit/components/jsdownloads/test/unit/head.js @@ +46,5 @@ > > const FAKE_SERVER_PORT = 4445; > const FAKE_BASE = "http://localhost:" + FAKE_SERVER_PORT; > > +const TEST_REFERRER_URI = NetUtil.newURI(HTTP_BASE + "/referrer.txt"); mini-mini nit: "/referrer.html" :-) ::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js @@ +63,5 @@ > + } else if (testCount == 1) { > + // Referrer should not exist for private download. > + do_check_true(aRequest.hasHeader("Referer")); > + do_check_eq(aRequest.getHeader("Referer"), TEST_REFERRER_URI.spec); > + } These became the same test, you should just remove the testCount variable and the conditions, leaving only the two checks. @@ +95,5 @@ > + do_check_true(download.stopped); > + do_check_true(download.succeeded); > + do_check_false(download.canceled); > + do_check_true(download.error === null); > + do_check_eq(download.progress, 100); You can omit the state checks here, the fact that the last download.start() succeeded is enough. r+ with these changes.

Pushed to try and waiting for results https://tbpl.mozilla.org/?tree=Try&rev=ab469294e072

(In reply to Raymond Lee [:raymondlee] from comment #8) > Created attachment 754411 [details] [diff] [review] > Patch for checkin > > Pushed to try and waiting for results > https://tbpl.mozilla.org/?tree=Try&rev=ab469294e072 There are two oranges so I've pushed to try again and all green. https://tbpl.mozilla.org/?tree=Try&rev=adc606e1699d