Closed Bug 820491 Opened 7 years ago Closed 7 years ago

PBM - Unable to initiate a download in Private Browsing mode

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 21
Tracking Status
firefox20 --- verified
firefox21 --- verified
fennec 20+ ---

People

(Reporter: aaronmt, Assigned: bnicholson)

References

Details

Attachments

(3 files, 4 obsolete files)

Currently, when one attempts to initiate a download in a private tab, nothing seems to happen. No Gecko exceptions are shown in console.


STR:

i) Open a new private tab
ii) http://nightly.mozilla.org
iii) Attempt to download Nightly for Android

Verify that you can do so normally in a regular tab

--
Nightly (12/11)
Samsung Galaxy Nexus (Android 4.1.2)
Presumably the code that interacts with nsIDownloadManager needs to be updated to use GUIDs (getDownloadByGUID, aDownload.guid), and things like activePrivateDownloads and  activePrivateDownloadCount. Note that new methods like aDownload.retry/remove/pause/cancel can help with the guid conversion if you already have a download object.
Also addPrivacyAwareListener instead of addListener.
Josh is this something you can take  care of?
Assignee: nobody → josh
tracking-fennec: ? → 20+
This is completely untested, as I don't have a mobile build set up, nor do I possess a device that can run Fennec. However, this should be the lion's share of the work.
Attachment #692083 - Flags: review?(bnicholson)
And of course I forgot to qref.
Attachment #692084 - Flags: review?(bnicholson)
Attachment #692083 - Attachment is obsolete: true
Attachment #692083 - Flags: review?(bnicholson)
One thing to note is that right now the downloads page will show you either public downloads or private ones, depending on what kind of page you're looking at when you launch it (theoretically, at least). You might want to just create a combined view and observe the last-pb-context-exited notification and rebuild the view if it occurs.
Comment on attachment 692084 [details] [diff] [review]
Update download manager code to work with private downloads.

(In reply to Josh Matthews [:jdm] from comment #7)
> One thing to note is that right now the downloads page will show you either
> public downloads or private ones, depending on what kind of page you're
> looking at when you launch it (theoretically, at least). You might want to
> just create a combined view and observe the last-pb-context-exited
> notification and rebuild the view if it occurs.

Yeah, I think that's how we'll want the downloads page to work. There's some errors with the patch as-is, but I can take over from here - thanks for doing this much!
Attachment #692084 - Flags: review?(bnicholson)
Assignee: josh → bnicholson
Hijacked jdm's patch; fixed the errors, made about:downloads show a combined view, and uses alternate styling for private downloads. I kept the downloads list separated by showing all private downloads above the non-private ones.

Test build here: http://dl.dropbox.com/u/35559547/fennec-pb-downloads.apk
Attachment #692084 - Attachment is obsolete: true
Attachment #694142 - Flags: review?(wjohnston)
As jdm pointed out, our DownloadManagerUI implementation is pretty broken. This fixes it.
Attachment #694143 - Flags: review?(wjohnston)
Minor fixes to JSDoc syntax.
Attachment #694143 - Attachment is obsolete: true
Attachment #694143 - Flags: review?(wjohnston)
Attachment #694147 - Flags: review?(wjohnston)
Comment on attachment 694142 [details] [diff] [review]
Part 1: Update download manager code to work with private downloads

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

This breaks the "No downloads" message.

::: mobile/android/chrome/content/aboutDownloads.js
@@ +418,5 @@
>    _removeItem: function dl_removeItem(aItem) {
>      // Make sure we have an item to remove
>      if (!aItem)
>        return;
>    

White space here and below.

@@ +449,5 @@
> +      aDownload.remove();
> +      try {
> +        if (f) f.remove();
> +      } catch (ex) {
> +        console.log("Error: removeDownload() " + ex);

Leave this logging in. If you want to hide it in private mode (I assume that console.log has no idea what's private and what's not in this context), that's fine. Same for other logging in here. Maybe a helper logForDownload(aDownload, aMessage, aException)?

::: mobile/android/chrome/content/aboutDownloads.xhtml
@@ +28,5 @@
>      <div>&aboutDownloads.header;</div>
>    </div>
> +  <ul id="downloads-list">
> +    <li id="private-divider" />
> +  </ul>

This will break the "No downloads" message, which we currently show using the all too clever

#downloads-list:empty + #no-downloads-indicator { display: block; }

If we want these in a separate list, I'd just add a new UL for it and update the selector to check both.

::: mobile/android/themes/core/aboutDownloads.css
@@ +63,3 @@
>  }
>  
> +li.normal {

I don't think we need "normal". Its either private or not.

@@ +99,5 @@
> +  background-image: none;
> +  color: #000;
> +}
> +
> +li:active .details {

This is changing from what we showed before, right?
Attachment #694142 - Flags: review?(wjohnston) → review-
Moved logging to a utility function and separated the private/normal downloads into separate lists.
Attachment #694142 - Attachment is obsolete: true
Attachment #694549 - Flags: review?(wjohnston)
There's lots of more whitespace in that file besides what was pointed out in comment 12, so removes it all.
Attachment #694550 - Flags: review?(wjohnston)
Comment on attachment 694549 [details] [diff] [review]
Part 1: Update download manager code to work with private downloads, v2

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

Looks and works good. Some little nitpick things.

::: mobile/android/chrome/content/aboutDownloads.js
@@ +179,5 @@
> +  observe: function (aSubject, aTopic, aData) {
> +    let privateDownloads = this._privateList.children;
> +    for (let i = 0; i < privateDownloads.length; ++i) {
> +      this._privateList.removeChild(privateDownloads[i]);
> +    }

Can you just do this._privateList.innerHTML = ""; ?

@@ +372,4 @@
>      }
>    },
>  
> +  getDownloads: function dl_getDownloads(isPrivate) {

I don't like this boolean. As a caller, the API doesn't make any sense at a glance. I think you're better to allow passing an optional aType string (default to "") and then in isPrivate you can check aType == "private" instead.

@@ +512,5 @@
> +      }
> +    }.bind(this));
> +  },
> +
> +  _updateDownloadRow: function dl_updateDownloadRow(aItem, aDownload){

nit: space before {

::: mobile/android/chrome/content/downloads.js
@@ +22,5 @@
>        return;
>      this._initialized = true;
>  
>      // Monitor downloads and display alerts
> +    this._dlmgr = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);

Make this lazy.

@@ +24,5 @@
>  
>      // Monitor downloads and display alerts
> +    this._dlmgr = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
> +    this._progressAlert = new AlertDownloadProgressListener();
> +    this._dlmgr.addPrivacyAwareListener(this._progressAlert);

You might as well make this lazy too.

::: mobile/android/themes/core/aboutDownloads.css
@@ -45,5 @@
>  }
>  
>  li {
>    color: black;
> -  background-image: url("chrome://browser/skin/images/row-bg-normal.png");

I'd rather just leave these rules and just have the special rules for private downloads. i.e. If we add a third list it should look fine even if we don't style it.

@@ +59,5 @@
>  
> +#normal-downloads-list li {
> +  background-image: url("chrome://browser/skin/images/row-bg-normal.png");
> +  color: #000;
> +}

