Implement PresentationRequestUIGlue for Fennec to support XUL browser as the presentation receiver.

Comment on attachment 8796114 [details] Bug 1305352 - (Part 2) Implement PresentationRequestUIGlue on Fennec. https://reviewboard.mozilla.org/r/82074/#review80632

Comment on attachment 8796113 [details] Bug 1305352 - (Part 1) Make Presentation API support XUL browser element. https://reviewboard.mozilla.org/r/82072/#review81730 ::: docshell/base/nsDocShell.cpp:14206 (Diff revision 1) > + nsCOMPtr<nsPIDOMWindowOuter> win = GetWindow(); > + if (!win) { > + return NS_OK; > + } > + nsCOMPtr<Element> frameElement = win->GetFrameElementInternal(); > + *aIsXULElement = frameElement->IsXULElement(); Please null check frameElement (just in case things have been unlinked or so, we might be in an odd state) ::: docshell/base/nsIDocShell.idl:840 (Diff revision 1) > [infallible] readonly attribute boolean isInMozBrowserOrApp; > > /** > + * Returns true if this docshel corresponds to an <xul:browser>. > + */ > + [infallible] readonly attribute boolean isXULElement; So this works only in non-e10s. Better to document that. Also, could you call it isInXULElement. And the documentation says this is only about xul:browser, but the property returns true also for xul:iframe and xul:editor cases, so, better to fix the comment. But. Wouldn't it make more sense to have an attribute in docshell telling whether presentation receiver is supported in it, or something like that.

