Add ParamTraits<nsAutoString> template specialization in IPCMessageUtils.h

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Dexter, Assigned: Dexter)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [measurement:client:tracking])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
I'm happy to take this bug once bug 1278556 lands.
Whiteboard: [measurement:client:tracking]
(Assignee)

Updated

2 years ago
Assignee: nobody → alessio.placitelli
(Assignee)

Comment 2

2 years ago
Created attachment 8827463 [details] [diff] [review]
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.
Attachment #8827463 - Flags: review?(nfroyd)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
Comment on attachment 8827463 [details] [diff] [review]
bug1330247.patch

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

Looks good, thank you!
Attachment #8827463 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 4

2 years ago
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.
Attachment #8827463 - Flags: review?(chutten)

Comment 5

2 years ago
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.
Attachment #8827463 - Flags: review?(chutten) → review+
(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

2 years ago
INTERNAL_API is going to be defined for 100% of stuff in this. The external string API is going away imminently.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 9

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02057c9d45e5
Add ParamTraits<nsAutoString> template specialization in IPCMessageUtils.h. r=froydnj, r=chutten
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/02057c9d45e5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.