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

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect)

Description

[Bogdan Maris, Desktop Test Engineering]

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

Comment 1

[Mark Banner (:standard8)]

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

Comment 2

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

Tracking for 43+. Mark can you find an owner for this soon?

Comment 3

[Mark Banner (:standard8)]

This is a WebRTC issue... passing to Maire.

Comment 4

[Mark Banner (:standard8)]

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.

Comment 5

[Maire Reavy [:mreavy]]

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.

Comment 6

[Mark Banner (:standard8)]

Ok, I took another look and spotted it, patch coming up.

Comment 7

[Mark Banner (:standard8)]

Attachment: 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.

Comment 8

[Dão Gottwald [:dao]]

We should move webRTC-indicator.css out of shared/ if it's not used everywhere.

Comment 9

[Mark Banner (:standard8)]

(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?

Comment 10

[Dão Gottwald [:dao]]

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.

Comment 11

[Justin Dolske [:Dolske]]

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

Comment 12

[Justin Dolske [:Dolske]]

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.

Comment 13

[Justin Dolske [:Dolske]]

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/[e-mail]
-  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/[e-mail]          (loop/[e-mail])

Comment 14

[Mark Banner (:standard8)]

I'm happy with either approach, using the override flag seems to be reasonable for me.

Comment 15

[Dão Gottwald [:dao]]

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

Comment 16

[Mark Banner (:standard8)]

Attachment: Revised fix

Revised fixed with the suggestions.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8668397 [details] [diff] [review]
Revised fix

Thanks!

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/53483ede42c2

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/53483ede42c2

Comment 20

[Mark Banner (:standard8)]

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

Comment 21

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

Comment on attachment 8668397 [details] [diff] [review]
Revised fix

Approved for uplift; we do need the icons!

Comment 22

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b32c31942ba2

Comment 23

[Bogdan Maris, Desktop Test Engineering]

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.