Closed Bug 1442045 Opened 2 years ago Closed 2 years ago

macwindowmenu.inc removal

Categories

(Thunderbird :: Toolbars and Tabs, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 60.0

People

(Reporter: bdahl, Assigned: Paenglab)

Details

Attachments

(1 file)

Over in bug 1441378 we're planning to remove macwindowmenu.inc. You'll probably want to fork the file or inline it like we did in Firefox.
Philipp, I'm asking you for r? because you the whole overlay mechanism very good and this patch has also some Mac specialties.

Instead of overlays (baseMenuOverlay.xul and macMenuOverlay.xul) I'm using now includes (helpMenu.inc and macWindowMenu.inc).

The viewSourceOverlay.xul is a bit special with the help menu as I think this window doesn't need all help item like "Restart with Add-ons disabled" etc. I've added only the "TB Help Contents".
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8955310 - Flags: review?(philipp)
Bug 1441378 is in autoland and will bust us because of the removal of toolkit/content/macWindowMenu.inc.

Jörg, maybe you can look at this patch and Philipp does a post review on Mac. All I can say is, it works on Mac too.
Comment on attachment 8955310 [details] [diff] [review]
helpIncludes.patch

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

I'll try it now to get it landed before 12:00 PM.

::: mail/base/content/mailWindowOverlay.xul
@@ +54,5 @@
>  <script type="application/javascript" src="chrome://global/content/printUtils.js"/>
>  <script type="application/javascript" src="chrome://messenger/content/msgViewPickerOverlay.js"/>
>  <script type="application/javascript" src="chrome://messenger/content/plugins.js"/>
>  <script type="application/javascript" src="chrome://global/content/viewZoomOverlay.js"/>
> +<script type="application/javascript" src="chrome://communicator/content/utilityOverlay.js"/>

What's "communicator" doing here?
(In reply to Jorg K (GMT+1) from comment #3)
> Comment on attachment 8955310 [details] [diff] [review]
> helpIncludes.patch
> 
> Review of attachment 8955310 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'll try it now to get it landed before 12:00 PM.
> 
> ::: mail/base/content/mailWindowOverlay.xul
> @@ +54,5 @@
> >  <script type="application/javascript" src="chrome://global/content/printUtils.js"/>
> >  <script type="application/javascript" src="chrome://messenger/content/msgViewPickerOverlay.js"/>
> >  <script type="application/javascript" src="chrome://messenger/content/plugins.js"/>
> >  <script type="application/javascript" src="chrome://global/content/viewZoomOverlay.js"/>
> > +<script type="application/javascript" src="chrome://communicator/content/utilityOverlay.js"/>
> 
> What's "communicator" doing here?

It was removed from here: https://dxr.mozilla.org/comm-central/source/mail/base/content/baseMenuOverlay.xul#16 and needs to be added to work. utilityOverlay.js is placed from mail to communicator here: https://dxr.mozilla.org/comm-central/source/mail/base/jar.mn#141
Hmm, Windows is still working without the patch, so that must only affect Mac.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/bf7adc1ba8ee
Replace baseMenuOverlay.xul, macMenuOverlay.xul and macWindowMenu.inc with inlining and preprocessing. rs=jorgk
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Yes it affects only Mac because of the removal of macwindowmenu.inc. The other parts are to remove the overlays.
My build was so slow that I didn't get to test before pushing. So I had to trust Swiss quality. Now my build is finished and it still appears to be working on Windows, although I'm not sure where to look. I checked the help menus on the main window, compose window, address book window and view source window and that one doesn't appear to work, it only shows "Help Contents". Looks like we need a follow-up here.
Whiteboard: [PLR]
(In reply to Jorg K (GMT+1) from comment #8)
> My build was so slow that I didn't get to test before pushing. So I had to
> trust Swiss quality. Now my build is finished and it still appears to be
> working on Windows, although I'm not sure where to look. I checked the help
> menus on the main window, compose window, address book window and view
> source window and that one doesn't appear to work, it only shows "Help
> Contents". Looks like we need a follow-up here.

Iwrote in comment #1:
> 
> The viewSourceOverlay.xul is a bit special with the help menu as I think
> this window doesn't need all help item like "Restart with Add-ons disabled"
> etc. I've added only the "TB Help Contents".

Do we really need release notes etc. in this window? It's not a main window and all what would be needed is this item. Or am I wrong?
Sorry, I missed that comment. How about removing the Help menu from that window altogether?
Target Milestone: --- → Thunderbird 60.0
TI think this help menu entry can stay. Then the user can use it when he searches something the source shows. And on Mac then the menu search field is also accessible.
Attachment #8955310 - Flags: review?(philipp) → review+
You need to log in before you can comment on or make changes to this bug.