Closed Bug 1444634 Opened 2 years ago Closed 2 years ago

Replace all addressbook overlays with inlining and preprocessing

Categories

(Thunderbird :: Address Book, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 60.0

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(1 file, 1 obsolete file)

The addressbook and the sub dialogs uses some overlays too.
Attached patch addressbookOverlay.patch (obsolete) — Splinter Review
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.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8957819 - Flags: review?(jorgk)
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: '' != 'meister@example.com'.
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.
Flags: needinfo?(acelists)
Attachment #8957819 - Flags: review?(jorgk)
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
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.
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?
Flags: needinfo?(acelists)
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.
(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 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 .
(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.
Attachment #8957819 - Attachment is obsolete: true
Attachment #8957904 - Flags: feedback?(acelists)
Comment on attachment 8957904 [details] [diff] [review]
addressbookOverlay.patch

Try passed this time :-)
Attachment #8957904 - Flags: review+
Thanks to aceman for the hint. :-)
Jörg, up to you if you want to land before or after branching.
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/56248d8b301e
Replace all addressbook overlays with inlining and pre-processing. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 60.0
Attachment #8957904 - Flags: feedback?(acelists)
You need to log in before you can comment on or make changes to this bug.