Closed Bug 1234492 Opened 8 years ago Closed 8 years ago

[Presentation WebAPI] Add role in PresentationService

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
blocking-b2g 2.6+
Tracking Status
firefox49 --- fixed
b2g-v2.6 --- fixed

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+
Details | Diff | Splinter Review
20.01 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.70 MB, image/png
Details
42.98 KB, patch
kuoe0.tw
: review+
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.
Hi, S.C.. I think this bug can use the patch you already created before work week.
Flags: needinfo?(schien)
Feel free to take my patch on bug 1195221 attachment 8695727 [details] [diff] [review]!
Flags: needinfo?(schien)
Attached patch Bug-1234492-hg.patch (obsolete) — Splinter Review
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.
Blocks: 1208417
Hi smaug, can you review this patch?
Flags: needinfo?(bugs)
Attachment #8702183 - Flags: review?(bugs)
Flags: needinfo?(bugs)
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.
ah, that comment is about what the patch fixes, not some issue in the patch :) 
Ok, reviewing.
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-
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)
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.
role sounds good. CONTROLLER an RECEIVER roles.
Flags: needinfo?(bugs)
Attached patch Bug-1234492-hg.patch (obsolete) — Splinter Review
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)
Attached patch Using enum for roles (obsolete) — Splinter Review
About using enum to instead of using the value of nsIPresentationService. Is the patch what you meant?
Attachment #8709917 - Flags: feedback?(bugs)
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+
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)
.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)
Attached patch patch with using enum (obsolete) — Splinter Review
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)
Attached patch patch without enum (obsolete) — Splinter Review
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)
Attachment #8712060 - Attachment description: Bug-1234492-hg.patch → patch with using enum
Attachment #8712060 - Flags: review?(bugs)
Attachment #8713542 - Attachment description: Bug-1234492-hg.patch → patch without enum
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)
Updated the patch by adding more assertion for role setting.
Attachment #8712060 - Attachment is obsolete: true
Attachment #8713542 - Attachment is obsolete: true
Attached patch [Part 2] Add test case (obsolete) — Splinter Review
Add test case to make sure that sender and receiver can co-exist in the same side.
fix conflict with m-c
Attachment #8726599 - Attachment is obsolete: true
Attached patch [Part 2] Add test case (obsolete) — Splinter Review
fix conflict with m-c
Attachment #8726600 - Attachment is obsolete: true
Attachment #8726602 - Flags: review?(bugs)
Attachment #8726603 - Flags: review?(bugs)
Hi smaug, I already finish the patch and the test. Could you review it again? Thanks!
Flags: needinfo?(bugs)
Summary: [Presentation WebAPI] Add direction in PresentationService → [Presentation WebAPI] Add role in PresentationService
Back from vacation and I'll try to get through my review queue still during this week.
Flags: needinfo?(bugs)
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 on attachment 8726603 [details] [diff] [review]
[Part 2] Add test case

rs+ for this
Attachment #8726603 - Flags: review?(bugs) → review+
Except that please explain SimpleTest.expectAssertions(0, 5);
Just update the author name. Carry r+ from smaug in commet 25.
Attachment #8726602 - Attachment is obsolete: true
Attachment #8732709 - Flags: review+
Attached patch [Part 2] Add test case, r=smaug (obsolete) — Splinter Review
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+
Keywords: checkin-needed
Keywords: checkin-needed
Rebased with mozilla-inbound. Carry r+ from comment #25.
Attachment #8732757 - Flags: review+
Attached patch [Part 2] Add test case, r=smaug (obsolete) — Splinter Review
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+
- 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+
Keywords: checkin-needed
Rebase. carry r+ from comment #25.
Attachment #8732757 - Attachment is obsolete: true
Flags: needinfo?(kuoe0)
Attachment #8742234 - Flags: review+
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)
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)
Update with better naming.
Attachment #8742291 - Attachment is obsolete: true
Attachment #8742291 - Flags: review?(bugs)
Attachment #8742692 - Flags: review?(bugs)
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)
Attached image [PNG] Test Flow
There is the test flow of my test case. Hope it would help anyone who want to understand the entire execution flow.
I'm not rather lost with the patches. The previous Part 2 looked totally different comparing to the latest Part 2.
Ahaa, the old part 2 is new part 3.
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 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 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+
(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)
initMockAndListener sounds ok.

And the patch is about testing, so it is largely about you feeling we have good enough tests :)
Flags: needinfo?(bugs)
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+
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+
Keywords: checkin-needed
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!
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
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Whiteboard: [ft:conndevices]
blocking-b2g: --- → 2.6+
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
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?
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?
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 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+
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 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 on attachment 8745829 [details] [diff] [review]
[Part 3] (v5) Add test case, r=smaug

Approve for TV 2.6
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.