Closed
Bug 1234492
Opened 8 years ago
Closed 8 years ago
[Presentation WebAPI] Add role in PresentationService
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
People
(Reporter: kuoe0.tw, Assigned: kuoe0.tw)
References
Details
(Whiteboard: [ft:conndevices])
Attachments
(4 files, 18 obsolete files)
47.17 KB,
patch
|
kuoe0.tw
:
review+
jocheng
:
approval-mozilla-b2g48+
|
Details | Diff | Splinter Review |
20.01 KB,
patch
|
smaug
:
review+
jocheng
:
approval-mozilla-b2g48+
|
Details | Diff | Splinter Review |
4.70 MB,
image/png
|
Details | |
42.98 KB,
patch
|
kuoe0.tw
:
review+
jocheng
:
approval-mozilla-b2g48+
|
Details | Diff | Splinter Review |
For 1-UA use case, the sender is also the receiver. So, PresentationService need to know its direction to call functions.
Assignee | ||
Updated•8 years ago
|
Blocks: 1-UA_Presentation_API
Assignee | ||
Comment 1•8 years ago
|
||
Hi, S.C.. I think this bug can use the patch you already created before work week.
Flags: needinfo?(schien)
Comment 2•8 years ago
|
||
Feel free to take my patch on bug 1195221 attachment 8695727 [details] [diff] [review]!
Flags: needinfo?(schien)
Assignee | ||
Comment 3•8 years ago
|
||
Add direction for some functions in presentationService to make them know their roles. And use different hash table to store controlling info and presenting info.
Assignee | ||
Updated•8 years ago
|
Attachment #8702183 -
Flags: review?(bugs)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(bugs)
Assignee | ||
Comment 5•8 years ago
|
||
Problem: ControllingInfo and PresentingInfo cannot coexist when system is both the sender and the receiver. E.g. 1-UA, unit test. Reason: ControllingInfo and PresentingInfo stored in the same hash table. Solution: - Put ControllingInfo and PresentingInfo into two different hash tables. - Add a direction attribute to make each instance of PresentationConnection know its role (sender or receiver). - Also add a direction parameter to the functions which need to get SessionInfo to make them get the right SessionInfo.
Comment 6•8 years ago
|
||
ah, that comment is about what the patch fixes, not some issue in the patch :) Ok, reviewing.
Comment 7•8 years ago
|
||
Comment on attachment 8702183 [details] [diff] [review] Bug-1234492-hg.patch > nsString mUrl; > nsString mSessionId; >+ uint8_t mDirection; Is direction the interesting thing here, or could it be about "controller" or "receiver" ? In which case some enum might be nice. The enum could have same values as the idl. > [scriptable, uuid(c177a13a-bf1a-48bf-8032-d415c3343c46)] > interface nsIPresentationService : nsISupports update uuid > { >+ const unsigned short DIRECTION_SENDER = 0x1; >+ const unsigned short DIRECTION_RECEIVER = 0x2; I'm trying to understand the meaning of these. It is not clear what 'direction' refers to here. Is it about the target or source? I guess target. Could we call these CONTROLLER_TO_RECEIVER and RECEIVER_TO_CONTROLLER or some such. The spec after all talks about controller and receiver. But even then + nsTArray<nsString> mSenderSessionIds; + nsTArray<nsString> mReceiverSessionIds; isn't quite clear to me. What sessions ids do those actually hold? Could those be something like mSessionIdsAtController and mSessionIdsAtReceiver; ? >+ > /* > * Start a new presentation session and display a prompt box which asks users > * to select a device. > * > * @param url: The url of presenting page. > * @param sessionId: An ID to identify presentation session. > * @param origin: The url of requesting page. > * @param callback: Invoke the callback when the operation is completed. >@@ -55,31 +58,34 @@ interface nsIPresentationService : nsISu > > /* > * Send the message wrapped with an input stream to the session. > * > * @param sessionId: An ID to identify presentation session. > * @param stream: The message is converted to an input stream. > */ > void sendSessionMessage(in DOMString sessionId, >+ in uint8_t direction, > in nsIInputStream stream); please document direction param > /* > * Close the session. > * > * @param sessionId: An ID to identify presentation session. > */ >- void closeSession(in DOMString sessionId); >+ void closeSession(in DOMString sessionId, >+ in uint8_t direction); > and here > /* > * Terminate the session. > * > * @param sessionId: An ID to identify presentation session. > */ >- void terminateSession(in DOMString sessionId); >+ void terminateSession(in DOMString sessionId, >+ in uint8_t direction); and here, and elsewhere. >-PresentationParent::RecvUnregisterSessionHandler(const nsString& aSessionId) >+PresentationParent::RecvUnregisterSessionHandler(const nsString& aSessionId, >+ const uint8_t& aDirection) > { > MOZ_ASSERT(mService); >- mSessionIds.RemoveElement(aSessionId); >- NS_WARN_IF(NS_FAILED(mService->UnregisterSessionListener(aSessionId))); >+ if (nsIPresentationService::DIRECTION_SENDER == aDirection) { >+ mSenderSessionIds.RemoveElement(aSessionId); >+ } else { >+ mReceiverSessionIds.RemoveElement(aSessionId); perhaps assert in the else that aDirection is DIRECTION_RECEIVER The code itself is fine, but I'd like to see some clarifications in naming, so that it is crystal clear to the reader which sessions ids are being handled and so on.
Attachment #8702183 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 8•8 years ago
|
||
How do you think if we use "role" (sender or receiver) instead of "direction"? I think "role" maybe a more clear naming to identify what it is.
> But even then
> + nsTArray<nsString> mSenderSessionIds;
> + nsTArray<nsString> mReceiverSessionIds;
> isn't quite clear to me. What sessions ids do those actually hold?
> Could those be something like mSessionIdsAtController and mSessionIdsAtReceiver; ?
I agree that with you. I'll update the names to mSessionIdsAtReceriver and mSessionIdsAtSender.
Flags: needinfo?(bugs)
Comment 9•8 years ago
|
||
Hi Tommy, please add a test case that launches both sender and receiver. That will help guarantee no body break this restriction in the future.
Assignee | ||
Comment 11•8 years ago
|
||
Hi smaug, can you review this patch again?
Assignee: nobody → kuoe0
Attachment #8702183 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8709914 -
Flags: review?(bugs)
Assignee | ||
Comment 12•8 years ago
|
||
About using enum to instead of using the value of nsIPresentationService. Is the patch what you meant?
Attachment #8709917 -
Flags: feedback?(bugs)
Comment 13•8 years ago
|
||
Comment on attachment 8709917 [details] [diff] [review] Using enum for roles Could we define the enum just in one place? but yeah, I think this makes the code easier to follow, and less error prone when the member variable can get only expected valued. Perhaps enum class PresentationRole somewhere? And the entry names should start with e. eController, eReceiver?
Attachment #8709917 -
Flags: feedback?(bugs) → feedback+
Assignee | ||
Comment 14•8 years ago
|
||
Actually, I have a question about using enum. Why don't we use the value defined in nsPresentationService.idl? I just want to know why :)
Flags: needinfo?(bugs)
Comment 15•8 years ago
|
||
.idl doesn't define enum. And enum values can be validated during compile time. That was my original idea. Enums tend to also make the code easier to read. But, I guess if you want to use the values from .idl, add some assertions that no unexpected values are being used. Then enum isn't needed.
Flags: needinfo?(bugs)
Assignee | ||
Comment 16•8 years ago
|
||
Finally, I decide to declare the enum type in .idl. And I add assertions before accessing SessionInfo to make sure the parameter is right.
Attachment #8709914 -
Attachment is obsolete: true
Attachment #8709917 -
Attachment is obsolete: true
Attachment #8709914 -
Flags: review?(bugs)
Attachment #8712060 -
Flags: review?(bugs)
Assignee | ||
Comment 17•8 years ago
|
||
Hi smaug. I removed all enum in this patch. After thinking, I think we should not use enum. There are some drawbacks. First, the consistency between IDL and enum. If someone update the value of enum, he/she may forget to update the value in IDL, vice versa. Second, despite we use enum, I also have to cast the value to uint8 while we pass it to some functions defined in IDL. So, I intend to not use enum. What do you think?
Flags: needinfo?(bugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8712060 -
Attachment description: Bug-1234492-hg.patch → patch with using enum
Attachment #8712060 -
Flags: review?(bugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8713542 -
Attachment description: Bug-1234492-hg.patch → patch without enum
Comment 18•8 years ago
|
||
Ok, fine, but assert that whenever mRole is set, it is set to a valid value. Assert also that the role parameter passed to closeSession/etc methods is an expected value. Such assertions would help also with readability.
Flags: needinfo?(bugs)
Assignee | ||
Comment 19•8 years ago
|
||
Updated the patch by adding more assertion for role setting.
Attachment #8712060 -
Attachment is obsolete: true
Attachment #8713542 -
Attachment is obsolete: true
Assignee | ||
Comment 20•8 years ago
|
||
Add test case to make sure that sender and receiver can co-exist in the same side.
Assignee | ||
Comment 21•8 years ago
|
||
fix conflict with m-c
Attachment #8726599 -
Attachment is obsolete: true
Assignee | ||
Comment 22•8 years ago
|
||
fix conflict with m-c
Attachment #8726600 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8726602 -
Flags: review?(bugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8726603 -
Flags: review?(bugs)
Assignee | ||
Comment 23•8 years ago
|
||
Hi smaug, I already finish the patch and the test. Could you review it again? Thanks!
Flags: needinfo?(bugs)
Updated•8 years ago
|
Summary: [Presentation WebAPI] Add direction in PresentationService → [Presentation WebAPI] Add role in PresentationService
Comment 24•8 years ago
|
||
Back from vacation and I'll try to get through my review queue still during this week.
Flags: needinfo?(bugs)
Comment 25•8 years ago
|
||
Comment on attachment 8726602 [details] [diff] [review] [Part 1] Add direction in PresentationService, r=smaug Sorry about delay here. Have had tons of stuff to review. Excuses excuses...
Attachment #8726602 -
Flags: review?(bugs) → review+
Comment 26•8 years ago
|
||
Comment on attachment 8726603 [details] [diff] [review] [Part 2] Add test case rs+ for this
Attachment #8726603 -
Flags: review?(bugs) → review+
Comment 27•8 years ago
|
||
Except that please explain SimpleTest.expectAssertions(0, 5);
Assignee | ||
Comment 28•8 years ago
|
||
Just update the author name. Carry r+ from smaug in commet 25.
Attachment #8726602 -
Attachment is obsolete: true
Attachment #8732709 -
Flags: review+
Assignee | ||
Comment 29•8 years ago
|
||
Add reviewer's name in commit message. Carry r+ from comment 26. (In reply to Olli Pettay [:smaug] from comment #27) > Except that please explain SimpleTest.expectAssertions(0, 5); Sorry, I copied the test structure from other test case, and forgot to delete this line.
Attachment #8726603 -
Attachment is obsolete: true
Attachment #8732726 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 30•8 years ago
|
||
Rebased with mozilla-inbound. Carry r+ from comment #25.
Attachment #8732757 -
Flags: review+
Assignee | ||
Comment 31•8 years ago
|
||
Rebased with mozilla-inbound. Carry r+ from comment #26.
Attachment #8732709 -
Attachment is obsolete: true
Attachment #8732726 -
Attachment is obsolete: true
Attachment #8732758 -
Flags: review+
Assignee | ||
Comment 32•8 years ago
|
||
- Make test skip in e10s mode. (Instead of OOP version in non-e10s mode.) - Add send message test in original test. - Add OOP version of the same test. Carry r+ from comment #26.
Attachment #8732758 -
Attachment is obsolete: true
Attachment #8736118 -
Flags: review+
Assignee | ||
Comment 33•8 years ago
|
||
Try result: https://treeherder.mozilla.org/#/jobs?repo=try&tochange=18aa552876e4&fromchange=13baa59c75ee&selectedJob=18772217
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 34•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f74a43d0b9eb https://hg.mozilla.org/integration/mozilla-inbound/rev/75aff80f34a5
Keywords: checkin-needed
Comment 35•8 years ago
|
||
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=24844836&repo=mozilla-inbound
Flags: needinfo?(kuoe0)
Comment 36•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/6dd2e8d7ecfc https://hg.mozilla.org/integration/mozilla-inbound/rev/1450d0e37119
Assignee | ||
Comment 37•8 years ago
|
||
Rebase. carry r+ from comment #25.
Attachment #8732757 -
Attachment is obsolete: true
Flags: needinfo?(kuoe0)
Attachment #8742234 -
Flags: review+
Assignee | ||
Comment 38•8 years ago
|
||
After rebased with m-c, I found there is another role definition in 'nsIPresentationSessionTransportBuilder.idl'. I think we should put the role definition in one place.
Attachment #8736118 -
Attachment is obsolete: true
Attachment #8742280 -
Flags: review?(bugs)
Assignee | ||
Comment 39•8 years ago
|
||
Due to the original test case time-out sometimes and the big change on bug 1148307, I rewrite the entire test case. Please review it again. Thanks.
Attachment #8742291 -
Flags: review?(bugs)
Assignee | ||
Comment 40•8 years ago
|
||
Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a7b2c8731a3&selectedJob=19598477
Assignee | ||
Comment 41•8 years ago
|
||
Update with better naming.
Attachment #8742291 -
Attachment is obsolete: true
Attachment #8742291 -
Flags: review?(bugs)
Attachment #8742692 -
Flags: review?(bugs)
Assignee | ||
Comment 42•8 years ago
|
||
Use role definition in Ci.nsIPresentationService instead of the definition in Ci.nsIPresentationSessionTransportBuilder. Like the part 2 patch.
Attachment #8742692 -
Attachment is obsolete: true
Attachment #8742692 -
Flags: review?(bugs)
Attachment #8744238 -
Flags: review?(bugs)
Assignee | ||
Comment 43•8 years ago
|
||
There is the test flow of my test case. Hope it would help anyone who want to understand the entire execution flow.
Comment 44•8 years ago
|
||
I'm not rather lost with the patches. The previous Part 2 looked totally different comparing to the latest Part 2.
Comment 45•8 years ago
|
||
Ahaa, the old part 2 is new part 3.
Assignee | ||
Comment 46•8 years ago
|
||
reply to Olli Pettay [:smaug] from comment #44) > I'm not rather lost with the patches. The previous Part 2 looked totally > different comparing to the latest Part 2. (In reply to Olli Pettay [:smaug] from comment #45) > Ahaa, the old part 2 is new part 3. Hi smaug. Yes, the old part 2 is named part 3 now. Because bug 1148307 define the same role definition in other place. I think we should put them in the same place, so I create the part 2 patch to do that!(In
Comment 47•8 years ago
|
||
Comment on attachment 8744238 [details] [diff] [review] [Part 3] (v3) Add test case, r=smaug >+function mockChannelDescription(role) { >+ this.QueryInterface = XPCOMUtils.generateQI([Ci.nsIPresentationChannelDescription]); >+ this.role = role; >+ this.type = Ci.nsIPresentationChannelDescription.TYPE_TCP; >+ this.tcpAddress = addresses; >+ this.tcpPort = (role === 'sender' ? 1234 : 4321); // either sender or receiver you have some \t characters here. use spaces for indentation. >+} >+ >+function init() { Could you call this something else, to hint that the function is initializing. >+ addMessageListener('teardown', function() { >+ teardown(); >+ }); nit, why not just addMessageListener('teardown', teardown); ? This is really rs+ (rubberstamping). This patch for testing is massive.
Attachment #8744238 -
Flags: review?(bugs) → review+
Comment 48•8 years ago
|
||
Comment on attachment 8742280 [details] [diff] [review] [Part 2] (v1) Use the role definition in nsIPresentationService, r=smaug This is simple
Attachment #8742280 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 49•8 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #47) > Comment on attachment 8744238 [details] [diff] [review] > [Part 3] (v3) Add test case, r=smaug > >+function init() { > Could you call this something else, to hint that the function is > initializing. I think this function is used to register all mock classes and all event listener. So, how about 'initMockAndListener'? > This is really rs+ (rubberstamping). This patch for testing is massive. Haha, the setup process of presentation API is too complex. Most part of the patch is to create the PresentationConnection Object.
Flags: needinfo?(bugs)
Comment 50•8 years ago
|
||
initMockAndListener sounds ok. And the patch is about testing, so it is largely about you feeling we have good enough tests :)
Flags: needinfo?(bugs)
Assignee | ||
Comment 51•8 years ago
|
||
Issues mentioned in comment #47 fixed: - Replace '\t' with ' '. - Replace init() with initMockAndListener(). Carry r+ from comment #47.
Attachment #8744238 -
Attachment is obsolete: true
Attachment #8744416 -
Flags: review+
Assignee | ||
Comment 52•8 years ago
|
||
Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6e4c296e5bf
Keywords: checkin-needed
Comment 53•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/801cac365dd9 https://hg.mozilla.org/integration/mozilla-inbound/rev/ac0e65743b5d https://hg.mozilla.org/integration/mozilla-inbound/rev/94ec70bf8c22
Keywords: checkin-needed
Comment 54•8 years ago
|
||
Backed out for frequent OSX test_presentation_dc_receiver.html failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/18ff4b609573 https://treeherder.mozilla.org/logviewer.html#?job_id=26522445&repo=mozilla-inbound
Comment 55•8 years ago
|
||
And others: https://treeherder.mozilla.org/logviewer.html#?job_id=26522782&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=26522878&repo=mozilla-inbound
Assignee | ||
Comment 56•8 years ago
|
||
Apply the same way used in bug 1263878 to fix test failure. Carry r+ from comment #47.
Attachment #8744416 -
Attachment is obsolete: true
Attachment #8745829 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 57•8 years ago
|
||
Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f26e17efe2b8 I already re-triggered it many times. It seems like the test is okay!
Comment 58•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8daa1ed7f3d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/a37a5dc532eb https://hg.mozilla.org/integration/mozilla-inbound/rev/de07bf8eed4f
Keywords: checkin-needed
Comment 59•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8daa1ed7f3d1 https://hg.mozilla.org/mozilla-central/rev/a37a5dc532eb https://hg.mozilla.org/mozilla-central/rev/de07bf8eed4f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Updated•8 years ago
|
Whiteboard: [ft:conndevices]
Updated•8 years ago
|
blocking-b2g: --- → 2.6+
Assignee | ||
Comment 60•8 years ago
|
||
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 1234492 User impact if declined: Every patches for Presentation API need two versions of them, one for m-c and another one for b2g. Testing completed: manual testing passed Risk to taking this patch (and alternatives if risky): I think there is no risk with this patch. String or UUID changes made by this patch: uuid updated
Assignee | ||
Comment 61•8 years ago
|
||
Comment on attachment 8745829 [details] [diff] [review] [Part 3] (v5) Add test case, r=smaug NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 1234492 User impact if declined: Every patches for Presentation API need two versions of them, one for m-c and another one for b2g. Testing completed: manual testing passed Risk to taking this patch (and alternatives if risky): I think there is no risk with this patch. String or UUID changes made by this patch: uuid updated
Flags: needinfo?(jocheng)
Attachment #8745829 -
Flags: approval-mozilla-b2g48?
Assignee | ||
Comment 62•8 years ago
|
||
Comment on attachment 8742280 [details] [diff] [review] [Part 2] (v1) Use the role definition in nsIPresentationService, r=smaug NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed: Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch: NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 1234492 User impact if declined: Every patches for Presentation API need two versions of them, one for m-c and another one for b2g. Testing completed: manual testing passed Risk to taking this patch (and alternatives if risky): I think there is no risk with this patch. String or UUID changes made by this patch: uuid updated
Attachment #8742280 -
Flags: approval-mozilla-b2g48?
Assignee | ||
Comment 63•8 years ago
|
||
Comment on attachment 8742234 [details] [diff] [review] [Part 1] (v2) Add direction in PresentationService, r=smaug NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 1234492 User impact if declined: Every patches for Presentation API need two versions of them, one for m-c and another one for b2g. Testing completed: manual testing passed Risk to taking this patch (and alternatives if risky): I think there is no risk with this patch. String or UUID changes made by this patch: uuid updated
Attachment #8742234 -
Flags: approval-mozilla-b2g48?
Comment 64•8 years ago
|
||
Comment on attachment 8742234 [details] [diff] [review] [Part 1] (v2) Add direction in PresentationService, r=smaug Approve for TV 2.6
Flags: needinfo?(jocheng)
Attachment #8742234 -
Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Updated•8 years ago
|
status-b2g-v2.6:
--- → affected
Comment 65•8 years ago
|
||
Comment on attachment 8742280 [details] [diff] [review] [Part 2] (v1) Use the role definition in nsIPresentationService, r=smaug Approve for TV 2.6
Attachment #8742280 -
Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Comment 66•8 years ago
|
||
Comment on attachment 8745829 [details] [diff] [review] [Part 3] (v5) Add test case, r=smaug Approve for TV 2.6
Attachment #8745829 -
Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Comment 67•8 years ago
|
||
Comment on attachment 8745829 [details] [diff] [review] [Part 3] (v5) Add test case, r=smaug Approve for TV 2.6
Comment 68•8 years ago
|
||
https://github.com/mozilla-b2g/gecko-b2g/commit/8b500eab52c5e223dccc4ac04aac4589696a694f
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•