Closed Bug 1386600 Opened 2 years ago Closed 2 years ago

Change nsIStringBundle methods to return |AString| instead of |wstring|

Categories

(Core :: Internationalization, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(3 files)

We want to remove nsXPIDLString (see bug 1377356). A lot of the uses come from
the return values of the methods in nsIStringBundle. We can remove these by
changing the return type from |wstring| to |AString|.
dbaron, can you give this a brief look-over to see if it looks like you'd expect?

emk, can you please do a normal review. Apologies for the size of the patch.
Attachment #8892858 - Flags: review?(dbaron)
Attachment #8892858 - Flags: review?(VYV03354)
Comment on attachment 8892858 [details] [diff] [review]
Change nsIStringBundle methods to return |AString| instead of |wstring|

Yeah, the general approach here looks fine.
Attachment #8892858 - Flags: review?(dbaron) → superreview+
Comment on attachment 8892858 [details] [diff] [review]
Change nsIStringBundle methods to return |AString| instead of |wstring|

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

::: docshell/base/nsDocShell.cpp
@@ +5301,5 @@
>      const char16_t* strs[kMaxFormatStrArgs];
>      for (uint32_t i = 0; i < formatStrCount; i++) {
>        strs[i] = formatStrs[i].get();
>      }
>      nsXPIDLString str;

Not changed to nsAutoString.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +9130,5 @@
>  // I can just use that for sized broken images, that can happen, maybe.
>  void
>  nsCSSFrameConstructor::GetAlternateTextFor(nsIContent*    aContent,
>                                             nsIAtom*       aTag,
> +                                           nsAString& aAltText)

Could you remove extra spaces between type names and param names above?

::: layout/base/nsCSSFrameConstructor.h
@@ +66,5 @@
>  
>    // get the alternate text for a content node
>    static void GetAlternateTextFor(nsIContent*    aContent,
>                                    nsIAtom*       aTag,  // content object's tag
> +                                  nsAString& aAltText);

ditto

::: layout/style/ErrorReporter.cpp
@@ +330,5 @@
>    const char16_t *params[1] = { qparam.get() };
>  
>    nsAutoString str;
>    sStringBundle->FormatStringFromName(aMessage,
> +                                      params, ArrayLength(params), str);

params and ArrayLength(params) should move to the previous line.

::: toolkit/mozapps/extensions/AddonContentPolicy.cpp
@@ +367,5 @@
>  
>        nsCOMPtr<nsIStringBundle> stringBundle = GetStringBundle();
>  
>        if (stringBundle) {
> +        rv = stringBundle->FormatStringFromName(aName, aParams, aLength, mError);

over 80 columns

::: widget/cocoa/OSXNotificationCenter.mm
@@ +329,2 @@
>      }
> +    bundle->GetStringFromName("webActions.settings.label", settingsButtonTitle);

over 80 columns
Attachment #8892858 - Flags: review?(VYV03354) → review+
jorgk, I have compiled this but not tested it.

The patch is large but mostly formulaic. Most of the changes are one of the
following two things.

- Removing getter_Copies() calls where necessary from arguments, because we no
  longer need |char16_t*|-to-|nsAString&| conversions.
  
- Adding ToNewUnicode() calls where necessary for return values, because we now
  need |nsAString&|-to-|char16_t*| conversions.
Attachment #8893616 - Flags: review?(jorgk)
Thanks, I've been really busy with fighting bustage all week, so I haven't had a chance to look at this.

Apart from the formulaic changes, there seem to be a few other ones, like the removal of
    NS_Free(kLocalizedInboxName);	
That's because that's now a nsString nsMsgDBFolder::kLocalizedInboxName; which will take care of its own destruction, right?

Please go ahead and land your M-C patch before it rots.
Oh, I meant to say: I'll look at it on Monday the latest.
> Apart from the formulaic changes, there seem to be a few other ones, like
> the removal of
>     NS_Free(kLocalizedInboxName);	
> That's because that's now a nsString nsMsgDBFolder::kLocalizedInboxName;
> which will take care of its own destruction, right?

Yes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e26e9804c545afcb33549da0f7fd693cc3fc671f
Bug 1386600 - Change nsIStringBundle methods to return |AString| instead of |wstring|. r=emk,sr=dbaron.
https://hg.mozilla.org/mozilla-central/rev/e26e9804c545
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8893616 [details] [diff] [review]
Update nsIStringBundle method uses to account for them returning |AString| instead of |wstring|

Nicholas, thank you so much for helping us in keeping Thunderbird afloat. Preparing the patch would have been a nightmare. I assume you wrote a script for it.

I haven't done any testing, but I'll land this now and I'll deal with any fallout later.
Attachment #8893616 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1a7802a4bf9d
Update nsIStringBundle method uses to account for them returning |AString| instead of |wstring|. r=jorgk.
https://hg.mozilla.org/comm-central/rev/6b0c4e39b0d6
Update nsIStringBundle method uses to account for them returning |AString| instead of |wstring| (Windows-only files). r=me
> I assume you wrote a script for it.

Nope! It wasn't mechanical enough for that. But I did use Vim macros to semi-automate it.
You need to log in before you can comment on or make changes to this bug.