1444634 - Replace all addressbook overlays with inlining and preprocessing

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Richard Marti (:Paenglab)]

The addressbook and the sub dialogs uses some overlays too.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: addressbookOverlay.patch

I've also removed all "Overlay" from the files. This affects also one file from the previous de-overlay patches. Because of this, this patch needs to be applied as last of them.

I also removed abResultsPaneOverlay.xul from packaging because it's not used in TB.

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 8957819 [details] [diff] [review]
addressbookOverlay.patch

This doesn't work, I have a test failure now:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6df2a8159b72c957cc444eb147ab016137dda55a

mozmill/message-window/test-vcard-actions.js | test-vcard-actions.js::test_check_vcard_icon

Something wrong with a vCard icon, the comment in the failing test says:
Check if clicking attached vcard image opens new card dialog and adds a contact.

There is a sample message in bug 1374779, I tried that and it's still working, so I don't know why the test fails, it says:

INFO -  SUMMARY-UNEXPECTED-FAIL | test-vcard-actions.js | test-vcard-actions.js::test_check_vcard_icon
INFO -    EXCEPTION: a != b: '' != '[e-mail]'.
INFO -      at: test-folder-display-helpers.js line 2913
INFO -         assert_true test-folder-display-helpers.js:2913 11
INFO -         assert_equals test-folder-display-helpers.js:2900 3
INFO -         subtest_check_card test-vcard-actions.js:40 5

Perhaps consult Aceman.

Comment 3

[Jorg K (CEST = GMT+2)]

Oh, and another failure:
mozmill/addrbook/test-update-mailing-list.js | test-update-mailing-list.js::test_contact_in_mailing_list_updated

INFO -  SUMMARY-UNEXPECTED-FAIL | test-update-mailing-list.js | test-update-mailing-list.js::test_contact_in_mailing_list_updated
INFO -    EXCEPTION: EditCardOKButton is not defined
INFO -      at: dialog.xml line 407 > Function line 3
INFO -         anonymous dialog.xml line 407 > Function:3 1

Comment 4

[Richard Marti (:Paenglab)]

Jörg, could you try with removing all files in obj-dir dist\bin\chrome\messenger\content\messenger\addressbook and the messenger.manifest file and build again. Then run the tests again? Maybe it's again a issue with the old files laying in the obj-dir.

Comment 5

[:aceman]

Jorg pasted the failures from the try server, which should have a clean build.
Both tests pass for me locally on Linux. Could there be OS X specific problem/difference?

Comment 6

[:aceman]

Comment on attachment 8957819 [details] [diff] [review]
addressbookOverlay.patch

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

::: mail/components/addrbook/content/abCard.inc
@@ +1,3 @@
> +# 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/.

Will this file be preprocessed? The parent including it will, but this one? Have you confirmed the comments are removed?

::: mail/components/addrbook/content/abEditCardDialog.xul
@@ +9,2 @@
>  
> +<!DOCTYPE overlay SYSTEM "chrome://messenger/locale/addressbook/abCard.dtd">

Why 'overlay'? Did you mean 'dialog' as that is the main document tag here?

@@ +18,5 @@
> +  <stringbundle id="bundle_addressBook" src="chrome://messenger/locale/addressbook/addressBook.properties"/>
> +</stringbundleset>
> +
> +<script type="application/javascript" src="chrome://messenger/content/addressbook/abCommon.js"/>
> +<script type="application/javascript" src="chrome://messenger/content/addressbook/abCardOverlay.js"/>

Wrong indent in all the added lines.

::: mail/components/addrbook/content/abNewCardDialog.xul
@@ +24,5 @@
> +  <stringbundle id="bundle_addressBook" src="chrome://messenger/locale/addressbook/addressBook.properties"/>
> +</stringbundleset>
> +
> +<script type="application/javascript" src="chrome://messenger/content/addressbook/abCommon.js"/>
> +<script type="application/javascript" src="chrome://messenger/content/addressbook/abCardOverlay.js"/>

