Closed Bug 1648802 Opened 10 months ago Closed 7 months ago

Adapt Thunderbird to removal of NS_*LITERAL*STRING macros

Categories

(Thunderbird :: General, task, P1)

Tracking

(thunderbird_esr78 unaffected, thunderbird79 unaffected, thunderbird82 unaffected)

RESOLVED FIXED
Thunderbird 80.0
Tracking Status
thunderbird_esr78 --- unaffected
thunderbird79 --- unaffected
thunderbird82 --- unaffected

People

(Reporter: sg, Assigned: mkmelin)

References

Details

(Keywords: leave-open)

Attachments

(3 files, 1 obsolete file)

Bug 1648010 is going to the remove the NS_*LITERAL*STRING macros after replacing all uses by user-defined string literals using "_ns" and uses of the named variants with regular constants. The plan is to remove the non-named variants a few days after the replacements, but not wait too long to avoid too many new uses being introduced in the meantime. There are far fewer uses of the named ones, and these are planned to be removed along the replacements.

Component: String → General
Product: Core → Thunderbird

Thanks for filing!

Assignee: nobody → mkmelin+mozilla
Priority: -- → P1

Is this correct (it does compile): https://searchfox.org/comm-central/rev/5f9db723cef371840b19c1b547a8a51da7b262f4/mailnews/base/util/nsMsgDBFolder.cpp#1560-1561

NS_NAMED_LITERAL_CSTRING(MozillaStatus,
                           "X-Mozilla-Status: 0001" MSG_LINEBREAK);

->

constexpr auto MozillaStatus = "X-Mozilla-Status: 0001"_ns MSG_LINEBREAK;

