Closed
Bug 1007577
Opened 11 years ago
Closed 11 years ago
[RTSP] Remove nsIInterfaceRequestor from RtspControllerParent
Categories
(Firefox OS Graveyard :: RTSP, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.0 S2 (23may)
People
(Reporter: ethan, Assigned: ethan)
References
Details
(Whiteboard: [p=1])
Attachments
(1 file, 2 obsolete files)
RtspControllerParent inherits from the interface nsIInterfaceRequestor.
But actually we think no object would call RtspControllerParent's GetInterface().
Besides, RtspControllerParent only implements the interface nsIStreamingProtocolListener. It seems there is not much value to provide GetInterface() utility.
Thus, I suggest to remove it from RtspControllerParent.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → ettseng
Whiteboard: [p=1]
Target Milestone: --- → 2.0 S2 (23may)
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•11 years ago
|
||
By the way, I was aware of this issue while working on bug 1006530.
RtspControllerParent inherits from nsIInterfaceRequestor and nsIStreamingProtocolListener, which both inherit from nsISupports.
This causes an "ambiguous base" compile error while passing RtspControllerParent pointer to a function as an nsISupports argument.
We can remedy this kind of error by static_cast or using NS_INTERFACE_MAP_ENTRY_AMBIGUOUS().
However, since RtspControlleParent doesn't need nsIInterfaceRequestor at all, we should simply remove it.
Assignee | ||
Comment 2•11 years ago
|
||
Steve, can you help to review this minor fix?
Attachment #8419297 -
Flags: review?(sworkman)
Comment 3•11 years ago
|
||
How sure are you that nothing calls GetInterface? It might be an idea to have a patch that calls MOZ_CRASH in GetInterface. Land that and then run it for a couple of weeks just to make sure.
Just an idea. If you're convinced that GetInterface isn't called, I'll r+ the current patch.
Assignee | ||
Comment 4•11 years ago
|
||
Rebase because of bug 1006530.
Attachment #8419297 -
Attachment is obsolete: true
Attachment #8419297 -
Flags: review?(sworkman)
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Steve Workman [:sworkman] from comment #3)
> How sure are you that nothing calls GetInterface? It might be an idea to
> have a patch that calls MOZ_CRASH in GetInterface. Land that and then run it
> for a couple of weeks just to make sure.
> Just an idea. If you're convinced that GetInterface isn't called, I'll r+
> the current patch.
This is what I think.
Before anyone calls GetInterface() of class Foo, it would first obtain an instance or pointer/reference of Foo.
Currently, RtspControllerParent is only created in NeckoParent::AllocPRtspControllerParent() and is only responsible to pass IPC events between RtspControllerChild.
Although it implements the interface nsIStreamingProtocolListener, the use of this interface is also only in RTSP codes and we are sure nothing in our codes calls RtspControllerParent::GetInterface() through this interface.
Is this argument convincing enough?
If you still have concern, I could apply your suggestion to call MOZ_CRASH first.
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(sworkman)
Comment 6•11 years ago
|
||
Comment on attachment 8419947 [details] [diff] [review]
Remove nsIInterfaceRequestor from RtspControllerParent
Review of attachment 8419947 [details] [diff] [review]:
-----------------------------------------------------------------
You convinced me! :) Thanks for taking the time to explain your thinking. r=me.
Attachment #8419947 -
Flags: review+
Updated•11 years ago
|
Flags: needinfo?(sworkman)
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Steve Workman [:sworkman] from comment #6)
> Review of attachment 8419947 [details] [diff] [review]:
> You convinced me! :) Thanks for taking the time to explain your thinking.
> r=me.
YA! You are welcome. ^^
I'm glad you propose the question so that I could further sort our my thought.
Assignee | ||
Comment 8•11 years ago
|
||
Update commit message "r=sworkman".
Attachment #8419947 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
This patch is minor and B2G-specific. Would not affect test cases, so just run builds and B2G crash test.
Try server
Syntax: – try: -b o -p all -u crashtest-1,crashtest-2,crashtest-3 -t none
Result: https://tbpl.mozilla.org/?tree=Try&rev=26077966db82
Keywords: checkin-needed
Assignee | ||
Updated•11 years ago
|
Attachment #8420504 -
Attachment description: bug-1007577-fix.patch → Remove nsIInterfaceRequestor from RtspControllerParent. r=sworkman
Comment 10•11 years ago
|
||
Keywords: checkin-needed
Comment 11•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Blocks: b2g-RTSP-2.0
You need to log in
before you can comment on or make changes to this bug.
Description
•