782289 - Headphones status notification

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[:gerard-majax]

We should be able to forward the notification of headphones plugging/unplugging from Gonk to Chrome, so for example we can add an icon in statusbar when the headphones are plugged in.

Comment 1

[:gerard-majax]

Attachment: Generating event from Gonk

A first patch that fires an event which we will later catch to create a mozChromeEvent.

Comment 2

[:gerard-majax]

Attachment: Create mozChromeEvent for headphones

Now the mozChromeEvent creation that will be later catched by statusbar.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 651398 [details] [diff] [review]
Generating event from Gonk

Redirecting the review since I'm not qualified to r? cpp changes in dom/system/gonk/AudioManager.cpp

Comment 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 651398 [details] [diff] [review]
Generating event from Gonk

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

::: dom/system/gonk/AudioManager.cpp
@@ +60,5 @@
> +{
> +  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +  if (obs) {
> +    if (aState == SWITCH_STATE_ON) {
> +      obs->NotifyObservers(nullptr, "headphones-status", NS_LITERAL_STRING("on").get());

Better topic name: "headphones-status-changed". Also, please const all strings (the topic and the three different data strings). #defines at the top of the file are ok.

r=me with that.

Comment 5

[:gerard-majax]

Attachment: Generating event from Gonk

Comment 6

[:gerard-majax]

Attachment: Create mozChromeEvent for headphones

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Why does the r+'d review got obsolete? Did the patch got updated?

I believe the next step should be uploading "Patch for commit" patches with commit messages that confirms the format m-c commits and use the "checkin-needed" keyword.

Comment 8

[Dave Hylands [:dhylands]]

Hey Tim,

Yeah, if you upload a new patch, then it removes the r+. If the r+ just had some nits to address and that's all you did in the new patch, then I normally put the patch as r+ and myself, but when I push the patch I put r=reviewer.

I think that's the correct flow.

Comment 9

[:gerard-majax]

ping?

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to Alexandre LISSY from comment #9)
> ping?

Oops, what happened here? Did we forget to check these patches in? (Alexandre, when you've uploaded final patches for a bug, it's a good idea to set the "checkin-needed" keyword on the bug.)

Anyway, are these patches are good to go? They still apply on m-c.

Comment 11

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67441523f021
https://hg.mozilla.org/integration/mozilla-inbound/rev/f608672b8fdf

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67441523f021
https://hg.mozilla.org/mozilla-central/rev/f608672b8fdf

Comment 13

[:gerard-majax]

I just noticed that it's not the good patches that have been merged ? Looking at https://hg.mozilla.org/mozilla-central/rev/67441523f021 I don't see the changes I did after suggestions of comment 4.

Comment 14

[Dave Hylands [:dhylands]]

I think I fixd it up.

https://hg.mozilla.org/integration/mozilla-inbound/rev/76ef86e0fd8c

Comment 15

[Dave Hylands [:dhylands]]

And the second part here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f996f5815d

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/76ef86e0fd8c
https://hg.mozilla.org/mozilla-central/rev/a8f996f5815d