Closed Bug 1279533 Opened 8 years ago Closed 8 years ago

Use SVG icons for the global sharing indicator on Windows/Linux

Categories

(Firefox :: Site Permissions, defect, P3)

Product:
Firefox
Component:
Site Permissions
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: florian, Assigned: sajarvis, Mentored)

References

Details

(Whiteboard: [fxprivacy][good first bug])

Attachments

(2 files, 1 obsolete file)

Bug 1279533 - Use SVG sharing icons on Linux and Windows.
58 bytes, text/x-review-board-request
florian
: review-
Details
Bug 1279533 - Color the active sharing indicators white.
58 bytes, text/x-review-board-request
florian
: review+
Details 
Bug 1274480 added glyphs for the camera, microphone and screen icons in chrome://browser/skin/glyphs.svg

I think we can remove the remaining files in https://hg.mozilla.org/integration/fx-team/file/a1558d048e76/browser/themes/shared/webrtc and replace them with the SVG version.
Whiteboard: [fxprivacy][triage]
Priority: -- → P3
Whiteboard: [fxprivacy][triage] → [fxprivacy]
This seems like a mentored bug or even a good first bug, right?
Mentor: florian
The file that needs to be edited is http://searchfox.org/mozilla-central/source/browser/themes/windows/webRTC-indicator.css

It would be a good idea to merge the windows and linux versions of this file into a single file in the browser/themes/shared/ folder.
Whiteboard: [fxprivacy] → [fxprivacy][good first bug]
How can I reproduce and where to view the result of the css files changed by me.
You can only reproduce this on Windows or Linux, the UI is different on Mac.

To make the global sharing indicator appear, you need to use a page that will request access to the camera/microphone/screen, eg. https://mozilla.github.io/webrtc-landing/gum_test.html
So basically what i need to do is replace these files with their svg version.And delete the two files one from windows folder and another from linux folder and put that file in shared folder with the reference changed to svg version.
Am i right florian?
Thanks
(In reply to rgargrajatgarg from comment #5)
> So basically what i need to do is replace these files with their svg
> version.And delete the two files one from windows folder and another from
> linux folder and put that file in shared folder with the reference changed
> to svg version.
> Am i right florian?

That's right :-).
Comment on attachment 8798552 [details]
Bug 1279533 Replaced the files in browser/themes/shared/webrtc with their svg versions and merged linux and windows webRTC-indicator.css in shared folder changing the reference to svg files.

https://reviewboard.mozilla.org/r/83994/#review82576

Using SVG, we don't need 2 different files for camera-white-16 and camera-white-16@2x, an SVG icon can scale gracefully. We also no longer need a different file to show the icon in a different color.

See for example http://searchfox.org/mozilla-central/rev/76609a05d6ef7ba4223ed79e479c73fb2543a107/browser/themes/shared/notification-icons.svg#61 used at http://searchfox.org/mozilla-central/rev/76609a05d6ef7ba4223ed79e479c73fb2543a107/browser/themes/shared/notification-icons.inc.css#122

The bootstrap.py and patchfile files seem to have been added by accident here.
Attachment #8798552 - Flags: review?(florian)
Comment on attachment 8798552 [details]
Bug 1279533 Replaced the files in browser/themes/shared/webrtc with their svg versions and merged linux and windows webRTC-indicator.css in shared folder changing the reference to svg files.

https://reviewboard.mozilla.org/r/83994/#review82586

I'm not sure if my previous comment made this clear, but the patch should not create new svg files.
Attachment #8798552 - Flags: review?(florian)
Comment on attachment 8798552 [details]
Bug 1279533 Replaced the files in browser/themes/shared/webrtc with their svg versions and merged linux and windows webRTC-indicator.css in shared folder changing the reference to svg files.

https://reviewboard.mozilla.org/r/83994/#review82594

When removing chrome files, you need to stop packaging them. The package manifest for the files you removed is at http://searchfox.org/mozilla-central/rev/76609a05d6ef7ba4223ed79e479c73fb2543a107/browser/themes/shared/jar.inc.mn#133

::: browser/themes/shared/webRTC-indicator.css:32
(Diff revision 3)
> +#firefoxButton:hover {
> +  background-color: #f2f2f2;
> +}
> +
> +#screenShareButton {
> +  background-image: url("webRTC-screen-16.svg");

