798280 - Save As PDF busted - (NS_ERROR_XPC_NOT_ENOUGH_ARGS @ nsIDownloadManager.addDownload)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ioana Chiorean]

Build ID: 18.0 (2012-10-04) Nightly Channel
Device: Samsung Galaxy Nexus
OS: Android 4.1

Steps to reproduce:
1. Open Fennec -> Go to gmail.com
2. Tap Menu -> Tools -> Save as PDF

Expected result:
- page should be saved as pdf

Actual result: 
- save as pdf doesn't trigger any action

Last build working : 
http://hg.mozilla.org/integration/mozilla-inbound/rev/31ae286fff78

First build not working: 
http://hg.mozilla.org/integration/mozilla-inbound/rev/9a71c92af5a4

Comment 1

[Aaron Train [:aaronmt]]

E/GeckoConsole( 4384): [JavaScript Error: "NS_ERROR_XPC_NOT_ENOUGH_ARGS: Not enough arguments [nsIDownloadManager.addDownload]" {file: "chrome://browser/content/browser.js" line: 825}]

Comment 2

[Brian Nicholson (:bnicholson)]

Looks like the call to addDownload() for saveAsPDF() wasn't updated in bug 795065.

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Pass privacy status to addDownload() in saveAsPDF

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 670455 [details] [diff] [review]
Pass privacy status to addDownload() in saveAsPDF

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 795065
User impact if declined: save as pdf won't work
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 670455 [details] [diff] [review]
Pass privacy status to addDownload() in saveAsPDF

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+    let isPrivate = BrowserApp.getTabForBrowser(aBrowser).isPrivate;

This might be faster?
  let isPrivate = aBrowser.docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing

>+  this.isPrivate = false;

I think we should make a getter for this. The property could change under us. I want it to be 'live':

get isPrivate() {
  if (this.browser)
    return this.browser.docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing;
  return false;
}

>-    let isPrivate = ("isPrivate" in aParams) && aParams.isPrivate;
>-    if (isPrivate) {
>+    this.isPrivate = ("isPrivate" in aParams) && (aParams.isPrivate == true);
>+    if (this.isPrivate) {
>       this.browser.docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing = true;
>     }

Reverting this should be OK with the getter

r+ with changes

Comment 6

[(no longer active)]

Isn't the privacy flag only on trunk?

Comment 7

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e15c81a5bf1d

(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> Isn't the privacy flag only on trunk?

I changed the patch to just call aBrowser.docShell.QueryInterface() directly in saveAsPDF(), so this same patch should work on Aurora now.

Comment 8

[(no longer active)]

(In reply to comment #7)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/e15c81a5bf1d
> 
> (In reply to Ehsan Akhgari [:ehsan] from comment #6)
> > Isn't the privacy flag only on trunk?
> 
> I changed the patch to just call aBrowser.docShell.QueryInterface() directly in
> saveAsPDF(), so this same patch should work on Aurora now.

Oh, ok.  yeah that's better anyways.

Comment 9

[(no longer active)]

(In reply to Brian Nicholson (:bnicholson) from comment #7)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/e15c81a5bf1d

    1.12 +    let isPrivate = aBrowser.docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing;

Instead of doing this, can you please use PrivateBrowsingUtils?  You should do something like:

let isPrivate = PrivateBrowsingUtils.isWindowPrivate(aBrowser.currentWindow);

(assuming that currentWindow is a property that retrieves the current window object for the focused tab)

Comment 10

[Brian Nicholson (:bnicholson)]

Attachment: Follow-up for using PrivateBrowsingUtils

Comment 11

[(no longer active)]

Comment on attachment 670610 [details] [diff] [review]
Follow-up for using PrivateBrowsingUtils

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

You need to import PrivateBrowsingUtils.jsm here.

Also, while you're here, would you mind converting over the other usage of the usePrivateBrowsing property here as well?

Thanks!

Comment 12

[Brian Nicholson (:bnicholson)]

Attachment: Follow-up for using PrivateBrowsingUtils, v2

Imported the PB jsm. Can't change the other usePrivateBrowsing since we're setting the flag there.

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e15c81a5bf1d

Comment 14

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76071e66c36b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76071e66c36b

Should this have a test?

Comment 16

[Cristian Nicolae (:xti)]

(In reply to Ryan VanderMeulen from comment #15)
> https://hg.mozilla.org/mozilla-central/rev/76071e66c36b
> 
> Should this have a test?

It already has a testcase https://moztrap.mozilla.org/manage/cases/?filter-id=1024#caseversion-id-8619

Comment 17

[Ryan VanderMeulen [:RyanVM]]

in-testsuite refers to automated tests that run on checkin

Comment 18

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/5d4dac6cfded

Comment 19

[Cristian Nicolae (:xti)]

Save as PDF is working now on Firefox 18.

--
Firefox 18.0a2 (2012-11-19)
Device: Galaxy S2
OS: Android 4.0.3