1446609 - Remove the overlays for messenger.xul except mailWindowOverlay.xul

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Richard Marti (:Paenglab)]

We have very much overlays in c-c. A big one is messenger.xul which has NINE overlays. It is not possible to only remove the overlays from messenger.xul, we need to do this on all files that use this overlays too.

I leave mailWindowOverlay as a overlay for the next bug because this is a very huge file and would make the patch of this bug much bigger than it is now.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: messenger-overlay.patch

I know it's a very huge patch but mostly it's only a shuffling of code with includes instead of overlays. Because of this I added you, Jörg and aceman, as reviewer in the hope you find more bugs than only one. This means, the code is directly in the final files instead of in a overlay that is inserted during program execution. This means too that we have now the same code in multiple files.

To make the resulting code better readable, I also changed the indentations of some files. For easier review I attach a diff -w patch.

Please use the patch applied a bit with your builds to see if it has regressions. I started also a try build to see if I have broken something: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=995e911891127dbca67248a863d054daa5989e87

Comment 2

[Richard Marti (:Paenglab)]

Attachment: The same patch without whitespace changes. Almost 100k smaller.

Comment 3

[Richard Marti (:Paenglab)]

It seems, I broke test-search-window.js. Please can you look what is wrong with my patch?

Comment 4

[Jorg K (CEST = GMT+2)]

Not right now. There are a few failures:
search-window/test-search-window.js
addrbook/test-address-book-panes.js
addrbook/test-address-book.js
folder-display/test-right-click-middle-click-messages.js

You could read the test to see what it does and then do the same action manually.

test-address-book.js fails in setupModule() at the beginning where it opens the address book window to create address books and mailing lists. Does that still work manually?

If you look at a log:
https://taskcluster-artifacts.net/NPbxq5OyS5Kuq5q2VnGqfg/0/public/logs/live_backing.log
you can see
INFO -  SUMMARY-UNEXPECTED-FAIL | test-address-book-panes.js | test-address-book-panes.js::test_hide_directory_pane
INFO -    EXCEPTION: abController.window.togglePaneSplitter is not a function

so that indicates that something is wrong with the togglePaneSplitter.

Comment 5

[Jorg K (CEST = GMT+2)]

mail/base/content/utilityOverlay.js
175 function togglePaneSplitter(splitterId)

Comment 6

[Richard Marti (:Paenglab)]

Attachment: messenger-overlay.patch

My bad, I renamed utilityOverlay.js to utility.js. But this files is referenced in many many files. I decided now to leave the name and rename it in a separate bug.

This rename was only in two files and I think the no whitespace patch need no update for this. If you want one, I can do it.

New try; https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=95b11368473cfecea69dad144b9c15587c13dcf6

Comment 7

[Jorg K (CEST = GMT+2)]

Looking better, but still two failing tests:
search-window/test-search-window.js
folder-display/test-right-click-middle-click-messages.js

The logs say:
INFO -  SUMMARY-UNEXPECTED-FAIL | test-right-click-middle-click-messages.js | test-right-click-middle-click-messages.js::test_right_click_deletion_nothing_selected
INFO -    EXCEPTION: Timeout waiting for events: [DeleteOrMoveMsgCompleted,DeleteOrMoveMsgFailed]

and

INFO -  SUMMARY-UNEXPECTED-FAIL | test-search-window.js | test-search-window.js::test_open_single_search_result_in_tab
INFO -    EXCEPTION: Message not present in view that should be there. (27: )
INFO -      at: test-folder-display-helpers.js line 2819

Maybe that test does a search and then opens a singe result in a tab. I don't know. Does that work when done manually?

Comment 8

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) from comment #7)
> Looking better, but still two failing tests:
> search-window/test-search-window.js
> folder-display/test-right-click-middle-click-messages.js
> 
> The logs say:
> INFO -  SUMMARY-UNEXPECTED-FAIL | test-right-click-middle-click-messages.js
> |
> test-right-click-middle-click-messages.js::
> test_right_click_deletion_nothing_selected
> INFO -    EXCEPTION: Timeout waiting for events:
> [DeleteOrMoveMsgCompleted,DeleteOrMoveMsgFailed]

