Closed Bug 1382563 Opened 7 years ago Closed 7 years ago

Remove ns*String::AssignWithConversion

Categories

(Core :: XPCOM, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jseward, Assigned: jseward)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

There are actually 4 functions:

void nsCString::AssignWithConversion( const nsAString& aData )

void nsString::AssignWithConversion( const nsACString& aData )

void nsString::AssignWithConversion( const char* aData, int32_t aLength )

void nsCString::AssignWithConversion( const char16_t* aData, int32_t aLength )

The first two are simply wrappers around LossyCopyUTF16toASCII and
CopyASCIItoUTF16.  We could live without them.  The second two use
the first two and are more difficult to get rid of.
Priority: -- → P3
Attached patch bug1382563-5.cset (obsolete) — Splinter Review
Assignee: nobody → jseward
Component: XPCOM → String
Attachment #8890265 - Attachment is obsolete: true
Attachment #8890417 - Flags: review?(erahm)
Comment on attachment 8890417 [details] [diff] [review]
bug1382563-6.cset

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

::: dom/xbl/nsXBLPrototypeBinding.cpp
@@ +634,5 @@
>          nsCOMPtr<nsIAtom> attribute;
>          int32_t attributeNsID = kNameSpaceID_None;
>  
>          // Figure out if this token contains a :.
> +        nsAutoString attrTok; CopyASCIItoUTF16(token, attrTok);

Might as well split this into two lines while you're here.

::: editor/composer/nsComposerCommands.cpp
@@ +1405,5 @@
>    // do we have an href to use for creating link?
>    nsXPIDLCString s;
>    nsresult rv = aParams->GetCStringValue(STATE_ATTRIBUTE, getter_Copies(s));
>    NS_ENSURE_SUCCESS(rv, rv);
> +  nsAutoString attrib; CopyASCIItoUTF16(s, attrib);

Same here.

::: xpcom/string/nsTStringObsolete.cpp
@@ -731,5 @@
> -  // for a nullptr input buffer :-(
> -  if (!aData)
> -  {
> -    Truncate();
> -  }

For all calls to this variant, did you ensure that aData could not be null?
Comment on attachment 8890417 [details] [diff] [review]
bug1382563-6.cset

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

This seems fine as-is, I've noted a few patterns that we could clean up while doing this but I don't think it's absolutely necessary.

::: dom/webbrowserpersist/nsWebBrowserPersist.cpp
@@ +2090,5 @@
>  
>          if (localFile)
>          {
>              nsAutoString filenameAsUnichar;
> +            CopyASCIItoUTF16(filename.get(), filenameAsUnichar);

We could remove `.get()` while we're here.

@@ +2125,5 @@
>          nsAutoCString nameFromURL;
>          url->GetFileName(nameFromURL);
>          if (mPersistFlags & PERSIST_FLAGS_DONT_CHANGE_FILENAMES)
>          {
> +            CopyASCIItoUTF16(NS_UnescapeURL(nameFromURL).BeginReading(),

Same here.

::: dom/xbl/nsXBLPrototypeBinding.cpp
@@ +634,5 @@
>          nsCOMPtr<nsIAtom> attribute;
>          int32_t attributeNsID = kNameSpaceID_None;
>  
>          // Figure out if this token contains a :.
> +        nsAutoString attrTok; CopyASCIItoUTF16(token, attrTok);

For something like this we could just do |NS_ConvertASCIItoUTF16 attrTok(token)|

@@ +655,5 @@
>              return;
>          }
>          else {
>            nsAutoString tok;
> +          CopyASCIItoUTF16(token, tok);

Same here.

::: dom/xul/nsXULCommandDispatcher.cpp
@@ +263,5 @@
>  #ifdef DEBUG
>        if (MOZ_LOG_TEST(gCommandLog, LogLevel::Debug)) {
>          nsAutoCString eventsC, targetsC, aeventsC, atargetsC;
> +        LossyCopyUTF16toASCII(updater->mEvents, eventsC);
> +        LossyCopyUTF16toASCII(updater->mTargets, targetsC);

It's a little weird that we're doing a lossy copy and then a proper conversion. We might want to changes these to CopyUTF16toUTF8.

::: editor/libeditor/HTMLEditor.cpp
@@ +3487,5 @@
>      rv = aSheet->GetSheetURI()->GetSpec(spec);
>  
>      if (NS_SUCCEEDED(rv)) {
>        // Save it so we can remove before applying the next one
> +      CopyASCIItoUTF16(spec.get(), mLastStyleSheetURL);

We can get rid of the `.get()`.

::: toolkit/components/printingui/win/nsPrintDialogUtil.cpp
@@ +255,5 @@
>                             const char*      aStr,
>                             const nsIntRect& aRect)
>  {
>    nsString cStr;
> +  CopyASCIItoUTF16(aStr, cStr);

Maybe fix this name while we're here, that's definitely not a c-string :)

::: xpcom/string/nsTStringObsolete.cpp
@@ -731,5 @@
> -  // for a nullptr input buffer :-(
> -  if (!aData)
> -  {
> -    Truncate();
> -  }

The append functions used by copy et al check the pointer.

::: xpfe/components/directory/nsDirectoryViewer.cpp
@@ +406,5 @@
>    if (entry && NS_SUCCEEDED(rv)) {
>      nsCOMPtr<nsIRDFLiteral> lit;
>      nsString str;
>  
> +    CopyASCIItoUTF16(entryuriC.get(), str);

No need for `.get()`.
Attachment #8890417 - Flags: review?(erahm) → review+
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b134b2048e02
Remove ns*String::AssignWithConversion.  r=erahm.
https://hg.mozilla.org/mozilla-central/rev/b134b2048e02
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.