Closed Bug 1131584 Opened 5 years ago Closed 5 years ago

Add option to screen share dropdown to share tabs

Categories

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

defect
Points:
3

Tracking

(firefox38 verified)

VERIFIED FIXED
mozilla38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- verified
Blocking Flags:
backlog Fx38+

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

(Whiteboard: [screensharing][strings])

Attachments

(1 file, 2 obsolete files)

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.
Flags: qe-verify+
Whiteboard: [strings]
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.
backlog: --- → Fx38+
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 38.3 - 23 Feb
Flags: firefox-backlog+
Target Milestone: --- → mozilla38
Whiteboard: [strings] → [screensharing][strings]
Blocks: 1133493
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.
Attachment #8565417 - Flags: review?(standard8) → review-
Whoops, missed a comment.
Attachment #8567045 - Attachment is obsolete: true
Attachment #8567045 - Flags: review?(standard8)
Attachment #8567048 - Flags: review?(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.
Attachment #8567048 - Flags: review?(standard8) → review+
Pushed to fx-team with comment addressed: https://hg.mozilla.org/integration/fx-team/rev/bdf97ff74fea
https://hg.mozilla.org/mozilla-central/rev/bdf97ff74fea
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Why use "my" when similar / nearby options use "your"?
(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.
Flags: needinfo?(sfranks)
matej: Share my tabs...or Share your tabs?
Flags: needinfo?(sfranks) → needinfo?(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?).
Flags: needinfo?(matej)
Sounds good. Let's go with "Your", Mike. Good catch!
QA Contact: bogdan.maris
Depends on: 1150501
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.