1135045 - Add infobar for when tab sharing is activated

Status: VERIFIED FIXED

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

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: add strings for the Hello infobar to be added soon

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: add strings for the Hello infobar to be added soon

Forgot to add accesskeys

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8567113 [details] [diff] [review]
Patch 1.1: add strings for the Hello infobar to be added soon

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

Please drop the browser.css change. r=Standard8 with that fixed.

Comment 4

[Mike de Boer [:mikedeboer]]

Pushed this single patch to fx-team: https://hg.mozilla.org/integration/fx-team/rev/522de3fc7a8a

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/522de3fc7a8a

Comment 6

[Mike de Boer [:mikedeboer]]

As per IRC wrt the icon shown in the notification bar:

[17:00:32] <sevaan> mikedeboer: Sorry! We don't want to use that sharing icon...that was there because we didn't have a screen-sharing icon created at the
                    time.
[17:00:38] <sevaan> But I wonder if we should use a Firefox Hello logo instead
[17:00:43] <sevaan> Let me look
[17:01:48] <sevaan> Use the screen-sharing icon that we have in the Conversation window

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: support notification bar buttons of type menu-button with a custom menupopup

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3: show an infobar when tab sharing is activated

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: support notification bar buttons of type menu-button with a custom menupopup

Comment 10

[Mike de Boer [:mikedeboer]]

[Tracking Requested - why for this release]: Loop/ Hello window & tab sharing is slated for Fx38, this requesting to track this bug for uplift when it's resolved.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8573255 [details] [diff] [review]
Patch 2.1: support notification bar buttons of type menu-button with a custom menupopup

Officially, I'm not a browser/ peer yet, so passing to Jared.

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4: add tests to confirm that the tab-sharing infoBar is working as expected

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8573248 [details] [diff] [review]
Patch 3: show an infobar when tab sharing is activated

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

::: browser/base/content/browser-loop.js
@@ +489,5 @@
>  
> +      let wasVisible = false;
> +      // Hide the infobar from the previous tab.
> +      if (typeof event.fromIndex == "number" && event.fromIndex > 0) {
> +        wasVisible = this._hideBrowserSharingInfoBar(false,

For some reason this swapping of tabs doesn't seem to work for me.

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.2: support notification bar buttons of type menu-button with a custom menupopup

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.1: show an infobar when tab sharing is activated

Comment 16

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4.1: add tests to confirm that the tab-sharing infoBar is working as expected

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.2: show an infobar when tab sharing is activated

Now also works in non-e10s windows!

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.3: show an infobar when tab sharing is activated

Looks like this is working _much_ better now. Mark, can you confirm that too?

Comment 19

[:shell escalante]

Per Gavin's request - just updating bugs that carried over before they were 100% resolved to the next iteration.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 8576027 [details] [diff] [review]
Patch 4.1: add tests to confirm that the tab-sharing infoBar is working as expected

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

Looks good, one minor comment.

::: browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js
@@ +147,5 @@
> +  };
> +
> +  let testBarProps = function() {
> +    // We now know that the second tab is selected and should be displaying the
> +    // infobar.

I think this comment would be better situated at the first call to testBarProps()

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 8576115 [details] [diff] [review]
Patch 3.3: show an infobar when tab sharing is activated

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

Looking good, just a couple of minor things to clean up.

::: browser/base/content/browser-loop.js
@@ +464,5 @@
> +    _hideBrowserSharingInfoBar: function(permanently = false, browser) {
> +      browser = browser || gBrowser.selectedTab.linkedBrowser;
> +      let box = gBrowser.getNotificationBox(browser);
> +      let notification = box.getNotificationWithValue(kBrowserSharingNotificationId);
> +      let removed = false;

You're not doing anything with the return value here, so I don't think you need to track whether or not it was removed.

::: toolkit/content/widgets/tabbox.xml
@@ +662,5 @@
>            if (val < 0 || val >= this.childNodes.length)
>              return val;
>            var panel = this._selectedPanel;
>            this._selectedPanel = this.childNodes[val];
> +          var fromIndex = this.selectedIndex;

This is now redundant.

Comment 22

[Mike de Boer [:mikedeboer]]

Super!

Pushed to fx-team as:

https://hg.mozilla.org/integration/fx-team/rev/0761779e008b
https://hg.mozilla.org/integration/fx-team/rev/77e37031bb11
https://hg.mozilla.org/integration/fx-team/rev/5e797718340b

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this for 38 based on comment 10.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0761779e008b
https://hg.mozilla.org/mozilla-central/rev/77e37031bb11
https://hg.mozilla.org/mozilla-central/rev/5e797718340b

Comment 25

[Mike de Boer [:mikedeboer]]

Comment on attachment 8576025 [details] [diff] [review]
Patch 2.2: support notification bar buttons of type menu-button with a custom menupopup

Approval Request Comment
[Feature/regressing bug #]: Loop/ Hello screensharing milestone
[User impact if declined]: User will see a new option that allows her/ him to share a window or Firefox tabs inside a room (aka. conversation).
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass.
[Risks and why]: minor
[String/UUID change made/needed]: n/a

Comment 26

[Mike de Boer [:mikedeboer]]

Comment on attachment 8576027 [details] [diff] [review]
Patch 4.1: add tests to confirm that the tab-sharing infoBar is working as expected

Please see comment 25 for approval request.

Comment 27

[Mike de Boer [:mikedeboer]]

Comment on attachment 8576115 [details] [diff] [review]
Patch 3.3: show an infobar when tab sharing is activated

Please see comment 25 for approval request.

Comment 28

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/249da78cf780
https://hg.mozilla.org/releases/mozilla-aurora/rev/f4c0e3fd326a
https://hg.mozilla.org/releases/mozilla-aurora/rev/b3d399af0d4c

Comment 29

[Bogdan Maris, Desktop Test Engineering]

Verified that the tab sharing infobar was implemented and works as expected across platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit) on latest Nightly. I`m gonna close this bug as verified but will get back to verify on Aurora tomorrow.

Comment 30

[Bogdan Maris, Desktop Test Engineering]

Verified on Firefox Developer edition as well across platforms (Windows 8.1 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit).