1330247 - Add ParamTraits<nsAutoString> template specialization in IPCMessageUtils.h

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Alessio Placitelli [:Dexter]]

Bug 1278556 would probably benefit from using nsAutoString(s) rather than nsString(s), as we're expecting small strings to be passed between the processes using IPC.

As suggested by Nathan in bug 1278556 comment 50, we chould add ParamTraits<nsAutoString> template specialization in IPCMessageUtils.h.

Comment 1

[Alessio Placitelli [:Dexter]]

I'm happy to take this bug once bug 1278556 lands.

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: bug1330247.patch

Nathan, since you've got some background of content scalars too (Bug 1278556), would you mind reviewing the IPCMessageUtils.h part?

If that looks ok to you, I'll let a Telemetry peer review the TelemetryComms.h bits as well.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8827463 [details] [diff] [review]
bug1330247.patch

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

Looks good, thank you!

Comment 4

[Alessio Placitelli [:Dexter]]

Comment on attachment 8827463 [details] [diff] [review]
bug1330247.patch

Chris, would you mind reviewing the TelemetryComms.h bits?
We're switching from nsString to nsAutoString there (it was the original plan) as the latter might be more performant, given the expected length of the strings we'll send.

Comment 5

[Chris H-C :chutten|PTO (back June 30)]

Comment on attachment 8827463 [details] [diff] [review]
bug1330247.patch

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

It seems legit to me. String scalars will likely be fewer than 64 codepoints long.

I'm not sure under what situations MOZILLA_INTERNAL_API will be defined, but if :nfroyd's okay with it, then so am I.

Comment 6

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=874711d4d9f0

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Chris H-C :chutten from comment #5)
> I'm not sure under what situations MOZILLA_INTERNAL_API will be defined, but
> if :nfroyd's okay with it, then so am I.

MOZILLA_INTERNAL_API is defined when we're compiling code that's going to go in libxul.  It's not clear to me why we'd have MOZILLA_INTERNAL_API in that IPC header, but perhaps it gets included in some torturous route in browser code or otherwise?  It could possibly be removed, but I'd prefer to attempt that in a separate bug, rather than possibly making Alessio deal with weird fallout.

Comment 8

[Benjamin Smedberg]

INTERNAL_API is going to be defined for 100% of stuff in this. The external string API is going away imminently.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02057c9d45e5
Add ParamTraits<nsAutoString> template specialization in IPCMessageUtils.h. r=froydnj, r=chutten

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/02057c9d45e5