Closed Bug 1034385 Opened 7 years ago Closed 7 years ago

Empty private tabs panel can be saved as pdf

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: mcomella, Assigned: transfusion, Mentored)

Details

(Whiteboard: [lang=java])

Attachments

(1 file)

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
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...
Assignee: nobody → bryan.wyern1
Status: NEW → ASSIGNED
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.
Attachment #8453398 - Flags: feedback+
(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.
(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.
WONTFIX, as per comment 4.

Sorry about that Bryan!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.