[Mac only] gum sharing icons not shown in mac toolbar when making a call

VERIFIED FIXED in Firefox 43

Status

()

Firefox
Device Permissions
--
blocker
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: bogdan_maris, Assigned: standard8)

Tracking

({regression})

Trunk
Firefox 44
All
Mac OS X
regression
Points:
2

Firefox Tracking Flags

(firefox42 unaffected, firefox43+ verified, firefox44+ verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Affected builds:
- latest Nightly 44.0a1
- latest Developer Edition 43.0a2

Affected OS`s:
- Mac OS X 10.10.5

STR:
1. Start Firefox
2. Open https://mozilla.github.io/webrtc-landing/gum_test.html
3. Start Audio&Video or any other test.

Expected results: Icons are shown in Mac menu bar.

Actual results: Icons can`t be seen in Mac menu bar.

Notes:
1. This is a recent regression.

m-c:

Last Good: 2015-09-16
First Bad: 2015-09-17

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3e8dde8f8c174cce2c0b65c951808f88e35d1875&tochange=e7d613b3bcfe1e865378bfac37de64560d1234ec

m-i

Last good revision: 5d135736c0a11630113a1e1a74a70b12f9dbf1ec
First bad revision: e7d613b3bcfe1e865378bfac37de64560d1234ec

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5d135736c0a11630113a1e1a74a70b12f9dbf1ec&tochange=e7d613b3bcfe1e865378bfac37de64560d1234ec
(Reporter)

Updated

2 years ago
status-firefox43: --- → affected
(Reporter)

Updated

2 years ago
Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression
(Assignee)

Comment 1

2 years ago
[Tracking Requested - why for this release]: Regression in privacy functionality visually - although the menus get shown on the mac toolbar, there's no actual icons for them so the user doesn't necessarily spot them/know what they are.
tracking-firefox43: --- → ?
Tracking for 43+. Mark can you find an owner for this soon?
tracking-firefox43: ? → +
tracking-firefox44: --- → +
(Assignee)

Comment 3

2 years ago
This is a WebRTC issue... passing to Maire.
Flags: needinfo?(mreavy)
(Assignee)

Comment 4

2 years ago
Actually, I've just confirmed this is a regression from bug 1204182. I can't see what's wrong there though, so ni to Justin.
Blocks: 1204182
Flags: needinfo?(mreavy) → needinfo?(dolske)
Updating category.  This isn't a WebRTC issue, but rather a Firefox::DevicePermissions issue.  FYI: Florian is a good person to ping if anyone needs more help digging into this.
Component: WebRTC → Device Permissions
Product: Core → Firefox
(Assignee)

Comment 6

2 years ago
Ok, I took another look and spotted it, patch coming up.
Flags: needinfo?(dolske)
(Assignee)

Comment 7

2 years ago
Created attachment 8667817 [details] [diff] [review]
webRTC-indicator.css is only shared across Windows and Linux, not Mac as well.

The indicator.css is in the shared theme directory as its used across Windows & Linux. However, on Mac, there's a separate webRTC-indicator.css. The second inclusion of the file doesn't seem to override the first, so this reverts the part of bug 1204182 that altered how *indicator.css were included in the build.
(Assignee)

Updated

2 years ago
Assignee: nobody → standard8
Iteration: --- → 44.1 - Oct 5
Points: --- → 2
We should move webRTC-indicator.css out of shared/ if it's not used everywhere.
(Assignee)

Comment 9

2 years ago
(In reply to Dão Gottwald [:dao] from comment #8)
> We should move webRTC-indicator.css out of shared/ if it's not used
> everywhere.

Given that its used for both Windows & Linux, but not Mac, where would you suggest it is moved to?
Move to windows/ and copy to linux/, i.e. we just duplicate it. I think that's preferable over having to read the jar manifests to figure out which "shared" files are used where.
(In reply to Mark Banner (:standard8) from comment #7)
> Created attachment 8667817 [details] [diff] [review]
> webRTC-indicator.css is only shared across Windows and Linux, not Mac as
> well.
> 
> The indicator.css is in the shared theme directory as its used across
> Windows & Linux. However, on Mac, there's a separate webRTC-indicator.css.
> The second inclusion of the file doesn't seem to override the first, so this
> reverts the part of bug 1204182 that altered how *indicator.css were
> included in the build.

Hmm. In bug 1204182 comment 0 and 3 I did note that this file was only partially shared, and verified that individual themes could still override shared files when they needed to (although I don't recall if I tested this specific override). So either I screwed up the override testing, or some other funky thing is going on here. :( I'll take a look.


(In reply to Dão Gottwald [:dao] from comment #10)
> Move to windows/ and copy to linux/, i.e. we just duplicate it.

I don't care strongly any way, but there are at least 3 approaches and they all have ups and downs...

1) Shared + override: as you note, easy to miss that "shared" might be overridden by something else.

2) Make copies: Duplicating source, which is why we added shared in the first place.

3) Have Linux pull it from ../windows (much like we use ../shared, or how Toolkit's Linux theme is based on Windows). Single source instance, but easy to forget that the thing one is modifying impacts another platform.
Ah, so we're running afoul of this code in _processEntryLine() in /python/mozbuild/mozbuild/jar.py:

...
430         if m.group('optOverwrite') or getModTime(realsrc) \
431             > outHelper.getDestModTime(m.group('output')):
432             if self.outputFormat == 'symlink':
433                 outHelper.symlink(realsrc, out)
434                 return
...

So the mozilla-central/obj/dist/bin/browser/chrome/browser/skin/classic/browser/webRTC-indicator.css symlink seems to be initially created pointed to the shared indicator.css, and the desired "override" line in the OS X jar.mn doesn't actually do anything unless the non-shared CSS has a newer modtime. Either I touched that locally in my testing, or it so happened that the non-shared file had last been modified in the repo.

I think that means we either need to be explicit about overrides in jar.mn (by using "+" as the first character for the line in jar.mn), or make jar.py do the right thing when it detects a duplicate manifest entry.
E.G., I verified the following patch would also fix this issue. (But I'm fine with Dao's suggestion in comment 10, just clarifying what bug 1204182 actually should have done to work correctly...)

diff --git a/browser/themes/osx/jar.mn b/browser/themes/osx/jar.mn
--- a/browser/themes/osx/jar.mn
+++ b/browser/themes/osx/jar.mn
@@ -87,5 +87,5 @@ browser.jar:
   skin/classic/browser/webRTC-sharingScreen-menubar.png
   skin/classic/browser/webRTC-sharingScreen-menubar@2x.png
-  skin/classic/browser/webRTC-indicator.css
++ skin/classic/browser/webRTC-indicator.css
   skin/classic/browser/loop/menuPanel.png             (loop/menuPanel.png)
   skin/classic/browser/loop/menuPanel@2x.png          (loop/menuPanel@2x.png)
(Assignee)

Comment 14

2 years ago
I'm happy with either approach, using the override flag seems to be reasonable for me.
I still prefer not sharing this. It's expexted that when changing e.g. a stylesheet in windows/, you'll need to look into translating that change to other platforms (unless it's explicitly Windows-specific). The file in linux/ currently being identical to the one in windows/ is just a special case of that, but one that isn't a footgun like sharing what isn't consistent across platforms or having linux/ pull the file from Windows/.
(Assignee)

Comment 16

2 years ago
Created attachment 8668397 [details] [diff] [review]
Revised fix

Revised fixed with the suggestions.
Attachment #8667817 - Attachment is obsolete: true
Attachment #8668397 - Flags: review?(dao)
Comment on attachment 8668397 [details] [diff] [review]
Revised fix

Thanks!
Attachment #8668397 - Attachment is patch: true
Attachment #8668397 - Flags: review?(dao) → review+

Comment 18

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/53483ede42c2
https://hg.mozilla.org/mozilla-central/rev/53483ede42c2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
(Assignee)

Comment 20

2 years ago
Comment on attachment 8668397 [details] [diff] [review]
Revised fix

Approval Request Comment
[Feature/regressing bug #]: Bug 1204182
[User impact if declined]: Sharing controls on Mac's menu bar aren't visible. The buttons are there but there are no icons.
[Describe test coverage new/current, TreeHerder]: Landed in m-c
[Risks and why]: Low, fixes build to get the correct css files in the correct places.
[String/UUID change made/needed]: None
Attachment #8668397 - Flags: approval-mozilla-aurora?
Flags: qe-verify+
status-firefox42: --- → unaffected
Comment on attachment 8668397 [details] [diff] [review]
Revised fix

Approved for uplift; we do need the icons!
Attachment #8668397 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 22

2 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/b32c31942ba2
status-firefox43: affected → fixed
(Reporter)

Comment 23

2 years ago
I confirm this is fixed using latest Developer edition 43.0a2 and latest Nightly 44.0a1 on Mac OS X 10.9.5, Mac OS X 10.10.5 and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
status-firefox43: fixed → verified
status-firefox44: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.