1230946 - Tab sharing is unpaused when switching tabs

Status: VERIFIED FIXED

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

Description

[Bogdan Maris, Desktop QA]

Affected builds:
- latest Aurora 44.0a2
- latest Nightly 45.0a1

Affected OS`s:
- Windows 10 64-bit
- Ubuntu 14.04 32-bit
- Mac OS X 10.10

STR:
1. Start Firefox
2. Start a conversation
3. Press Pause to pause the sharing
4. Switch tabs

Expected results: Don't quite know the intended behavior here but I think the Share bar should not change even if I switch through tabs, it should remain 'Paused' until one hits 'Resume'

Actual results: Sharing is resumed on all tabs (does not matter which tab I focus).

Notes:
- This is not a regression, it was introduced when the Share bar was added, bug 1214214, or the bug where pause/unpause were added (could not find the bug).

Comment 1

[Bogdan Maris, Desktop QA]

Attachment: Gif showing the issue

Forgot to attach a GIF showing the issue.

Comment 2

[Mike de Boer [:mikedeboer]]

Manuel, can you take this as a follow-up to bug 1214214?

Comment 3

[Manuel Casas Barrado [:mancas]]

Sure!

Comment 4

[Manuel Casas Barrado [:mancas]]

Attachment: Tab sharing is unpaused when switching tabs

Hey Mike, I showed you the patch last week so the review will be quick.

Thank you!

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8697942 [details] [diff] [review]
Tab sharing is unpaused when switching tabs

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

r=me with comments addressed. This was close to an r-, so please review your patch files yourself before posting.

::: browser/extensions/loop/bootstrap.js
@@ +518,5 @@
>            return;
>          }
>  
>          let box = gBrowser.getNotificationBox();
> +        let pauseButtonLabel = this._browserSharePaused ? this._getString("infobar_button_resume_label") :

I think it'd look best if you align the `this._getString(...` calls, each on their own line.

Since you're alternating string IDs, perhaps it'd be better to put the conditional _inside_ `this._getString`:

```js
let pauseButtonLabel = this._getString(this._browserSharePaused ?
  "infobar_button_resume_label" : "infobar_button_pause_label");
```

@@ +538,4 @@
>              isDefault: false,
>              callback: (event, buttonInfo, buttonNode) => {
> +              this._browserSharePaused = !this._browserSharePaused;
> +              bar.label = this._browserSharePaused ? this._getString("infobar_screenshare_paused_browser_message") :

Same wrt alignment.

@@ +558,5 @@
>              }
>            }]
>          );
>  
> +        // Sets 'paused' class if needed

nit: missing '.'.

@@ +561,5 @@
>  
> +        // Sets 'paused' class if needed
> +        if (this._browserSharePaused) {
> +          bar.classList.toggle("paused", this._browserSharePaused);
> +        }

Please remove the conditional and rewrite as:
```js
bar.classList.toggle("paused", !!this._browserSharePaused);
```

::: browser/extensions/loop/content/shared/js/activeRoomStore.js
@@ +890,5 @@
>       *
>       * @param {Number} windowId  The new windowId to start sharing.
>       */
>      _handleSwitchBrowserShare: function(windowId) {
> +      console.info(this);

*ahum*

Comment 6

[alex_mayorga]

¡Hola Manuel!

This just bite me on Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:46.0) Gecko/20100101 Firefox/46.0 ID:20151217072309 CSet: 0711218a018d912036f7d3be2ae2649e213cfb85

Any hopes of a polished patch soon?

¡Gracias!

Comment 7

[GitHub Autolander Bot]

Attachment: [loop] mancas:bug1230946 > mozilla:master

Comment 8

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8700499 [details] [review]
[loop] mancas:bug1230946 > mozilla:master

Keeping r+ from Mike with comments addressed.

Comment 9

[Mark Banner (:standard8)]

Merged:

https://github.com/mozilla/loop/commit/f108448d16d12bae41d49a760243e6e48a17714f
https://github.com/mozilla/loop/commit/5a6ad7d9ea0f4b2fd32a89a00ec33dd3a4c56792

Mancas: You should have github access to land these yourself, but I've landed it for you given the afk.

Comment 10

[Bogdan Maris, Desktop QA]

Since this is marked fixed, I went ahead and tested on latest Nightly and the issue is still there. I see that no push has been made to Nightly and only merged in github (maybe this is because hello is now a add-on), is there a way I can test on this fix and others to come in latest Nightly?

Comment 11

[Mark Banner (:standard8)]

Answered on irc: we're still getting processes set up. The add-on can currently be built from the repo.

Comment 12

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce this issue on Windows 10 x86, using Firefox 45.0a1(2015-10-07).
The issue is no longer reproducible on Firefox 46.0a2(2016-03-02) and on Firefox 47.0a1(2016-03-02), using the default version of Hello(1.1.2 for Aurora build and 1.1.9 for Nightly build).
The issue was verified on Windows 10 x86, Ubuntu 14.04 x64 and on Mac OS X 10.11.