Closed Bug 1291971 Opened 7 years ago Closed 7 years ago

[Presentation WebAPI] enable Presentation-API-related web platform test on treeherder

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: schien, Assigned: schien)

References

Details

(Whiteboard: [ETA Fx52])

Attachments

(8 files)

Bug 1291971 - Part 1, enable controller idlharness test. .
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1291971 - Part 2, enable controller PresentationRequest_success test. .
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1291971 - Part 3, enable controller PresentationRequest_error test. .
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1291971 - Part 4, enable controller startNewPresentation_error test. .
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1291971 - Part 5, enable controller getAvailability test. .
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1291971 - Part 6, enable receiver idlharness test. .
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1291971 - Part 0, remove deprecated test case for presentation API and correct manifest.
58 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1291971 - Part 7, enable mixed-content and sandboxing test cases. .
58 bytes, text/x-review-board-request
smaug
: review+
Details 
      No description provided.
Assignee: nobody → schien
Status: NEW → ASSIGNED
Whiteboard: [ETA 9/2]
Whiteboard: [ETA 9/2] → [ETA 9/16]
Whiteboard: [ETA 9/16] → [ETA 9/30]
Comment on attachment 8795742 [details]
Bug 1291971 - Part 1, enable controller idlharness test. .

https://reviewboard.mozilla.org/r/81698/#review80320

