Trim down macWindow.inc.xul

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

unspecified
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

10 months ago
The way non-browser windows get loaded is now a lot simpler without overlays, but I think there's still more that we could remove to make the loading process more straightforward and start paving the way to share this behavior with HTML windows.
(Assignee)

Updated

10 months ago
See Also: → 1445764
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

10 months ago
mozreview-review
Comment on attachment 8988347 [details]
Bug 1471734 - Move OSX dock menu markup to hiddenWindow.xul;

https://reviewboard.mozilla.org/r/253594/#review260338

::: commit-message-9c7bb:4
(Diff revision 1)
> +Bug 1471734 - Move OSX dock menu markup to hiddenWindow.xul;r=Gijs
> +
> +It's currently in macWindow.inc.xul which means it gets created for
> +all non-browser windows, but it's only ever set up for the hidden window.

Hrm. This took me some figuring out. Can we move the whole nonBrowserWindowStartup stuff out of browser.js into something more obviously correct / related to where this stuff lives? Doesn't need to happen here.

::: browser/base/content/hiddenWindow.xul:8
(Diff revision 1)
>  #
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  #ifdef XP_MACOSX

Huh, why do we even ship this on non-mac?
Attachment #8988347 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 5

10 months ago
mozreview-review
Comment on attachment 8988348 [details]
Bug 1471734 - Remove FULL_BROWSER_WINDOW preprocessor directive;

https://reviewboard.mozilla.org/r/253596/#review260344

I don't think this is right. On current nightly, the menu *doesn't* appear in normal windows. It only appears if you close all the windows, or switch to a non-browser window like "About Firefox".

Your patch makes it so it appears everywhere (also on non-mac) and disables it in the non-browser windows like the hidden window and "About Firefox".
Attachment #8988348 - Flags: review?(gijskruitbosch+bugs) → review-

Comment 6

10 months ago
mozreview-review
Comment on attachment 8988349 [details]
Bug 1471734 - Remove FULL_BROWSER_WINDOW preprocessor directive;

https://reviewboard.mozilla.org/r/253598/#review260348

This seems like it's the right way around, though it'll need rebasing if we drop the middle cset...
Attachment #8988349 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 7

10 months ago
(In reply to :Gijs (he/him) from comment #5)
> Comment on attachment 8988348 [details]
> Bug 1471734 - Remove MAC_NON_BROWSER_WINDOW preprocessor directive;
> 
> https://reviewboard.mozilla.org/r/253596/#review260344
> 
> I don't think this is right. On current nightly, the menu *doesn't* appear
> in normal windows. It only appears if you close all the windows, or switch
> to a non-browser window like "About Firefox".
> 
> Your patch makes it so it appears everywhere (also on non-mac) and disables
> it in the non-browser windows like the hidden window and "About Firefox".

Thank you - I misread the condition here. I think we should set it to hidden in the markup and then un-hide it for non-browser windows.
(Assignee)

Comment 8

10 months ago
(In reply to :Gijs (he/him) from comment #4)
> Comment on attachment 8988347 [details]
> Bug 1471734 - Move OSX dock menu markup to hiddenWindow.xul;
> 
> https://reviewboard.mozilla.org/r/253594/#review260338
> 
> ::: commit-message-9c7bb:4
> (Diff revision 1)
> > +Bug 1471734 - Move OSX dock menu markup to hiddenWindow.xul;r=Gijs
> > +
> > +It's currently in macWindow.inc.xul which means it gets created for
> > +all non-browser windows, but it's only ever set up for the hidden window.
> 
> Hrm. This took me some figuring out. Can we move the whole
> nonBrowserWindowStartup stuff out of browser.js into something more
> obviously correct / related to where this stuff lives? Doesn't need to
> happen here.

For sure. I'm hoping to figure this out and simplify it as part of the work tracked in Bug 1453783.

> ::: browser/base/content/hiddenWindow.xul:8
> (Diff revision 1)
> >  #
> >  # This Source Code Form is subject to the terms of the Mozilla Public
> >  # License, v. 2.0. If a copy of the MPL was not distributed with this
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> >  #ifdef XP_MACOSX
> 
> Huh, why do we even ship this on non-mac?

It's not referenced on non-mac (https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/xpfe/appshell/nsAppShellService.cpp#126-129) so the the preprocessor directive here seems removable. It looks like Bug 71895 (!) is opened to remove it from the build as per: https://searchfox.org/mozilla-central/rev/ed2763bea882619ccb48b0aecc54e523d2bdd2ae/browser/base/jar.mn#104.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

10 months ago
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?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Updated

10 months ago
Blocks: 1472751
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8988349 - Attachment is obsolete: true
(Assignee)

Comment 15

10 months ago
Moving the ni? over into Bug 1472751.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 16

10 months ago
MozReview re-flagged you for review on the FULL_BROWSER_WINDOW part. That's just a rebase after part 2 was pulled out into another bug.

Comment 17

10 months ago
mozreview-review
Comment on attachment 8988348 [details]
Bug 1471734 - Remove FULL_BROWSER_WINDOW preprocessor directive;

https://reviewboard.mozilla.org/r/253596/#review261122

r=me per comment that this is just a rebase. :-)
Attachment #8988348 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 18

10 months ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/98c95a5cac9d
Move OSX dock menu markup to hiddenWindow.xul;r=Gijs
https://hg.mozilla.org/integration/autoland/rev/6899be9874e1
Remove FULL_BROWSER_WINDOW preprocessor directive;r=Gijs
(Assignee)

Updated

10 months ago
See Also: → 1472787

Comment 19

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/98c95a5cac9d
https://hg.mozilla.org/mozilla-central/rev/6899be9874e1
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(Assignee)

Updated

10 months ago
Blocks: 1473160
You need to log in before you can comment on or make changes to this bug.