1122032 - Setup minimal screensharing for Loop desktop in Rooms

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Comment 1

[Mark Banner (:standard8)]

Matej/Sevaan: I need a couple of tooltips for the screen sharing button. I'm thinking:

- Share your screen or windows
- Stop sharing

I'm not quite sure about the first one.

btw, here's the UX: https://projects.invisionapp.com/share/GU1Z0XTXR#/screens/56154705

Comment 2

[Matej Novak [:matej]]

I feel like "Share your screen" may be sufficient for the first one.

Comment 3

[Mark Banner (:standard8)]

Attachment: Part 1 - Setup minimal screen sharing for Loop from desktop (disabled by default).

These depend on bug 1093780 & its dependencies. This sets up a screen sharing button on desktop.

Comment 4

[Mark Banner (:standard8)]

Attachment: Part 2 - Show the Loop screenshare video in place of the remote video for now.

Enables standalone to receive and display the video reasonably nicely.

Comment 5

[Mark Banner (:standard8)]

Attachment: Part 3 - Automatically allow screensharing for the Loop in-desktop pages.

Comment 6

[Mark Banner (:standard8)]

Attachment: Part 4 - Support the screensharing doorhanger in Social API chat windows and allow for multiple independent notification icons per type.

Comment 7

[Mark Banner (:standard8)]

Attachment: Part 5 - Allow multiple anchors for multiple notifications without using an iconbox.

Comment 8

[Mark Banner (:standard8)]

Attachment: Part 3 - Automatically allow screensharing for the Loop in-desktop pages.

This is what we need for Loop's screen sharing functionality.

