[music] Unplugging headphones while playing music should pause it

RESOLVED FIXED in Firefox 18

Status

defect
P1
normal
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: dcoloma, Assigned: slee)

Tracking

({feature})

unspecified
B2G C2 (20nov-10dec)
x86
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox20 fixed)

Details

Attachments

(4 attachments, 10 obsolete attachments)

186 bytes, text/html
slee
: review+
Details
1.40 KB, patch
justin.lebar+bug
: review+
cjones
: feedback+
Details | Diff | Splinter Review
17.35 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
2.10 KB, patch
slee
: review+
Details | Diff | Splinter Review
As a user when I unplug my headphones, the music will pause so I don't have my music playing through the handset speaker inadvertently.

User Story Music-024
Keywords: feature
blocking-basecamp: --- → ?
Priority: -- → P1
blocking-basecamp: ? → +
We're marking this bug with the C1 milestone since it follows the criteria of "unfinished feature work" (see https://etherpad.mozilla.org/b2g-convergence-schedule).

If this work is not finished by Nov19, this bug will need an exception and will be called out at the upcoming Exec Review.
Target Milestone: --- → B2G C1 (to 19nov)
Assignee: nobody → dkuo
blocked by bug #805333.
Depends on: 805333
(In reply to Dominic Kuo [:dkuo] from comment #2)
> blocked by bug #805333.

I think the music App can listen the headset status by itself then stop the music activly.
Then this will not be blocked by bug 805333. Any concern on this?
Right, currently, we can use WebFM api to detect to headset status, by using onantennaavailablechange of mozFM. (antenna = headset on our devices)

But I have discussed with Tim, and he said we shouldn't use the WebFM api to achieve this because not every device supports FM radio, and mozFM may not be available.
I am not sure about this, but if WebFM is not depend on the hardware (chip set?), I think it's okay to use it instead of creating a new api of headset.

What we think might be a possible way is, maybe there will be an event that fires when audio track switches to the headset, and the audio tag will receive it, then Music app can listen to it to do some logic like pausing the song.

So is it possible to do this by using the Policy & Mechanism for Audio you are proposing? or you suggest we just use the WebFM api?
I think that's right -- bug 805333 is going to need to provide some set of APIs that allow you to both query state and set policies.  Marco, i have heard, is thinking about this.  Marco, can you put up a strawman?
The API at https://wiki.mozilla.org/WebAPI/AudioChannels is intended to support this. I wasn't aware that this was needed for v1 though.
Why I proposed to do this by App itself is that status bar already can listen the headphones-status-changed by mozChromeEvent. But I didn't know whether music App can listen this kind of event or not.

Or maybe we can just modify this headphones-status-changed event to broadcast one not mozChromeEvent. (ex: system-messages)
Only the system app can use mozChromeEvent.

System messages aren't appropriate here since system messages wake up an app if it's not running, and we don't want to wake up the music player app any time the headphones are plugged in or unplugged.

But I totally agree that it should be up to the app to pause the music as needed. That's why I added the onheadphoneschange event. The idea is that we don't start playing audio to the new target (headphones when the headphones are plugged in, speaker when the headphones are unplugged) until we've fired this event and the page has had a chance to pause any audio as needed.
The event naming may change to another proper name that user may remove the 
1. headset/headphone 2. a2dp 3. hdmi 4. usb audio and etc....(even those isn't support on v1...)
Don't hesitate to provide other suggestions.

Though it's generally nice to use names which are understandable to authors even if they aren't 100% accurate. So for example we use an event named "click", which is a mouse specific name, even though the event can be fired in response to mouse, touch or keyboard.

But that said, I'm totally fine with coming up with a better name.
Dear Jonas,

Refer to https://wiki.mozilla.org/WebAPI/AudioChannels.
After the discussion for bug 805333, it seems almost of things from AudioChannelManager can be eliminated. But I think we need to keep the headphone status in that API for solving this bug.

So my suggestion is to implement that interface but keep headphones and onheadphoneschange only.
May I know your advice?

Thanks.
Flags: needinfo?(jonas)
Yup, I agree.
Flags: needinfo?(jonas)
Attachment #681354 - Flags: review?(jonas)
Comment on attachment 681354 [details] [diff] [review]
part 1, AudioChannelManager interface

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

Looks great!

::: dom/system/gonk/nsIAudioChannelManager.idl
@@ +10,5 @@
> +   * Indicates the headphones is plugged or not.
> +   */
> +  readonly attribute boolean headphones;
> +
> +  /** 

Nit, whitespace at the end of the line.
Attachment #681354 - Flags: review?(jonas) → review+
Comment on attachment 681915 [details]
Use mozAudioChannelManager to handle headphones plugging states

Looks good~
Attachment #681915 - Flags: review?(slee) → review+
Address comment 14
Attachment #681354 - Attachment is obsolete: true
Attachment #682326 - Flags: review+
Hi Justin,

This is my first time to implement object inherits from nsDOMEventTarget. This patch has a problem that when I new an nsAudioChannelManager and backtrace, gdb told me that the stack may corrupt. I spent much on debugging the bug but I cannot find out where the problem is.

If you are OK, please give me some feedback about this patch. If you are busy, can you suggest someone else? Thanks in advance.
Attachment #682328 - Flags: feedback?(justin.lebar+bug)
(In reply to StevenLee from comment #18)
> Created attachment 682328 [details] [diff] [review]
> Part 2, AudioChannelManager implementation
> 
> Hi Justin,
> 
> This is my first time to implement object inherits from nsDOMEventTarget.
> This patch has a problem that when I new an nsAudioChannelManager and
> backtrace, gdb told me that the stack may corrupt. I spent much on debugging
> the bug but I cannot find out where the problem is.
> 
> If you are OK, please give me some feedback about this patch. If you are
> busy, can you suggest someone else? Thanks in advance.
Hi Justin,

The problem seems not resulted from nsAudioChannelManager. I found that if I don't call RegisterSwitchObserver in the constructor of nsAudioChannelManager, the stack over flow problem does not exist. I will find out where the problem is. Sorry for bothering you.
> I found that if I don't call RegisterSwitchObserver in the constructor of 
> nsAudioChannelManager, the stack over flow problem does not exist. I will find out where 
> the problem is.

In general you have to be very careful about calling methods on |this| during an object's constructor.

During a constructor, an object's refcount is 0.  So suppose you call foo(this) during the constructor, and foo does:

  void foo(SwitchObserver* observer) {
    nsCOMPtr<nsIFoo> obj = do_QueryInterface(observer);
  }

or something like that.  Assigning observer into an nsCOMPtr increases its refcount to 1.  Then when the function exits, its refcount goes to 0, which means the object is deleted!  Then the constructor returns.  Now any use of the object is a use-after-free error.

Anyway, it doesn't look like that's what is happening here.

I'd rather not spend a lot of time giving feedback on a broken patch; can you ask me for review once it works properly?  I'm happy to help you figure out the problem, if you can post more details about it.

Also, nit: The "ns" in class names is for classes which are in the empty namespace.  Since nsAudioChannelManager lives in mozilla::dom::system, it should be called AudioChannelManager.
Attachment #682328 - Flags: feedback?(justin.lebar+bug)
Hi Justin,

Thanks for your comment. I should remove the feedback flag first. I am trying to figure out where the problem is. If my patch is done, I will post another one for review.
Summary: [music] Unplug headphones while playing music should pause it → [music] Unplugging headphones while playing music should pause it
Attachment #682328 - Attachment is obsolete: true
Attachment #682884 - Flags: review?(justin.lebar+bug)
Justin,

I found the crash problem is resulted from that content process cannot call hal::Sandbox::RegisterSwitchObserver. It is modified in bug 776835. I have asked cjones about this. When you try my patch, please apply this one to prevent the crash problem.
Thanks.
Comment on attachment 682884 [details] [diff] [review]
Part 2, AudioChannelManager implementation V2

Looks good with a few nits.  I also need Jonas's input on one point.

>+interface nsIAudioChannelManager : nsIDOMEventTarget {
>+  /**
>+   * Indicates the headphones is plugged or not.

  Indicates /whether/ the headphones /are/ plugged /in/ or not.

("headphones", like "pants" and "scissors", is a plural noun with no real
singular; you can't have "a headphone", "a pant", or "a scissor", and we always
say "the headphones/pants/scissors *are*", not "is".

Except in the previous sentence, it's correct to say "'headphones' *is* a
plural noun" because I'm referring to the word "headphones", not to the
physical object.)

>+  readonly attribute boolean headphones;

>+  /**
>+   * Always fired before audio start playing through the new channel

This should say something like:

  Fired before audio start/s/ playing through /a/ new channel/./  That is, this
  event is fired when the headphones are unplugged, before audio starts playing
  through the speakers, and is fired when the headphones are plugged in, before
  audio starts playing through the headphones.

But I don't see how we can actually make this work, because that claims to
guarantee that the event fires in /all processes/ before the audio switches,
which is definitely not happening.

Maybe we should say something like

  Fired when the headphones are plugged in or unplugged.

  When the headphones are unplugged, we may start playing audio through the
  system's speakers.  Similarly, when headphones are plugged in, we may switch
  audio from speakers to headphones.
  
  If audio is currently playing in this window or in one of its children, we
  will fire this event before we switch the audio output from headphones to
  speakers (or vice versa).  This allows you to, for example, pause your
  window's audio when the headphones are unplugged.

>diff --git a/dom/system/gonk/nsINavigatorAudioChannelManager.idl b/dom/system/gonk/nsINavigatorAudioChannelManager.idl
>new file mode 100644
>--- /dev/null
>+++ b/dom/system/gonk/nsINavigatorAudioChannelManager.idl
>@@ -0,0 +1,13 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "nsISupports.idl"
>+
>+interface nsIAudioChannelManager;
>+
>+[scriptable, uuid(4f94f833-6ab4-4c45-aecc-ebe0e100194c)]
>+interface nsIMozNavigatorAudioChannelManager : nsISupports

Jonas, does this need to be called nsIDOMMozNavigatorAudioChannelmanager?  I
can never remember the rule for when we need these types to be exposed on the
window object.

>+{
>+  readonly attribute nsIAudioChannelManager mozAudioChannelManager;
>+};

> class Navigator : public nsIDOMNavigator
>                 , public nsIDOMClientInformation
>                 , public nsIDOMNavigatorDeviceStorage
>                 , public nsIDOMNavigatorGeolocation
>                 , public nsIDOMNavigatorDesktopNotification
>                 , public nsINavigatorBattery
>                 , public nsIDOMMozNavigatorSms
> #ifdef MOZ_MEDIA_NAVIGATOR
>@@ -100,16 +109,19 @@ class Navigator : public nsIDOMNavigator
> #ifdef MOZ_B2G_BT
>                 , public nsIDOMNavigatorBluetooth
> #endif
>                 , public nsIDOMNavigatorCamera
>                 , public nsIDOMNavigatorSystemMessages
> #ifdef MOZ_TIME_MANAGER
>                 , public nsIDOMMozNavigatorTime
> #endif
>+#ifdef MOZ_B2G
>+                , public nsIMozNavigatorAudioChannelManager
>+#endif

We have a policy of not using ifdef MOZ_B2G for this sort of thing.  You need
to add a new guard, just like we have for the rest of the interfaces here.

>+//*****************************************************************************
>+//    Navigator::nsINavigatorAudioChannelManager
>+//*****************************************************************************
>+NS_IMETHODIMP
>+Navigator::GetMozAudioChannelManager(nsIAudioChannelManager** aAudioChannelManager)
>+{
>+#ifdef MOZ_B2G
>+  *aAudioChannelManager = nullptr;
>+
>+  if (!mAudioChannelManager) {
>+    nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
>+    NS_ENSURE_TRUE(window, NS_OK);
>+    mAudioChannelManager = new system::AudioChannelManager();
>+    mAudioChannelManager->Init(window);
>+  }
>+
>+  NS_ADDREF(*aAudioChannelManager = mAudioChannelManager);
>+  return NS_OK;
>+#else
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+#endif
>+}

It looks like the way we're doing these guards is to not define the method if
the guard is not true.  See e.g. how Navigator::GetMozTime is not defined if
MOZ_TIME_MANAGER is not defined.  I think we should do the same here.

>diff --git a/dom/system/gonk/AudioChannelManager.h b/dom/system/gonk/AudioChannelManager.h
>new file mode 100644
>--- /dev/null
>+++ b/dom/system/gonk/AudioChannelManager.h
>@@ -0,0 +1,46 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef mozilla_dom_system_AudioChannelManager_h
>+#define mozilla_dom_system_AudioChannelManager_h
>+
>+#include "mozilla/HalTypes.h"
>+#include "nsIAudioChannelManager.h"
>+#include "nsDOMEventTargetHelper.h"
>+
>+namespace mozilla {
>+namespace hal {
>+class SwitchEvent;
>+typedef Observer<SwitchEvent> SwitchObserver;
>+} // namespace hal
>+
>+namespace dom {
>+namespace system {
>+
>+class AudioChannelManager : public nsDOMEventTargetHelper
>+                            , public nsIAudioChannelManager
>+                            , public hal::SwitchObserver

Nit: Please line the commas up underneath the colon.

>+{
>+public:
>+  NS_DECL_ISUPPORTS_INHERITED
>+  NS_DECL_NSIAUDIOCHANNELMANAGER
>+  NS_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper::)
>+
>+  AudioChannelManager();
>+
>+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(AudioChannelManager,
>+                                           nsDOMEventTargetHelper)
>+  void Notify(const hal::SwitchEvent& aEvent);
>+
>+  void Init(nsPIDOMWindow* aWindow);
>+private:
>+  ~AudioChannelManager();

I don't think you want a private destructor.  In fact, I think you want a
public, virtual destructor here, because you're inheriting addref/release from
nsDOMEventTargetHelper, and release() is the function which calls delete.

That is to say, if we run nsDOMEventTargetHelper's release() and it ends up
deleting this object, we want to make sure this class's destructor gets run.

http://www.parashift.com/c++-faq/virtual-dtors.html
http://stackoverflow.com/questions/461203/when-to-use-virtual-destructors

Note that ~nsDOMEventTargetHelper() is already virtual (as we'd hope).

>diff --git a/dom/system/gonk/AudioChannelManager.cpp b/dom/system/gonk/AudioChannelManager.cpp
>new file mode 100644
>--- /dev/null
>+++ b/dom/system/gonk/AudioChannelManager.cpp
>@@ -0,0 +1,82 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "mozilla/Hal.h"
>+#include "mozilla/HalTypes.h"
>+#include "AudioChannelManager.h"
>+#include "nsIDOMClassInfo.h"
>+
>+using namespace mozilla::hal;
>+
>+#define HEADPHONESCHANGE NS_LITERAL_STRING("headphoneschange")

Nit: I don't think this define is adding anything; I'd just use the literal
string in the one place where you need it.

>+AudioChannelManager::AudioChannelManager() : mState(SWITCH_STATE_UNKNOWN)

Nit: We usually put the initializer list (": mState(...)") on a new line,
indented two spaces.
Attachment #682884 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 682884 [details] [diff] [review]
Part 2, AudioChannelManager implementation V2

r?sicking re the name of nsIMozNavigatorAudioChannelManager (see previous comment).
Attachment #682884 - Flags: review?(jonas)
Attachment #682885 - Flags: feedback?(jones.chris.g)
Component: Gaia → Gaia::Music
I don't remember either. Simplest solution is to use what you have now and check that window.MozNavigatorAudioChannelManager returns undefined. If it does we are good to go.
Comment on attachment 682885 [details] [diff] [review]
Part 0, RegisterSwitchObserver through hal::Sandbox

Spreading this information around content processes makes me a little nervous wrt maintaining distributed higher-level state.  But there's certainly no harm that would specifically be caused by content processes listening to these, and if jlebar is OK with this approach that's good enough for me.
Attachment #682885 - Flags: feedback?(jones.chris.g) → feedback+
Address comment 24.
Attachment #682326 - Attachment is obsolete: true
Attachment #684319 - Flags: review+
Hi Justin,

Would you please review the patch again? I am afraid that if I missed something.
Thanks.
Attachment #682884 - Attachment is obsolete: true
Attachment #682884 - Flags: review?(jonas)
Attachment #684320 - Flags: review?(justin.lebar+bug)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #27)
> Comment on attachment 682885 [details] [diff] [review]
> Part 0, RegisterSwitchObserver through hal::Sandbox
> 
> Spreading this information around content processes makes me a little
> nervous wrt maintaining distributed higher-level state.  But there's
> certainly no harm that would specifically be caused by content processes
> listening to these, and if jlebar is OK with this approach that's good
> enough for me.
Hi Justin,

How do you think about it? Could I land the patch in this bug?
Attachment #684319 - Attachment is obsolete: true
Attachment #684324 - Flags: review+
Remove some trailing whitespace and fix one preprocessor error.
Attachment #684320 - Attachment is obsolete: true
Attachment #684320 - Flags: review?(justin.lebar+bug)
Attachment #684327 - Flags: review?(justin.lebar+bug)
>diff -U8 b/dom/system/gonk/AudioChannelManager.h b/dom/system/gonk/AudioChannelManager.h
>--- b/dom/system/gonk/AudioChannelManager.h
>+++ b/dom/system/gonk/AudioChannelManager.h
> class AudioChannelManager : public nsDOMEventTargetHelper
>-                            , public nsIAudioChannelManager
>-                            , public hal::SwitchObserver
>+                          , public nsIAudioChannelManager
>+                          , public hal::SwitchObserver
> {
> public:
>   NS_DECL_ISUPPORTS_INHERITED
>   NS_DECL_NSIAUDIOCHANNELMANAGER
>   NS_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper::)
> 
>   AudioChannelManager();
>+  ~AudioChannelManager();

This must be virtual; see comment 24.

I also don't see any changes to the interface comments here.
Attachment #684327 - Flags: review?(justin.lebar+bug)
Assignee: dkuo → slee
(In reply to Justin Lebar [:jlebar] from comment #34)
> This must be virtual; see comment 24.
Sorry for that I misunderstood what you meant. I will update a patch later.
> 
> I also don't see any changes to the interface comments here.
It's addressed in comment 31
> It's addressed in comment 31

Oh, I see.  Sorry.

Given that this IDL file is the only documentation which exists for this web-facing interface, and given that the contract of onheadphoneschange is specifically that it fires /before/ the audio channel switches, so you have a chance to mute the audio, it seems to me that a longer comment, like the one I proposed, would be appropriate.  Is there some reason you think a shorter one is better here?
(In reply to Justin Lebar [:jlebar] from comment #36)
> Given that this IDL file is the only documentation which exists for this
> web-facing interface, and given that the contract of onheadphoneschange is
> specifically that it fires /before/ the audio channel switches, so you have
> a chance to mute the audio, it seems to me that a longer comment, like the
> one I proposed, would be appropriate.  Is there some reason you think a
> shorter one is better here?
I just thought it's clear. But you are right, it's the only documentation that others can know about the interface. We should provide more details. I will update a new version.
Thanks.
Attachment #684327 - Attachment is obsolete: true
Attachment #685447 - Flags: review?(justin.lebar+bug)
Attachment #684324 - Attachment is obsolete: true
Attachment #685448 - Flags: review+
I forgot qrefresh.
Attachment #685448 - Attachment is obsolete: true
Attachment #685450 - Flags: review+
Comment on attachment 685450 [details] [diff] [review]
part 1, AudioChannelManager interface v4

Nit: Please separate paragraphs with an empty line.  (In this case, there's a new paragraph between "headphones." and "If audio".)
Comment on attachment 685447 [details] [diff] [review]
Part 2, AudioChannelManager implementation V4

Thanks, Steven.
Attachment #685447 - Flags: review?(justin.lebar+bug) → review+
Target Milestone: B2G C1 (to 19nov) → B2G C2 (20nov-10dec)
Address comment 41.
Attachment #685450 - Attachment is obsolete: true
Attachment #686027 - Flags: review+
Justin,
Thanks for your comments in this bug. BTW, could we also land "Part 0, RegisterSwitchObserver through hal::Sandbox" in this bug or I should file another bug to land this patch?
> BTW, could we also land "Part 0, RegisterSwitchObserver through hal::Sandbox" in this bug 
> or I should file another bug to land this patch?

I'm fine if you land it as part of this bug.
Blocks: 802677
No longer blocks: 808607
Try server log:
* try: -b do -p all -u none
** https://tbpl.mozilla.org/?tree=Try&rev=1b8efc147f48
*  try: -b d -p linux64 -u all
** https://tbpl.mozilla.org/?tree=Try&rev=565c09158025

Justin,

Could you please help me check in these patches?
Thanks.
> Could you please help me check in these patches?

If you put checkin-needed in the keyword field, someone will probably come around and check these in within a day or so.  If not, I can handle it.
Keywords: checkin-needed
I'm pretty sure that this broke b2g desktop nightlies:
/builds/slave/m-cen-osx64-gecko-ntly/build/dom/base/Navigator.h:19:10: fatal error: 'nsINavigatorAudioChannelManager.h' file not found

https://tbpl.mozilla.org/php/getParsedLog.php?id=17491093&tree=Firefox#error0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ben, did you back this changeset out?

Otherwise, this bug shoudln't be re-opened.
No. Sorry, I'm not sure what the proper bug hygiene is. The builds that are failing because of this are blocking C2 though, so however we track the problem - it needs to be fixed pretty soon.
We use bug status to track whether the relevant csets are currently on m-c, so re-opening bugs which are currently landed on m-c is pretty confusing.

I'll see if I can spin a patch for this, since it's the weekend in Taiwan.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Hi Ben,

I just saw the updated status. Sorry for the inconvenient. 

Hi Justin,

Thanks for solving this defeat. It seems that running "try: -b do -p all -u none" and "try: -b d -p linux64 -u all" are not enough. I will test more platforms in the future.
> It seems that running "try: -b do -p all -u none" and "try: -b d -p linux64 -u all" are 
> not enough.

In fact B2G desktop builds are not run on tryserver, mozilla-inbound, or mozilla-central.  They're only run on a nightly basis.  So there's really no way you would have known.

(If we had these builds on try, then try: -b do -p all -u none would have caught this.)
(In reply to Ed Morley [UTC+0; email:edmorley@moco] from comment #49)
> https://hg.mozilla.org/mozilla-central/rev/9d2db964af92
> https://hg.mozilla.org/mozilla-central/rev/673414bba6c8
> https://hg.mozilla.org/mozilla-central/rev/805961fa3300

Should these changes be merged to the beta branch? AFAIK that is the target branch for B2G v1
> Should these changes be merged to the beta branch?

Yes.  Because this bug is marked as blocking-basecamp+, someone will come through and merge the relevant patches in the next few days.
(In reply to Justin Lebar [:jlebar] from comment #55)
> In fact B2G desktop builds are not run on tryserver, mozilla-inbound, or
> mozilla-central.  They're only run on a nightly basis.  So there's really no
> way you would have known.
> 
> (If we had these builds on try, then try: -b do -p all -u none would have
> caught this.)
Yes, I asked Ed, and he told me that bad news. :(
Will "try: -b do -p all -u none" cost much resources on try server? If not I think I should run this configuration. If it does, I will test desktop build on my PC.
Thanks.
> Will "try: -b do -p all -u none" cost much resources on try server? If not I think I 
> should run this configuration

This is much cheaper than -u all, but it's still not free.

If it was me, I wouldn't spend a lot of time testing that every patch you have builds on B2G desktop.  Unless you test on Windows, Mac, and Linux, debug and opt, you're not going to be able to guarantee that your patch builds everywhere.

But then again, I burn the tree a lot, so maybe you shouldn't take my advice.  :)
(In reply to Justin Lebar [:jlebar] from comment #59)
> > Will "try: -b do -p all -u none" cost much resources on try server? If not I think I 
> > should run this configuration
> 
> This is much cheaper than -u all, but it's still not free.
> 
> If it was me, I wouldn't spend a lot of time testing that every patch you
> have builds on B2G desktop.  Unless you test on Windows, Mac, and Linux,
> debug and opt, you're not going to be able to guarantee that your patch
> builds everywhere.
> 
> But then again, I burn the tree a lot, so maybe you shouldn't take my
> advice.  :)
Hi Justin,

When my code is platform dependent, I will be more careful.

Really thanks for your reply. I feel better. I was guilty about breaking the tree. :(
Component: Gaia::Music → General
So the way I'm reading the comments here, I'm thinking I should hold off on uplifting this for now. Please correct me if that's wrong.
(In reply to Ryan VanderMeulen from comment #61)
> So the way I'm reading the comments here, I'm thinking I should hold off on
> uplifting this for now. Please correct me if that's wrong.

FYI, this was the cset that fixed it.
https://hg.mozilla.org/mozilla-central/rev/d3ad98d05b2a

In the future, posting it to the relevant bug and including the bug # in the commit message is appreciated.
We need this fix in beta
If nobody else beats me to it, I'll get it in tomorrow.
> In the future, posting it to the relevant bug and including the bug # in the commit 
> message is appreciated.

Actually, I checked in [1], which was bug 817071.

I don't know where Fabrice's patch came from...

[1] https://hg.mozilla.org/mozilla-central/5c8ee6600533
Comment on attachment 685447 [details] [diff] [review]
Part 2, AudioChannelManager implementation V4

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

::: dom/system/gonk/AudioChannelManager.cpp
@@ +20,5 @@
> +NS_IMPL_CYCLE_COLLECTION_CLASS(AudioChannelManager)
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(AudioChannelManager,
> +                                                  nsDOMEventTargetHelper)
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

Just as a note, if you don't traverse anything you shouldn't add an inherited cycle collection participant, the base class' will do everything needed. We're removing this again in bug 838172, but please avoid adding this in the future.
This issue is present again in v1.4 (Gecko-7dac743 Gaia-c9416de) on Keon device. The "Music" will play audio file trough phone speakers for <1s (it is noticable).

I'm using regular audio headphones (without microphone).

Regression?
If the final result is the music player paused, then I think the 1s problem should be another issue which we can file a new bug to fix it, would you please file one and probably attach some materials for us to investigate? thanks.
You need to log in before you can comment on or make changes to this bug.