When I move a message, I see no error.
 
> INFO -  SUMMARY-UNEXPECTED-FAIL | test-search-window.js |
> test-search-window.js::test_open_single_search_result_in_tab
> INFO -    EXCEPTION: Message not present in view that should be there. (27: )
> INFO -      at: test-folder-display-helpers.js line 2819
> 
> Maybe that test does a search and then opens a singe result in a tab. I
> don't know. Does that work when done manually?

This works here with one and multiple messages. It open one or more tabs.

Mac does sometimes not take the changed files. I'll try a Linux build: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9edb4d76b0ac5c875a958deaf6a900468f35dcc3

and build a clobbered build on my Mac to see what happens there.

Comment 9

[Jorg K (CEST = GMT+2)]

I don't think that funny things happen on TaskCluster builds any more. It's all built from scratch. I'll try the patch locally.

Comment 10

[Richard Marti (:Paenglab)]

On Mac only I get a TypeError: gFolderDisplay is null[Learn More]  msgViewPickerOverlay.js:103:1
I need to look where this comes from.

Comment 11

[Jorg K (CEST = GMT+2)]

I haven't done a clobber build, but I can't delete messages using the context menu any more. That's what test-right-click-middle-click-messages.js is doing, there is a delete_via_popup() call.

Comment 12

[Richard Marti (:Paenglab)]

Attachment: messenger-overlay.patch

Found the delete issue. Please can you retest?

Please can you check what is failing with the test-search-window.js test? Then I can look into it.

Comment 13

[Jorg K (CEST = GMT+2)]

OK, manual deletion via context menu works now and
  mozmake SOLO_TEST=folder-display/test-right-click-middle-click-messages.js mozmill-one
passes. You Linux try also shows folder-tree-modes/test-mode-switching.js failing, but that passes locally here.

search-window/test-search-window.js still fails with 6 of 12 subtests failing. The failing ones are:
test_open_single_search_result_in_tab
test_open_multiple_search_results_in_new_tabs
test_open_search_result_in_new_window
test_open_search_result_in_existing_window
test_close_search_window
test_verify_saved_search

Some might be subsequent errors. Let me try it manually.

Comment 14

[Jorg K (CEST = GMT+2)]

Well, trying search manually works, one or more messages open in tabs, etc.

The Mozmill test fails at
test_open_single_search_result_in_tab test-search-window.js:127
assert_selected_and_displayed(msgHdr);
where it checks that a message with a certain header is displayed.

If it's not successful, it logs
EXCEPTION: Message not present in view that should be there. (27: )
where 27 is the number of the message in the folder and after the colon (:) you should see the subject of the message.

So something is subtly wrong with message header. Interestingly there are many tests using
assert_selected_and_displayed(msgHdr)
but only this one here fails.

Watching the test, an empty windows opens when I assume a window that shows the message content should have opened.

Comment 15

[Richard Marti (:Paenglab)]

Attachment: Patch without white space changes

Comment 16

[Richard Marti (:Paenglab)]

Attachment: messenger-overlay.patch

Adding the two functions from the removed reference of mailOverlay.xul to mailWindow.js seems to fix the tests. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=01b9e0a22274972ffcc640c8e62c71b019c5204d

Thank you, aceman, for pointing to it.

Comment 17

[Richard Marti (:Paenglab)]

Attachment: Patch without white space changes

Comment 18

[Jorg K (CEST = GMT+2)]

Hmm, on Mac the search failure was in the Z2 which didn't run, so I retriggered it.

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 8959926 [details] [diff] [review]
messenger-overlay.patch

Sorry, the test failure is still there.

Comment 20

[Richard Marti (:Paenglab)]

As nobody sees this failure when doing it manually, I suspect something is wrong with the test.

Comment 21

[Jorg K (CEST = GMT+2)]

Hard to say without debugging it. The test fails on assert_selected_and_displayed(msgHdr), so something related to message headers. In the manual test we don't see or check any message headers.

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: messenger-overlay.patch - rebased

