Closed Bug 1254104 Opened 4 years ago Closed 4 years ago

Sharing notifications are not displayed on the Conversation window toolbar in e10s mode

Categories

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

defect
Points:
2

Tracking

(e10s+, firefox46 affected, firefox47 verified, firefox48 verified)

RESOLVED FIXED
mozilla48
Iteration:
48.1 - Mar 21
Tracking Status
e10s + ---
firefox46 --- affected
firefox47 --- verified
firefox48 --- verified

People

(Reporter: standard8, Assigned: mikedeboer)

References

(Depends on 1 open bug)

Details

(Whiteboard: [btpp-fix-now])

Attachments

(2 files)

I just noticed a regression in e10s mode - we're not displaying the sharing (mic/camera nor screen) notifications in the conversation window toolbar.

They are fine in non-e10s mode. If I go to the global toolbar and select the icon, then "Control Sharing", the pop-ups and icons appear fine.

Side note: Although we want to change these at some stage (bug 1216679), we haven't decided exactly on how yet, therefore these should be present until we have a good resolution for that bug. I'm also cautious, as this means something isn't working as it should be in e10s mode.

Florian, any ideas on where we should start looking? or for things for e10s that might have affected this?
Flags: needinfo?(florian)
Rank: 16
Whiteboard: [btpp-fix-now]
I'll just try to fix this.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 48.1 - Mar 21
Points: --- → 5
Flags: needinfo?(florian)
This issue was introduced by bug 896748, in other words: a regression that we know about since Loop started running in e10s mode.
Points: 5 → 2
Hi Tom, can you review this for me?
Attachment #8730827 - Flags: review?(evilpies)
Comment on attachment 8730827 [details] [diff] [review]
Patch v1: use docShellIsActive instead of introducing logic that relies on tabbrowser

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

Makes sense.
Attachment #8730827 - Flags: review?(evilpies) → review+
This test fix could be the fix for bug 1233587 as well, if it still occurs that is.
Try is looking nice and green.
Attachment #8731201 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/integration/fx-team/rev/30e050c04c4e32e77a7fa4afeb05c8c9f1093c30
Bug 1254104: use docShellIsActive instead of introducing logic that relies on tabbrowser implementation specifics. r=evilpies

https://hg.mozilla.org/integration/fx-team/rev/4f18cd7a0bc81ce85d0668c86e66156455d9cb84
Bug 1254104: fix timing issues in browser_bug553455.js, which started perma-failing after fixing docShellIsActive getters in PopupNotifications.jsm. r=Mossop
Thanks for the reviews, Tom & Dave!
https://hg.mozilla.org/mozilla-central/rev/30e050c04c4e
https://hg.mozilla.org/mozilla-central/rev/4f18cd7a0bc8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8730827 [details] [diff] [review]
Patch v1: use docShellIsActive instead of introducing logic that relies on tabbrowser

Approval Request Comment
[Feature/regressing bug #]: bug 896748, loop-e10s
[User impact if declined]: The user won't see the camera/ mic and browser sharing icons in the titlebar of chat windows in e10s mode without this patch applied. Thus users won't be able to access the device permission doorhangers.
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass provided that Patch 2 is also present.
[Risks and why]: minor, since test coverage is pretty good here.
[String/UUID change made/needed]: n/a.
Attachment #8730827 - Flags: approval-mozilla-beta?
Attachment #8730827 - Flags: approval-mozilla-aurora?
Comment on attachment 8731201 [details] [diff] [review]
Patch 2: fix timing issues in browser_bug553455.js

See comment 13 for approval request comment. Thank you!
Attachment #8731201 - Flags: approval-mozilla-beta?
Attachment #8731201 - Flags: approval-mozilla-aurora?
Comment on attachment 8730827 [details] [diff] [review]
Patch v1: use docShellIsActive instead of introducing logic that relies on tabbrowser

The e10s experiment isn't going to last much longer on 46 and we aren't shipping with e10s for 46. I don't think it's worth the risk to uplift this to beta 4.
Attachment #8730827 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8731201 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8730827 [details] [diff] [review]
Patch v1: use docShellIsActive instead of introducing logic that relies on tabbrowser

Fix for Hello + e10s. Aurora47+
Attachment #8730827 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8731201 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
Unable to reproduce this issue with Nightly from 2016-03-07. 
Confirming that all sharing notifications are displayed with 47 beta (e10s on/off), across platforms [1]. 

[1] Windows 10 64-bit, Ubuntu 14.04 32-bit, Mac OS X 10.10.5
I wasn't able to reproduce the issue on Nightly 2016-03-07.

I can confirm that the sharing notifications are correctly displayed with Firefox 48 Beta 9, e10s on/off (buildID: 20160718142219), on Windows 7 64bit, Ubuntu 14.04 64bit and Mac OSX 10.9.5.
Note that this was a backout of a backout, so essentially, this has re-landed.
Flags: needinfo?(mconley)
Depends on: 1328069
You need to log in before you can comment on or make changes to this bug.