[presentation-api] add several tests for a receiving browsing context #5562

tomoyukilabs · 2017-04-14T04:46:28Z

This PR adds three tests for the following events in a receiving browsing context:

a connectionavailable event fired at PresentationConnectionList
a close event fired at PresentationConnection
terminating a presentation in a receiving browsing context

Now I'm wondering whether a test for connect event at a receiving side is needed or not, since state of a presentation connection at the receiving side is already set to connected when it is added to the set of presentation controllers.

This change is

- Monitoring incoming presentation connections - Closing a PresentationConnection - Terminating a presentation in a receiving browsing context

wpt-pr-bot · 2017-04-14T04:46:36Z

Notifying @louaybassbouss, @tidoust, and @zqzhang. (Learn how reviewing works.)

ghost · 2017-04-14T04:56:21Z

View the complete job log.

Lint

Passed

ghost · 2017-04-14T04:56:22Z

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision c236c59
Using browser at version BuildID 20170418100213; SourceStamp bb38d935d699e0529f9e0bb35578d381026415c4
Starting 10 test iterations
No tests run.

ghost · 2017-04-14T04:56:22Z

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision c236c59
Using browser at version 59.0.3071.9 dev
Starting 10 test iterations
No tests run.

tidoust

Hi @tomoyukilabs,

First, thanks a ton for these tests. As usual, reviewing these has proved immensely useful for me as I had to dig into algorithms and wonder about what the spec says (or should say).

I see one issue with termination, and issues due to the use of SHOULD in the spec.

tidoust · 2017-04-14T12:46:27Z

...-api/receiving-ua/support/PresentationConnectionList_onconnectionavailable_receiving-ua.html

+        return checkConnectionList(evt.connection, 'connect');
+      };
+
+      let eventNotFired = false;


Not sure what that variable was meant for, but test only sets it, so it can be dropped.

Sorry, but I forgot to remove it. Thanks! I'll drop it in the next update.

tidoust · 2017-04-14T14:10:44Z

presentation-api/receiving-ua/support/PresentationConnection_terminate_receiving-ua.html

+      connection.terminate();
+
+      // the following code must not be invoked since the presentation is terminated
+      stash.send({ type: 'error' });


This seems to assume that calling "terminate" will immediately kill the browsing context. I don't think that is true. Scripts may still run at the end of the unload a document algorithm in particular. I do receive the error message when I test this on Chrome and my Chromecast device.

Typically, in a window opened by a script, you may still post a message to the opener after calling window.close():

window.close(); window.opener.postMessage('Still there!', '*');

Now, after the call to terminate, window.closed should be true (actually, "unload" is not enough, the context should also be "discarded", see w3c/presentation-api#423), so you could write:

if (!window.closed) { stash.send({ type: 'error' }); }

You could also send an "ok" message if the window is indeed closed, but this time I'm not entirely sure there is a guarantee that the message will reach the stash server before the context gets interrupted for real. That seems to work in practice, but maybe you could write something such as:

stash.send({ type: (window.closed ? 'ok' : 'error') }); }

... and a conditional check on the controller that stops the test immediately on "ok", fails on "error", and admits things are good after 2s.

Right. I'll change the code into the following:

stash.send({ type: (window.closed ? 'ok' : 'error') });

FYI: As far as I have tested on Firefox Nightly for Android, window.closed is not set to true yet as soon as calling "terminate", and it becomes true when an unload event is fired. Anyway, I will fix the codes as you mentioned above.

tidoust · 2017-04-14T14:33:25Z

presentation-api/receiving-ua/support/PresentationConnection_onclose_receiving-ua.html

