1131584 - Add option to screen share dropdown to share tabs

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

Once bug 1131581 is complete, an additional option can be added to be the first-most option inside the dropdown menu: 'Share my Tabs'.

The action to be executed when clicked should be similar to sharing the video source 'window', but instead the source value should be 'browser'.

For the feature to work, we'll need a working copy of the OpenTok SDK that supports this feature.

Comment 1

[:shell escalante]

triaging to blocking-loop:Fx38+ for tracking this release, all bugs in 38+ and 38? now, will move to "backlog+" for Fx39.  After that - we won't triage to a specific release any longer.  we will work from an ordered backlog.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: add 'Share my Tabs' button to the screenshare dropdown menu in the conversation window

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8565417 [details] [diff] [review]
Patch v1: add 'Share my Tabs' button to the screenshare dropdown menu in the conversation window

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

I think this is almost there, but there's a few changes needed so I'd like to see it again before landing.

There's a test failure in the shared tests in the loop.OTSdkDriver section:

Events
accessAllowed

    "before each" hook
    missing required type
    Validator.prototype._checkRequiredProperties@http://localhost:3000/content/shared/js/validate.js:97:15
    Validator.prototype.validate@http://localhost:3000/content/shared/js/validate.js:60:7
    Action@http://localhost:3000/content/shared/js/actions.js:20:25
    @http://localhost:3000/test/shared/otSdkDriver_test.js:620:31
    callFn@http://localhost:3000/test/shared/vendor/mocha-2.0.1.js:4428:18
    Runnable.prototype.run@http://localhost:3000/test/shared/vendor/mocha-2.0.1.js:4421:7
    next@http://localhost:3000/test/shared/vendor/mocha-2.0.1.js:4708:5
    Runner.prototype.hook/<@http://localhost:3000/test/shared/vendor/mocha-2.0.1.js:4725:5
    timeslice@http://localhost:3000/test/shared/vendor/mocha-2.0.1.js:5989:5

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +198,5 @@
> +      this._onUpdateListener = this._handleRoomUpdate.bind(this);
> +      this._onDeleteListener = this._handleRoomDelete.bind(this);
> +
> +      this._mozLoop.rooms.on("update:" + actionData.roomToken, this._onUpdateListener);
> +      this._mozLoop.rooms.on("delete:" + actionData.roomToken, this._onDeleteListener);

These changes seem part of a separate (incomplete?) bug/patch.

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

It is required parameter for the action, so I think we should just go with actionData.type.

::: browser/components/loop/content/shared/js/views.jsx
@@ +103,5 @@
>      },
>  
> +    _startScreenShare: function(type) {
> +      this.props.dispatcher.dispatch(new sharedActions.StartScreenShare({
> +        type: type || "window"

I think we could just pass "window" in and simplify the code here.

::: browser/components/loop/test/shared/views_test.js
@@ +165,5 @@
>            ".conversation-window-dropdown > li"));
>  
>          sinon.assert.calledOnce(dispatcher.dispatch);
>          sinon.assert.calledWithExactly(dispatcher.dispatch,
> +          new sharedActions.StartScreenShare({ type: "browser" }));

We should add a test for the other item as well, to check we get the right parameter.

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: add 'Share my Tabs' button to the screenshare dropdown menu in the conversation window

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2.1: add 'Share my Tabs' button to the screenshare dropdown menu in the conversation window

Whoops, missed a comment.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8567048 [details] [diff] [review]
Patch v2.1: add 'Share my Tabs' button to the screenshare dropdown menu in the conversation window

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

::: browser/components/loop/test/shared/views_test.js
@@ +168,5 @@
>          sinon.assert.calledWithExactly(dispatcher.dispatch,
> +          new sharedActions.StartScreenShare({ type: "browser" }));
> +
> +        TestUtils.Simulate.click(comp.getDOMNode().querySelector(
> +          ".conversation-window-dropdown > li:last-child"));

Can you make it a separate "it" please, so that its clearer for people scanning through the tests for what is being tested.

Comment 7

[Mike de Boer [:mikedeboer]]

Pushed to fx-team with comment addressed: https://hg.mozilla.org/integration/fx-team/rev/bdf97ff74fea

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/bdf97ff74fea

Comment 9

[Ton [:Tonnes]]

Why use "my" when similar / nearby options use "your"?

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Ton from comment #9)
> Why use "my" when similar / nearby options use "your"?

This is a question for Sevaan, he'd be interested to keep messaging consistent.

Comment 11

[Sevaan Franks [:sevaan]]

matej: Share my tabs...or Share your tabs?

Comment 12

[Matej Novak [:matej]]

(In reply to Sevaan Franks [:sevaan] from comment #11)
> matej: Share my tabs...or Share your tabs?

I always prefer "your" in product. "My" ends up being too ambiguous (is the browser talking to you, or you talking to the browser?).

Comment 13

[Sevaan Franks [:sevaan]]

Sounds good. Let's go with "Your", Mike. Good catch!

Comment 14

[Bogdan Maris, Desktop QA]

Verified on Windows 8.1 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit using Firefox 38 beta 1 that the option to share tabs is implemented and also did some exploratory testing with the option. 
The remaining dependences will be handeled in their own bugs.