1589922 - Change OnItem{Unichar}PropertyChanged to use smart strings

Status: RESOLVED FIXED

(MailNews Core :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

If seen this while working on bug 1575512. It's a little stone-age and up for some renovation.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1589922-prop-changed-folders.patch

Part 1 for nsIFolderListener.idl.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1589922-prop-changed-AB.patch

Part 2 for AB.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=10f3722fce98c43366cc56a114dd0cd5e45d9ff6

Note: I left the property parameters of notifyItemPropertyChanged and onItemPropertyChanged as a char* to avoid too many changes, particularly in the two Mac files, for no added benefit.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1589922-prop-changed-AB.patch (v1b)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d88fd521f05e3f469a80fc232597f0d9c7efb3a2
Looks like using VoidString() wasn't a total success :-(

Comment 4

[Jorg K (CEST = GMT+2)]

No more crashes, but two tests need to be adapted:
TEST-UNEXPECTED-FAIL | xpcshell-mdbaddrbook.ini:comm/mailnews/addrbook/test/unit/test_notifications.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | xpcshell-mdbaddrbook.ini:comm/mailnews/addrbook/test/unit/test_notifications.js | - "" == null
TEST-UNEXPECTED-FAIL | xpcshell-mdbaddrbook.ini:comm/mailnews/addrbook/test/unit/test_jsaddrbook.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | xpcshell-mdbaddrbook.ini:comm/mailnews/addrbook/test/unit/test_jsaddrbook.js | addMailingListMember - [addMailingListMember : 83] "" == null

Anyway, the folder patch is good to go.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1589922-AB-test-changes.patch

I think the tests weren't really good insisting on one particular falsy value when now we deliver a different one.

Comment 6

[:aceman]

(In reply to Jorg K (GMT+2) from comment #2)

Note: I left the property parameters of notifyItemPropertyChanged and onItemPropertyChanged as a char* to avoid too many changes, particularly in the two Mac files, for no added benefit.

OK, but those should probably be done later too, as the folder listeners already have property parameters as nsACString.

Comment 7

[Jorg K (CEST = GMT+2)]

That would be unnecessary churn. Looks like we mostly use literal C-strings for this parameter. The aim of the patches is to get rid of the unnecessary .get() and nsString(xx).get() which should have been a PromiseFlatString(xx).get() to start with.

Comment 8

[:aceman]

Comment on attachment 9102788 [details] [diff] [review]
1589922-prop-changed-folders.patch

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

Looks good, some other event handlers already use the ns*String parameters.

Comment 9

[:aceman]

Comment on attachment 9102797 [details] [diff] [review]
1589922-prop-changed-AB.patch (v1b)

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

::: mailnews/addrbook/src/nsAbMDBDirectory.cpp
@@ +188,5 @@
>        do_GetService(NS_ABMANAGER_CONTRACTID, &rv);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  rv = abManager->NotifyItemPropertyChanged(item, nullptr, EmptyString(),
> +                                            EmptyString());

Have you checked all callers of the functions for this EmptyString() change? Not just those that the test crashes uncovered?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/32e57b35f3f0
Change OnItem{Unichar}PropertyChanged to use smart strings in nsIFolderListener.idl. r=aceman

Comment 11

[Geoff Lankow (:darktrojan)]

Comment on attachment 9102813 [details] [diff] [review]
1589922-AB-test-changes.patch

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

These properties are near-useless anyway, so I'm okay with this. You need to update the comment on onItemPropertyChanged in nsIAbListener.idl.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1589922-prop-changed-AB.patch

Thanks. Folded AB patch with the comments fixed.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/432747ceaabe
Change OnItemPropertyChanged/NotifyItemPropertyChanged to use smart strings in nsIAbListener.idl/nsIAbManager.idl. r=aceman,darktrojan