Closed Bug 1267965 Opened 4 years ago Closed 4 years ago

[Presentation WebAPI] PresentationService should call NotifySessionConnect for new coming session

Categories

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

defect
Not set

Tracking

()

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

People

(Reporter: kershaw, Assigned: kershaw)

References

Details

(Whiteboard: btpp-fixlater [ft:conndevices])

Attachments

(3 files, 4 obsolete files)

No description provided.
Blocks: 1258602
Currently, NotifySessionConnect is not called when there is a new session request. We should let PresentationService do it.
Whiteboard: btpp-fixlater
S.C.,

Not sure if this is a correct approach. Could you take a quick look?

Thanks.
Assignee: nobody → kechang
Attachment #8748046 - Flags: feedback?(schien)
Comment on attachment 8748046 [details] [diff] [review]
Call NotifySessionConnect when getting a new session

Review of attachment 8748046 [details] [diff] [review]:
-----------------------------------------------------------------

Similar to PesentationService::StartSession, should create a callback path for PresentationPresentingInfo.
1. PresentationService should maintain a table of mapping presentationId to a valid windowId, and use it as a gate keeper for the listener owner.
2. PresentationPresentingInfo should store a listener of nsIPresentationRespondingListener and the corresponding windowId.
3. While session transport is connected, notify via nsIPresentationRespondingListener::NotifySessionConnect. Or, delay the callback until listener is set.

::: dom/presentation/PresentationService.h
@@ +57,5 @@
>  
>    // Store the responding listener based on the window ID of the (in-process or
>    // OOP) receiver page.
>    // TODO Bug 1195605 - Support many-to-one session.
>    // So far responding listeners are registered but |notifySessionConnect| hasn't

you'll need to update this comment.

@@ +73,4 @@
>    nsDataHashtable<nsStringHashKey, uint64_t> mRespondingWindowIds;
> +
> +  // Queue the session ID if the corresponding responding listener is not registered yet.
> +  nsClassHashtable<nsUint64HashKey, nsTArray<nsString>> mPendingSessionIds;

You don't need to store the pending session Id, just delay the PresentationSessionInfo::ReplySuccess until mCallback is provided.
Attachment #8748046 - Flags: feedback?(schien) → feedback-
Attachment #8748046 - Attachment is obsolete: true
Attachment #8749035 - Flags: feedback?(schien)
Since the window ID is unique across processes, I think we could also store this in parent process.
Attachment #8749039 - Flags: feedback?(schien)
Attached patch Part3: Call NotifySessionConnect (obsolete) — Splinter Review
Attachment #8749040 - Flags: feedback?(schien)
Comment on attachment 8749035 [details] [diff] [review]
Part1: Refactor to reduce duplicate code

Review of attachment 8749035 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm
Attachment #8749035 - Flags: feedback?(schien) → feedback+
Attachment #8749039 - Flags: feedback?(schien) → feedback+
Attachment #8749040 - Flags: feedback?(schien) → feedback+
Attachment #8749035 - Flags: review?(bugs)
Attachment #8749039 - Flags: review?(bugs)
Attachment #8749040 - Flags: review?(bugs)
Comment on attachment 8749035 [details] [diff] [review]
Part1: Refactor to reduce duplicate code


