Closed
Bug 1121673
Opened 9 years ago
Closed 9 years ago
Use move references in IPDL
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: khuey, Assigned: khuey)
References
Details
Attachments
(1 file)
122.52 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
This doesn't change consumers, just method signatures. Some of the consumers could later be changed to take advantage of this.
Attachment #8549148 -
Flags: review?(bent.mozilla)
Comment on attachment 8549148 [details] [diff] [review] Patch Yeah, bug 968520 has us trying to remove InfallibleTArray...
Comment on attachment 8549148 [details] [diff] [review] Patch Review of attachment 8549148 [details] [diff] [review]: ----------------------------------------------------------------- r=me, but the last question is important: ::: dom/network/UDPSocketParent.h @@ +32,5 @@ > const bool& aAddressReuse, const bool& aLoopback) MOZ_OVERRIDE; > > virtual bool RecvOutgoingData(const UDPData& aData, const UDPSocketAddr& aAddr) MOZ_OVERRIDE; > > virtual bool RecvClose() MOZ_OVERRIDE; Unrelated! ::: dom/notification/Notification.cpp @@ +333,5 @@ > nsTArray<nsString> emptyOptions; > return nsContentPermissionUtils::CreatePermissionArray(NS_LITERAL_CSTRING("desktop-notification"), > + NS_LITERAL_CSTRING("unused"), > + emptyOptions, > + aTypes); Line endings? ::: ipc/ipdl/ipdl/lower.py @@ +552,5 @@ > + t = _cxxBareType(ipdltype, side) > + if ipdltype.isIPDL() and ipdltype.isArray(): > + t.ref = 2 > + return t > + if ipdltype.isIPDL() and ipdltype.isShmem(): Nit: We could combine these two blocks since they're identical: if ipdltype.isIPDL() and (ipdltype.isArray() or ipdltype.isShmem()): ... @@ +958,5 @@ > > def makeDecl(d, sems): > if sems is 'in': > return Decl(d.inType(side), d.name) > + elif sems is 'move': Hm, since moveType falls back to the same as inType is there any reason to introduce a new 'sems' type? What happens if you just make 'in' use the moveType directly?
Attachment #8549148 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #2) > Comment on attachment 8549148 [details] [diff] [review] > Patch > > Review of attachment 8549148 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me, but the last question is important: > > ::: dom/network/UDPSocketParent.h > @@ +32,5 @@ > > const bool& aAddressReuse, const bool& aLoopback) MOZ_OVERRIDE; > > > > virtual bool RecvOutgoingData(const UDPData& aData, const UDPSocketAddr& aAddr) MOZ_OVERRIDE; > > > > virtual bool RecvClose() MOZ_OVERRIDE; > > Unrelated! Fixed. > ::: dom/notification/Notification.cpp > @@ +333,5 @@ > > nsTArray<nsString> emptyOptions; > > return nsContentPermissionUtils::CreatePermissionArray(NS_LITERAL_CSTRING("desktop-notification"), > > + NS_LITERAL_CSTRING("unused"), > > + emptyOptions, > > + aTypes); > > Line endings? Tabs, actually. idk how that happened. > ::: ipc/ipdl/ipdl/lower.py > @@ +552,5 @@ > > + t = _cxxBareType(ipdltype, side) > > + if ipdltype.isIPDL() and ipdltype.isArray(): > > + t.ref = 2 > > + return t > > + if ipdltype.isIPDL() and ipdltype.isShmem(): > > Nit: We could combine these two blocks since they're identical: > > if ipdltype.isIPDL() and (ipdltype.isArray() or ipdltype.isShmem()): > ... Done. > @@ +958,5 @@ > > > > def makeDecl(d, sems): > > if sems is 'in': > > return Decl(d.inType(side), d.name) > > + elif sems is 'move': > > Hm, since moveType falls back to the same as inType is there any reason to > introduce a new 'sems' type? What happens if you just make 'in' use the > moveType directly? mmm, I think you're correct.
Assignee | ||
Comment 4•9 years ago
|
||
> > @@ +958,5 @@
> > >
> > > def makeDecl(d, sems):
> > > if sems is 'in':
> > > return Decl(d.inType(side), d.name)
> > > + elif sems is 'move':
> >
> > Hm, since moveType falls back to the same as inType is there any reason to
> > introduce a new 'sems' type? What happens if you just make 'in' use the
> > moveType directly?
>
> mmm, I think you're correct.
Actually, as we discussed on IRC, this is because we want the semantics to be different between SendFoo. and RecvFoo. After IPDL deserializes a message and calls out it will always have no more need for the data, so it makes sense to provide move references to allow the callee to adopt the data. But when we're serializing messages, a) the caller may want to hold on to the data and b) IPDL can't make much use of the move reference anyways since everything has to be serialized into the Pickle.
Assignee | ||
Comment 5•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f498989ed56f
https://hg.mozilla.org/mozilla-central/rev/f498989ed56f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•