[Presentation WebAPI] Add role in PresentationService

RESOLVED FIXED in Firefox 49, Firefox OS v2.6

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Tommy Kuo (away forever...), Assigned: Tommy Kuo (away forever...))

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.6+, firefox49 fixed, b2g-v2.6 fixed)

Details

(Whiteboard: [ft:conndevices])

Attachments

(4 attachments, 18 obsolete attachments)

47.17 KB, patch
Tommy Kuo (away forever...)
: 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
Tommy Kuo (away forever...)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
For 1-UA use case, the sender is also the receiver. So, PresentationService need to know its direction to call functions.
(Assignee)

Updated

2 years ago
Blocks: 1184036
(Assignee)

Comment 1

2 years ago
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)
(Assignee)

Comment 3

2 years ago
Created attachment 8702183 [details] [diff] [review]
Bug-1234492-hg.patch

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

2 years ago
Blocks: 1208417
(Assignee)

Comment 4

2 years ago
Hi smaug, can you review this patch?
Flags: needinfo?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8702183 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Flags: needinfo?(bugs)
(Assignee)

Comment 5

2 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

2 years ago
ah, that comment is about what the patch fixes, not some issue in the patch :) 
Ok, reviewing.

Comment 7

2 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

2 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)
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)
(Assignee)

Comment 11

2 years ago
Created attachment 8709914 [details] [diff] [review]
Bug-1234492-hg.patch

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

2 years ago
Created attachment 8709917 [details] [diff] [review]
Using enum for roles

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+
(Assignee)

Comment 14

2 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)
.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

2 years ago
Created attachment 8712060 [details] [diff] [review]
patch with using enum

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

2 years ago
Created attachment 8713542 [details] [diff] [review]
patch without enum

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

2 years ago
Attachment #8712060 - Attachment description: Bug-1234492-hg.patch → patch with using enum
Attachment #8712060 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 19

2 years ago
Created attachment 8726599 [details] [diff] [review]
[Part 1] Add direction in PresentationService, r=smaug

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

2 years ago
Created attachment 8726600 [details] [diff] [review]
[Part 2] Add test case

Add test case to make sure that sender and receiver can co-exist in the same side.
(Assignee)

Comment 21

2 years ago
Created attachment 8726602 [details] [diff] [review]
[Part 1] Add direction in PresentationService, r=smaug

fix conflict with m-c
Attachment #8726599 - Attachment is obsolete: true
(Assignee)

Comment 22

2 years ago
Created attachment 8726603 [details] [diff] [review]
[Part 2] Add test case

fix conflict with m-c
Attachment #8726600 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8726602 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8726603 - Flags: review?(bugs)
(Assignee)

Comment 23

2 years ago
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);
(Assignee)

Comment 28

2 years ago
Created attachment 8732709 [details] [diff] [review]
[Part 1] Add direction in PresentationService, r=smaug

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

2 years ago
Created attachment 8732726 [details] [diff] [review]
[Part 2] Add test case, r=smaug

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

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 30

2 years ago
Created attachment 8732757 [details] [diff] [review]
[Part 1] Add direction in PresentationService, r=smaug

Rebased with mozilla-inbound. Carry r+ from comment #25.
Attachment #8732757 - Flags: review+
(Assignee)

Comment 31

2 years ago
Created attachment 8732758 [details] [diff] [review]
[Part 2] Add test case, r=smaug

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

2 years ago
Created attachment 8736118 [details] [diff] [review]
[Part 2] (v2) Add test case, r=smaug

- 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)

Updated

2 years ago
Keywords: checkin-needed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=24844836&repo=mozilla-inbound
Flags: needinfo?(kuoe0)
(Assignee)

Comment 37

2 years ago
Created attachment 8742234 [details] [diff] [review]
[Part 1] (v2) Add direction in PresentationService, r=smaug

Rebase. carry r+ from comment #25.
Attachment #8732757 - Attachment is obsolete: true
Flags: needinfo?(kuoe0)
Attachment #8742234 - Flags: review+
(Assignee)

Comment 38

2 years ago
Created attachment 8742280 [details] [diff] [review]
[Part 2] (v1) Use the role definition in nsIPresentationService, r=smaug

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

2 years ago
Created attachment 8742291 [details] [diff] [review]
[Part 3] (v1) Add test case, r=smaug

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 41

2 years ago
Created attachment 8742692 [details] [diff] [review]
[Part 3] (v2) Add test case, r=smaug

Update with better naming.
Attachment #8742291 - Attachment is obsolete: true
Attachment #8742291 - Flags: review?(bugs)
Attachment #8742692 - Flags: review?(bugs)
(Assignee)

Comment 42

2 years ago
Created attachment 8744238 [details] [diff] [review]
[Part 3] (v3) Add test case, r=smaug

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

2 years ago
Created attachment 8744243 [details]
[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.
(Assignee)

Comment 46

2 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 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+
(Assignee)

Comment 49

2 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)
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

2 years ago
Created attachment 8744416 [details] [diff] [review]
[Part 3] (v4) Add test case, r=smaug

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 56

2 years ago
Created attachment 8745829 [details] [diff] [review]
[Part 3] (v5) Add test case, r=smaug

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

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 57

2 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 59

2 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
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

2 years ago
Whiteboard: [ft:conndevices]

Updated

2 years ago
blocking-b2g: --- → 2.6+
(Assignee)

Comment 60

2 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

2 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

2 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

2 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

2 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

2 years ago
status-b2g-v2.6: --- → affected

Comment 65

2 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

2 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

2 years ago
Comment on attachment 8745829 [details] [diff] [review]
[Part 3] (v5) Add test case, r=smaug

Approve for TV 2.6
You need to log in before you can comment on or make changes to this bug.