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

VERIFIED FIXED

Status

()

Firefox for Android
General
P3
normal
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: aaronmt, Assigned: Margaret)

Tracking

unspecified
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

7.15 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 1

6 years ago
Created attachment 579422 [details] [diff] [review]
patch
Attachment #579422 - Flags: review?(blassey.bugs)
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
Attachment #579422 - Flags: review?(blassey.bugs) → review-
Summary: Save as PDF option should be disabled for about:// chrome:// file:// → Save as PDF option should be disabled for about:home and any XUL documents

Updated

6 years ago
Priority: -- → P3
(Assignee)

Comment 3

6 years ago
(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?
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.
Along with LocationChange maybe?
(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?
(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.
(Assignee)

Comment 8

6 years ago
So for disabling saveAsPDF, which mimetypes do we want to explicitly disable? Or do we want a whitelist of types to enable?
(Assignee)

Comment 9

6 years ago
Created attachment 581353 [details] [diff] [review]
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.
Attachment #579422 - Attachment is obsolete: true
Attachment #581353 - Flags: review?(mark.finkle)
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 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.
Attachment #581353 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 12

6 years ago
https://hg.mozilla.org/mozilla-central/rev/ceffbd680e2c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
tracking-fennec: --- → 11+
status-firefox11: --- → fixed

Comment 13

4 years ago
Verified fixed on:
-build:  Firefox for Android 21.0a1 (2013-02-15)
-device: Samsung Galaxy Tab 2 7.0
-OS: Android 4.0.4
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.