1695274 - Port bug 1691913: Rename nsBaseHashtable::Put to InsertOrUpdate

Status: RESOLVED FIXED

(Thunderbird :: Upstream Synchronization, defect)

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1695274-rename-put-to InsertOrUpdate.patch

Simple exchanges. This builds for me.

Comment 2

[Alessandro Castellani [:aleca]]

Comment on attachment 9205745 [details] [diff] [review]
1695274-rename-put-to InsertOrUpdate.patch

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

I'm stealing this review since I tested it and it unbusts the build.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3a6e2573838c
Port bug 1691913: Rename nsBaseHashtable::Put to InsertOrUpdate. r=aleca,rjl

Comment 4

[Alessandro Castellani [:aleca]]

This didn't work entirely, there are other things that need porting.
I'll give it a try.

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: 1695274-bustage-fix.diff

Comment 6

[Alessandro Castellani [:aleca]]

Comment on attachment 9205760 [details] [diff] [review]
1695274-bustage-fix.diff

empty patch, sorry.

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: 1695274-bustage-fix.diff

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9205761 [details] [diff] [review]
1695274-bustage-fix.diff

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

LGTM, r=mkmelin

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c9b0b24eb44e
Port bug 1691913: Rename nsBaseHashtable::Put to InsertOrUpdate. r=mkmelin

Comment 10

[Rob Lemley [:rjl]]

Attachment: bug1695274_bustage_part3.patch

Comment 11

[Rob Lemley [:rjl]]

Comment on attachment 9205804 [details] [diff] [review]
bug1695274_bustage_part3.patch

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

This doesn't quite work.. See the test fails at https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=c46b4ba2e8c5d8dcbbe3c26add7c00f3d5777abd

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +2855,2 @@
>      newMsgHdr->GetMessageId(getter_Copies(newMessageId));
> +    nsMsgKey pseudoKey = m_pseudoHdrs.Get(newMessageId);

I'm pretty sure this is where I went wrong -- It looks like .Get will return false when the key is not found, so maybe that's what needs to be checked for instead of nsMsgKey_None?

@@ +5858,5 @@
>      // in the acl response.
>      server->GetRealUsername(userName);
>    }
>    ToLowerCase(userName);
> +  rights = m_rightsHash.Get(userName);

also not sure if this is okay? &rights is passed into the function by the caller.

Comment 12

[Alessandro Castellani [:aleca]]

Comment on attachment 9205804 [details] [diff] [review]
bug1695274_bustage_part3.patch

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

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +2855,2 @@
>      newMsgHdr->GetMessageId(getter_Copies(newMessageId));
> +    nsMsgKey pseudoKey = m_pseudoHdrs.Get(newMessageId);

Maybe adding this here it'll do the trick?
```
if (!pseudoKey) {
      pseudoKey = nsMsgKey_None;
    }
```

I don't know if it's possible to do a short-circuit evaluation in C++ to assign that variable.

@@ +5858,5 @@
>      // in the acl response.
>      server->GetRealUsername(userName);
>    }
>    ToLowerCase(userName);
> +  rights = m_rightsHash.Get(userName);

This should be correct because the rights variable is a reference, so updating it will correctly pass the new value to the parent method.

Comment 13

[Rob Lemley [:rjl]]

Attachment: bug1695274_bustage_part3.patch

This should fix it and the broken test.

Comment 14

[Alessandro Castellani [:aleca]]

Comment on attachment 9205814 [details] [diff] [review]
bug1695274_bustage_part3.patch

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

This works locally.

Comment 15

[Addons]

This isn't right. You need to use something like:
https://hg.mozilla.org/integration/autoland/rev/cf2c619858c1#l11.30

So instead of

-    nsMsgKey pseudoKey = nsMsgKey_None;
     newMsgHdr->GetMessageId(getter_Copies(newMessageId));
-    m_pseudoHdrs.Get(newMessageId, &pseudoKey);
-    if (notifier && pseudoKey != nsMsgKey_None) {
+    nsMsgKey pseudoKey = m_pseudoHdrs.Get(newMessageId);
+    if (notifier && pseudoKey) {

Add this:

+  nsMsgKey pseudoKey = m_pseudoHdrs.MaybeGet(newMessageId).valueOr(nsMsgKey_None);

and leave if (notifier && pseudoKey != nsMsgKey_None) {.

Comment 16

[Rob Lemley [:rjl]]

Attachment: bug1695274_bustage_part3.patch

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/958977bd1ded
Port bug 1691913: More bustage fixes. rs=bustage-fix

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1283451c0292
followup - clang-format. rs=clang-format