Comment 23

[Richard Marti (:Paenglab)]

Attachment: messenger-overlay.patch

I found a missing dtd rename in safeMode.xul.

Comment 24

[Jorg K (CEST = GMT+2)]

I'm looking at test-search-window.js again. I've disabled the subtests
  test_open_single_search_result_in_tab(),
  test_open_multiple_search_results_in_new_tabs(),
  test_open_search_result_in_new_window() and
  test_open_search_result_in_existing_window()
which all fail.

Then I came to test_close_search_window() which also fails. This test is dead simple:

function test_close_search_window() {
  swc.window.focus();
  // now close the search window
  plan_for_window_close(swc);
  swc.keypress(null, "VK_ESCAPE", {});
  wait_for_window_close(swc);
  swc = null;
}

It simply presses ESC on the search window. Well, that doesn't work, even when doing it manually. Perhaps that's a clue.

Comment 25

[Richard Marti (:Paenglab)]

Attachment: messenger-overlay.patch

This fixes the search dialog close with ESC key.

Comment 26

[Jorg K (CEST = GMT+2)]

Richard found a fix for the failing test_close_search_window() test, a subtle re-ordering of keys in SearchDialog.xul:
  <keyset id="mailKeys">
    <key key="&closeCmd.key;" modifiers="accel" oncommand="window.close();"/>
    <key keycode="VK_ESCAPE" oncommand="window.close();"/>
    <key id="key_delete" oncommand="goDoCommand('cmd_delete');"/>

I've restored <?xul-overlay href="chrome://communicator/content/utilityOverlay.xul"?> in SearchDialog.xul, and the tests pass again. So there is something about the removal that causes this.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: messenger-overlay.patch

This fixes the test failures. The observation is that utilityOverlay.xul had:

#ifdef XP_MACOSX
  <key id="key_delete" keycode="VK_BACK"     command="cmd_delete"/>
  <key id="key_delete2" keycode="VK_DELETE"  command="cmd_delete"/>
#else
  <key id="key_delete" keycode="VK_DELETE" command="cmd_delete"/>
#endif

If I'm not mistaken, that overrode the definitions in SearchDialog.xul. If we take away the overlay, we must apply this change directly.

Comment 28

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b5719f411a04e8be98e8271bae8549c47c8cef20

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 8962088 [details] [diff] [review]
messenger-overlay.patch

Oops, please don't rename .dtd files since that will cause retranslation of all the strings contained in them :-(

Comment 30

[Jorg K (CEST = GMT+2)]

Wait a second, we renamed .dtd files in bug 1444634:
https://hg.mozilla.org/comm-central/rev/56248d8b301e#l10.1

Did that cause any re-translation, I can't make sense of bug 1357707 comment #40, maybe they just moved the files? Or maybe not all translators are as well-versed as Onno.

Comment 31

[Jorg K (CEST = GMT+2)]

Francesco, following the discussion in bug 1444926 comment #20: Will renaming of DTD files cause retranslation? We're removing overlays and would prefer to rename xyzOverlay.dtd to xyz.dtd. We have already done so in one case, see comment #30. One localiser just moved (or copied?) the files:
https://hg.mozilla.org/l10n-central/nl/rev/b4bcbe000383d8b94248d8e5ed38c8a485bb2018

Comment 32

[Richard Marti (:Paenglab)]

Attachment: messenger-overlay.patch without locale file rename

The same patch without locale file rename. Already rebased after bug 1448600.

To be sure I haven't broken something startet a try build: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1991d9c8d253842e24407e6b8436216661b71bbe

Comment 33

[Jorg K (CEST = GMT+2)]

Looks like some locales re-translated, at least this one with some surprising results:

Original overlay:
https://hg.mozilla.org/l10n-central/it/file/tip/mail/chrome/messenger/addressbook/abCardOverlay.dtd
<!ENTITY WorkPhone.label                "Lavoro:">
<!ENTITY WorkPhone.accesskey            "v">
<!ENTITY HomePhone.label                "Abitazione:">
<!ENTITY HomePhone.accesskey            "b">
<!ENTITY FaxNumber.label                "Fax:">
<!ENTITY FaxNumber.accesskey            "x">
<!ENTITY PagerNumber.label              "Cerca persone:">
<!ENTITY PagerNumber.accesskey          "o">
<!ENTITY CellularNumber.label           "Cellulare:">
<!ENTITY CellularNumber.accesskey       "u">

