Closed Bug 1274237 Opened 8 years ago Closed 8 years ago

Investigate delaying starting tab sharing until after the A/V media has connected.

Categories

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

Product:
Hello (Loop)
Component:
Client
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: standard8, Assigned: dcritchley)

References

Details

User Story

First hack:

- Move the startBrowserShare action dispatch from roomViews to activeRoomStore#mediaStreamCreated
-- Only call the action for the desktop, and only when the remote stream is received.
- Ensure it works in manual tests
- Fix Functional tests.
- Attach patch/PR to bug labelled as WIP.

We'll then test it on the functional test servers

For landing:

- Check the UX is OK
- Do we need to make the sharing bar display earlier?

Attachments

(1 file)

WIP [loop] daveccrit:1274237-delayedTabStart > mozilla:master
40 bytes, text/x-github-pull-request
Mardak
: review+
Details | Review 
To help with the connectivity issues seen in bug 1256244, we want to try delaying the tab sharing start-up until after the A/V media has connected.

See user story for more details.
Rank: 5
Priority: -- → P1
Assignee: nobody → dcritchley
Attachment #8756403 - Flags: review?(standard8)
Attachment #8756403 - Flags: review?(edilee)
Attachment #8756403 - Flags: review?(dmose)
Attachment #8756403 - Flags: review?(standard8)
Attachment #8756403 - Flags: review?(edilee)
Dan, not sure if you're reviewing here. The PR I'd consider as WIP at the moment. I want to get it on the functional test servers once bug 1276182 is fixed.

I'm not quite sure if the UX is good enough at the moment. We probably need to see what we think about the share bar not opening etc.
Flags: needinfo?(dmose)
I had been planning to review last night, which is why I grabbed it (since review was requested of it from you, me, or Mardak).  However, I ran out of time.  You're welcome to take it back, or I can review today based on your guidelines in comment 2...
Flags: needinfo?(dmose)
Bug 1245150 was looking to change the timing by sharing tabs on JOINING instead of SESSION_CONNECTED:

https://github.com/mozilla/loop/pull/187/files#diff-2
Blocks: 1245150
Attachment #8756403 - Attachment description: [loop] daveccrit:1274237-delayedTabStart > mozilla:master → WIP [loop] daveccrit:1274237-delayedTabStart > mozilla:master
Comment on attachment 8756403 [details] [review]
WIP [loop] daveccrit:1274237-delayedTabStart > mozilla:master

After chatting with Standard8 on irc, the next steps are ui-review from someone + any relevant changes, then having Standard8 monkeypatch the functional tests to try it, then code review.
Attachment #8756403 - Flags: review?(dmose)
I'm not sure how the stuff mentioned in comment 4 fits here, but we presumably don't want to block on that.
> - Do we need to make the sharing bar display earlier?

Yes we do. The infobar and the conversation window should both appear at the same time. This is to associate both pieces of UI together for the user.

We cam up with a design and string whereby the infobar would display some other text to the effect of "When you share this link and your friend joins, they will be able to see this tab" which is very helpful for hte user to know before someone joins and they're suddenly sharing their online banking page or something by accident.

We've made the design for it and have a string, but I"m not sure where it all lives at the moment (in AHA! or in a bug somewhere). RT, can you shed some light?
Flags: needinfo?(rtestard)
Bug 1245150 deals with displaying the sharing bar at the same time as the conversation window (blocked by this bug).
Bug 1239972 dealt with the string improvement and was landed as part of v1.3 so it seems that this bug can deal with delaying tab sharing only without any UX constraints but cannot be shipped without Bug 1245150.
Given that I think the dependency with Bug 1245150 is the wrong way around, changing this now.
No longer blocks: 1245150
Depends on: 1245150
Flags: needinfo?(rtestard)
Comment on attachment 8756403 [details] [review]
WIP [loop] daveccrit:1274237-delayedTabStart > mozilla:master

The extra commit I added to Dave's work makes the browser sharing infobar appear straight away and function correctly.

I think it all seems to be working fine. The only slight inconsistency is if an owner enters a room, pauses, and then a guest enters -> the display on the left is blank. However, I think that's good enough for now.

There's potentially an issue shown up a bit by the tests for the standalone - where we currently have to set the receivingScreenShare flag. It probably means that the displayScreenShare in the render calculation isn't quite right, but it does seem to work in my testing. Since we need to get this patch moving, I haven't looked into that further. Worst case, the tile wouldn't get shown after the peer has exited the room (but there is a message in the chat).
Attachment #8756403 - Flags: review?(edilee)
Comment on attachment 8756403 [details] [review]
WIP [loop] daveccrit:1274237-delayedTabStart > mozilla:master

r=Mardak with various nits
Attachment #8756403 - Flags: review?(edilee) → review+
Blocks: 1282899
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: