Closed Bug 1589922 Opened 23 days ago Closed 22 days ago

Change OnItem{Unichar}PropertyChanged to use smart strings

Categories

(MailNews Core :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: jorgk, Assigned: jorgk)

Details

Attachments

(2 files, 3 obsolete files)

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

Part 1 for nsIFolderListener.idl.

Assignee: nobody → jorgk
Attachment #9102788 - Flags: review?(acelists)
Attached patch 1589922-prop-changed-AB.patch (obsolete) — Splinter Review

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.

Attachment #9102790 - Flags: review?(acelists)
Attachment #9102790 - Attachment is obsolete: true
Attachment #9102790 - Flags: review?(acelists)
Attachment #9102797 - Flags: review?(acelists)
Attachment #9102797 - Attachment is patch: true

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.

Attached patch 1589922-AB-test-changes.patch (obsolete) — Splinter Review

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

Attachment #9102813 - Flags: review?(geoff)
Attachment #9102813 - Flags: feedback?(acelists)

(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.

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 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.
Attachment #9102788 - Flags: review?(acelists) → review+
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?
Attachment #9102797 - Flags: review?(acelists) → review+
Attachment #9102813 - Flags: feedback?(acelists) → feedback+
Keywords: leave-open
Target Milestone: --- → Thunderbird 71.0
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/32e57b35f3f0
Change OnItem{Unichar}PropertyChanged to use smart strings in nsIFolderListener.idl. r=aceman
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.
Attachment #9102813 - Flags: review?(geoff) → review+

Thanks. Folded AB patch with the comments fixed.

Attachment #9102797 - Attachment is obsolete: true
Attachment #9102813 - Attachment is obsolete: true
Attachment #9102852 - Flags: review+
Keywords: leave-open

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

Status: NEW → RESOLVED
Closed: 22 days ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.