1472751 - Remove MAC_NON_BROWSER_WINDOW preprocessor directive

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Brian Grinstead [:bgrins]]

+++ This bug was initially created as a clone of Bug #1471734 +++

Going to spin this patch out of Bug 1471734 since the other two changesets can land independently.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1472751 - Remove MAC_NON_BROWSER_WINDOW preprocessor directive;

The menuitem used to be only included in the markup for the non-browser window
case. This changes it to include it as a hidden <menuitem> for the browser window
case, and then unhide it for the non-browser window case.

Review commit: https://reviewboard.mozilla.org/r/254272/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254272/

Comment 2

[Brian Grinstead [:bgrins]]

Was thinking to add a test to make sure the openLocation menu item is hidden in browser windows and unhidden in non-browser windows but I couldn't think of a good place to put it. This is the type of thing I would have usually dumped in general/... maybe a new folder like 'windowing' or something? Or can you think of a good existing place to put it?

Comment 3

[:Gijs (he/him)]

Comment on attachment 8989227 [details]
Bug 1472751 - Remove MAC_NON_BROWSER_WINDOW preprocessor directive;

https://reviewboard.mozilla.org/r/254272/#review261124

::: browser/base/content/browser.js:1969
(Diff revision 1)
>        if (element)
>          element.setAttribute("disabled", "true");
>      }
>  
> +    // Show menus that are only visible in non-browser windows
> +    var shownItems = ["menu_openLocation"];

Nit: let

::: browser/base/content/browser.js:1972
(Diff revision 1)
> +      if (element)
> +        element.removeAttribute("hidden");

This is fine, though I wonder how much of this JS (and the other JS for hidden/non-hidden items) we could replace with judicious use of CSS.

Comment 4

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Was thinking to add a test to make sure the openLocation menu item is hidden
> in browser windows and unhidden in non-browser windows but I couldn't think
> of a good place to put it. This is the type of thing I would have usually
> dumped in general/... maybe a new folder like 'windowing' or something? Or
> can you think of a good existing place to put it?

I think we just shouldn't add a test for it. People will notice it pretty quickly if it starts showing up in browser windows, and if it disappears from non-browser windows, it's basically an alias for new window or new tab, which seems like a more natural thing to do in that case anyway. I might even argue that we should get rid of the menuitem entirely...  :-)

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8989227 [details]
Bug 1472751 - Remove MAC_NON_BROWSER_WINDOW preprocessor directive;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254272/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7750a13f9b5
Remove MAC_NON_BROWSER_WINDOW preprocessor directive;r=Gijs

Comment 7

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/d7750a13f9b5