Closed Bug 1230946 Opened 4 years ago Closed 4 years ago

Tab sharing is unpaused when switching tabs


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



(firefox46 verified, firefox47 verified)

46.1 - Dec 28
Tracking Status
firefox46 --- verified
firefox47 --- verified


(Reporter: bogdan_maris, Assigned: mancas)




(3 files)

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

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).

- 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).
Attached image 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: nobody → b.mcb
Flags: needinfo?(b.mcb)
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`:

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:
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) {
> +;

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

Flags: needinfo?(b.mcb)
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+

Mancas: You should have github access to land these yourself, but I've landed it for you given the afk.
Iteration: --- → 46.1 - Dec 28
Closed: 4 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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.