809106 - [music] Unplugging headphones while playing music should pause it

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Daniel Coloma:dcoloma]

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

Comment 1

[Alex Keybl [:akeybl]]

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.

Comment 2

[Dominic Kuo [:dkuo]]

blocked by bug #805333.

Comment 3

[Marco Chen [:mchen]]

(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?

Comment 4

[Dominic Kuo [:dkuo]]

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?

Comment 5

[Doug Turner (:dougt)]

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?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 7

[Marco Chen [:mchen]]

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)

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 9

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

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...)

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 11

[Marco Chen [:mchen]]

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.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yup, I agree.

Comment 13

[StevenLee[:slee]]

Attachment: part 1, AudioChannelManager interface

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 15

[Dominic Kuo [:dkuo]]

Attachment: Use mozAudioChannelManager to handle headphones plugging states

Comment 16

[StevenLee[:slee]]

Comment on attachment 681915 [details]
Use mozAudioChannelManager to handle headphones plugging states

Looks good~

Comment 17

[StevenLee[:slee]]

Attachment: part 1, AudioChannelManager interface v2

Address comment 14

Comment 18

[StevenLee[:slee]]

Attachment: 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.

Comment 19

[StevenLee[:slee]]

(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.

Comment 20

[Justin Lebar (not reading bugmail)]

> 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.

Comment 21

[StevenLee[:slee]]

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.

Comment 22

[StevenLee[:slee]]

Attachment: Part 2, AudioChannelManager implementation V2

Comment 23

[StevenLee[:slee]]

Attachment: Part 0, RegisterSwitchObserver through hal::Sandbox

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 24

[Justin Lebar (not reading bugmail)]

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.

Comment 25

[Justin Lebar (not reading bugmail)]

Comment on attachment 682884 [details] [diff] [review]
Part 2, AudioChannelManager implementation V2

r?sicking re the name of nsIMozNavigatorAudioChannelManager (see previous comment).

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 28

[StevenLee[:slee]]

Attachment: part 1, AudioChannelManager interface v3

Address comment 24.

Comment 29

[StevenLee[:slee]]

Attachment: Part 2, AudioChannelManager implementation V3

Hi Justin,

Would you please review the patch again? I am afraid that if I missed something.
Thanks.

Comment 30

[StevenLee[:slee]]

(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?

Comment 31

[StevenLee[:slee]]

Attachment: part 1, AudioChannelManager interface v3

Comment 32

[StevenLee[:slee]]

Attachment: Part 2, AudioChannelManager implementation V3

Remove some trailing whitespace and fix one preprocessor error.

Comment 34

[Justin Lebar (not reading bugmail)]

>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.

Comment 35

[StevenLee[: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

Comment 36

[Justin Lebar (not reading bugmail)]

> 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?

Comment 37

[StevenLee[:slee]]

(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.

Comment 38

[StevenLee[:slee]]

Attachment: Part 2, AudioChannelManager implementation V4

Comment 39

[StevenLee[:slee]]

Attachment: part 1, AudioChannelManager interface v4

Comment 40

[StevenLee[:slee]]

Attachment: part 1, AudioChannelManager interface v4

I forgot qrefresh.

Comment 41

[Justin Lebar (not reading bugmail)]

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 42

[Justin Lebar (not reading bugmail)]

Comment on attachment 685447 [details] [diff] [review]
Part 2, AudioChannelManager implementation V4

Thanks, Steven.

Comment 43

[StevenLee[:slee]]

Attachment: part 1, AudioChannelManager interface v5

Address comment 41.

Comment 44

[StevenLee[:slee]]

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?

Comment 45

[Justin Lebar (not reading bugmail)]

> 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.

Comment 46

[StevenLee[:slee]]

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.

Comment 47

[Justin Lebar (not reading bugmail)]

> 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.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d2db964af92
https://hg.mozilla.org/integration/mozilla-inbound/rev/673414bba6c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/805961fa3300

Comment 49

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9d2db964af92
https://hg.mozilla.org/mozilla-central/rev/673414bba6c8
https://hg.mozilla.org/mozilla-central/rev/805961fa3300

Comment 50

[[e-mail] (:bhearsum)]

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

Comment 51

[Justin Lebar (not reading bugmail)]

Ben, did you back this changeset out?

Otherwise, this bug shoudln't be re-opened.

Comment 52

[[e-mail] (:bhearsum)]

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.

Comment 53

[Justin Lebar (not reading bugmail)]

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.

Comment 54

[StevenLee[:slee]]

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.

Comment 55

[Justin Lebar (not reading bugmail)]

> 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.)

Comment 56

[Diego Wilson [:diego]]

(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

Comment 57

[Justin Lebar (not reading bugmail)]

> 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.

Comment 58

[StevenLee[:slee]]

(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.

Comment 59

[Justin Lebar (not reading bugmail)]

> 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.  :)

Comment 60

[StevenLee[:slee]]

(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. :(

Comment 61

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

(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.

Comment 63

[Michael Vines [:m1] [:evilmachines]]

We need this fix in beta

Comment 64

[Ryan VanderMeulen [:RyanVM]]

If nobody else beats me to it, I'll get it in tomorrow.

Comment 65

[Justin Lebar (not reading bugmail)]

> 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 66

[Justin Lebar (not reading bugmail)]

Sorry, that should be

[1] https://hg.mozilla.org/mozilla-central/rev/5c8ee6600533

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/84feb55f2c42
https://hg.mozilla.org/releases/mozilla-aurora/rev/e4b9c8d4e80c
https://hg.mozilla.org/releases/mozilla-aurora/rev/c8075c1b8d5b
https://hg.mozilla.org/releases/mozilla-aurora/rev/b33ece56fc6b

https://hg.mozilla.org/releases/mozilla-beta/rev/25304dbcfa69
https://hg.mozilla.org/releases/mozilla-beta/rev/841c4c4cad81
https://hg.mozilla.org/releases/mozilla-beta/rev/1c202b6c1025
https://hg.mozilla.org/releases/mozilla-beta/rev/6053b51b95a0

Comment 68

[Peter Van der Beken [:peterv]]

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.

Comment 69

[nulti-korisnik]

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?

Comment 70

[Dominic Kuo [:dkuo]]

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.