Closed Bug 782289 Opened 8 years ago Closed 8 years ago

Headphones status notification

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gerard-majax, Assigned: gerard-majax)

Details

Attachments

(2 files, 2 obsolete files)

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.
Attached patch Generating event from Gonk (obsolete) — Splinter Review
A first patch that fires an event which we will later catch to create a mozChromeEvent.
Now the mozChromeEvent creation that will be later catched by statusbar.
Assignee: nobody → lissyx+mozillians
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
Attachment #651398 - Flags: review?(21) → review?(philipp)
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.
Attachment #651398 - Flags: review?(philipp) → review+
Attachment #651398 - Attachment is obsolete: true
Attachment #651399 - Attachment is obsolete: true
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.
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.
ping?
(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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/76ef86e0fd8c
https://hg.mozilla.org/mozilla-central/rev/a8f996f5815d
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.