Please see https://w3c.github.io/presentation-api/#interface-presentationrequest.

Per discussion with @kershaw, I filed a spec bug for the prohibits mixed security contexts algorithm on multiple presentation URLs. https://github.com/w3c/presentation-api/issues/329

@SC, Please take a look. Thanks.

Comment on attachment 8781875 [details] [diff] [review] Construct PresentationRequest with multiple URLs Review of attachment 8781875 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/presentation/PresentationCallbacks.cpp @@ +74,5 @@ > > +NS_IMETHODIMP > +PresentationRequesterCallback::NotifyRequestUrlSelected(const nsAString& aUrl) > +{ > + mUrl = aUrl; Assert if mUrl is already assigned. ::: dom/presentation/PresentationCallbacks.h @@ +41,3 @@ > nsString mSessionId; > RefPtr<Promise> mPromise; > + nsString mUrl; This modification seems not necessary. ::: dom/presentation/PresentationService.cpp @@ +569,5 @@ > + > + nsAutoString requestUrl; > + if (NS_WARN_IF(NS_FAILED(SelectRequestUrl(aUrls, requestUrl)))) { > + return aCallback->NotifyError(NS_ERROR_DOM_OPERATION_ERR); > + } Request URL can only be decided after device selection.

Fixed the previous comments. Please take a look. Thanks.

Comment on attachment 8783422 [details] [diff] [review] Construct PresentationRequest with multiple URLs - v2 Review of attachment 8783422 [details] [diff] [review]: ----------------------------------------------------------------- Let's move the device related modification to bug 1228526. I've upload the corresponding patches.

Rebased on bug 1228526.

Comment on attachment 8786277 [details] [diff] [review] Construct PresentationRequest with multiple URLs - v3 Review of attachment 8786277 [details] [diff] [review]: ----------------------------------------------------------------- Just a reminder, I'm landing Bug 1228508 and you'll need to add multiple URL support for PresentationAvailability as well. ::: dom/presentation/interfaces/nsIPresentationService.idl @@ +42,5 @@ > + * Called when the url is selected for starting a session. > + * > + * @param url: the selected url. > + */ > + void notifyRequestUrlSelected(in DOMString url); pass |url| in notifySuccess() directly, file a bug for renaming nsIPresentationServiceCallback to nsIPresentationRequestCallback.

Summary: 1. Implement constructing PresentationRequest with multiple URLs 2. Add url as a parameter in |nsIPresentationServiceCallback::NotifySuccess|.

(I'm not sure I like the API supporting multiple urls. Seems to make the API a bit awkward. And as far as I see, supporting multiple urls could have been build using JS on top of supporting just one url)

Comment on attachment 8787108 [details] [diff] [review] Construct PresentationRequest with multiple URLs - v4 > /* static */ already_AddRefed<PresentationRequest> > PresentationRequest::Constructor(const GlobalObject& aGlobal, > const nsAString& aUrl, > ErrorResult& aRv) > { >+ Sequence<nsString> urls; >+ urls.AppendElement(aUrl, fallible); >+ return Constructor(aGlobal, urls, aRv); >+} >+ >+/* static */ already_AddRefed<PresentationRequest> >+PresentationRequest::Constructor(const GlobalObject& aGlobal, >+ const Sequence<nsString>& aUrls, >+ ErrorResult& aRv) >+{ > nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(aGlobal.GetAsSupports()); > if (!window) { > aRv.Throw(NS_ERROR_UNEXPECTED); > return nullptr; > } > >- // Ensure the URL is not empty. >- if (NS_WARN_IF(aUrl.IsEmpty())) { >- aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR); >- return nullptr; >+ // Ensure each URL is not empty. >+ for (const auto& url : aUrls) { >+ if (NS_WARN_IF(url.IsEmpty())) { >+ aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR); >+ return nullptr; >+ } > } This is not what the spec say. if aUrls is empty array, then NotSupportedError should be thrown > > // Resolve relative URL to absolute URL > nsCOMPtr<nsIURI> baseUri = window->GetDocBaseURI(); >+ nsTArray<nsString> urls; >+ for (const auto& url : aUrls) { >+ nsAutoString absoluteUrl; >+ nsresult rv = >+ GetAbsoluteURL(url, baseUri, window->GetExtantDoc(), absoluteUrl); The spec says use 'entry settings object', but here we use something else, current settings object. https://html.spec.whatwg.org/multipage/webappapis.html#realms-settings-objects-global-objects I believe new specs shouldn't use entry settings object though, so perhaps our code is right, and the spec is wrong. >+ if (NS_WARN_IF(NS_FAILED(rv))) { >+ aRv.Throw(NS_ERROR_UNEXPECTED); >+ return nullptr; >+ } If resolving absolute url fails, then throw SyntaxError

I filed https://github.com/w3c/presentation-api/issues/336

Thanks for the detailed review. Sorry that I didn't check the spec thoroughly. I just modify based on the original code. Please take a look again.

Comment on attachment 8787287 [details] [diff] [review] Construct PresentationRequest with multiple URLs - v5 > GetAbsoluteURL(const nsAString& aUrl, > nsIURI* aBaseUri, > nsIDocument* aDocument, > nsAString& aAbsoluteUrl) > { >+ if (aUrl.IsEmpty()) { >+ return NS_ERROR_FAILURE; >+ } >+ Why this change? Nothing in the spec says empty strings as urls should be handled specially. So, remove this.

(In reply to Olli Pettay [:smaug] from comment #13) > Comment on attachment 8787287 [details] [diff] [review] > Construct PresentationRequest with multiple URLs - v5 > > > GetAbsoluteURL(const nsAString& aUrl, > > nsIURI* aBaseUri, > > nsIDocument* aDocument, > > nsAString& aAbsoluteUrl) > > { > >+ if (aUrl.IsEmpty()) { > >+ return NS_ERROR_FAILURE; > >+ } > >+ > Why this change? Nothing in the spec says empty strings as urls should be > handled specially. > So, remove this. Will be removed in next version.

Rebase and carry r+ from smaug. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fec1b0ad4be

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/41bad373bd63 Construct PresentationRequest with multiple URLs, r=smaug