The url here should be something like chrome://browser/skin/notification-icons.svg#screen-sharing

We shouldn't create new svg files here. You may need to add a few lines to notification-icons.svg though.
Attachment #8798552 - Flags: review?(florian)
Attachment #8814446 - Flags: review?(florian)
Hey all, first time here but think I navigated the wikis all right and came up with a patch for this. Let me know if I missed anything (technical or otherwise). Happy to be here! Thanks.

The patch was tested on Linux. My understanding is the "try server" will build cross-platform, is that right?
Comment on attachment 8814446 [details]
Bug 1279533 - Use SVG sharing icons on Linux and Windows.

https://reviewboard.mozilla.org/r/95676/#review95724

Thanks, these changes look good. One last thing to handle: we want to keep the white color of these icons (the current patch turns them into red, which isn't very visible above an orange background). You can do that in notification-icons.svg, the same way the #*-sharing icons are colored. Maybe use camera-indicator, microphone-indicator, etc... for the ids.
Attachment #8814446 - Flags: review?(florian) → review-
(In reply to Steve Jarvis [:sajarvis] from comment #14)
> Hey all, first time here but think I navigated the wikis all right and came
> up with a patch for this.

Welcome here, great start for a first patch! :-)

> The patch was tested on Linux. My understanding is the "try server" will
> build cross-platform, is that right?

Our build infrastructure will build on all platforms if we push to the try server or once we check in the patch. The try server won't be very useful in this case because automated tests don't cover the appearance of these icons.
Assignee: nobody → sajarvis
Attachment #8798552 - Attachment is obsolete: true
Attachment #8814525 - Flags: review?(florian)
Comment on attachment 8814446 [details]
Bug 1279533 - Use SVG sharing icons on Linux and Windows.

https://reviewboard.mozilla.org/r/95674/#review95760

::: browser/themes/shared/notification-icons.svg:45
(Diff revision 2)
>      }
> +
> +    #camera-indicator,
> +    #microphone-indicator,
> +    #screen-indicator {
> +      fill: white;

I know the #\*-sharing color is defined with RGB, but the included file (icon-colors.inc.svg) uses 'black' and 'white' for fill definitions, so it defining white here seemed most consistent.
Comment on attachment 8814525 [details]
Bug 1279533 - Color the active sharing indicators white.

https://reviewboard.mozilla.org/r/95736/#review95932

Looks good and works well, thanks! :-)
Attachment #8814525 - Flags: review?(florian) → review+
Congratulations for your first bug fixed! If you would be interested in other bugs about removing files we no longer need, you may want to have a look at the list of bugs marked as dependencies of bug 1316187. For example bug 1319762.
Blocks: 1316187
backed this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=670ad168fb708a7eca83480419f6206833b2566e because of continued test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=39930510&repo=mozilla-inbound

maybe this need a try run to find out what was going wrong
Flags: needinfo?(sajarvis)
(In reply to Carsten Book [:Tomcat] from comment #23)
> backed this out in
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=670ad168fb708a7eca83480419f6206833b2566e because of
> continued test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=39930510&repo=mozilla-
> inbound
> 
> maybe this need a try run to find out what was going wrong
 
Judging by the #developers IRC there were a handful of changes backed out. The linked failure is a network timeout error, and I can't imagine how this chageset might have caused that. 

I don't yet have access to the try server, I was planning on requesting that once this bug is resolved. Florian, how should I proceed?
Flags: needinfo?(sajarvis) → needinfo?(florian)
The failures are related to bug 1319854, I'll reland soon, don't worry the failures aren't related to your patch :-).
Flags: needinfo?(florian)
https://hg.mozilla.org/mozilla-central/rev/7636e8dc84d6
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: