1199213 - Drop old call url handling code from Loop's standalone UI (to be released after 23rd Sept)

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

On Monday 24th August, we deployed the change to the server to stop producing call-urls.

Any old ones will expire by 23rd Sept, so we can release a change soon after that which drops support completely.

See the user story for the initial list of things to remove.

Suggest we do this towards the end of iteration 43.3, then it can be released soon after.

Comment 1

[Mark Banner (:standard8)]

Attachment: Part 1. Remove the old standalone call-url handling code from Loop.

Ok, this is big enough that I'm a little concerned about bitrot etc that I think I'm splitting it into two parts. The second part will be the css removals, that I'm still looking at.

This gets rid of all the functional code wrt to call-url handling.

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 8661087 [details] [diff] [review]
Part 1. Remove the old standalone call-url handling code from Loop.

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

WOW. Well, as long as the tests all pass, I'm super-mega-happy with this patch!!

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0159f007b402

Comment 4

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

https://hg.mozilla.org/mozilla-central/rev/0159f007b402

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

This looks like it broke the saucelabs tests as soon as it was pushed to loop-client:

tests/test_hello.py:17: in test_supported_browser
    assert hello_page.is_header_title_visible
E   assert <pages.hello.HelloPage object at 0x7fbd658a9bd0>.is_header_title_visible

FAIL tests/test_hello.py::TestHelloPage::()::test_supported_browser
===== 1 tests deselected by "-ktest_supported_browser -m 'nondestructive'

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/733a8ef024c7

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/733a8ef024c7

Comment 8

[Mark Banner (:standard8)]

Attachment: Part 2. Remove the old css relating to standalone call-url handling from Loop.

This removes all the redundant css that I could find. The roomViews.jsx change is due to also finding an unused variable referencing some old css.

I used a cut-down showcase plus Google Chrome's audit tools to find the obsolete classes.

There's a bit more I want to do with respect to cleaning up some of the *-video classes at the bottom of conversation.css, but this is enough of a cleanup to leave that to another patch.

Comment 9

[Mark Banner (:standard8)]

Attachment: Part 3. More cleanup of conversation related css after call-url code removal.

Ok, last part for this bug. This:

- Re-arranges some of the "global" *-video classes to be underneath the media-wrapper/layout class, and reorder them in conversation.css
- Fixes a bug with the max-width of 640px which hadn't removed the 38px from the calculation when we changed the toolbar styles
- Moves .room-inner-info-area and .prompt-media-message to webapp.css (from conversation.css) and switches them to use child selectors, and drop the .standalone prefix as it is no longer necessary (nor was it before).
- A few other obsolete class removals.

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8663658 [details] [diff] [review]
Part 2. Remove the old css relating to standalone call-url handling from Loop.

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

LGTM!

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8663858 [details] [diff] [review]
Part 3. More cleanup of conversation related css after call-url code removal.

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

Ah, I see what you did here. Nice to have this CSS be standalone specific.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ffd0a8940f15
https://hg.mozilla.org/integration/fx-team/rev/83c750c79338

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/ffd0a8940f15
https://hg.mozilla.org/mozilla-central/rev/83c750c79338

Comment 14

[Adam Roach [:abr]]

(Commenting on User Story)
> Things to remove:

> - multiplexGum.js
> -- Remove the file completely
> -- Remove any remaining references
> -- Update any .html files referencing it

I'm not sure this was wise. This fundamentally changes the way camera access is granted, and was added specifically to prevent users from getting into a session flow without first ensuring they have a camera and/or microphone, as necessary. It did not, in any case, have anything to do with direct calls, and was added specifically to improve the rooms experience.

Unless I'm missing something big, we need a follow-up bug here to restore this file and its inclusion.

Comment 15

[Adam Roach [:abr]]

(In reply to Adam Roach [:abr] from comment #14)
> (Commenting on User Story)
> > Things to remove:
> 
> > - multiplexGum.js
> > -- Remove the file completely
> > -- Remove any remaining references
> > -- Update any .html files referencing it
> 
> I'm not sure this was wise. This fundamentally changes the way camera access
> is granted, and was added specifically to prevent users from getting into a
> session flow without first ensuring they have a camera and/or microphone, as
> necessary. It did not, in any case, have anything to do with direct calls,
> and was added specifically to improve the rooms experience.
> 
> Unless I'm missing something big, we need a follow-up bug here to restore
> this file and its inclusion.

Dan corrected me here, and points out that the original bug (Bug 1073410) was, in fact, implemented during the direct-calling era. He said that there is something similar in place for rooms, but I can't find anything on a quick glance through the existing code. I'm mostly concerned that we're adding people to rooms without any prior validation that the microphone and/or camera work, which is going to make our failure numbers a lot higher than they should be.

Mark: how is this handled for rooms?

Comment 16

[Mark Banner (:standard8)]

(In reply to Adam Roach [:abr] from comment #15)
> Dan corrected me here, and points out that the original bug (Bug 1073410)
> was, in fact, implemented during the direct-calling era. He said that there
> is something similar in place for rooms, but I can't find anything on a
> quick glance through the existing code. I'm mostly concerned that we're
> adding people to rooms without any prior validation that the microphone
> and/or camera work, which is going to make our failure numbers a lot higher
> than they should be.
> 
> Mark: how is this handled for rooms?

The issue that bug 1073410 fixed was to do with our UI providing a cancel button but the gUM API doesn't. As a result the user could get into a state where they've cancelled on our UI but not on the gUM prompt. Attempting to do restart a call (and hence get a second prompt) would therefore make everything confused, even if the user did then accept the prompt.

For rooms, we handled this from the start by removing the option to cancel joining, and just displaying the gUM prompt - the user can either do something with the prompt or reload the page. Either will result in a good state.

We never invoked the multiplexGum code for rooms.

Whilst it'd be nice to provide the cancel button, I'd assert it doesn't actually help users very far, and the workarounds we put in place to get it weren't really worth the cost. I'd rather us put effort towards getting the gUM API extended so that it could be cancelled by the application.