Closed Bug 1208020 Opened 5 years ago Closed 5 years ago
[Mac only] gum sharing icons not shown in mac toolbar when making a call
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
Has Regression Range: --- → yes
Has STR: --- → yes
[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 for 43+. Mark can you find an owner for this soon?
This is a WebRTC issue... passing to Maire.
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.
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
Ok, I took another look and spotted it, patch coming up.
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: nobody → standard8
Iteration: --- → 44.1 - Oct 5
Points: --- → 2
We should move webRTC-indicator.css out of shared/ if it's not used everywhere.
(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-sharingScreenemail@example.com - 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)
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/.
Revised fixed with the suggestions.
Comment on attachment 8668397 [details] [diff] [review] Revised fix Thanks!
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?
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+
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.
You need to log in before you can comment on or make changes to this bug.