Tab sharing is unpaused when switching tabs

VERIFIED FIXED

Status

P2
normal
Rank:
25
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: bogdan_maris, Assigned: mancas)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(firefox46 verified, firefox47 verified)

Details

Attachments

(3 attachments)

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).
Created attachment 8696467 [details]
Gif showing the issue

Forgot to attach a GIF showing the issue.
Manuel, can you take this as a follow-up to bug 1214214?
Blocks: 1214214
Flags: needinfo?(b.mcb)
(Assignee)

Comment 3

3 years ago
Sure!
Assignee: nobody → b.mcb
Flags: needinfo?(b.mcb)
(Assignee)

Comment 4

3 years ago
Created attachment 8697942 [details] [diff] [review]
Tab sharing is unpaused when switching tabs

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

Thank you!
Attachment #8697942 - Flags: review?(mdeboer)
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*
Attachment #8697942 - Flags: review?(mdeboer) → review+
Rank: 25
Priority: -- → P2

Comment 6

3 years ago
¡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!
Flags: needinfo?(b.mcb)

Updated

3 years ago
status-firefox46: --- → affected
Created attachment 8700499 [details] [review]
[loop] mancas:bug1230946 > mozilla:master
(Assignee)

Comment 8

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

Keeping r+ from Mike with comments addressed.
Flags: needinfo?(b.mcb)
Attachment #8700499 - Flags: review+
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.
Status: NEW → RESOLVED
Iteration: --- → 46.1 - Dec 28
Last Resolved: 3 years ago
Resolution: --- → FIXED
Flags: qe-verify+
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?
Flags: needinfo?(standard8)
Answered on irc: we're still getting processes set up. The add-on can currently be built from the repo.
Flags: needinfo?(standard8)
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.
Status: RESOLVED → VERIFIED
status-firefox46: affected → verified
status-firefox47: --- → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.