Moving the Loop privilege detection allows the about:loopconversation to bypass the screen-sharing url whitelist (since we're browser anyway, and that would depend on https). We then turn the privilege off for window/application sharing as we currently want the prompt to appear. This may change in future, but for now this is the simplest option we're taking to get something landed.

I've also extended the mochitests to check that the permissions are granted or prompted at the right time. I'm not testing the full functionality for the permissions prompt for screen share as that should be tested elsewhere.

Comment 9

[Mark Banner (:standard8)]

Try push for part 3:

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

Comment 10

[Mark Banner (:standard8)]

Attachment: Part 3 - Automatically allow screensharing for the Loop in-desktop pages.

Updated patch for try server results. I've disabled the added tests on WinXP, Mac 10.6 and Linux.

WinXP and Mac 10.6 are unsupported except by pref, and I don't think it makes sense to turn that on just for these tests (and risk something breaking).

Linux tests are failing for some unknown reason. As we've not got any other screenshare prompt tests around, I'm going to suggest that we file a bug for screenshare prompt tests and have it attempt to fix the Linux issues at the same time.

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8555281 [details] [diff] [review]
Part 5 - Allow multiple anchors for multiple notifications without using an iconbox.

Review of attachment 8555281 [details] [diff] [review]:
-----------------------------------------------------------------

Florian, I'd really appreciate your thoughts on part 4 and part 5; they're extending the PopupNotifications.jsm implementation to support multiple anchors that can target a specific notification (type).

Part 4 adds the separate anchor for screensharing notifications.
Part 5 adds logic to connect a specific anchor to notifications, without the specific need for an iconbox, like the urlbar uses. This is most definitely compatible with the urlbar notifications, so not regression sensitive. The non-specific anchor will still be used as a catch-all for all other notifications for the chat window.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8556495 [details] [diff] [review]
Part 3 - Automatically allow screensharing for the Loop in-desktop pages.

Review of attachment 8556495 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if you make it prompt for all types of sharing, or explain why not and ask again.

::: dom/media/MediaManager.cpp
@@ +1740,5 @@
> +
> +    // For window and application sharing, Loop needs to prompt.
> +    if (isLoop &&
> +        (src == dom::MediaSourceEnum::Window ||
> +         src == dom::MediaSourceEnum::Application)) {

screen sharing?
tab sharing?
They all should prompt for now, right?

Comment 13

[Mark Banner (:standard8)]

Attachment: Part 3 - Automatically allow screensharing for the Loop in-desktop pages.

Yes, we do want to disable the prompts for tab sharing (screen is TBD at the moment, so I've added it in) - we'll be implementing active tab sharing, with our own UX to control it.

For window sharing we're currently using the permissions dialog.

We're probably going to be moving to a UX which would mean we circumvent all the permissions dialog by providing our own UX (since we're build into FF we're using different rules).

At the moment, screen and application do prompt, we're not using those for the first versions, so it doesn't really matter atm.

I've also commented in the patch to try and explain this a bit.

Comment 14

[Mark Banner (:standard8)]

Attachment: Part 3 - Automatically allow screensharing for the Loop in-desktop pages.

This one removes the test that now doesn't work due to the patch no longer allowing screen wthout prompt.

Comment 15

[Mark Banner (:standard8)]

Attachment: Part 1 - Setup minimal screen sharing for Loop from desktop (disabled by default).

Updated patch on top of current sdk patches.

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8557100 [details] [diff] [review]
Part 1 - Setup minimal screen sharing for Loop from desktop (disabled by default).

Review of attachment 8557100 [details] [diff] [review]:
-----------------------------------------------------------------

Pfew, big patch!

I could only really find a few nits, the whole thing looks nicely done. Plus, I've seen it working, which helps.

::: browser/components/loop/content/conversation.html
@@ +39,5 @@
>      <script type="text/javascript" src="loop/shared/js/roomStates.js"></script>
>      <script type="text/javascript" src="loop/shared/js/fxOSActiveRoomStore.js"></script>
>      <script type="text/javascript" src="loop/shared/js/activeRoomStore.js"></script>
>      <script type="text/javascript" src="loop/shared/js/feedbackStore.js"></script>
> +    <script type="text/javascript" src="loop/shared/js/views.js"></script>

why here, right between the two feedback feature related files?

::: browser/components/loop/content/js/roomViews.jsx
@@ +171,5 @@
>  
>      propTypes: {
> +      dispatcher: React.PropTypes.instanceOf(loop.Dispatcher).isRequired,
> +      mozLoop:
> +        React.PropTypes.object.isRequired,

Why the newline?

@@ +246,5 @@
> +        state: this.state.screenSharingState,
> +        visible: false
> +      };
> +
> +      if (this.props.mozLoop.getLoopPref("screenshare.enabled")) {

since this returns a bool anyway, can you simply do: 

```js
var screenShareData = {
  visible: this.props.mozLoop.getLoopPref("screenshare.enabled")
}
```

?

::: browser/components/loop/content/shared/js/actions.js
@@ +198,5 @@
>        enabled: Boolean
>      }),
>  
>      /**
> +     * Used to start a screen share

nit: missing dot.

@@ +204,5 @@
> +    StartScreenShare: Action.define("startScreenShare", {
> +    }),
> +
> +    /**
> +     * Used to end a screen share

nit: missing dot.

@@ +213,5 @@
> +    /**
> +     * Used to notifiy that screen sharing is active or not.
> +     */
> +    ScreenSharingState: Action.define("screenSharingState", {
> +      // One of loop.shared.utils.SCREEN_SHARE_STATES

nit: missing dot.

::: browser/components/loop/content/shared/js/otSdkDriver.js
@@ +94,5 @@
> +        state: SCREEN_SHARE_STATES.PENDING
> +      }));
> +
> +      var config = this._getCopyPublisherConfig();
> +      config.videoSource = "window";

Please add a comment that this is temporary, pending a dropdown share type selector

@@ +421,5 @@
> +    _onScreenShareDenied: function() {
> +      this.dispatcher.dispatch(new sharedActions.ScreenSharingState({
> +        state: SCREEN_SHARE_STATES.INACTIVE
> +      }));
> +    },

nit: trailing comma

::: browser/components/loop/standalone/content/index.html
@@ +111,5 @@
>      <script type="text/javascript" src="shared/js/roomStates.js"></script>
>      <script type="text/javascript" src="shared/js/fxOSActiveRoomStore.js"></script>
>      <script type="text/javascript" src="shared/js/activeRoomStore.js"></script>
>      <script type="text/javascript" src="shared/js/feedbackStore.js"></script>
> +    <script type="text/javascript" src="shared/js/views.js"></script>

Same question here ;)