Wrong indent.

Comment 7

[:aceman]

(In reply to :aceman from comment #5)
> Jorg pasted the failures from the try server, which should have a clean
> build.
> Both tests pass for me locally on Linux. Could there be OS X specific
> problem/difference?

Sorry, I actually ran without the patch applied, I thought it is already landed :)

With the patch, I see the failures too.

Comment 8

[:aceman]

Comment on attachment 8957819 [details] [diff] [review]
addressbookOverlay.patch

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

::: mail/components/addrbook/content/abNewCardDialog.xul
@@ +24,5 @@
> +  <stringbundle id="bundle_addressBook" src="chrome://messenger/locale/addressbook/addressBook.properties"/>
> +</stringbundleset>
> +
> +<script type="application/javascript" src="chrome://messenger/content/addressbook/abCommon.js"/>
> +<script type="application/javascript" src="chrome://messenger/content/addressbook/abCardOverlay.js"/>

This may be the bug, you renamed the file to abCard.js .

Comment 9

[Richard Marti (:Paenglab)]

Attachment: addressbookOverlay.patch

(In reply to :aceman from comment #6)
> Comment on attachment 8957819 [details] [diff] [review]
> addressbookOverlay.patch
> 
> Review of attachment 8957819 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/components/addrbook/content/abCard.inc
> @@ +1,3 @@
> > +# 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/.
> 
> Will this file be preprocessed? The parent including it will, but this one?
> Have you confirmed the comments are removed?

They are removed.

> ::: mail/components/addrbook/content/abEditCardDialog.xul
> @@ +9,2 @@
> >  
> > +<!DOCTYPE overlay SYSTEM "chrome://messenger/locale/addressbook/abCard.dtd">
> 
> Why 'overlay'? Did you mean 'dialog' as that is the main document tag here?

Copy/paste error. Changed to "dialog"

> @@ +18,5 @@
> > +  <stringbundle id="bundle_addressBook" src="chrome://messenger/locale/addressbook/addressBook.properties"/>
> > +</stringbundleset>
> > +
> > +<script type="application/javascript" src="chrome://messenger/content/addressbook/abCommon.js"/>
> > +<script type="application/javascript" src="chrome://messenger/content/addressbook/abCardOverlay.js"/>
> 
> Wrong indent in all the added lines.

Fixed.

> ::: mail/components/addrbook/content/abNewCardDialog.xul
> @@ +24,5 @@
> > +  <stringbundle id="bundle_addressBook" src="chrome://messenger/locale/addressbook/addressBook.properties"/>
> > +</stringbundleset>
> > +
> > +<script type="application/javascript" src="chrome://messenger/content/addressbook/abCommon.js"/>
> > +<script type="application/javascript" src="chrome://messenger/content/addressbook/abCardOverlay.js"/>
> 
> Wrong indent.

Fixed.

(In reply to :aceman from comment #8)
> Comment on attachment 8957819 [details] [diff] [review]
> addressbookOverlay.patch
> 
> Review of attachment 8957819 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/components/addrbook/content/abNewCardDialog.xul
> @@ +24,5 @@
> > +  <stringbundle id="bundle_addressBook" src="chrome://messenger/locale/addressbook/addressBook.properties"/>
> > +</stringbundleset>
> > +
> > +<script type="application/javascript" src="chrome://messenger/content/addressbook/abCommon.js"/>
> > +<script type="application/javascript" src="chrome://messenger/content/addressbook/abCardOverlay.js"/>
> 
> This may be the bug, you renamed the file to abCard.js .

Thanks to spot this. Fixed.

Comment 10

[Jorg K (CEST = GMT+2)]

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

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8957904 [details] [diff] [review]
addressbookOverlay.patch

Try passed this time :-)

Comment 12

[Richard Marti (:Paenglab)]

Thanks to aceman for the hint. :-)
Jörg, up to you if you want to land before or after branching.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/56248d8b301e
Replace all addressbook overlays with inlining and pre-processing. r=jorgk