New translation:
https://hg.mozilla.org/l10n-central/it/file/tip/mail/chrome/messenger/addressbook/abCard.dtd
<!ENTITY WorkPhone.label                "Lavoro:">
<!ENTITY WorkPhone.accesskey            "l">
<!ENTITY HomePhone.label                "Casa:">
<!ENTITY HomePhone.accesskey            "s">
<!ENTITY FaxNumber.label                "Fax:">
<!ENTITY FaxNumber.accesskey            "x">
<!ENTITY PagerNumber.label              "Cercapersone:">
<!ENTITY PagerNumber.accesskey          "p">
<!ENTITY CellularNumber.label           "Cellulare:">
<!ENTITY CellularNumber.accesskey       "e">

Comment 34

[Francesco Lodolo [:flod]]

Yes, changing a name involves retranslating for anyone not using mercurial, which means all of Thunderbird localizers minus 2/3 teams.

As for example above, Casa/Abitazione are synonyms, "Cercapersone" is a better spelling than "Cerca persone".

Comment 35

[Jorg K (CEST = GMT+2)]

I looked at some other locales and also es-ES and fr retranslated with differences:
https://hg.mozilla.org/l10n-central/fr/file/tip/mail/chrome/messenger/addressbook/abCardOverlay.dtd
https://hg.mozilla.org/l10n-central/fr/file/tip/mail/chrome/messenger/addressbook/abCard.dtd

https://hg.mozilla.org/l10n-central/es-ES/file/tip/mail/chrome/messenger/addressbook/abCardOverlay.dtd
https://hg.mozilla.org/l10n-central/es-ES/file/tip/mail/chrome/messenger/addressbook/abCard.dtd

I've also written to the l10n mailing list:
https://groups.google.com/d/msg/mozilla.dev.l10n/yCA_4qiJWGc/BIimkRiaAgAJ

Let's land it without renaming the DTD files for now.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6bea4bcfa60f
Remove the overlays for messenger.xul except mailWindowOverlay.xul. r=jorgk,aceman

Comment 37

[Joachim Herb]

My addon (CompactHeader) is overlaying msgHdrViewOverlay.xul:
overlay chrome://messenger/content/msgHdrViewOverlay.xul chrome://CompactHeader/content/compactHeaderOverlay.xul

Do I have to change this? Or the other way around, why is the addon still working with the nightly builds of today, e.g. 20180321051647 (Windows 64) or 20180321051647 (Linux 64), even though msgHdrViewOverlay had been removed.

Comment 38

[Jorg K (CEST = GMT+2)]

Umm, today is the 26th, well, 27th, and you're testing with binaries of the 21st: 2018 03 *21* 051647.

msgHdrViewOverlay.xul has indeed been removed:
https://hg.mozilla.org/comm-central/rev/6bea4bcfa60f#l22.22

Dailies are broken, if you want to test, please read:
https://mail.mozilla.org/pipermail/tb-planning/2018-March/005959.html
https://mail.mozilla.org/pipermail/tb-planning/2018-March/005972.html

Comment 39

[Richard Marti (:Paenglab)]

(In reply to Joachim Herb from comment #37)
> My addon (CompactHeader) is overlaying msgHdrViewOverlay.xul:
> overlay chrome://messenger/content/msgHdrViewOverlay.xul
> chrome://CompactHeader/content/compactHeaderOverlay.xul
> 
> Do I have to change this? Or the other way around, why is the addon still
> working with the nightly builds of today, e.g. 20180321051647 (Windows 64)
> or 20180321051647 (Linux 64), even though msgHdrViewOverlay had been removed.

Yes, we have removed msgHdrViewOverlay.xul to not use overlays. I changed your extension to use messenger.xul instead and it works again.