That means you don't need this

@@ +72,5 @@
> +}
> +
> +#private-downloads-list div.details {
> +  background-color: #393e43;
> +}

And you can move the other details bit up and just use it here.

@@ +87,2 @@
>    background-image: none;
> +  background-color: transparent;

And this should be:

li:active,
#private li:active { }.
Attachment #694549 - Flags: review?(wjohnston) → review+
Comment on attachment 694147 [details] [diff] [review]
Part 2: Fix DownloadManagerUI component, v2

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

::: mobile/android/components/DownloadManagerUI.js
@@ +20,5 @@
>    show: function show(aWindowContext, aID, aReason) {
>      if (!aReason)
>        aReason = Ci.nsIDownloadManagerUI.REASON_USER_INTERACTED;
>  
> +    this._getBrowserApp().selectOrOpenTab("about:downloads");

How sure do you feel that _getBrowserApp() will always return something and not throw? This could potentially be called early in startup before our window was open, or during shutdown....

Maybe we want to throw in those cases though?
Attachment #694147 - Flags: review?(wjohnston) → review+
Attachment #694550 - Flags: review?(wjohnston) → review+
jdm, It looks like this stuff requires us to make a connect to the dlmanager at startup (so that we can attach some listeners and things). File I/O/memory/etc being as bad as it is on some phones, we've always been really careful in Fennec to not start services until we need them.

We previously just added an observer for dl-start notifications and didn't bother to initialize any of this until we heard from that (i.e. most browsing sessions we'd hear nothing and not bother with this).

How confident are you that this is safe/ok to do during startup?
Flags: needinfo?(josh)
It's going to be reading in the downloads SQL database from disk when the service is initialized. One thing to note, though, is that unless the service is initialized at startup, any in-progress downloads from the last session won't resume until the service is initialized. I don't really know what your other options here are.
Flags: needinfo?(josh)
(In reply to Wesley Johnston (:wesj) from comment #15)
> > +    this._dlmgr = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
> 
> Make this lazy.

I was wrong that we can't just make this lazy. I talked to mfinkle and I think for now we're just best to land this bit as is and see how big the startup hit is. If there is a significant one, we can start looking at ways to fix it.

Thanks.
Comment on attachment 694147 [details] [diff] [review]
Part 2: Fix DownloadManagerUI component, v2

>   show: function show(aWindowContext, aID, aReason) {
show() now has a fourth optional parameter, the privacy status, you could use this to ensure you open a tab in an appropriately private window.

>   get visible() {
[Hmm, there's no way of distinguishing between normal and private visibility...]
Neil, please file follow-up bugs for these types of issues.  Commenting to indicate problems in a fixed bug almost always goes unnoticed.

Thanks!
Status: RESOLVED → VERIFIED
Comment on attachment 694147 [details] [diff] [review]
Part 2: Fix DownloadManagerUI component, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 582244
User impact if declined: downloads can't be started in private browsing mode
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none
Attachment #694147 - Flags: approval-mozilla-aurora?
Comment on attachment 694549 [details] [diff] [review]
Part 1: Update download manager code to work with private downloads, v2

See above
Attachment #694549 - Flags: approval-mozilla-aurora?
Comment on attachment 694550 [details] [diff] [review]
Part 3: Whitespace cleanup in aboutDownloads.js

See above
Attachment #694550 - Flags: approval-mozilla-aurora?
Comment on attachment 694147 [details] [diff] [review]
Part 2: Fix DownloadManagerUI component, v2

Approved. We'll want downloads in pb mode :)
Attachment #694147 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #694549 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #694550 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 834048
Depends on: 834400
Verified fixed on:
-build: Firefox for Android 20.0b6 (2013-03-21)
-device: Samsung Galaxy Nexus
-OS: Android 4.1.1
You need to log in before you can comment on or make changes to this bug.