(In reply to Magnus Melin [:mkmelin] from comment #2)

Is this correct (it does compile): https://searchfox.org/comm-central/rev/5f9db723cef371840b19c1b547a8a51da7b262f4/mailnews/base/util/nsMsgDBFolder.cpp#1560-1561

NS_NAMED_LITERAL_CSTRING(MozillaStatus,
                           "X-Mozilla-Status: 0001" MSG_LINEBREAK);

->

constexpr auto MozillaStatus = "X-Mozilla-Status: 0001"_ns MSG_LINEBREAK;

Interesting, I don't think we had this case in mozilla-central. I guess it's correct, following http://eel.is/c++draft/lex.ext#8

Status: NEW → ASSIGNED
Comment on attachment 9160644 [details] [diff] [review]
bug1648802_rm_NS_NAMED_LITERAL_STRING.patch

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

LGTM overall!

A few formatting fixes should probably be moved to a separate patch.

The manual changes in nsProfileMigrator.cpp work as is, but could be further improved.

::: mail/components/migration/src/nsProfileMigrator.cpp
@@ +88,4 @@
>    nsAutoCString migratorID;
>    if (!forceMigrationType.IsEmpty()) {
>      bool exists = false;
> +    migratorID.Append(NS_MAILPROFILEMIGRATOR_CONTRACTID_PREFIX);

This could better use AppendLiteral rather than Append

@@ +105,5 @@
>  #define MAX_SOURCE_LENGTH 10
>    const char sources[][MAX_SOURCE_LENGTH] = {"seamonkey", "outlook", ""};
>    for (uint32_t i = 0; sources[i][0]; ++i) {
> +    migratorID.Truncate();
> +    migratorID.Append(NS_MAILPROFILEMIGRATOR_CONTRACTID_PREFIX);

This could better use AssignLiteral rather than Truncate followed by Append

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +2675,5 @@
>            nsCString myEmail;
>            // Get senders address from composeField or from identity,
>            nsAutoCString sender(_compFields->GetFrom());
>            ExtractEmail(EncodedHeader(sender), myEmail);
> +          if (myEmail.IsEmpty()) mIdentity->GetEmail(myEmail);

nit: this formatting change is unrelated to the literal adaptations

::: mailnews/imap/src/nsImapProtocol.h
@@ +516,5 @@
>    // SendData not only writes the NULL terminated data in dataBuffer to our
>    // output stream but it also informs the consumer that the data has been
>    // written to the stream. aSuppressLogging --> set to true if you wish to
>    // suppress logging for this particular command. this is useful for making
> +  // sure we don't log authentication information like the user's password

nit: this formatting change is unrelated to the literal adaptations
Attachment #9160644 - Flags: review?(sgiesecke) → review+
Keywords: leave-open
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/cc901b85280c
preparation patch - clang-format. rs=clang-format. DONTBUILD

Thanks!

Attachment #9160644 - Attachment is obsolete: true
Attachment #9160659 - Flags: review+

This covers the most easily greppable cases and is not complete - will have to find the cases where " is on the next line too, manually.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b100d6a8e613b15749ca37e0e1fa6a602cc9f8ac

Attachment #9160661 - Flags: review?(sgiesecke)
Comment on attachment 9160661 [details] [diff] [review]
bug1648802_replace_NSLITERAL.patch

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

LGTM!

The comment's are just minor things related to general string handling I noticed during review, nothing that needs or should be addressed in this patch.

::: calendar/base/backend/libical/calUtils.cpp
@@ +63,5 @@
>  }
>  
>  void logMissingTimezone(char const* tzid) {
>    // xxx todo: needs l10n
> +  nsString msg(u"Timezone \""_ns);

not directly related to the migration, but using `nsString msg = ... + ... + ...;` in such a case would ensure that the buffer of the target string is only allocated once with the overall size (since this constructs a temporary nsTSubstringTuple).

::: mailnews/news/src/nsNntpUrl.cpp
@@ +196,4 @@
>      m_newsAction = nsINntpUrl::ActionSearch;
>      return NS_OK;
>    }
> +  if (StringBeginsWith(query, "part="_ns) || query.Find("&part=") > 0) {

not directly related to the change, but to the handling of literals: `query.Find("&part=")` will determine the length of the literal potentially at run-time. I guess it would be more beneficial to use a `""_ns` literal here as well. since that is guaranteed determines the length at compile-time. (Either case will convert to a  temporary `nsCString`).
Attachment #9160661 - Flags: review?(sgiesecke) → review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/732ab4ae2b4e
remove Thunderbird usage of NS_NAMED_LITERAL_CSTRING and NS_NAMED_LITERAL_STRING macros (removed in bug 1648010). r=sg
https://hg.mozilla.org/comm-central/rev/1aa86015cd87
Replace Thunderbird uses of NS_LITERAL_STRING/NS_LITERAL_CSTRING macros by _ns literals.. r=sg

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 80.0

I guess I shouldn't have closed this in case there's more to do…

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/1a865e7616f4
follow-up - Fix stray closing bracket. rs=bustage-fix

The rest. Builds locally with the patch from autoland applied as well. Sent to try now: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d763a1df38acbea40c1254b42f6f6490dd26413c

Attachment #9161246 - Flags: review?(sgiesecke)
Comment on attachment 9161246 [details] [diff] [review]
bug1648802_replace_NSLITERAL_harderones.patch

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

LGTM!

The two comments are just suggestions for future improvements.

::: mailnews/addrbook/src/nsAbLDAPDirectory.cpp
@@ +262,5 @@
>        return NS_OK;
>      }
>  
>      // Get the local directory.
> +    nsAutoCString localDirectoryURI(nsLiteralCString(kJSDirectoryRoot));

nit: this could be written simpler and, if the auto storage size is exceeded, more optimal, as:
```
const nsAutoCString localDirectoryURI = nsLiteralCString(kJSDirectoryRoot) + fileName;
```

::: mailnews/base/util/nsMsgProtocol.cpp
@@ +792,5 @@
>  
>      /* escape starting periods
>       */
>      if (line.CharAt(0) == '.') line.Insert('.', 0);
> +    line.Append(nsLiteralCString(CRLF));

nit: this could also use the slightly shorter
```
line.AppendLiteral(CRLF);
```
Attachment #9161246 - Flags: review?(sgiesecke) → review+
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/901d639e9f6a
Replace Thunderbird constant uses of NS_LITERAL_STRING/NS_LITERAL_CSTRING macros . r=sg

(In reply to Simon Giesecke [:sg] [he/him] from comment #14)

nit: this could be written simpler and, if the auto storage size is
exceeded, more optimal, as:

const nsAutoCString localDirectoryURI = nsLiteralCString(kJSDirectoryRoot) +
fileName;

This one couldn't use const.

Thanks for the reviews and suggestions!

Regressions: 1652115
Status: REOPENED → RESOLVED
Closed: 9 months ago7 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.