Add ParamTraits<nsAutoString> template specialization in IPCMessageUtils.h
RESOLVED
FIXED
in Firefox 53
Status
()
People
(Reporter: Dexter, Assigned: Dexter)
Tracking
Firefox Tracking Flags
(firefox53 fixed)
Details
(Whiteboard: [measurement:client:tracking])
Attachments
(1 attachment)
|
2.60 KB,
patch
|
froydnj
:
review+
chutten
:
review+
|
Details | Diff | Splinter Review |
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 3•2 years ago
|
||
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+
|
|
(Assignee) | |
Comment 6•2 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=874711d4d9f0
|
|
||
Comment 7•2 years ago
|
||
(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
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.
Description
•