Closed
Bug 1267965
Opened 9 years ago
Closed 9 years ago
[Presentation WebAPI] PresentationService should call NotifySessionConnect for new coming session
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
People
(Reporter: kershaw, Assigned: kershaw)
References
Details
(Whiteboard: btpp-fixlater [ft:conndevices])
Attachments
(3 files, 4 obsolete files)
22.66 KB,
patch
|
kershaw
:
review+
jocheng
:
approval-mozilla-b2g48+
|
Details | Diff | Splinter Review |
6.07 KB,
patch
|
kershaw
:
review+
jocheng
:
approval-mozilla-b2g48+
|
Details | Diff | Splinter Review |
3.36 KB,
patch
|
kershaw
:
review+
jocheng
:
approval-mozilla-b2g48+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Currently, NotifySessionConnect is not called when there is a new session request. We should let PresentationService do it.
Updated•9 years ago
|
Whiteboard: btpp-fixlater
Assignee | ||
Comment 2•9 years ago
|
||
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 3•9 years ago
|
||
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-
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8748046 -
Attachment is obsolete: true
Attachment #8749035 -
Flags: feedback?(schien)
Assignee | ||
Comment 5•9 years ago
|
||
Since the window ID is unique across processes, I think we could also store this in parent process.
Attachment #8749039 -
Flags: feedback?(schien)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8749040 -
Flags: feedback?(schien)
Comment 7•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8749039 -
Flags: feedback?(schien) → feedback+
Updated•9 years ago
|
Attachment #8749040 -
Flags: feedback?(schien) → feedback+
Assignee | ||
Updated•9 years ago
|
Attachment #8749035 -
Flags: review?(bugs)
Assignee | ||
Updated•9 years ago
|
Attachment #8749039 -
Flags: review?(bugs)
Assignee | ||
Updated•9 years ago
|
Attachment #8749040 -
Flags: review?(bugs)
Comment 8•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8749039 -
Flags: review?(bugs) → review+
Comment 9•9 years ago
|
||
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+
Assignee | ||
Comment 10•9 years ago
|
||
(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?
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Comment 13•9 years ago
|
||
Assignee | ||
Comment 14•9 years ago
|
||
Rebase.
Attachment #8749035 -
Attachment is obsolete: true
Attachment #8749039 -
Attachment is obsolete: true
Attachment #8749040 -
Attachment is obsolete: true
Attachment #8756250 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 17•9 years ago
|
||
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
Keywords: checkin-needed
Comment 18•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ccfb3317dd4c
https://hg.mozilla.org/mozilla-central/rev/793ef42cbfe0
https://hg.mozilla.org/mozilla-central/rev/6bc253b91d79
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 19•9 years ago
|
||
Clear the needinfo flag since this bug is resolved. We can file another bug to discuss comment #10 if needed.
Flags: needinfo?(bugs)
Assignee | ||
Updated•9 years ago
|
Whiteboard: btpp-fixlater → btpp-fixlater [ft:conndevices]
Assignee | ||
Updated•9 years ago
|
Attachment #8756250 -
Flags: approval-mozilla-b2g48?
Assignee | ||
Updated•9 years ago
|
Attachment #8756251 -
Flags: approval-mozilla-b2g48?
Assignee | ||
Updated•9 years ago
|
Attachment #8756252 -
Flags: approval-mozilla-b2g48?
Assignee | ||
Comment 20•9 years ago
|
||
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 21•9 years ago
|
||
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 22•9 years ago
|
||
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 23•9 years ago
|
||
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+
Updated•9 years ago
|
status-b2g-v2.6:
--- → affected
Comment 24•9 years ago
|
||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•