Closed Bug 1325553 Opened 6 years ago Closed 6 years ago

Sound of "take screenshot" button should respect relevant preference

Categories

(DevTools :: Responsive Design Mode, defect)

53 Branch
defect
Not set
normal

Tracking

(firefox53 fixed)

VERIFIED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: regspam, Assigned: Towkir)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.0; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161208153507

Steps to reproduce:

Currently, if you click the camera icon on the Developer Tools toolbar to take a screenshot of the entire page, Firefox produces a camera clicking like sound; there's no way do disable it.
At the same time, if you switch to Responsive Design Mode and click the camera icon there to take a screenshot, no sound is produced (and there's no way to enable a sound for it).

It would be handy to be able enable/disable the sound for both icons with one preference.
Component: Untriaged → Developer Tools
Severity: normal → enhancement
> Firefox produces a camera clicking like sound; there's no way do disable it.

FWIW there is a pref for this and I see a checkbox for it in the devtools preferences panel.
This was added in bug 1257913.

What I've observed is that the RDM screenshot also makes the sound, but that the pref
doesn't seem to disable it.  That's because the RDM has its own implementation of
simulateCameraEffects that doesn't query the pref:

https://dxr.mozilla.org/mozilla-central/rev/2785aaf276ba29fb2e1f5607d90d441fee42efb4/devtools/client/responsive.html/actions/screenshot.js#57
Keywords: good-first-bug
https://dxr.mozilla.org/mozilla-central/rev/2785aaf276ba29fb2e1f5607d90d441fee42efb4/devtools/shared/gcli/commands/screenshot.js#190

simulateCameraEffect(context.environment.chromeDocument, "shutter");
Severity: enhancement → normal
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Ever confirmed: true
Summary: Implement a preference for enabling/disabling the "take screenshot" sound → Sound of "take screenshot" button should respect relevant preference
Version: 50 Branch → 53 Branch
I could take this one if anyone is interested in mentoring.
Flags: needinfo?(yfdyh000)
Flags: needinfo?(ttromey)
I'm not a mentor.
Flags: needinfo?(yfdyh000)
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(ttromey)
Resolution: --- → DUPLICATE
Duplicate of bug: 1257913
Missed comment 1
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(In reply to [:Towkir] Ahmed from comment #3)
> I could take this one if anyone is interested in mentoring.

On top of the file [0], you'll need to add `const Services = require("Services");`: this is necessary because the function you need to check the preference is on Services.

Then you'll need to wrap these 2 lines [1] with an if condition: `if (Services.prefs.getBoolPref("devtools.screenshot.audio.enabled")) {`

This makes sure the audio is only emitted when the pref is enabled. If you're interested in how the pref system works, see [2].

Towkir, let me know if you need extra info :)


[0] https://dxr.mozilla.org/mozilla-central/rev/2785aaf276ba29fb2e1f5607d90d441fee42efb4/devtools/client/responsive.html/actions/screenshot.js#57

[1] https://dxr.mozilla.org/mozilla-central/rev/2785aaf276ba29fb2e1f5607d90d441fee42efb4/devtools/client/responsive.html/actions/screenshot.js#57-58

[2] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPrefBranch#getBoolPref()
Flags: needinfo?(3ugzilla)
Attached patch screenshotsound.patch (obsolete) — Splinter Review
Hi Tim,
Hope this helps
Assignee: nobody → 3ugzilla
Status: REOPENED → ASSIGNED
Flags: needinfo?(3ugzilla)
Attachment #8822201 - Flags: review?(ntim.bugs)
Comment on attachment 8822201 [details] [diff] [review]
screenshotsound.patch

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

Works great, thanks!

::: devtools/client/responsive.html/actions/screenshot.js
@@ +17,4 @@
>  const e10s = require("../utils/e10s");
>  
>  const CAMERA_AUDIO_URL = "resource://devtools/client/themes/audio/shutter.wav";
> +const Services = require("Services");

Can you move this require just after const e10s = require("...")
Attachment #8822201 - Flags: review?(ntim.bugs) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Missed the suggestion first time. Sorry about that.
Attachment #8822201 - Attachment is obsolete: true
Attachment #8822207 - Flags: review+
Hope it's alright to check-in now
Keywords: checkin-needed
Component: Developer Tools → Developer Tools: Responsive Design Mode
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47ead489b52e
Sound of "take screenshot" button respects relevant preference now; r=ntim
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/47ead489b52e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
This bug  was about  to " add a preference to enable/disable the sound when click camera icon on the Developer Tools toolbar to take a screenshot "  I have seen the feature being implemented with latest Nightly  on Windows 10 , 64 Bit !

This bug's fix is now verified in Latest Nightly .

Build ID   :  20170103030204
User Agent : Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0

[bugday-20170104]
Status: RESOLVED → VERIFIED
yes its working  https://www.wisdomjobs.com
thanks for your support
really its working.

<a href ="https://www.vacancyopen.com/"/>Thanks for posting.</a>
Its working https://www.vacancyopen.com/

Thanks for your support
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.