592190 - [PATCH] Disable some menu items that do not make sense for some "about:" internal URIs

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Chris Peterson [:cpeterson]]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en; rv:1.9.2.10pre) Gecko/20100828 Camino/2.1a1pre (like Firefox/3.6.10pre)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en; rv:1.9.2.10pre) Gecko/20100828 Camino/2.1a1pre (like Firefox/3.6.10pre)

Some "about:" internal URIs have menu items enabled that do not make sense. For example, "about:blank" can be printed or "about:bookmarks" can be bookmarked! And "about:config" has an enabled Print menu item, but then displays a "Printer error" dialog.

The menu items in question:

* File > Page Setup
* File > Print
* View > Reload Page
* View > Show Page Info
* Bookmarks > Bookmark Current Page


Reproducible: Always

Actual Results:  
The following menu items do not make sense for some "about:" internal URIs, but are surprising enabled:

* about:blank? Page Setup, Print, and Show Page Info
* about:bookmarks? Bookmark Current Page
* about:config? Page Setup, Print (but displays "Printer Error" dialog), Show Page Info, and Bookmark Current page
* about:history? Bookmark Current Page
* about:neterror? Page Setup, Print, Show Page Info, and Bookmark Current Page
* about:plugins? Page Setup, Print, Show Page Info, and Bookmark Current Page
* about:safebrowsingblocked? Page Setup, Print, Reload Page, Show Page Info, and Bookmark Current Page


Expected Results:  
This patch disables the following menu items for this internal URIs:

* about:blank? DISABLES Page Setup, Print, and Show Page Info
* about:bookmarks? DISABLES Bookmark Current Page
* about:config? DISABLES Page Setup, Print, Show Page Info, and Bookmark Current page
* about:history? DISABLES Bookmark Current Page
* about:neterror? DISABLES Show Page Info and Bookmark Current Page (but keep ENABLED Page Setup and Print in case user wants to share net error message with someone?)
* about:plugins? DISABLES Show Page Info and Bookmark Current Page (but keep ENABLED Page Setup and Print in case user wants a record of their installed plugins?)
* about:safebrowsingblocked? DISABLES Reload Page, Show Page Info, and Bookmark Current Page (but keep ENABLED Page Setup and Print in case user wants to share phishing error message with someone?)

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: disable-about-menu-items-v1.patch

Comment 2

[Samuel Sidler (:ss)]

Regarding about:bookmarks and about:history, I think at one point we decided that we should allow bookmarking them because users could place them throughout the bookmark bar instead of being limited to the navigation bar.

Comment 3

[philippe (part-time)]

see bug 587552 comment 4 for the most recent updates /changes to those menu items (if you hadn't seen it already).

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #0)
> Some "about:" internal URIs have menu items enabled that do not make sense. 

When a new contributor shows up and starts out fixing menu validation bugs, that's always a good sign :-)

Some additional drive-by comments on the design:

> Expected Results:  
> This patch disables the following menu items for this internal URIs:

Four years ago, cl, froodian and I did a (mostly-)comprehensive look at menu item validation and validation for internal URIs.  By and large we fixed things up,  although it appears we never got around to Page Setup/Print and Page Info and there have been some new about: pages added that probably didn't pick up additional validation.

You can see the results in http://wiki.caminobrowser.org/Development:Planning:Menu_item_validation and http://wiki.caminobrowser.org/Development:Planning:Internal_URIs (though I can't guarantee they still represent our current thoughts on every case).

> * about:blank? DISABLES Page Setup, Print, and Show Page Info

This seems fine.

> * about:bookmarks? DISABLES Bookmark Current Page

We don't want this; see comment 2.

> * about:config? DISABLES Page Setup, Print, Show Page Info, and Bookmark
> Current page

We should allow bookmarking all about: pages that can be loaded safely and successfully.

> * about:history? DISABLES Bookmark Current Page

See about:bookmarks above.

> * about:neterror? DISABLES Show Page Info and Bookmark Current Page (but keep
> ENABLED Page Setup and Print in case user wants to share net error message with
> someone?)

I left bookmarking enabled when fixing bug 587552 so that people can bookmark pages they've tried to visit but failed due to network error (i.e., so they can try again later), and Print/Page Setup for the reason you mentioned.

Page Info can go, though.

> * about:plugins? DISABLES Show Page Info and Bookmark Current Page (but keep
> ENABLED Page Setup and Print in case user wants a record of their installed
> plugins?)

Only disable Page Info.

> * about:safebrowsingblocked? DISABLES Reload Page, Show Page Info, and Bookmark
> Current Page (but keep ENABLED Page Setup and Print in case user wants to share
> phishing error message with someone?)

Most of this was already handled by bug 587552 (printing doesn't work on load-blocked pages, anyway), the exception being (surprise!) Page Info.

Have you checked other "recent" about: additions, like about:crashes?

In general, the test should be something like this:

1) Is it safe to do action <foo> on URI <bar>?
2) If it's safe, does action <foo> work properly on URI <bar>?
3) If it works properly, does action <foo> produce useful results on URI <bar>?

E.g., it's part 3 of the test that hangs up Page Info everywhere, part 1 that drops things like bookmarking from safebrowsingblocked pages, and part 2 that knocks out printing on about:config (and, as an example from back when, "Email Page Location" on all internal URIs).

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: internal-uri-menu-validation-v2.patch

Incorporate Smokey's feedback.

Comment 6

[Chris Peterson [:cpeterson]]

This new patch should more closely implement the menu validation guidelines in Camino wiki, adding some exceptions for printing or reloading of some internal URIs.

Here is a description of the patch's changes to meet the wiki's guidelines or add new exceptions:

** BrowserWindowController.mm:
1. Disable Show Tab Overview if window has fewer than two tabs.
2. Disable Page Info for all about: URIs _except_ about:certerror, about:neterror, and about:safebrowsingblocked.
3. Disable Save As for all about: URIs.
4. Enable Print and Page Setup for about:safebrowsingblocked.
5. Disable Print and Page Setup for about:config.

** BrowserWrapper.mm:
1. Enable bookmarking of about:blank.
2. Disable reloading of about:buildconfig and about:safebrowsingblocked.

I tested the internal URIs reported on the about:about page:
about:about
about:blank
about:bookmarks
about:buildconfig
about:cache
about:cache-entry
about:certerror
about:config
about:crashes
about:history
about:logo
about:mozilla
about:neterror
about:plugins
about:safebrowingblocked
view-source:

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #6)
> Here is a description of the patch's changes to meet the wiki's guidelines or
> add new exceptions:

Hmm, I should have read through the wiki more thoroughly first and made changes to bring it more up-to-date :(

> ** BrowserWindowController.mm:
> 1. Disable Show Tab Overview if window has fewer than two tabs.

We've explicitly WONTFIXed that before: bug 467508.  Stuart has a slightly different test in bug 467508 comment 1 than I gave in comment 3 above.

> 2. Disable Page Info for all about: URIs _except_ about:certerror,
> about:neterror, and about:safebrowsingblocked.

I'm not sure why we'd leave this enabled for these three, given that Page Info exposes a mish-mash of information on them, part from the overlay and part from the underlying page.

> 3. Disable Save As for all about: URIs.

Other than about:config and bookmarks/history, where this is already disabled, it's harmless to allow people to save their list of crash reports or installed plug-ins or whatnot.

> 4. Enable Print and Page Setup for about:safebrowsingblocked.

We disabled these for parity with Safari in bug 501246; we should discuss whether we want to enable them before flipping that.  Since the load is blocked, it's probably harmless to allow printing of the overlay/error messabges (which may not have been the case in Safari).

> 5. Disable Print and Page Setup for about:config.

Sounds good.

> ** BrowserWrapper.mm:
> 1. Enable bookmarking of about:blank.

I wasn't careful enough in wording my previous comment on this; I forgot about about:blank :(

about:blank never actually exists as far as the user is concerned (i.e., unlike most other about: pages, the URI never appears in the location bar), and suddenly bookmarking "empty" pages seems like a bad thing user-experience-wise.  There are also other checks throughout the code that enabling this will either break or leave inconsistent.  It's been disabled since 2002, so absent a very compelling argument otherwise, we ought to leave it that way.

> 2. Disable reloading of about:buildconfig and about:safebrowsingblocked.

We want to leave Reload enabled on safebrowsingblocked; see bug 501246 comment 3.  I don't care one way or the other about buildconfig, though I suppose the path of least resistance is to leave it alone.

Sorry I keep contradicting previous comments; we haven't thought about this comprehensively since 2006, and there's been a lot of time and code and ad-hoc changes since then that we need to coalesce. :(

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: internal-uri-menu-validation-v3.patch

* BrowserWindowController.mm:
1. Disable Page Info for all about: URIs (including about:certerror, about:neterror, and about:safebrowsingblocked).
2. Disable Save for about:blank and about:config.
3. Disable Print and Page Setup for about:blank and about:config.

Camino's default new tab location is a blank page, so disabling nonsensical menu items like Save or Print for about:blank might be nice.

Comment 9

[Chris Peterson [:cpeterson]]

Thanks for taking the time to review these changes. I didn't mean to create a lot of extra work for you regarding low priority changes like menu validation. <:)

I think this patch v3 will address the issues you raised.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #9)
> Thanks for taking the time to review these changes. I didn't mean to create a
> lot of extra work for you regarding low priority changes like menu validation.
> <:)

No worries; thanks for bearing with me through all the iterations!

> I think this patch v3 will address the issues you raised.

The set of items validated looks good to me; cl can review the actual implementation now :)

Comment 11

[Chris Lawson (gone)]

>+    BrowserWrapper* browser = [self browserWrapper];
>+    NSString* currentURI = [[browser currentURI] lowercaseString];

We use these two lines, or some variant of them, a LOT in this method now. Why not just declare |browser| and |currentURI| as above, but as local variables at the beginning of |validateActionBySelector:|, and then re-use them throughout? Seems like that'd save some cycles.

Otherwise, this looks reasonable, applies properly, builds properly, and works as described in comment 8. r=me regardless of whether you make the variable changes or not.

Comment 12

[Chris Lawson (gone)]

(In reply to comment #8)
> Camino's default new tab location is a blank page, so disabling nonsensical
> menu items like Save or Print for about:blank might be nice.

To be clear, the patch does this. I agree it's a good idea, but I wanted to make sure Smokey was OK with it, since it wasn't ever explicitly discussed above.

cl

Comment 13

[Chris Peterson [:cpeterson]]

Attachment: internal-uri-menu-validation-v4.patch

internal-uri-menu-validation-v4.patch incorporates cl's suggestion to consolidate the browserWrapper and currentURI getters at the top of the validateActionBySelector method.

Comment 14

[Stuart Morgan]

Comment on attachment 476697 [details] [diff] [review]
internal-uri-menu-validation-v4.patch

> Why not just declare |browser| and |currentURI| as above,
> but as local variables at the beginning of
> |validateActionBySelector:|, and then re-use them throughout?
> Seems like that'd save some cycles.

Except that it won't; it will actually waste cycles. That method is called for each menu item, and on any given call only one of the |if| blocks will be true. Now instead of doing it once only when necessary, it's being done once for every single menu item even though most don't need it.

So, v3 of this patch is the way to go, not v4.

>+            ![currentURI isEqualToString:@"about:blank"] &&

It would be better to use our NSString category isBlankURL here and below.

sr=smorgan with those changes.

Comment 15

[Chris Peterson [:cpeterson]]

Attachment: internal-uri-menu-validation-v5.patch

1. Move browser and currentURI initialization out of the hot path (like patch v3)
2. Replace isEqualToString with isBlankURL

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/a7271ccea64b

Thanks again for sticking with this patch in face of all the partially-thought-through feedback!