Last Comment Bug 707665 - Save as PDF option should be disabled for about:home and any XUL documents
: Save as PDF option should be disabled for about:home and any XUL documents
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P3 normal (vote)
: ---
Assigned To: :Margaret Leibovic
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-05 07:09 PST by Aaron Train [:aaronmt]
Modified: 2013-02-15 05:47 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
patch (2.21 KB, patch)
2011-12-06 12:56 PST, :Margaret Leibovic
mark.finkle: review-
Details | Diff | Review
patch v2 (7.15 KB, patch)
2011-12-13 11:55 PST, :Margaret Leibovic
mark.finkle: review+
Details | Diff | Review

Description Aaron Train [:aaronmt] 2011-12-05 07:09:54 PST
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 2011-12-06 12:56:50 PST
Created attachment 579422 [details] [diff] [review]
patch
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-06 14:47:23 PST
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 2011-12-08 11:59:49 PST
(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?) 2011-12-08 12:02:44 PST
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?) 2011-12-08 12:03:07 PST
Along with LocationChange maybe?
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2011-12-08 19:15:26 PST
(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?) 2011-12-08 20:06:41 PST
(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 2011-12-13 11:19:45 PST
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 2011-12-13 11:55:30 PST
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.
Comment 10 Lawrence Mandel [:lmandel] (use needinfo) 2011-12-13 12:07:59 PST
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?) 2011-12-13 16:08:13 PST
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 2011-12-13 16:28:43 PST
https://hg.mozilla.org/mozilla-central/rev/ceffbd680e2c
Comment 13 Andreea Pod 2013-02-15 05:47:58 PST
Verified fixed on:
-build:  Firefox for Android 21.0a1 (2013-02-15)
-device: Samsung Galaxy Tab 2 7.0
-OS: Android 4.0.4

Note You need to log in before you can comment on or make changes to this bug.