1288297 - [Presentation WebAPI] Support constructing PresentationRequest with multiple URLs

Status: RESOLVED FIXED

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

Description

[Kershaw Chang [:kershaw]]

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

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 2

[Kershaw Chang [:kershaw]]

Attachment: Construct PresentationRequest with multiple URLs

@SC,

Please take a look. Thanks.

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 4

[Kershaw Chang [:kershaw]]

Attachment: Construct PresentationRequest with multiple URLs - v2

Fixed the previous comments.

Please take a look. Thanks.

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Construct PresentationRequest with multiple URLs - v3

Rebased on bug 1228526.

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 8

[Kershaw Chang [:kershaw]]

Attachment: Construct PresentationRequest with multiple URLs - v4

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

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 12

[Kershaw Chang [:kershaw]]

Attachment: Construct PresentationRequest with multiple URLs - v5

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 13

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 14

[Kershaw Chang [:kershaw]]

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

Comment 15

[Kershaw Chang [:kershaw]]

Attachment: Construct PresentationRequest with multiple URLs, r=smaug

Rebase and carry r+ from smaug.

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fec1b0ad4be

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41bad373bd63
Construct PresentationRequest with multiple URLs, r=smaug

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/41bad373bd63