>+PresentationServiceBase::GetExistentSessionIdAtLaunchInternal(
>+  uint64_t aWindowId,
>+  nsAString& aSessionId)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+
>+  nsTArray<nsString>* sessionIdArray;
>+  if (mRespondingSessionIds.Get(aWindowId, &sessionIdArray) &&
>+      !sessionIdArray->IsEmpty()) {
>+    aSessionId.Assign((*sessionIdArray)[0]);
Note to myself: Ok, so for now we use only index 0

>+  }
>+  else {
nit,
} else {

>+  // Store the mapping between the window ID of the in-process and OOP page and the ID
>+  // of the responding session. It's used for an receiver page to retrieve
>+  // the correspondent session ID. Besides, also keep the mapping
>+  // between the responding session ID and the window ID to help look up the
>+  // window ID.
>+  nsClassHashtable<nsUint64HashKey, nsTArray<nsString>> mRespondingSessionIds;
So we don't yet use the array value for anything special, just the index 0 of it.
I assume it will be used in other patches, and the comment about the hashtable will be then changed a bit.
Attachment #8749035 - Flags: review?(bugs) → review+
Attachment #8749039 - Flags: review?(bugs) → review+
Comment on attachment 8749040 [details] [diff] [review]
Part3: Call NotifySessionConnect

>+++ b/dom/presentation/PresentationService.cpp
>@@ -596,16 +596,25 @@ PresentationService::RegisterRespondingListener(
>   MOZ_ASSERT(NS_IsMainThread());
>   MOZ_ASSERT(aListener);
> 
>   nsCOMPtr<nsIPresentationRespondingListener> listener;
>   if (mRespondingListeners.Get(aWindowId, getter_AddRefs(listener))) {
>     return (listener == aListener) ? NS_OK : NS_ERROR_DOM_INVALID_STATE_ERR;
>   }
> 
>+  nsTArray<nsString>* sessionIdArray;
>+  if (!mRespondingSessionIds.Get(aWindowId, &sessionIdArray)) {
>+    return NS_ERROR_INVALID_ARG;
>+  }
Why do we return error value here. Is it not ok to register listener before session?
Please explain, or fix so that we do something like [XXX]:
  mRespondingListeners.Put(aWindowId, aListener);
  nsTArray<nsString>* sessionIdArray;
  if (mRespondingSessionIds.Get(aWindowId, &sessionIdArray)) {
    for (const auto& id : *sessionIdArray) {
      aListener->NotifySessionConnect(aWindowId, id);
    }
  }
  return NS_OK;
  

>+
>+  for (const auto& id : *sessionIdArray) {
This is an edge case for auto... I guess I can live with it here.

>+    aListener->NotifySessionConnect(aWindowId, id);
>+  }
>+
>   mRespondingListeners.Put(aWindowId, aListener);
Don't we want to have this before calling NotifySessionConnect.
In theory NotifySessionConnect might want to remove the listener.


r+, but please explain or fix [XXX]
Attachment #8749040 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8749040 [details] [diff] [review]
> Part3: Call NotifySessionConnect
> 
> >+++ b/dom/presentation/PresentationService.cpp
> >@@ -596,16 +596,25 @@ PresentationService::RegisterRespondingListener(
> >   MOZ_ASSERT(NS_IsMainThread());
> >   MOZ_ASSERT(aListener);
> > 
> >   nsCOMPtr<nsIPresentationRespondingListener> listener;
> >   if (mRespondingListeners.Get(aWindowId, getter_AddRefs(listener))) {
> >     return (listener == aListener) ? NS_OK : NS_ERROR_DOM_INVALID_STATE_ERR;
> >   }
> > 
> >+  nsTArray<nsString>* sessionIdArray;
> >+  if (!mRespondingSessionIds.Get(aWindowId, &sessionIdArray)) {
> >+    return NS_ERROR_INVALID_ARG;
> >+  }
> Why do we return error value here. Is it not ok to register listener before
> session?

Before registering a listener, there should be at least one pair in mRespondingSessionIds where the mapping is created in PresentationService::NotifyReceiverReady. If we can not find the mapping in mRespondingSessionIds with the window ID, it is likely that this window is not a presented page. So, I think we should treat this as an error.

> 
> >+
> >+  for (const auto& id : *sessionIdArray) {
> This is an edge case for auto... I guess I can live with it here.

I don't quite understand why this is an edge case. Could you please explain more? Thanks.

> >+    aListener->NotifySessionConnect(aWindowId, id);
> >+  }
> >+
> >   mRespondingListeners.Put(aWindowId, aListener);
> Don't we want to have this before calling NotifySessionConnect.
> In theory NotifySessionConnect might want to remove the listener.
> 
Sorry, I don't understand why NotifySessionConnect might want to remove the listener. Could you elaborate, please?
Need info for comment 10. Thanks.
Flags: needinfo?(bugs)
blocking-b2g: --- → 2.6+
Rebase.
Attachment #8749035 - Attachment is obsolete: true
Attachment #8749039 - Attachment is obsolete: true
Attachment #8749040 - Attachment is obsolete: true
Attachment #8756250 - Flags: review+
Keywords: checkin-needed
Clear the needinfo flag since this bug is resolved. We can file another bug to discuss comment #10 if needed.
Flags: needinfo?(bugs)
Whiteboard: btpp-fixlater → btpp-fixlater [ft:conndevices]
Attachment #8756250 - Flags: approval-mozilla-b2g48?
Attachment #8756251 - Flags: approval-mozilla-b2g48?
Attachment #8756252 - Flags: approval-mozilla-b2g48?
Josh, please help to approve. Thanks.

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 #): Presentation API
User impact if declined: NotifySessionConnect would not work for PresentationReceiver.
Testing completed: with automation test
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: n/a
Flags: needinfo?(jocheng)
Comment on attachment 8756250 [details] [diff] [review]
Part1: Refactor to reduce duplicate code, r=smaug

Approve for TV 2.6
Flags: needinfo?(jocheng)
Attachment #8756250 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Comment on attachment 8756251 [details] [diff] [review]
Part2: Store responding window ID and  session ID in parent process, r=smaug

Approve for TV 2.6
Attachment #8756251 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Comment on attachment 8756252 [details] [diff] [review]
Part3: Call NotifySessionConnect, r=smaug

Approve for TV 2.6
Attachment #8756252 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.