707665 - Save as PDF option should be disabled for about:home and any XUL documents

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Aaron Train [:aaronmt]]

Akin to bug 703630; one can save as PDF for pages such as about:home (results in a white empty document), chrome://browser/content/browser.xul (no PDF is saved), etc ..

Save as PDF should be confined to actual web content.

Comment 1

[:Margaret Leibovic]

Attachment: patch

Comment 2

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

Comment on attachment 579422 [details] [diff] [review]
patch

This is trickier than sharing. We could turn about: pages to PDF, but about:home _is_ a blank page as far as Gecko is concerned.

file:// should also work fine. Technically, chrome:// should work too, except for XUL pages.

So, the rules for PDF might be, ignore if:
* about:home
* any XUL file

Comment 3

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #2)

> So, the rules for PDF might be, ignore if:
> * about:home
> * any XUL file

Can I just check to see if the url has a .xul extension? It doesn't look like we have any XUL about: pages - they all come from AboutRedirector.js, right?

Comment 4

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

We shouldn't have anymore XUL about: pages. One thing we could do is send the documentURI.spec along with one of the messages. I think documentURI for an about page is the true URL.

Comment 5

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

Along with LocationChange maybe?

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> We shouldn't have anymore XUL about: pages. One thing we could do is send
> the documentURI.spec along with one of the messages. I think documentURI for
> an about page is the true URL.

wouldn't it be better to check the mime type?

Comment 7

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

(In reply to Brad Lassey [:blassey] from comment #6)
> (In reply to Mark Finkle (:mfinkle) from comment #4)
> > We shouldn't have anymore XUL about: pages. One thing we could do is send
> > the documentURI.spec along with one of the messages. I think documentURI for
> > an about page is the true URL.
> 
> wouldn't it be better to check the mime type?

Sure. It would still need to be sent along in the OnLocationChange message to Java. Let's send both: documentURI.spec (good for about and error pages) and mimetype.

Comment 8

[:Margaret Leibovic]

So for disabling saveAsPDF, which mimetypes do we want to explicitly disable? Or do we want a whitelist of types to enable?

Comment 9

[:Margaret Leibovic]

Attachment: patch v2

So, testing this, I'm getting "about:home" for the documentURI and "application/xhtml+xml" for the mimetype, which is the same as a normal webpage, so I think we need to just special-case about:home. 

This patch isn't actually using documentURI for anything, but comment 7 makes is sound like you want it for other things? If that my assumption is wrong, I can just get rid of that.

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 581353 [details] [diff] [review]
patch v2

>+        // Disable save as PDF for about:home and xul pages
>+        saveAsPDF.setEnabled(!tab.getURL().equals("about:home") &&
>+                             !tab.getContentType().equals("application/vnd.mozilla.xul+xml"));
>+
Personally I prefer the form
saveAsPDF.setEnabled(!(tab.getURL().equals("about:home") ||
                     tab.getContentType().equals("application/vnd.mozilla.xul+xml")));
1. I think this form is easier to read. If the tab is about:home or it is XUL then setEnabled false. This again, is personal preference. 
2. I think there is a minor performance benefit my way as fewer functions may be called. As written two equals functions and two '!' functions must be called each time this line is executed. The way I listed, there is maximum two calls to equals and one call to '!'. Minimum there is only one call to equals and one call to '!'.

Are there any other mime types for which save as PDF will fail?

Comment 11

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

Comment on attachment 581353 [details] [diff] [review]
patch v2

* You could initialize the Tab.mContentType and Tab.mDocumentURI to "" in the Tab constructors
* If you decide to switch the setEnable call, you might want to change the line above for share.setEnabled too.
* documentURI might be helpful in other situations, like error pages. I'm fine with keeping it.

Comment 12

[:Margaret Leibovic]

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

Comment 13

[Andreea Pod]

Verified fixed on:
-build:  Firefox for Android 21.0a1 (2013-02-15)
-device: Samsung Galaxy Tab 2 7.0
-OS: Android 4.0.4