1034385 - Empty private tabs panel can be saved as pdf

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

1) Click options -> new private tab
2) Click options -> Page -> save as pdf

Expected: Save as pdf is disabled
Actual: Save as pdf is enabled

Comment 1

[Aaron Train [:aaronmt]]

Code is here maintained as a hard-coded list: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#2474

Comment 2

[Bryan Kok [:transfusion]]

Attachment: disablesaveaspdf.patch

Here's my proposed patch.
On a side note, I think it would make more sense to grey out the "Page >" similar to about:home...

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8453398 [details] [diff] [review]
disablesaveaspdf.patch

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

::: mobile/android/base/BrowserApp.java
@@ +782,5 @@
>  
>      private static boolean isAboutHome(final Tab tab) {
>          return AboutPages.isAboutHome(tab.getURL());
>      }
> +    

nit: We don't like to add excess end-of-line whitespace to the code, if we cah help it - can you remove this? Let me know if you'd like an explanation.

@@ +2474,5 @@
>          }
>  
>          // Disable save as PDF for about:home and xul pages.
>          saveAsPDF.setEnabled(!(isAboutHome(tab) ||
> +                               tab.getContentType().equals("application/vnd.mozilla.xul+xml") || 

nit: Whitespace.

@@ +2475,5 @@
>  
>          // Disable save as PDF for about:home and xul pages.
>          saveAsPDF.setEnabled(!(isAboutHome(tab) ||
> +                               tab.getContentType().equals("application/vnd.mozilla.xul+xml") || 
> +                               isTitlelessAboutPage(tab)));

I don't think I like using TitlelessAboutPages - this implies that we are not displaying Save As PDF because the page doesn't have a title when in actuality, we're not displaying Save as PDF because the UI is drawn by native code.

If you want to add an `AboutPages.isNativeAndroidUIAboutPage(final String url)` method with AboutHome and private browsing, I would not be opposed. Just remember that you wouldn't need the isAboutHome(tab) check above.

Comment 4

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

(In reply to Michael Comella (:mcomella) from comment #3)

> I don't think I like using TitlelessAboutPages - this implies that we are
> not displaying Save As PDF because the page doesn't have a title when in
> actuality, we're not displaying Save as PDF because the UI is drawn by
> native code.
> 
> If you want to add an `AboutPages.isNativeAndroidUIAboutPage(final String
> url)` method with AboutHome and private browsing, I would not be opposed.
> Just remember that you wouldn't need the isAboutHome(tab) check above.

This is not true. about:privatebrowsing is a normal HTML page. I'm sorry for jumping into this bug late, but why do we want to fix this? Is there any harm in letting someone save this page to a PDF? It works fine.

I'd rather not add more special code here. We block about:home because it's not possible to save the page as a PDF. It is a native Java UI. We block XUL pages because those pages also can't be saved to PDF.

It looks like Bryan is pretty comfortable with working in the code. If we decide to WONTFIX this bug (and I feel we should) then we should try to find a new bug for Bryan to start working on.

Comment 5

[Bryan Kok [:transfusion]]

(In reply to Michael Comella (:mcomella) from comment #3)
> nit: We don't like to add excess end-of-line whitespace to the code, if we
> cah help it - can you remove this? Let me know if you'd like an explanation.
Ahh, I've been using the Eclipse IDE to edit the code and it automatically adds a newline after the closing } before the next method directly below it.

It would be trivial to make an isAboutPrivateBrowsing() method, but...

(In reply to Mark Finkle (:mfinkle) from comment #4)
> (In reply to Michael Comella (:mcomella) from comment #3)
> This is not true. about:privatebrowsing is a normal HTML page. I'm sorry for
> jumping into this bug late, but why do we want to fix this? Is there any
> harm in letting someone save this page to a PDF? It works fine.
> 
> I'd rather not add more special code here. We block about:home because it's
> not possible to save the page as a PDF. It is a native Java UI. We block XUL
> pages because those pages also can't be saved to PDF.
> 
> It looks like Bryan is pretty comfortable with working in the code. If we
> decide to WONTFIX this bug (and I feel we should) then we should try to find
> a new bug for Bryan to start working on.

Hmm, great point. For consistency issues, perhaps ? (whether users should be allowed to save about: pages or not, but I see use for saving about:config and :reader and even "Problem Loading Page" for example, and you might as well allow them to save whatever can technically be saved at that point.. so leaving this "bug" alone for now.)

Haha, it's just that the code is logically laid out and an ease to browse through :)

P.S. I'm on IRC and idle a lot in #mobile with the nick Transfusion.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

WONTFIX, as per comment 4.

Sorry about that Bryan!