1311375 - [Presentation API] Fail to get expect result status when running "startNewPresentation_unsettledpromise-manual"

Status: RESOLVED FIXED

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

Description

[Cynthia Tang [:cynthiatang]]

Test name:
Calling "start" when there is already an unsetttled promise returns a Promise rejected with an OperationError exception.

Reproduce test steps:
 1. Launch Firefox with Presentation API
 2. Go the https://goo.gl/M6ntC7
 3. Click the button to start the manual test

Expected result:
 - Return result status is "PASS".

Actual result:
 - Return result status is "FAIL"

Test Builds
===========
Version	Test Date	Test Device
crosswalk beta 13.42.319.2	2015.04.14	Google Nexus 3

Comment 1

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

In https://w3c.github.io/presentation-api/#starting-a-presentation step 5:

```
If there is already an unsettled Promise from a previous call to start for the same controlling browsing context, return a Promise rejected with an OperationError exception and abort all remaining steps. 
```

This is a missing error handling in m-c.

Comment 2

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

Attachment: Bug 1311375 - promise reject when there is an unsettled presentation start procedure.

Review commit: https://reviewboard.mozilla.org/r/89558/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89558/

Comment 3

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8805971 [details]
Bug 1311375 - promise reject when there is an unsettled presentation start procedure.

https://reviewboard.mozilla.org/r/89558/#review88932

::: dom/presentation/PresentationRequest.cpp:185
(Diff revision 1)
>      return promise.forget();
>    }
>  
> +  RefPtr<Navigator> navigator =
> +    nsGlobalWindow::Cast(GetOwner())->GetNavigator(aRv);
> +  RefPtr<Presentation> presentation = navigator->GetPresentation(aRv);

please null-check navigator before using it.

::: dom/presentation/PresentationRequest.cpp:186
(Diff revision 1)
>    }
>  
> +  RefPtr<Navigator> navigator =
> +    nsGlobalWindow::Cast(GetOwner())->GetNavigator(aRv);
> +  RefPtr<Presentation> presentation = navigator->GetPresentation(aRv);
> +  if (!presentation->IsStartSessionUnsettled()) {

I must be missing something. Isn't this check backwards.
"If there is already an unsettled Promise" ..."OperationError exception".

But you throw OperationError when there isn't unsettled

::: dom/presentation/tests/mochitest/test_presentation_1ua_sender_and_receiver.js:132
(Diff revision 1)
>        ok(false, "Sender: Error occurred when establishing a connection: " + aError);
>        teardown();
>        aReject();
>      });
> +
> +    let request2 = new PresentationRequest("/");

I don't understand this test. Why should start() fail? there is just a single start() for for this PresentationRequest.

Comment 4

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8805971 [details]
Bug 1311375 - promise reject when there is an unsettled presentation start procedure.

https://reviewboard.mozilla.org/r/89558/#review88944

Comment 5

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

Comment on attachment 8805971 [details]
Bug 1311375 - promise reject when there is an unsettled presentation start procedure.

https://reviewboard.mozilla.org/r/89558/#review88932

> I must be missing something. Isn't this check backwards.
> "If there is already an unsettled Promise" ..."OperationError exception".
> 
> But you throw OperationError when there isn't unsettled

It turns out to be a bad idea writing code in the midnight of a holiday.

> I don't understand this test. Why should start() fail? there is just a single start() for for this PresentationRequest.

According to the spec, OperationError should be returned while there is an unsettled promise in *the same browing context*. Therefore, this test case creates another PresentationRequest to ensure we are not only fail on the same PresentationRequest.

Comment 6

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

Comment on attachment 8805971 [details]
Bug 1311375 - promise reject when there is an unsettled presentation start procedure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89558/diff/1-2/

Comment 7

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #5)
> According to the spec, OperationError should be returned while there is an
> unsettled promise in *the same browing context*. Therefore, this test case
> creates another PresentationRequest to ensure we are not only fail on the
> same PresentationRequest.
"If there is already an unsettled Promise from a previous call to start for the same controlling browsing context,"

Does that mean start on the same PresentationRequest? I think the spec is unclear.
https://github.com/w3c/presentation-api/issues/364

Comment 8

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8805971 [details]
Bug 1311375 - promise reject when there is an unsettled presentation start procedure.

https://reviewboard.mozilla.org/r/89558/#review89262

::: dom/presentation/PresentationRequest.cpp:186
(Diff revision 2)
>      return promise.forget();
>    }
>  
> +  RefPtr<Navigator> navigator =
> +    nsGlobalWindow::Cast(GetOwner())->GetNavigator(aRv);
> +  RefPtr<Presentation> presentation = navigator->GetPresentation(aRv);

Please null check navigator before using it.

::: dom/presentation/PresentationRequest.cpp:455
(Diff revision 2)
>      new AsyncEventDispatcher(this, event);
>    return asyncDispatcher->PostDOMEvent();
>  }
>  
> +void
> +PresentationRequest::NotifyPromiseSettled() {

{ goes to its own line.

::: dom/presentation/tests/mochitest/test_presentation_1ua_sender_and_receiver.js:132
(Diff revision 2)
>        ok(false, "Sender: Error occurred when establishing a connection: " + aError);
>        teardown();
>        aReject();
>      });
> +
> +    let request2 = new PresentationRequest("/");

ok, so I assume your interpretation of the spec is right and that the spec gets fixed.

Comment 9

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

Comment on attachment 8805971 [details]
Bug 1311375 - promise reject when there is an unsettled presentation start procedure.

https://reviewboard.mozilla.org/r/89558/#review89262

> Please null check navigator before using it.

I'll check aRv and throw if `GetNavigator` or `GetPresentation` have error.

Comment 10

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

Comment on attachment 8805971 [details]
Bug 1311375 - promise reject when there is an unsettled presentation start procedure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89558/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cecd173e205
promise reject when there is an unsettled presentation start procedure. r=smaug

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7cecd173e205