+          new Promise(resolve => { t.step_timeout(() => { resolve(null); }, 3000); })
+        ]);
+      }).then(evt => {
+        assert_true(!!evt, 'A presentation connection is closed when its controller\'s browsing context is discarded.');


I note that the Presentation API also has a SHOULD here, which seems to make more sense (it could perhaps take some time for garbage collection to operate, or a user agent might want to hastily stop spending computing resources on a closed context):
[[
When a PresentationConnection object S is discarded (because the document owning it is navigating or is closed) while the presentation connection state of S is connecting or connected, the user agent SHOULD start closing the presentation connection S with wentaway as closeReason and an empty closeMessage.
]]

So failing that particular check is possible in a conforming implementation. And this seems to fail in Chrome, actually (well it fails because of "isTrusted", but if you skip this check, it fails here). From a testing perspective, I guess the best would be to make that check in a separate test flagged as "[Optional]".

Okay, I'll separate it as an optional test.

Now that this SHOULD has been changed to MUST in the current spec, I'll cancel the change to "[Optional]".

tidoust · 2017-04-14T14:38:25Z

...-api/receiving-ua/support/PresentationConnectionList_onconnectionavailable_receiving-ua.html

+
+      const checkEvent = evt => {
+        assert_true(evt instanceof PresentationConnectionAvailableEvent, 'An event using PresentationConnectionAvailableEvent is fired.');
+        assert_true(evt.isTrusted, 'The event is a trusted event.');


Just pinging @mfoltzgoogle in case that is not a known bug: Chrome does not seem to set the "isTrusted" property of the "connectionavailable" event in the receiving browsing context (it is reported as undefined). The test would pass otherwise (same buggy behavior for the "close" event)

tidoust · 2017-04-14T14:49:40Z

presentation-api/receiving-ua/PresentationConnection_terminate-manual.https.html

+        return stash.init();
+      }).then(() => {
+        child.contentWindow.postMessage({ type: 'connect', id: connection.id, url: connection.url }, '*');
+        const eventWatcher1 = new EventWatcher(t, connection, 'terminate');


Note that a conforming implementation may not issue a "terminate" event when the receiving context is the one that calls "terminate", because it is a SHOULD in the Presentation API:
[[
When a receiving user agent is to send a termination confirmation for a presentation P, and that confirmation was received by a controlling user agent, the controlling user agent SHOULD run the following steps
]]

Ideally, that check would be done in a separate test marked as "[Optional]". Or we turn that SHOULD into a MUST in the spec, as I don't understand why it's only a SHOULD for now. I created an issue:
w3c/presentation-api#422

Since this SHOULD has been changed to MUST, I leave this code as it is.

tidoust · 2017-04-14T15:26:06Z

presentation-api/receiving-ua/support/iframe.html

+    else if (data.type === 'connect') {
+      const request = new PresentationRequest(data.url);
+      request.reconnect(data.id).then(c => {
+        c.onterminate = () => {


Same SHOULD problem as above. There is no real guarantee that a "terminate" signal from the receiving side will not be ignored by the controlling user agent.

Same change from SHOULD to MUST as above.

tidoust · 2017-04-14T15:27:45Z

presentation-api/receiving-ua/support/PresentationConnection_onclose_receiving-ua.html

+      }).then(evt => {
+        connection = evt.connection;
+        watchClose = new EventWatcher(t, connection, 'close');
+        return watchClose.wait_for('close');


Same SHOULD problem as mentioned elsewhere, see related open issue w3c/presentation-api#423

Same change from SHOULD to MUST as mentioned below.

markafoltz · 2017-04-14T16:38:40Z

Regarding the isTrusted property on connectionavailable and close, I filed https://bugs.chromium.org/p/chromium/issues/detail?id=711687 to investigate.

ghost · 2017-04-17T05:49:59Z

These tests are now available on w3c-test.org

tomoyukilabs · 2017-04-17T05:53:56Z

I have fixed several bugs except ones related to w3c/presentation-api#422 and w3c/presentation-api#423.

codecov-io · 2017-04-20T05:37:39Z

Codecov Report

❗ No coverage uploaded for pull request base (master@9d1ec68). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #5562   +/-   ##
=========================================
  Coverage          ?   87.22%           
=========================================
  Files             ?       24           
  Lines             ?     2418           
  Branches          ?      406           
=========================================
  Hits              ?     2109           
  Misses            ?      239           
  Partials          ?       70

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d1ec68...7bcf19e. Read the comment docs.

implement three test cases

c6aa60f

- Monitoring incoming presentation connections - Closing a PresentationConnection - Terminating a presentation in a receiving browsing context

wpt-pr-bot added presentation-api wg-secondscreen labels Apr 14, 2017

tidoust mentioned this pull request Apr 14, 2017

Turn SHOULD into MUST for close/terminate messages handling? w3c/presentation-api#422

Closed

tidoust requested changes Apr 14, 2017

View reviewed changes

fix bugs and conform to the spec better

8823dbd

tomoyukilabs added 2 commits April 18, 2017 09:35

remove trailing whitespace at EOL

23dd96d

cancel [Optional] and fix a minor bug

7bcf19e

tidoust approved these changes Apr 20, 2017

View reviewed changes

tidoust merged commit 9fd6466 into web-platform-tests:master Apr 20, 2017

tomoyukilabs deleted the receiver-connection branch April 24, 2017 07:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[presentation-api] add several tests for a receiving browsing context #5562

[presentation-api] add several tests for a receiving browsing context #5562

tomoyukilabs commented Apr 14, 2017 •

edited

Loading

wpt-pr-bot commented Apr 14, 2017

ghost commented Apr 14, 2017 •

edited by ghost

Loading

ghost commented Apr 14, 2017 •

edited by ghost

Loading

ghost commented Apr 14, 2017 •

edited by ghost

Loading

tidoust left a comment

tidoust Apr 14, 2017

tomoyukilabs Apr 17, 2017

tidoust Apr 14, 2017

tomoyukilabs Apr 17, 2017

tidoust Apr 14, 2017

tomoyukilabs Apr 17, 2017

tomoyukilabs Apr 20, 2017

tidoust Apr 14, 2017

tidoust Apr 14, 2017

tomoyukilabs Apr 20, 2017

tidoust Apr 14, 2017

tomoyukilabs Apr 20, 2017

tidoust Apr 14, 2017

tomoyukilabs Apr 20, 2017

markafoltz commented Apr 14, 2017

ghost commented Apr 17, 2017 •

edited by ghost

Loading

tomoyukilabs commented Apr 17, 2017

codecov-io commented Apr 20, 2017 •

edited

Loading

[presentation-api] add several tests for a receiving browsing context #5562

[presentation-api] add several tests for a receiving browsing context #5562

Conversation

tomoyukilabs commented Apr 14, 2017 • edited Loading

wpt-pr-bot commented Apr 14, 2017

ghost commented Apr 14, 2017 • edited by ghost Loading

Lint

ghost commented Apr 14, 2017 • edited by ghost Loading

Firefox (nightly channel)

ghost commented Apr 14, 2017 • edited by ghost Loading

Chrome (unstable channel)

tidoust left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markafoltz commented Apr 14, 2017

ghost commented Apr 17, 2017 • edited by ghost Loading

tomoyukilabs commented Apr 17, 2017

codecov-io commented Apr 20, 2017 • edited Loading

Codecov Report

tomoyukilabs commented Apr 14, 2017 •

edited

Loading

ghost commented Apr 14, 2017 •

edited by ghost

Loading

ghost commented Apr 14, 2017 •

edited by ghost

Loading

ghost commented Apr 14, 2017 •

edited by ghost

Loading

ghost commented Apr 17, 2017 •

edited by ghost

Loading

codecov-io commented Apr 20, 2017 •

edited

Loading