(In reply to Olli Pettay [:smaug] from comment #4) > ::: docshell/base/nsIDocShell.idl:840 > (Diff revision 1) > > [infallible] readonly attribute boolean isInMozBrowserOrApp; > > > > /** > > + * Returns true if this docshel corresponds to an <xul:browser>. > > + */ > > + [infallible] readonly attribute boolean isXULElement; > > So this works only in non-e10s. Better to document that. > Also, could you call it isInXULElement. > Sorry, I have no idea about why it only works in non-e10s. Does [infallible] cause that? > And the documentation says this is only about > xul:browser, but the property returns true also for xul:iframe and > xul:editor cases, so, better to fix the comment. > > > But. Wouldn't it make more sense to have an attribute in docshell telling > whether presentation receiver is supported in it, or something like that. I also agree that. It would make the functionality of attribute more clear.

(In reply to Tommy Kuo [:KuoE0] from comment #5) > > > > Sorry, I have no idea about why it only works in non-e10s. Does [infallible] > cause that? I mean the top level docshell in child process doesn't have access to its parent xul:browser element (since xul:browser lives in the parent process). So the property doesn't exactly tell anything too useful in e10s.

Hi S.C., I think the part 2 also need your review. Thanks!

Comment on attachment 8796113 [details] Bug 1305352 - (Part 1) Make Presentation API support XUL browser element. https://reviewboard.mozilla.org/r/82072/#review84564 ::: docshell/base/nsDocShell.h:1059 (Diff revision 3) > > // Notify consumers of a search being loaded through the observer service: > void MaybeNotifyKeywordSearchLoading(const nsString& aProvider, > const nsString& aKeyword); > > + bool IsInXULElement(); Could you add a comment that this check doesn't cross process boundary. ::: docshell/base/nsDocShell.cpp:14279 (Diff revision 3) > + > + return frameElement->IsXULElement(); > +} > + > +NS_IMETHODIMP > +nsDocShell::IsAllowedPresentation(nsIURI* aDocURI, bool *aIsAllowedPresentation) nit, bool* aIsAllowedPresentation ::: docshell/base/nsDocShell.cpp:14281 (Diff revision 3) > +} > + > +NS_IMETHODIMP > +nsDocShell::IsAllowedPresentation(nsIURI* aDocURI, bool *aIsAllowedPresentation) > +{ > + // Grant access to browser receiving pages and their same-origin iframes. (App I don't understand how this works in e10s. Shouldn't you add also some check whether the docshell is the top level docshell under TabChild? Note, nsContentUtils::GetPresentationURL does deal with e10s already, so I don't know why we'd break e10s support here. ::: dom/presentation/PresentationSessionInfo.cpp:1431 (Diff revision 3) > + return NS_OK; > + } > + > + // try to detect xul browser element > + nsXULElement* xulbrowser; > + rv = UNWRAP_OBJECT(XULElement, obj, xulbrowser); Hmm, couldn't you above just UNWRAP to Element, and then QueryInterface to nsIFrameLoaderOwner.

Comment on attachment 8796114 [details] Bug 1305352 - (Part 2) Implement PresentationRequestUIGlue on Fennec. https://reviewboard.mozilla.org/r/82074/#review84950 The lifecycle of PresentationView is the major issue to fix. ::: mobile/android/chrome/content/PresentationView.js:30 (Diff revision 3) > + DEBUG && log("Got observe: aTopic=" + aTopic); > + > + let requestData = JSON.parse(aData); > + switch (requestData.type) { > + case "presentation-launch-receiver": > + if (this._id == requestData.windowId) { Use early-return style to reduce indentation. ``` if (this._id !== requestData.windowId) { return; } ``` ::: mobile/android/chrome/content/PresentationView.js:45 (Diff revision 3) > + JSON.stringify(responseData)); > + } > + break; > + } > + }; > + Services.obs.addObserver(handleMozPresentationChromeEvent, I'll expect the PresentationView.js have its own lifecycle, remove the observer while presentation terminated. ::: mobile/android/components/PresentationRequestUIGlue.js:9 (Diff revision 3) > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict" > + > +var DEBUG = true; change to false before check-in ::: mobile/android/components/PresentationRequestUIGlue.js:32 (Diff revision 3) > + > + let localDevice; > + try { > + localDevice = aDevice.QueryInterface(Ci.nsIPresentationLocalDevice); > + } catch (e) { > + DEBUG && log("Not an 1-UA device.") Add a comment that describe this restriction is temporary and should be removed once we decide to support 2-UAs receiver in Fennec. ::: mobile/android/components/PresentationRequestUIGlue.js:40 (Diff revision 3) > + > + return new Promise((aResolve, aReject) => { > + > + let handleMozPresentationContentEvent = (aSubject, aTopic, aData) => { > + let data = JSON.parse(aData); > + if (data.id != aSessionId) { I'll prefer creating a local, unique "requestId" for identifying each request instead of using sessionId. In this case you can avoid depending on the assumption of "no two sendRequest is called with the same sessionId", which might not hold true in very complicated fail/retry scenario. ::: mobile/android/components/PresentationRequestUIGlue.js:47 (Diff revision 3) > + } > + > + Services.obs.removeObserver(handleMozPresentationContentEvent, > + "mozPresentationContentEvent"); > + > + switch(data.type) { `data.result` should be more clear and you can use simpler value like "success"/"error". ::: mobile/android/components/PresentationRequestUIGlue.js:61 (Diff revision 3) > + Services.obs.addObserver(handleMozPresentationContentEvent, > + "mozPresentationContentEvent", > + false); > + > + let data = { > + type: "presentation-launch-receiver", no need to provide extra type here since we don't multiplex this notification topic with other events. ::: mobile/android/components/PresentationRequestUIGlue.js:62 (Diff revision 3) > + url: aURL, > + id: aSessionId, > + windowId: localDevice.windowId nit: correct alignment ::: mobile/android/components/PresentationRequestUIGlue.js:66 (Diff revision 3) > + type: "presentation-launch-receiver", > + url: aURL, > + id: aSessionId, > + windowId: localDevice.windowId > + }; > + Services.obs.notifyObservers(null, "mozPresentationChromeEvent", JSON.stringify(data)); ChromeEvent/ContentEvent are old naming convention for B2G. I'll suggest to rename it like: - presentation-receiver:launch - presentation-receiver:launch:response

Comment on attachment 8796113 [details] Bug 1305352 - (Part 1) Make Presentation API support XUL browser element. https://reviewboard.mozilla.org/r/82072/#review84564 I decided to move the logic of isAllowedPresentation back to Presentation.cpp to prevent to increase too much LOC in DocShell.cpp. > I don't understand how this works in e10s. Shouldn't you add also some check whether the docshell is the top level docshell under TabChild? > > Note, nsContentUtils::GetPresentationURL does deal with e10s already, so I don't know why we'd break e10s support here. I added the e10s support for that!

Comment on attachment 8796113 [details] Bug 1305352 - (Part 1) Make Presentation API support XUL browser element. https://reviewboard.mozilla.org/r/82072/#review85542 Fix the issue in nsFrameLoader and explain why using GetSameTypeRootTreeItem example I added wouldn't be enough. If GetSameTypeRootTreeItem approach would be enough, then this all would become a lot simpler. Just few lines of code in Presentation.cpp, and nothing else. ::: dom/base/nsFrameLoader.cpp:3391 (Diff revision 5) > > nsCOMPtr<nsIDocShell> docShell = mOwnerContent->OwnerDoc()->GetDocShell(); > nsCOMPtr<nsILoadContext> parentContext = do_QueryInterface(docShell); > NS_ENSURE_STATE(parentContext); > > + bool isInXULElement = docShell->GetIsInXULElement(); This can't work. In Firefox ownerContent's docshell is the top level docshell which isn't inside any xul elements. What you may want here is bool isInXULElement = mOwnerContent->IsXULElement(); ::: dom/presentation/Presentation.cpp:61 (Diff revision 5) > nsCOMPtr<nsIDocShell> docshell = inner->GetDocShell(); > if (!docshell) { > return false; > } > > - if (!docshell->GetIsInMozBrowserOrApp()) { > + if (!docshell->GetIsInMozBrowserOrApp() && !docshell->GetIsInXULElement()) { I still wonder if "IsInXULElement" is the right question to ask. Don't you want more like "Is the top level content docshell"? That would be way simpler to implement than all this IsInXULElement flag propagation. Something like the following should work both in e10s and in non-e10s: bool isTopLevelContentShell = false; nsCOMPtr<nsILoadContent> lc = do_QueryInterface(docShell); bool isContent = false; lc->GetIsContent(&isContent); if (isContent) { nsCOMPtr<nsIDocShellTreeItem> root; docShell->GetSameTypeRootTreeItem(getter_AddRefs(root)); isTopLevelContentShell = root == docshell; } Would that be enough here? (Sorry that I didn't think this before)

Actually, we don't care about whether it is the top level docshell. We just wanna know whether it is a XUL element. In our work, I launch the PresentationView.xul (see the patch on Bug 1285870) on Fennec. And use the <xul:browser> as the presentation receiver. We just need to let the <xul:browser> can pass the HasReceiverSupport check. To handle the e10s and non-e10s, I think if we are in content process and can not get the frame element, we should query the IsInXULElement info from TabChild. Else, we query the IsXULElement from frame element directly.

Comment on attachment 8796114 [details] Bug 1305352 - (Part 2) Implement PresentationRequestUIGlue on Fennec. https://reviewboard.mozilla.org/r/82074/#review85798 r+ with my comments addressed. ::: mobile/android/chrome/content/PresentationView.js:1 (Diff revision 5) > /* This Source Code Form is subject to the terms of the Mozilla Public nit: add mode line ::: mobile/android/chrome/content/PresentationView.js:57 (Diff revision 5) > + Services.obs.notifyObservers(browser, > + PRESENTATION_RECEIVER_LAUNCH_RESPONSE_TOPIC, > + JSON.stringify({ result: "success", > + requestId: requestData.requestId })); Move this into try block. ::: mobile/android/chrome/content/PresentationView.js:59 (Diff revision 5) > + reason: e.message })); > + } > + > + Services.obs.notifyObservers(browser, > + PRESENTATION_RECEIVER_LAUNCH_RESPONSE_TOPIC, > + JSON.stringify({ result: "success", nit: remove extra space before "success". ::: mobile/android/components/PresentationRequestUIGlue.js:1 (Diff revision 5) > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ nit: fix mode line, see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line ::: mobile/android/components/PresentationRequestUIGlue.js:65 (Diff revision 5) > + }; > + Services.obs.addObserver(handleObserve, nit: add extra line after `};`. ::: mobile/android/components/PresentationRequestUIGlue.js:70 (Diff revision 5) > + let data = { url: aURL, > + windowId: localDevice.windowId, > + requestId: requestId }; I'll prefer style like: ``` let data = { url: aURL, windowId: localDeivce.windowId, requestId: requestId, }; ```

(In reply to Tommy Kuo [:KuoE0] from comment #20) > Actually, we don't care about whether it is the top level docshell. We just > wanna know whether it is a XUL element. In our work, I launch the > PresentationView.xul (see the patch on Bug 1285870) on Fennec. And use the > <xul:browser> as the presentation receiver. We just need to let the > <xul:browser> can pass the HasReceiverSupport check. > > To handle the e10s and non-e10s, I think if we are in content process and > can not get the frame element, we should query the IsInXULElement info from > TabChild. Else, we query the IsXULElement from frame element directly. Ok. But why top level content docshell isn't good enough for this?

+---+ | A | PresentationView.xul +-+-+ | | +---+ +---> B | <xul:browser> +-+-+ | +---+ | +---+ | C <--+--> D | +---+ +---+ Actually, I didn't understand the method of top-level content docshell. I think I should explain what I want to do in detail. In this figure, A is the top-level window (PresentationView.xul) and B is the <xul:browser> element in PresentationView.xul. C & D may be some other <iframe> in the page we opened by B. If some page uses Presentation API as the receiver in B, it should be passed the check. And if it uses Presentation API as the receiver in C or D, it should not be passed the check. So, if the method of top-level content docshell can totally do that, I think that would be a good way!

I don't understand that picture. Is B (1) some web page under <xul:browser> or (2) is B xul:browser? If (1) and xul:browser has type="content...", top level content docshell is same as B.

B is the <xul:browser> and I set the "src" attribute to make it load some page as the presentation receiver.

So nothing represent the docshell underneath xul:browser? And C and D are iframes... so where do those iframes live. But I think we're talking about the same thing. xul:browser owns an nsFrameLoader which owns in non-e10s a DocShell which contains the top level web page, and in e10s nsFrameLoader owns TabParent which then has connection to TabChild in child process and TabChild owns the docshell for the top level web page there.

Hi smaug, the top level content docshell works well. Please review again, thanks.

hmm, did you update the patch? I'm still seeing Presentation::HasReceiverSupport using && !docshell->GetIsInXULElement()). What does the spec say about Presentation.receiver? Should it be hidden in non-top-level receiving web pages or just null? Looks like "In any other browsing context, it MUST return null". So adding the check to HasReceiverSupport isn't right, but it should be in Presentation::GetReceiver(). Could you add a test for it - perhaps test could have an iframe in the receiver side and inside iframe 'receiver' property should still be there in Presentation object, but just returning null.

(In reply to Olli Pettay [:smaug] (reviewing overload, queue closed for a day or two) from comment #28) > hmm, did you update the patch? I'm still seeing > Presentation::HasReceiverSupport using && !docshell->GetIsInXULElement()). > > What does the spec say about Presentation.receiver? Should it be hidden in > non-top-level receiving web pages or just null? > Looks like "In any other browsing context, it MUST return null". So adding > the check to > HasReceiverSupport isn't right, but it should be in > Presentation::GetReceiver(). > Could you add a test for it - perhaps test could have an iframe in the > receiver side and inside iframe 'receiver' property should still be there in > Presentation object, but just returning null. There is already a test case for that: see https://dxr.mozilla.org/mozilla-central/source/dom/presentation/tests/mochitest/file_presentation_receiver.html#72-90

ahaa, but wouldn't the current patch break it? And, per spec, shouldn't .receiver be null (but not undefined!) in iframes, since inside iframe the browsing context isn't "receiving browsing context"?

I forgot to publish the changes, and the latest patch is there, now. I also ran the test and no test case is broken.

Comment on attachment 8796113 [details] Bug 1305352 - (Part 1) Make Presentation API support XUL browser element. https://reviewboard.mozilla.org/r/82072/#review88482 ::: dom/base/nsContentUtils.cpp:9371 (Diff revision 7) > > topFrameElement->GetAttribute(NS_LITERAL_STRING("mozpresentation"), aPresentationUrl); > } > > +/* static */ bool > +nsContentUtils::IsTopLevelContentDocShell(nsIDocShell* aDocShell) hmm, this is ok too, but would be even simpler to add [infallible] attribute boolean IsTopLevelContentDocShell() to nsIDocShell. And there the implementation could be something like NS_IMETHODIMP nsDocShell::IsTopLevelContentDocShell(bool* aRetVal) { if (mItemType == typeContent) { nsCOMPtr<nsIDocShellTreeItem> root; GetSameTypeRootTreeItem(getter_AddRefs(root)); *aRetVal = root.get() == static_cast<nsIDocShellTreeItem*>(this) } else { *aRetVal = false; } return NS_OK; } ::: dom/presentation/Presentation.cpp:62 (Diff revision 7) > if (!docshell) { > return false; > } > > - if (!docshell->GetIsInMozBrowserOrApp()) { > + if (!docshell->GetIsInMozBrowserOrApp() && > + !nsContentUtils::IsTopLevelContentDocShell(docshell)) { So, this is still wrong, as far as I see. iframes under the top level receiver page should have presentation.receiver == null;, not presentation.receiver == undefined.

Comment on attachment 8796113 [details] Bug 1305352 - (Part 1) Make Presentation API support XUL browser element. https://reviewboard.mozilla.org/r/82072/#review88482 > So, this is still wrong, as far as I see. > iframes under the top level receiver page should have presentation.receiver == null;, not > presentation.receiver == undefined. I filed bug 1314229 to do that and schien will fix it.

Comment on attachment 8796113 [details] Bug 1305352 - (Part 1) Make Presentation API support XUL browser element. https://reviewboard.mozilla.org/r/82072/#review89264

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d871bdab43f23f1faac6e838b460ceac2254b3e

Pushed by schien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a64e70dc94f7 (Part 1) Make Presentation API support XUL browser element. r=smaug https://hg.mozilla.org/integration/autoland/rev/47f86091dedd (Part 2) Implement PresentationRequestUIGlue on Fennec. r=schien,snorp

https://hg.mozilla.org/mozilla-central/rev/a64e70dc94f7 https://hg.mozilla.org/mozilla-central/rev/47f86091dedd