Closed Bug 1070488 Opened 5 years ago Closed 5 years ago

also add files downloaded in private browsing to Android download manager

Categories

(Firefox for Android :: Download Manager, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox35 --- verified
firefox36 --- verified
relnote-firefox --- 35+
fennec 35+ ---

People

(Reporter: c.ascheberg, Assigned: wesj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Files downloaded during private browsing are currently not added to the Android download manager. They should be, since about:privatebrowsing explicitly states that downloaded files are kept.
More importantly, adding those files to the Android DM would (partially) fix bug 927418 for all supported Android versions (there is some more discussion there, see bug 927418 comment 10).
tracking-fennec: --- → ?
Assignee: nobody → wjohnston
tracking-fennec: ? → 35+
There are two things to do with adding here:

1.) Adding it to the Android "Download Manager" app. That is basically a sqlite database on the device accessible to any app that asks for permission to query it. It also makes it easy to get to these files from anything that requests documents.

2.) Scanning it with the media scanner. That will cause it to be seen by any apps that ask to know when media is added and will make it appear in the gallery.

Sounds like the dev team is ok with doing the first. I don't have strong opinions about either, but pinging ux about the second.
Flags: needinfo?(ywang)
I think those two changes make sense to me. Android is not super great about handling downloaded files and surfacing them in the right context could be very helpful. 

Though there might be some privacy concerns of surfacing the files that were in the private mode, but I think the trade-off is worthy. Download on mobile browser is not highly used. I would imagine the downloads would be even lower on private browsing.
Flags: needinfo?(ywang)
Attached patch Patch (obsolete) — Splinter Review
This restores our old behavior, but leaves support for the pref so that users/we can turn this on/off.
Attachment #8497747 - Flags: review?(paolo.mozmail)
Comment on attachment 8497747 [details] [diff] [review]
Patch

This looks like all Java code. Am I the right reviewer for this patch, or is there something you wanted me to look at in particular?
Heh. No just not the patch I meant here. My brain does not seem to work this week. Will get the right one.
Attachment #8497747 - Flags: review?(paolo.mozmail)
I was just wondering if you forgot to upload the right patch, it seemed like you had created it already.
Flags: needinfo?(wjohnston)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8497747 - Attachment is obsolete: true
Attachment #8502251 - Flags: review?(paolo.mozmail)
Flags: needinfo?(wjohnston)
Comment on attachment 8502251 [details] [diff] [review]
Patch

Review of attachment 8502251 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +2809,5 @@
> +#if defined(MOZ_WIDGET_ANDROID)
> +      bool addToSystem = true;
> +      if (pref) {
> +        pref->GetBoolPref(PREF_BDM_ADDTORECENTDOCS, &addToSystem);
> +      }

This would need two more spaces of indentation, but I think you can probably apply the suggestion below to here as well.

::: toolkit/components/jsdownloads/src/DownloadPlatform.cpp
@@ +113,5 @@
> +      nsCOMPtr<nsIPrefBranch> pref(do_GetService(NS_PREFSERVICE_CONTRACTID));
> +      bool addToSystem = true;
> +      if (pref) {
> +        pref->GetBoolPref(PREF_BDM_ADDTORECENTDOCS, &addToSystem);
> +      }

You can just keep this code inside the block where "addToRecentDocs" is available.
Attachment #8502251 - Flags: review?(paolo.mozmail) → feedback+
Attached patch Patch v2Splinter Review
Attachment #8502251 - Attachment is obsolete: true
Attachment #8502806 - Flags: review?(paolo.mozmail)
Attachment #8502806 - Flags: review?(paolo.mozmail) → review+
Hi,

could you provide a try run, thanks!
Keywords: checkin-needed
Comment on attachment 8502806 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: 816318
[User impact if declined]: Without this, private downloads aren't shown in the Android download manager.
[Describe test coverage new/current, TBPL]: Need some.
[Risks and why]: Pretty low risk. Just moving code around in here so that this doesn't check private mode before adding anymore.
[String/UUID change made/needed]: none
Attachment #8502806 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/511ddc0a0e58
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Attachment #8502806 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Release Note Request (optional, but appreciated)
[Why is this notable]: might want to put this in the 'fixed' bugs section
relnote-firefox: --- → ?
Verified as fixed in
Builds:
Firefox for Android 36.0a1 (2014-10-23)
Firefox for Android 35.0a2 (2014-10-23)
Device: Nexus 4 (Android 4.4.4)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.