::: dom/presentation/Presentation.cpp:166
(Diff revision 1)
>  }
>  
>  bool
>  Presentation::IsInPresentedContent() const
>  {
> -  nsCOMPtr<nsIDocShell> docShell = GetOwner()->GetDocShell();
> +  nsCOMPtr<nsIDocShell> docShell = mWindow->GetDocShell();

I would prefer a null check before accessing mWindow, just because mWindow can be unlinked.

::: dom/webidl/Presentation.webidl:8
(Diff revision 1)
>   * 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/.
>   */
>  
>  [Pref="dom.presentation.enabled"]
> -interface Presentation : EventTarget {
> +interface Presentation {

oh, when did this change happen in the spec.
Attachment #8795742 - Flags: review?(bugs) → review+
Comment on attachment 8795742 [details]
Bug 1291971 - Part 1, enable controller idlharness test. .

https://reviewboard.mozilla.org/r/81698/#review80320

> oh, when did this change happen in the spec.

The spec change happened when PresentationReceiver is introduced, which moves the session/connection availability event from Presentation to PresentationReceiver.
Comment on attachment 8796777 [details]
Bug 1291971 - Part 2, enable controller PresentationRequest_success test. .

https://reviewboard.mozilla.org/r/82046/#review81140
Attachment #8796777 - Flags: review?(bugs) → review+
Comment on attachment 8796778 [details]
Bug 1291971 - Part 3, enable controller PresentationRequest_error test. .

https://reviewboard.mozilla.org/r/82048/#review81142

Either spec should have nullable strings as param and handle null specially, or ... something. Is there a bug in the tests?

::: dom/presentation/PresentationRequest.cpp:72
(Diff revision 1)
>                                   const nsAString& aUrl,
>                                   ErrorResult& aRv)
>  {
> +  // According to WebIDL overload resolution algorithm, null value will be
> +  // converted to "null" string.
> +  if (aUrl.EqualsLiteral("null")) {

This looks wrong. Nothing in the spec says this should happen.
Attachment #8796778 - Flags: review?(bugs) → review-
Comment on attachment 8796779 [details]
Bug 1291971 - Part 4, enable controller startNewPresentation_error test. .

https://reviewboard.mozilla.org/r/82050/#review81144
Attachment #8796779 - Flags: review?(bugs) → review+
Comment on attachment 8796780 [details]
Bug 1291971 - Part 5, enable controller getAvailability test. .

https://reviewboard.mozilla.org/r/82052/#review81146
Attachment #8796780 - Flags: review?(bugs) → review+
Comment on attachment 8796781 [details]
Bug 1291971 - Part 6, enable receiver idlharness test. .

https://reviewboard.mozilla.org/r/82110/#review81148

::: dom/base/nsContentUtils.cpp:9380
(Diff revision 1)
>  /* static */ void
>  nsContentUtils::GetPresentationURL(nsIDocShell* aDocShell, nsAString& aPresentationUrl)
>  {
>    MOZ_ASSERT(aDocShell);
>  
> +  // Simulate receiver context for web platform test

hmm, tiny bit odd that each docshell just returns its current url as the presentation url when doing testing.
But if this is what the tests need, fine.
Attachment #8796781 - Flags: review?(bugs) → review+
Comment on attachment 8796778 [details]
Bug 1291971 - Part 3, enable controller PresentationRequest_error test. .

https://reviewboard.mozilla.org/r/82048/#review81142

> This looks wrong. Nothing in the spec says this should happen.

The null string conversion comes with the Constructor overloading. The null value will be blocked by the generated WebIDL binding code if there is only one constructor that accepte single String as parameter.
Consult with @heycam and he think this is an expected behavior.
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #18)
> Comment on attachment 8796778 [details]
> Bug 1291971 - Part 3, enable controller PresentationRequest_error test. .
> 
> https://reviewboard.mozilla.org/r/82048/#review81142
> 
> > This looks wrong. Nothing in the spec says this should happen.
> 
> The null string conversion comes with the Constructor overloading. The null
> value will be blocked by the generated WebIDL binding code if there is only
> one constructor that accepte single String as parameter.
> Consult with @heycam and he think this is an expected behavior.

Hi @heycam, we've discussed about the null string conversion while overloading WebIDL constructor previously. Can you explain more about the background of the rule that converting null to "null" string for non-nullable parameter?
Flags: needinfo?(cam)
Whenever we decide to convert any JS value into an IDL DOMString value, we first convert it to a JS String value using the same internal method that JS uses when you write |String(value)| or |value + ""|:

  http://www.ecma-international.org/ecma-262/7.0/index.html#sec-tostring

That converts null to "null".  We don't treat that as any sort of error case.  Just like if you passed in, say, a DOM Node into something expecting a DOMString, it would probably convert to a string like "[object HTMLSpanElement]".

So, since we don't treat conversion of null to DOMString as an error, we should accept it as the real URL value passed in to the PresentationRequest constructor, and it'll get resolved against the appropriate base URL.  This is similar to if we assign null to an HTMLAnchorElement's href property:

  -- in http://example.com/ --
  var a = document.createElement("a");
  a.href = null;
  alert(a.href);  // alerts "http://example.com/null"

(If there were some real ergonomic need to make null be treated as a special value that throws an exception, then the IDL would need to change to make one of the arguments nullable (probably the DOMString one) and then the spec would say if the url argument is null, throw whatever exception.)

I don't think there will be any difference if the constructor did not have the sequence<DOMString> overload, and only had the one that takes a DOMString.  In this case, the overload resolution algorithm doesn't run, but we still convert any value passed in to a String using the internal ToString method.
Flags: needinfo?(cam)
If some special handling is needed for null passed to Constructor(DOMString url), then the
spec needs to be change to have Constructor(DOMString? url), and it needs to explain what should happen in that case.
Whiteboard: [ETA 9/30] → [ETA Fx52]
Comment on attachment 8802018 [details]
Bug 1291971 - Part 0, remove deprecated test case for presentation API and correct manifest.

https://reviewboard.mozilla.org/r/86590/#review85458

Hmm, so I have a wpt update in progress that I think will also pull in these tests. It's currently held up by some irritating chunking + websockets on Windows issues. Would you be OK with waiting on landing this change until I at least try to fix that? If we can avoid a merge conflict that would be great.
Attachment #8802018 - Flags: review?(james)
(In reply to James Graham [:jgraham] from comment #32)
> Comment on attachment 8802018 [details]
> Bug 1291971 - Part 0, sync latest test cases from W3C web-platform-test for
> presentation API. .
> 
> https://reviewboard.mozilla.org/r/86590/#review85458
> 
> Hmm, so I have a wpt update in progress that I think will also pull in these
> tests. It's currently held up by some irritating chunking + websockets on
> Windows issues. Would you be OK with waiting on landing this change until I
> at least try to fix that? If we can avoid a merge conflict that would be
> great.

Sure I can rebase after your patch landed on m-c. Will it available at these two days?
Flags: needinfo?(james)
Comment on attachment 8796778 [details]
Bug 1291971 - Part 3, enable controller PresentationRequest_error test. .

https://reviewboard.mozilla.org/r/82048/#review85532

I guess you could fix the broken test also here, and it would be merged to github wpt.

::: dom/presentation/PresentationRequest.cpp:70
(Diff revision 2)
>  /* static */ already_AddRefed<PresentationRequest>
>  PresentationRequest::Constructor(const GlobalObject& aGlobal,
>                                   const nsAString& aUrl,
>                                   ErrorResult& aRv)
>  {
> +  if (aUrl.IsVoid()) {

So the spec doesn't have anything like this

::: dom/webidl/PresentationRequest.webidl:10
(Diff revision 2)
>   *
>   * The origin of this IDL file is
>   * https://w3c.github.io/presentation-api/#interface-presentationrequest
>   */
>  
> -[Constructor(DOMString url),
> +[Constructor(DOMString? url),

So this isn't in the spec. If we want this, better to get this change to the spec.

I still don't understand why though.

::: testing/web-platform/meta/presentation-api/controlling-ua/PresentationRequest_error.html.ini
(Diff revision 2)
> -    expected: FAIL
> -
> -  [Call PresentationRequest() constructor with null presentation URL. TypeError Exception expected.]
> +          dom.presentation.controller.enabled: true,
> +          dom.presentation.discovery.enabled: true,
> +          dom.presentation.device.name: "Firefox"]
> -    expected: FAIL
> -
> -  [Call PresentationRequest constructor with null presentation URL. TypeError Exception expected.]

The test here is buggy

It has

var wrong_presentation_url = null;

    test(function() {
        assert_throws(new TypeError(), function() {
            new PresentationRequest(wrong_presentation_url);
        });

Per spec it should lead to the same behavior as passing "null" as string to the constructor.

I filed https://github.com/w3c/web-platform-tests/issues/4003
Attachment #8796778 - Flags: review?(bugs) → review-
Comment on attachment 8802019 [details]
Bug 1291971 - Part 7, enable mixed-content and sandboxing test cases. .

https://reviewboard.mozilla.org/r/86592/#review85534
Attachment #8802019 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #34)
> Comment on attachment 8796778 [details]
> Bug 1291971 - Part 3, enable controller PresentationRequest_error test. .
> 
> https://reviewboard.mozilla.org/r/82048/#review85532
> 
> I guess you could fix the broken test also here, and it would be merged to
> github wpt.
> 
> ::: dom/presentation/PresentationRequest.cpp:70
> (Diff revision 2)
> >  /* static */ already_AddRefed<PresentationRequest>
> >  PresentationRequest::Constructor(const GlobalObject& aGlobal,
> >                                   const nsAString& aUrl,
> >                                   ErrorResult& aRv)
> >  {
> > +  if (aUrl.IsVoid()) {
> 
> So the spec doesn't have anything like this
> 
> ::: dom/webidl/PresentationRequest.webidl:10
> (Diff revision 2)
> >   *
> >   * The origin of this IDL file is
> >   * https://w3c.github.io/presentation-api/#interface-presentationrequest
> >   */
> >  
> > -[Constructor(DOMString url),
> > +[Constructor(DOMString? url),
> 
> So this isn't in the spec. If we want this, better to get this change to the
> spec.
> 
> I still don't understand why though.
> 
> :::
> testing/web-platform/meta/presentation-api/controlling-ua/
> PresentationRequest_error.html.ini
> (Diff revision 2)
> > -    expected: FAIL
> > -
> > -  [Call PresentationRequest() constructor with null presentation URL. TypeError Exception expected.]
> > +          dom.presentation.controller.enabled: true,
> > +          dom.presentation.discovery.enabled: true,
> > +          dom.presentation.device.name: "Firefox"]
> > -    expected: FAIL
> > -
> > -  [Call PresentationRequest constructor with null presentation URL. TypeError Exception expected.]
> 
> The test here is buggy
> 
> It has
> 
> var wrong_presentation_url = null;
> 
>     test(function() {
>         assert_throws(new TypeError(), function() {
>             new PresentationRequest(wrong_presentation_url);
>         });
> 
> Per spec it should lead to the same behavior as passing "null" as string to
> the constructor.
> 
> I filed https://github.com/w3c/web-platform-tests/issues/4003

I kinda get confused. Do you mean automatically converting from null value to "null" string is an intended behavior for all WebAPIs? If so, correcting test case should be the right thing to do.
Attachment #8802018 - Flags: review?(james)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #36)

> > Per spec it should lead to the same behavior as passing "null" as string to
> > the constructor.
> > 
> > I filed https://github.com/w3c/web-platform-tests/issues/4003
> 
> I kinda get confused. Do you mean automatically converting from null value
> to "null" string is an intended behavior for all WebAPIs? If so, correcting
> test case should be the right thing to do.

If a method takes DOMString as a param, null is converted to "null" in the webidl layer. That is what the WebIDL
spec say. (Same with passing undefined. )
If you want nullable, then the parameter should be DOMString? .
(and to deal with undefined, one can would make the parameter optional)


As an example, a DOM API taking DOMString (and not DOMString? or optional DOMString) is createTextNode, and which is why all these evaluate to true:
document.createTextNode(null).data == "null"
document.createTextNode(null).data.length == 4
document.createTextNode(undefined).data == "undefined"
document.createTextNode(undefined).data.length == 9



https://heycam.github.io/webidl/#idl-DOMString
https://heycam.github.io/webidl/#dfn-nullable-type
Comment on attachment 8796778 [details]
Bug 1291971 - Part 3, enable controller PresentationRequest_error test. .

https://reviewboard.mozilla.org/r/82048/#review86210

::: testing/web-platform/tests/presentation-api/controlling-ua/PresentationRequest_error.html
(Diff revision 3)
>              new PresentationRequest();
>          });
>      }, 'Call PresentationRequest() constructor without presentation URL. TypeError Exception expected.');
>  
>      test(function() {
> -        assert_throws(new TypeError(), function() {

I guess this is one option, another option is to test that passing null does not throw.
Attachment #8796778 - Flags: review?(bugs) → review+
Comment on attachment 8796778 [details]
Bug 1291971 - Part 3, enable controller PresentationRequest_error test. .

https://reviewboard.mozilla.org/r/82048/#review86524

::: testing/web-platform/tests/presentation-api/controlling-ua/PresentationRequest_error.html
(Diff revision 3)
>              new PresentationRequest();
>          });
>      }, 'Call PresentationRequest() constructor without presentation URL. TypeError Exception expected.');
>  
>      test(function() {
> -        assert_throws(new TypeError(), function() {

This test case is mainly for testing error handling on PresentationRequest operation, so I'll suggest to simply remove the non-error scenario.
Comment on attachment 8796778 [details]
Bug 1291971 - Part 3, enable controller PresentationRequest_error test. .

https://reviewboard.mozilla.org/r/82048/#review86526
OK, my patch is still in flight. Please feel free to go ahead and I will deal with the fallout if you land first.
Flags: needinfo?(james)
(In reply to James Graham [:jgraham] from comment #49)
> OK, my patch is still in flight. Please feel free to go ahead and I will
> deal with the fallout if you land first.

Thanks @jgraham! My patch will be landed soon after you double check the part 0 patch. Just want to make sure I didn't mess up with files that should not be manually changed.
Comment on attachment 8802018 [details]
Bug 1291971 - Part 0, remove deprecated test case for presentation API and correct manifest.

https://reviewboard.mozilla.org/r/86590/#review87606

::: testing/web-platform/meta/MANIFEST.json:37407
(Diff revision 3)
>        "html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping.html"
>      ],
>      "deleted_reftests": {},
>      "items": {
>        "manual": {
> +        "html/browsers/windows/noreferrer-cross-origin-close-manual.sub.html": [

This change seems unrelated?
Attachment #8802018 - Flags: review?(james) → review+
Comment on attachment 8802018 [details]
Bug 1291971 - Part 0, remove deprecated test case for presentation API and correct manifest.

https://reviewboard.mozilla.org/r/86590/#review87606

> This change seems unrelated?

This is the manifest change after updating sourcefile.py. I got this change after executing |mach wpt-manifest-update|. Should I remove the change?
See comment #60
Flags: needinfo?(james)
Pushed by schien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/77bbf4625e50
Part 0, remove deprecated test case for presentation API and correct manifest. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/bd859aaaca12
Part 1, enable controller idlharness test. r=smaug.
https://hg.mozilla.org/integration/autoland/rev/1b8568fc7962
Part 2, enable controller PresentationRequest_success test. r=smaug.
https://hg.mozilla.org/integration/autoland/rev/8d072d3047b8
Part 3, enable controller PresentationRequest_error test. r=smaug.
https://hg.mozilla.org/integration/autoland/rev/54e937ecefe8
Part 4, enable controller startNewPresentation_error test. r=smaug.
https://hg.mozilla.org/integration/autoland/rev/5e9072e17105
Part 5, enable controller getAvailability test. r=smaug.
https://hg.mozilla.org/integration/autoland/rev/451ff0786416
Part 6, enable receiver idlharness test. r=smaug.
https://hg.mozilla.org/integration/autoland/rev/36880239ec41
Part 7, enable mixed-content and sandboxing test cases. r=smaug.
Flags: needinfo?(james)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.