Currently, NotifySessionConnect is not called when there is a new session request. We should let PresentationService do it.

S.C., Not sure if this is a correct approach. Could you take a quick look? Thanks.

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.

Since the window ID is unique across processes, I think we could also store this in parent process.

Comment on attachment 8749035 [details] [diff] [review] Part1: Refactor to reduce duplicate code Review of attachment 8749035 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

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.

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]

(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.

Rebase.

Rebase.

Rebase

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccfb3317dd4c https://hg.mozilla.org/integration/mozilla-inbound/rev/793ef42cbfe0 https://hg.mozilla.org/integration/mozilla-inbound/rev/6bc253b91d79

https://hg.mozilla.org/mozilla-central/rev/ccfb3317dd4c https://hg.mozilla.org/mozilla-central/rev/793ef42cbfe0 https://hg.mozilla.org/mozilla-central/rev/6bc253b91d79

Clear the needinfo flag since this bug is resolved. We can file another bug to discuss comment #10 if needed.

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

Comment on attachment 8756250 [details] [diff] [review] Part1: Refactor to reduce duplicate code, r=smaug Approve for TV 2.6

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

Comment on attachment 8756252 [details] [diff] [review] Part3: Call NotifySessionConnect, r=smaug Approve for TV 2.6