::: browser/components/loop/test/desktop-local/index.html
@@ +54,5 @@
>    <script src="../../content/shared/js/roomStates.js"></script>
>    <script src="../../content/shared/js/fxOSActiveRoomStore.js"></script>
>    <script src="../../content/shared/js/activeRoomStore.js"></script>
>    <script src="../../content/shared/js/feedbackStore.js"></script>
> +  <script src="../../content/shared/js/views.js"></script>

Same question here ;)

::: browser/components/loop/test/desktop-local/roomViews_test.js
@@ +283,5 @@
> +      view.setState({screenSharingState: SCREEN_SHARE_STATES.INACTIVE});
> +
> +      var muteBtn = view.getDOMNode().querySelector('.btn-mute-video');
> +
> +        React.addons.TestUtils.Simulate.click(muteBtn);

nit: indentation

::: browser/components/loop/test/shared/views_test.js
@@ +134,5 @@
> +      expect(comp.getDOMNode().classList.contains("disabled")).eql(false);
> +    });
> +
> +    it("should dispatch a StartScreenShare action on click when the state" +
> +      "is not active", function() {

nit: Really, this 80-column rule... it's kinda outdated and we're not sticking to it in desktop code IF it reduces readability of the code. Concatenating a string here is not what we want, so can you keep the string on one line?

@@ +151,5 @@
> +          }));
> +      });
> +
> +    it("should dispatch a EndScreenShare action on click when the state" +
> +      "is active", function() {

nit: same.

@@ +164,5 @@
> +
> +        sinon.assert.calledOnce(dispatcher.dispatch);
> +        sinon.assert.calledWithExactly(dispatcher.dispatch,
> +          new sharedActions.EndScreenShare({
> +          }));

nit: indentation.

Comment 17

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #16)
> ::: browser/components/loop/content/conversation.html
> @@ +39,5 @@
> >      <script type="text/javascript" src="loop/shared/js/roomStates.js"></script>
> >      <script type="text/javascript" src="loop/shared/js/fxOSActiveRoomStore.js"></script>
> >      <script type="text/javascript" src="loop/shared/js/activeRoomStore.js"></script>
> >      <script type="text/javascript" src="loop/shared/js/feedbackStore.js"></script>
> > +    <script type="text/javascript" src="loop/shared/js/views.js"></script>
> 
> why here, right between the two feedback feature related files?

views.js now depends on the dispatcher, it also is possible that it could, in future, depend on some of the stores. I didn't want us to keep on having to move it, if that was the case... However, also feedbackViews.js depends on views.js, so we can't swap the order.

Comment 18

[Mark Banner (:standard8)]

Attachment: Part 1 - Setup minimal screen sharing for Loop from desktop (disabled by default).

Updated for review comments.

Comment 19

[Mark Banner (:standard8)]

Attachment: Part 2 - Show the Loop screenshare video in place of the remote video for now.

This part is now ready for review. Updated with extra comments and tests.

The functional test has been updated, but I've disabled the screenshare testing parts at the moment - I think the tests should work, save for the permissions issues.

I'll probably take a look at the prompt issues in the functional test later, but I think this can land without those, hence putting up for review, I can always make them a separate bug/patch.

Comment 20

[Mike de Boer [:mikedeboer]]

Comment on attachment 8557971 [details] [diff] [review]
Part 2 - Show the Loop screenshare video in place of the remote video for now.

Review of attachment 8557971 [details] [diff] [review]:
-----------------------------------------------------------------

So I have one question, but that doesn't keep me from saying: ship it!

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +382,5 @@
>        this.setStoreState({screenSharingState: actionData.state});
>      },
>  
>      /**
> +     * Used to not the current state of receiving screenshare data.

nit: note.

::: browser/components/loop/content/shared/js/otSdkDriver.js
@@ +355,5 @@
> +
> +    /**
> +     * Handles the event when the remote stream is destroyed.
> +     *
> +     * @param  {StreamEvent} event The event details:

nit: superfluous space.

::: browser/components/loop/test/functional/config.py
@@ +13,5 @@
>      "devtools.debugger.remote-enabled": True,
>      "media.volume_scale": "0",
>      "loop.gettingStarted.seen": True,
>      "loop.seenToS": "seen",
> +    "loop.screenshare.enabled": True,

I thought you wanted to disable the screenshare tests for now?

Comment 21

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #20)
> ::: browser/components/loop/test/functional/config.py
> > +    "loop.screenshare.enabled": True,
> 
> I thought you wanted to disable the screenshare tests for now?

The pref only enables the button on the display. The code that runs the screenshare part is commented out. I don't think having it displayed affects anything significant.

Comment 22

[scaraveo]

Comment on attachment 8555280 [details] [diff] [review]
Part 4 - Support the screensharing doorhanger in Social API chat windows and allow for multiple independent notification icons per type.

This looks ok.  I wonder a little bit if it isn't time to just make an iconbox here, but that may be more effort than it's worth.

Comment 23

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/818a084951ec
https://hg.mozilla.org/integration/fx-team/rev/213565c829e4
https://hg.mozilla.org/integration/fx-team/rev/09c2a262ed60

Leave-open as we need to do some bug cleanup, which I'll make sure happens in the morning.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/818a084951ec
https://hg.mozilla.org/mozilla-central/rev/213565c829e4
https://hg.mozilla.org/mozilla-central/rev/09c2a262ed60

Comment 25

[Florian Quèze [:florian]]

Comment on attachment 8555280 [details] [diff] [review]
Part 4 - Support the screensharing doorhanger in Social API chat windows and allow for multiple independent notification icons per type.

Review of attachment 8555280 [details] [diff] [review]:
-----------------------------------------------------------------

Like Shane, I wonder if we shouldn't just make the chat window have an icon box. But this seems fine otherwise.

::: toolkit/modules/PopupNotifications.jsm
@@ +731,5 @@
>        // remove previous icon classes
>        let className = anchorElement.className.replace(/([-\w]+-notification-icon\s?)/g,"")
>        className = "default-notification-icon " + className;
> +      if (notifications.length > 0) {
> +        // Find the first notification this anchor used for.

I can't parse this comment.

Comment 26

[Florian Quèze [:florian]]

Comment on attachment 8555281 [details] [diff] [review]
Part 5 - Allow multiple anchors for multiple notifications without using an iconbox.

Review of attachment 8555281 [details] [diff] [review]:
-----------------------------------------------------------------

The code in PopupNotifications.jsm has pretty good test coverage, so if you are extending it, please add tests for your additions.

::: toolkit/modules/PopupNotifications.jsm
@@ +655,4 @@
>     *                      notifications will be retrieved off the current
>     *                      browser tab
>     * @param anchor is a XUL element that the notifications panel will be
>     *                      anchored to

Please update the comment here.

@@ +659,5 @@
>     * @param dismissShowing if true, dismiss any currently visible notifications
>     *                       if there are no notifications to show. Otherwise,
>     *                       currently displayed notifications will be left alone.
>     */
> +  _update: function PopupNotifications_update(notifications, anchors = new Set(), dismissShowing = false) {

I'm not really excited by the change of type of the second parameter here; I wonder if this method could have been used by add-ons despite the leading _ character.

How would you feel about something like:
if (anchors instanceof Ci.nsIDOMXULElement)
  anchors = new Set(anchors);

@@ +689,5 @@
>  
>        // If no anchor has been passed in, use the anchor of the first
>        // showable notification.
> +      if (!anchorElements.size && notificationsToShow.length)
> +        anchorElements.add(notificationsToShow[0].anchorElement);

Should this actually be:
  anchorsElements = this._getAnchorsForNotifications(notificationsToShow)?

Or should we set the anchors parameter automatically to this._getAnchorsForNotifications(notifications) when it's not been provided?

It seems it would simplify the callers in _swapBrowserNotifications.

@@ +734,5 @@
>      }
>    },
>  
>    _updateAnchorIcon: function PopupNotifications_updateAnchorIcon(notifications,
> +                                                                  anchorElements) {

Should this still support receiving a single anchorElement? Looks like it's not needed as you changed all the callers, so maybe rename this to _updateAnchorIcon*s*?