[Presentation WebAPI] Support constructing PresentationRequest with multiple URLs

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kershaw, Assigned: kershaw)

Tracking

Trunk
mozilla51
Points:
---

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

(Whiteboard: [ETA 9/2])

Attachments

(1 attachment, 5 obsolete attachments)

Assignee: nobody → kechang

Updated

2 years ago
Whiteboard: [ETA 8/19]
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
(Assignee)

Comment 2

2 years ago
Created attachment 8781875 [details] [diff] [review]
Construct PresentationRequest with multiple URLs

@SC,

Please take a look. Thanks.
Attachment #8781875 - Flags: feedback?(schien)
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.
Attachment #8781875 - Flags: feedback?(schien)
(Assignee)

Comment 4

2 years ago
Created attachment 8783422 [details] [diff] [review]
Construct PresentationRequest with multiple URLs - v2

Fixed the previous comments.

Please take a look. Thanks.
Attachment #8781875 - Attachment is obsolete: true
Attachment #8783422 - Flags: feedback?(schien)

Updated

2 years ago
Whiteboard: [ETA 8/19] → [ETA 9/2]
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.
Attachment #8783422 - Flags: feedback?(schien)
(Assignee)

Comment 6

2 years ago
Created attachment 8786277 [details] [diff] [review]
Construct PresentationRequest with multiple URLs - v3

Rebased on bug 1228526.
Attachment #8783422 - Attachment is obsolete: true
Attachment #8786277 - Flags: feedback?(schien)
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.
Attachment #8786277 - Flags: feedback?(schien) → feedback+
(Assignee)

Comment 8

2 years ago
Created attachment 8787108 [details] [diff] [review]
Construct PresentationRequest with multiple URLs - v4

Summary:
1. Implement constructing PresentationRequest with multiple URLs
2. Add url as a parameter in |nsIPresentationServiceCallback::NotifySuccess|.
Attachment #8786277 - Attachment is obsolete: true
Attachment #8787108 - Flags: review?(bugs)

Comment 9

2 years ago
(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
Attachment #8787108 - Flags: review?(bugs) → review-
(Assignee)

Comment 12

2 years ago
Created attachment 8787287 [details] [diff] [review]
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.
Attachment #8787108 - Attachment is obsolete: true
Attachment #8787287 - Flags: review?(bugs)
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.
Attachment #8787287 - Flags: review?(bugs) → review+
(Assignee)

Comment 14

2 years ago
(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.
(Assignee)

Comment 15

2 years ago
Created attachment 8788089 [details] [diff] [review]
Construct PresentationRequest with multiple URLs, r=smaug

Rebase and carry r+ from smaug.

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fec1b0ad4be
Attachment #8787287 - Attachment is obsolete: true
Attachment #8788089 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 16

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

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/41bad373bd63
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.