Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: slee, Assigned: slee)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 10 obsolete attachments)

B2G cannot detect insertion of the headset. It should detect it and route the audio correctly when headset(wired or Bluetooth).
Assignee: nobody → slee
Posted patch Added netlink poller (obsolete) — Splinter Review
Attachment #609272 - Flags: review?(jones.chris.g)
Posted patch Audio routing support (obsolete) — Splinter Review
Attachment #609273 - Flags: review?(jones.chris.g)
Posted patch Audio routing support - V2 (obsolete) — Splinter Review
Fixed the problem that AudioManager always routes to speaker in the constructor. It should get the current routing path from hal.
Attachment #609273 - Attachment is obsolete: true
Attachment #609273 - Flags: review?(jones.chris.g)
Attachment #609642 - Flags: review?(jones.chris.g)
Comment on attachment 609272 [details] [diff] [review]
Added netlink poller

Hi Steven, we already have code to poll uevents in GonkHal.cpp.  I don't want to maintain two different systems.  Can you use the existing code for the events you're polling here?  Something like

      if (strstr(buf, "battery"))  // ugh, btw
        NS_DispatchToMainThread(mUpdater);
      if (netlinkEvent.parse(buf, sz))
        //...

If there's a reason we can't or shouldn't do that please let me know.
Attachment #609272 - Flags: review?(jones.chris.g)
Hi, Chris,

We currently see the need for uevent in battery status update and audio switch. But there are potentially more uses for uevents such as usb hotplug and other hardware changes. A more generic uevent poller framework including listener registration and uevent content parsing can simplify future development. Another difference between the implementation in battery updater and the new one is we choose to use message loop for IO to reduce the need for another dedicated thread. Uevent is not very heavy traffic and dedicating a thread for it may be a little too much.

If you agree with the above rationale, then we will then refactor the battery updater using the new uevent implementation so there is no need for maintaining 2. Thanks.
That's ok by me.  I prefer the current implementation a bit because it's simpler, and the thread created doesn't add much overhead, but this new implementation does fix a bug that exists in the current one (not setting CLOEXEC).

My strongest opinion is on not having two separate systems.
I'll mention that we need uevent switch events for detecting USB cable insertion removal as well.
Just to make sure everyone is on the same page, the next steps here are to
 - figure out how to switch the battery status poller over the new proposed interface.  File a followup bug for that work, including removing the current polling code.
 - repost a part 1 that can support the current battery code

I really want to get this landed :).
Patches are attached to bug 742226 for the uevent poller implementation and refactoring of battery updater. Please ignore the 1st patch of this bug
Does the patch here work with the new machinery in bug 742226?
This patch should be rebased. Should I update a new version?
I think the patch will probably need to be updated again after the API changes in 742226.  It's OK to wait until that work is done.
Posted patch Audio routing support - V3 (obsolete) — Splinter Review
Attachment #609272 - Attachment is obsolete: true
Attachment #609642 - Attachment is obsolete: true
Attachment #609642 - Flags: review?(jones.chris.g)
Posted patch Audio routing support - V4 (obsolete) — Splinter Review
Changed the function names.
Attachment #613519 - Attachment is obsolete: true
Attachment #613526 - Flags: review?(jones.chris.g)
Comment on attachment 613526 [details] [diff] [review]
Audio routing support - V4

>diff --git a/dom/system/gonk/AudioManager.cpp b/dom/system/gonk/AudioManager.cpp

It would be better for this code to move into a separate patch, so
that it's easier to separate from the low-level mechanism we're
adding.

The code looks OK though! :)
 
>+template <class InfoType>
>+class NamedObserverList : public ObserverList<InfoType>
>+{
>+public:
>+  nsString mName;
>+};
>+

We don't need to add this if we move to an enum-based SwitchDevice.
I'll hold off on detailed review of this code until the next version.

>diff --git a/hal/Hal.h b/hal/Hal.h

>+void RegisterSwitchObserver(const nsString &aName, hal::SwitchObserver *aSwitchObserver);

Instead of using a string for this, let's use an enum of "known" devices.  For example

  enum SwitchDevice {
    HEADPHONES,
    ...
  };

There are three benefits to doing this

 - normalize the device names coming out of the kernel, in a single
   location.  This lets us deal with quirks from different drivers in
   one place, instead of spreading that logic around.  We could do
   that with strings too, but

 - "atomizes" the strings, so that comparing two device types can be
   one integer comparison instead of a strcmp.  This interface won't
   be perf critical but atomizing strings is a good habit to pick up
   :).

 - makes it easier to catch errors in switch statements etc., since
   the entire set of devices is known at compile time.

>+void EnableSwitchNotifications(const nsString &aName);
>+void DisableSwitchNotifications(const nsString &aName);

These two functions belong in HalInternal.h.

>+/**
>+ * Get current switch information by name.
>+ */
>+void GetCurrentSwitchEvent(const nsString &aName, hal::SwitchEvent *aEvent);

Please call this |GetCurrentSwitchState()|, and "return" a
|hal::SwitchState| (that is, use that type for the out-parameter).

>diff --git a/hal/HalSwitch.h b/hal/HalSwitch.h
>new file mode 100644
>--- /dev/null
>+++ b/hal/HalSwitch.h

All of the code in this file would fit well in HalTypes.h.  No need to
create a new file.

>+enum SwitchStatus {
>+  SWITCH_ON = 1,
>+  SWITCH_OFF = 2

We also need a "SWITCH_UNKNOWN" state.

Any reason to explicitly number these starting from "1"?  If so,
please add a comment explaining why, and if not, let's just use
default numbering for now.

You need a SWITCH_SENTINEL here too, see below.

>+namespace IPC {
>+  /**
>+   * Serializer for SwitchStatus
>+   */

Make sure your editor is set up to not indent code within |namespace *
{| blocks.

>+  template <>
>+  struct ParamTraits<mozilla::hal::SwitchStatus>:
>+    public EnumSerializer<mozilla::hal::SwitchStatus,
>+                          mozilla::hal::SWITCH_ON,
>+                          mozilla::hal::SWITCH_OFF> {

The third template parameter here is a "sentinel" value, the first
value that's not legal for the enum.  So this code isn't quite
correct; needs to be

    public EnumSerializer<mozilla::hal::SwitchStatus,
                          mozilla::hal::SWITCH_ON,
                          mozilla::hal::SWITCH_SENTINEL> {

>diff --git a/hal/fallback/FallbackSwitch.cpp b/hal/fallback/FallbackSwitch.cpp

>+namespace mozilla {
>+  namespace hal_impl {
>+
>+    void
>+      EnableSwitchNotifications(const nsAString &aName) {
>+      }
>+

Nit: make sure these function definitions aren't indented, and use the
formatting

void
EnableSwitchNotifications(const nsAString &aName)
{
}

>diff --git a/hal/gonk/GonkSwitch.cpp b/hal/gonk/GonkSwitch.cpp

>+#include "base/message_loop.h"
>+#include "Hal.h"
>+#include "HalSwitch.h"
>+#include "nsXULAppAPI.h"
>+#include "UeventPoller.h"
>+#include "sysutils/NetlinkEvent.h"
>+#include <android/log.h>
>+

Nit: please include these headers like this

#include <android/log.h>
#include <sysutils/NetlinkEvent.h>

#include "base/message_loop.h"

#include "Hal.h"
#include "UeventPoller.h"

(You shouldn't need to include the headers I didn't list.)

>+class SwitchEventObserver : public IUeventObserver

Moving to an enum-based "SwitchDevice" instead of a string is going to
change/simplify the following code quite a bit.  I'll hold off
detailed review for the next version.

But, it looks pretty good!
Attachment #613526 - Flags: review?(jones.chris.g)
Blocks: 737153
Posted patch Audio routing support - V5 (obsolete) — Splinter Review
Attachment #613526 - Attachment is obsolete: true
Attachment #614265 - Flags: review?(jones.chris.g)
Attachment #614266 - Flags: review?(jones.chris.g)
Comment on attachment 614265 [details] [diff] [review]
Audio routing support - V5

>diff --git a/hal/Hal.cpp b/hal/Hal.cpp

>+static SwitchObserverList *gSwitchObserverLists = NULL;
>+

Nit: this should be |sSwitchObserverLists|, because it's file-static.

>+static SwitchObserverList&
>+GetSwitchObserverList(hal::SwitchDevice aDevice) {
>+  MOZ_ASSERT(aDevice < NUM_SWITCH_DEVICE); 

MOZ_ASSERT(0 <= aDevice && aDevice < NUM_SWITCH_DEVICE);

>+static void
>+ReleaseObserversIfNeeded() {
>+  //The length of every list is 0, no observer in the list.
>+  delete [] gSwitchObserverLists;

Need to null out sSwitchObserverLists after it's deleted ... this code
right now is a security vulnerability.  Whups! :)

>+void
>+NotifySwitchChange(const hal::SwitchEvent& aEvent)
>+{
>+  SwitchObserverList& observer = GetSwitchObserverList(aEvent.device());

It's possible for a DisableSwitchNotifications() call to race with a
NotifySwitchChange() incoming message, and result in this function
being called when there are no observers.  So we need to null-check
sSwitchObserverLists here, just before this statement.

>diff --git a/hal/gonk/GonkSwitch.cpp b/hal/gonk/GonkSwitch.cpp

We're going to need code to read the state of switch devices at any
point in time.  This will be important on startup, for example: the
USB mass storage manager needs to know whether a USB cable is plugged
in.  There might never be a uevent notifying a state change (or we
might miss it due to a race condition on startup), so our code will
never know that a USB cable is plugged in.  (And similarly for
headphones.)  This code has to exist in android, so we can borrow from
there.  Please file a follow up bug for this.

Let's reorganize this code a little bit.  The responsibilities here
are the following

 main thread:
  - maintains current state of all switches
  - when an observer registers, makes sure the uevent watcher on the
    IO thread is active
  - when an observer unregisters, tells the uevent watcher in case the
    watcher wants to shut down

 IO thread:
  - when a uevent arrives that's recognized as a switch-device state
    change, notifies the main thread

I'd like to see the code separated here in this file, so that it's
clear what runs on the main thread and what state is accessed by it,
and what runs on the IO thread and what state is accessed by that
thread.  Something like

//-----------------------------------------------------------------------------
// Main thread
static size_t sObserverCount;
static SwitchState sSwitchState[NUM_SWITCHES];

// on Register(), when the sObserverCount goes from 0->1, post a task
// to the IO thread notifying it of that

// on Remove(), when the sObserverCount goes from 1->0, post a task to
// the IO thread notifying it of that.  Also, reset all the
// sSwitchStates to UNKNOWN.

// when the IO thread posts a notification of switch change to here,
// check if sObserverCount > 0, and if so, update sSwitchState and then
// send the hal:: broadcast.

//-----------------------------------------------------------------------------
// IO thread
static const kSwitchDeviceNameMap = { ... //see below

// when the main thread notifies of switch listeners, ensure the
// uevent watcher is enabled

// when the main thread notifies of *no more* switch listeners, ensure
// the uevent listener is *disabled*

// (filtering uevents, notifying main thread etc.)

This code all exists below already, but I would prefer to see it
reorganized for clarity :).

>+// Define the switch names here
>+#define HEAD_PHONE_SWITCH NS_LITERAL_STRING("h2w")
>+
>+class SwitchMap
>+{
>+public:
>+  SwitchMap(const nsString &aName, SwitchDevice aDevice): mName(aName), mDevice(aDevice) {}
>+  
>+  nsString mName;
>+  SwitchDevice mDevice;
>+};
>+
>+static const SwitchMap gSwitchMap [NUM_SWITCH_DEVICE] = {
>+  SwitchMap(HEAD_PHONE_SWITCH, HEADPHONES)// Headphone switch
>+};
>+

Let's do something a little simpler here:

struct { const char* name; SwitchDevice device; } kSwitchNameMap[] = {
  { "h2w", SWITCH_HEADPHONES },
  { NULL, SWITCH_UNKNOWN },
};

static SwitchDevice NameToDevice(const char* aName)
{
  // do lookup here
}

>+  void EnableSwitch(SwitchDevice aDevice) {

For now, we don't need to track whether particular devices are being
listened for ... just whether there's a watcher of *any* device.
Tracking listeners at a coarser grain will simplify this code a fair
amount.

>+    for (int i = 0; i < NUM_SWITCH_DEVICE; i++) {
>+      if (gSwitchMap[i].mName.EqualsASCII(name, strlen(name))) {

Let's use the NameToDevice() helper here, so that we can write
something like

  SwitchDevice device = NameToDevice(name);
  if (SWITCH_UNKNOWN == device)
    continue;

  sSwitchState[device] = // ...;
  DispatchToMainThread(...);

>+SwitchEventObserver gSwitchObserver;
>+

Make this file static: |static SwitchEventObserver sSwitchObserver;|.

This needs to be a pointer, allocated on demand.  We don't want to run
nontrivial C++ constructors on startup.

OK!  Almost there :).  I have to mark r- because of the security
vulnerability above, but this is close to ready to land.
Attachment #614265 - Flags: review?(jones.chris.g) → review-
Comment on attachment 614266 [details] [diff] [review]
AudioManager supports routing audio by SwitchObserver

>diff --git a/dom/system/gonk/AudioManager.cpp b/dom/system/gonk/AudioManager.cpp

>+#include "mozilla/Hal.h"
> #include "AudioManager.h"
> #include "gonk/AudioSystem.h"

Nit: please order these as follows (alphabetically)

#include "AudioManager.h"
#include "gonk/AudioSystem.h"
#include "mozilla/Hal.h"

>+static AudioSystem::audio_devices
>+GetRoutingMode(int aType) {
>+  if (aType == nsIAudioManager::FORCE_SPEAKER) {
>+    return AudioSystem::DEVICE_OUT_SPEAKER;
>+  } else if (aType == nsIAudioManager::FORCE_HEADPHONES) {
>+    return AudioSystem::DEVICE_OUT_WIRED_HEADSET;
>+  } else if (aType == nsIAudioManager::FORCE_BT_SCO) {
>+    return AudioSystem::DEVICE_OUT_BLUETOOTH_SCO;
>+  } else if (aType == nsIAudioManager::FORCE_BT_A2DP) {
>+    return AudioSystem::DEVICE_OUT_BLUETOOTH_A2DP;
>+  } else {
>+    return AudioSystem::DEVICE_IN_DEFAULT;
>+  }
>+}
>+
>+void InternalSetAudioRoutes(SwitchState aState)

|static void| please.

>+class AudioSwitchObserver : public SwitchObserver

Please call this "HeadphoneSwitchObserver", since that's what it's
watching now.  If we generalize this code, we can change the name.

>diff --git a/dom/system/gonk/AudioManager.h b/dom/system/gonk/AudioManager.h

> namespace mozilla {
>+namespace hal {
>+  class SwitchEvent;
>+  typedef Observer<SwitchEvent> SwitchObserver;
>+} // namespace hal
>+

Don't indent inside namespace {}, please :).

r=me with those changes.
Attachment #614266 - Flags: review?(jones.chris.g) → review+
Posted patch Audio routing support - V6 (obsolete) — Splinter Review
Attachment #614265 - Attachment is obsolete: true
Attachment #614674 - Flags: review?(jones.chris.g)
1. Fixed the problems addressed by Chris
2. For the include sequence, Hal.h needs to be prior than others. So that the current sequence is 
#include "mozilla/Hal.h"
#include "AudioManager.h"
#include "gonk/AudioSystem.h"
Attachment #614266 - Attachment is obsolete: true
Attachment #614675 - Flags: review+
Depends on: 745078
(In reply to StevenLee from comment #21)
> Created attachment 614675 [details] [diff] [review]
> 2. For the include sequence, Hal.h needs to be prior than others. So that
> the current sequence is 
> #include "mozilla/Hal.h"
> #include "AudioManager.h"
> #include "gonk/AudioSystem.h"

That's bad ... what happens when Hal.h isn't first?
Comment on attachment 614674 [details] [diff] [review]
Audio routing support - V6

>diff --git a/hal/gonk/GonkSwitch.cpp b/hal/gonk/GonkSwitch.cpp

>+static SwitchDevice
>+NameToDevice(const char *name) {
>+  for (int i = 0; kSwitchNameMap[i].device != SWITCH_DEVICE_UNKNOWN; i++) {
>+    if (strcmp(name, kSwitchNameMap[i].name) == 0)

Nit: add braces around this if-stmt: if (...) {\n ...\n}

>+      return kSwitchNameMap[i].device;
>+  }
>+

Nit: please remove this newline.

>+class SwitchEventObserver : public IUeventObserver

>+  void Notify(const NetlinkEvent &event) {
>+    const char* name;
>+    const char* state;
>+   
>+    if (ShouldSkip(const_cast<NetlinkEvent*>(&event), &name, &state)) {

Why do you need to const_cast this value?  Can you change ShouldSkip()
so that you don't need to do that?

>+      return;
>+    }
>+
>+    for (int i = 0; i < NUM_SWITCH_DEVICE; i++) {
>+      if (NameToDevice(name) == SWITCH_DEVICE_UNKNOWN)
>+        continue;
>+
>+      // Update all switches in the table, even they are not registered.
>+      // Apps can get the latest information if they need in the future.
>+      mEventInfo[i].mEvent.status() = atoi(state) == 0 ? SWITCH_STATE_OFF : SWITCH_STATE_ON;

This code doesn't make sense to me ... if we get a switch event for
the HEADPHONES device, isn't this code going to change *all* device
states to whatever the latest HEADPHONES state is?  For example, if we
had HEADPHONES and USB_PLUG devices, and we saw HEADPHONES == ON,
wouldn't this code change USB_PLUG = ON too?  That's not right :).

I don't quite understand why we loop through all the devices ... does
this code do what we want? ---

  if (!GetSwitchInfo(event, &name, &state)) {
    return;
  }
  SwitchDevice device = NameToDevice(name);
  if (device == SWITCH_DEVICE_UNKNOWN) {
    return;
  }
  EventInfo& info = mEventInfo[device].mEvent;
  info.mEvent.status() = atoi(state) == 0 ? SWITCH_STATE_OFF : SWITCH_STATE_ON;
  if (info.mEnable) {
     NS_DispatchToMainThread(new SwitchEventRunnable(info.mEvent));
  }

>+  int mEnableNum;

Make this size_t please.

>+  bool ShouldSkip(NetlinkEvent *event, const char **name, const char **state) {

This name is a little confusing because of the out-parameters.  Let's
call this |GetEventInfo()|, returning true when the |name| and |state|
are successfully extracted from the uevent.

Also, nit: please snuggle the "*" to the typename, like |NetlinkEvent*
event|.
Attachment #614674 - Flags: review?(jones.chris.g)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #22)
> (In reply to StevenLee from comment #21)
> > Created attachment 614675 [details] [diff] [review]
> > 2. For the include sequence, Hal.h needs to be prior than others. So that
> > the current sequence is 
> > #include "mozilla/Hal.h"
> > #include "AudioManager.h"
> > #include "gonk/AudioSystem.h"
> 
> That's bad ... what happens when Hal.h isn't first?
If I don't include Hal.h in the first, basictypes.h would complain, 
basictypes.h:12:2: error: #error You_must_include_basictypes.h_before_prtypes.h!
Argh!  I hate that code :).  OK.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #23)
> Comment on attachment 614674 [details] [diff] [review]
> Audio routing support - V6
> 
> >+class SwitchEventObserver : public IUeventObserver
> 
> >+  void Notify(const NetlinkEvent &event) {
> >+    const char* name;
> >+    const char* state;
> >+   
> >+    if (ShouldSkip(const_cast<NetlinkEvent*>(&event), &name, &state)) {
> 
> Why do you need to const_cast this value?  Can you change ShouldSkip()
> so that you don't need to do that?
> 
In ShouldSkip(), it calls 'const char* NetlinkEvent::getSubsystem()', 
'const char* NetlinkEvent::findParam(const char*)', and 
'const char* NetlinkEvent::findParam(const char*)' which are not 
const functions. When I declaring 
ShoulSkip(const NetlinkEvent&, const char**, const char**), compiler 
complains 
passing 'const NetlinkEvent' as 'this' argument of 'const char* NetlinkEvent::getSubsystem()' discards qualifiers.

NetLinkEvents is the implementation of Android. I think I had better
not to modify its interface.
That's why I should do const cast. Or should I change the interface of
NetlinkEvents?
Ugh.  No, that's OK.  But please const_cast in GetEventInfo(const NetlinkEvent&), and add a comment that we're working around the android code not being const-correct.
Question: If I call RegisterSwitchObserver, I get switch events when the state changes (and this seems to be working). Prior to any state change should GetCurrentSwitchEvent return the current state of the switch?

Right now, it doesn't seem to.
Also, the default constructor (which is in PHal.h, generated from PHal.ipdl) for SwitchEvent doesn't initialize status_, so it just gets random values. The default constructor is used when expanding the mEvent array.

I think that this is because SwitchStatus is an enum rather then a class.
(In reply to Dave Hylands [:dhylands] from comment #28)
> Question: If I call RegisterSwitchObserver, I get switch events when the
> state changes (and this seems to be working). Prior to any state change
> should GetCurrentSwitchEvent return the current state of the switch?
> 

It currently returns UNKNOWN, because we don't read the initial values on startup.  That's bug 745078.

(In reply to Dave Hylands [:dhylands] from comment #29)
> Also, the default constructor (which is in PHal.h, generated from PHal.ipdl)
> for SwitchEvent doesn't initialize status_, so it just gets random values.
> The default constructor is used when expanding the mEvent array.
> 
> I think that this is because SwitchStatus is an enum rather then a class.

Yeah it's a primitive type, the default implicit ctor doesn't initialize them.  The default *explicit* ctor zeros, but that doesn't always make sense either.  We should always be initializing those explicitly.
Posted patch Audio routing support - V7 (obsolete) — Splinter Review
Attachment #614674 - Attachment is obsolete: true
Attachment #616484 - Flags: review?(jones.chris.g)
Comment on attachment 616484 [details] [diff] [review]
Audio routing support - V7

>diff --git a/hal/gonk/GonkSwitch.cpp b/hal/gonk/GonkSwitch.cpp

>+  nsresult GetEventInfo(const NetlinkEvent& event, const char** name, const char** state) {

This should return |bool|.  |-1| isn't a valid nsresult.

r=me with change to bool.
Attachment #616484 - Flags: review?(jones.chris.g) → review+
Posted patch Audio routing support - V8 (obsolete) — Splinter Review
Change the return type of GetEventInfo to bool
Attachment #616484 - Attachment is obsolete: true
Attachment #616917 - Flags: review+
Keywords: checkin-needed
These patches don't compile.

/tools/python/bin/python2.5 /builds/slave/try-lnx/build/config/pythonpath.py -I../../config /builds/slave/try-lnx/build/config/expandlibs_exec.py --uselist --  /usr/bin/ccache /tools/gcc-4.5-0moz3/bin/g++  -fno-rtti -pedantic -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -fno-exceptions -fno-strict-aliasing -std=gnu++0x -ffunction-sections -fdata-sections -pthread -pipe  -DNDEBUG -DTRIMMED -g -Os -freorder-blocks -finline-limit=50 -fomit-frame-pointer -fPIC -shared -Wl,-z,defs -Wl,--gc-sections -Wl,-h,libxul.so -o libxul.so  stdc++compat.o nsStaticXULComponents.o nsUnicharUtils.o nsBidiUtils.o nsUnicodeProperties.o nsRDFResource.o    -lpthread  -lrt   -Wl,-rpath-link,/builds/slave/try-lnx/build/obj-firefox/dist/bin -Wl,-rpath-link,/usr/local/lib   ../../toolkit/xre/libxulapp_s.a  ../../staticlib/components/libnecko.a ../../staticlib/components/libuconv.a ../../staticlib/components/libi18n.a ../../staticlib/components/libchardet.a ../../staticlib/components/libjar50.a ../../staticlib/components/libstartupcache.a ../../staticlib/components/libpref.a ../../staticlib/components/libhtmlpars.a ../../staticlib/components/libimglib2.a ../../staticlib/components/libgkgfx.a ../../staticlib/components/libgklayout.a ../../staticlib/components/libdocshell.a ../../staticlib/components/libembedcomponents.a ../../staticlib/components/libwebbrwsr.a ../../staticlib/components/libnsappshell.a ../../staticlib/components/libtxmgr.a ../../staticlib/components/libcommandlines.a ../../staticlib/components/libtoolkitcomps.a ../../staticlib/components/libpipboot.a ../../staticlib/components/libpipnss.a ../../staticlib/components/libappcomps.a ../../staticlib/components/libjsreflect.a ../../staticlib/components/libcomposer.a ../../staticlib/components/libtelemetry.a ../../staticlib/components/libjsinspector.a ../../staticlib/components/libjsdebugger.a ../../staticlib/components/libstoragecomps.a ../../staticlib/components/librdf.a ../../staticlib/components/libwindowds.a ../../staticlib/components/libjsctypes.a ../../staticlib/components/libjsperf.a ../../staticlib/components/libgkplugin.a ../../staticlib/components/libunixproxy.a ../../staticlib/components/libjsd.a ../../staticlib/components/libautoconfig.a ../../staticlib/components/libauth.a ../../staticlib/components/libcookie.a ../../staticlib/components/libpermissions.a ../../staticlib/components/libuniversalchardet.a ../../staticlib/components/libfileview.a ../../staticlib/components/libplaces.a ../../staticlib/components/libtkautocomplete.a ../../staticlib/components/libsatchel.a ../../staticlib/components/libpippki.a ../../staticlib/components/libwidget_gtk2.a ../../staticlib/components/libimgicon.a ../../staticlib/components/libprofiler.a ../../staticlib/components/libaccessibility.a ../../staticlib/components/libremoteservice.a ../../staticlib/components/libspellchecker.a ../../staticlib/components/libzipwriter.a ../../staticlib/components/libservices-crypto.a ../../staticlib/libjsipc_s.a ../../staticlib/libdomipc_s.a ../../staticlib/libdomplugins_s.a ../../staticlib/libmozipc_s.a ../../staticlib/libmozipdlgen_s.a ../../staticlib/libipcshell_s.a ../../staticlib/libgfx2d.a ../../staticlib/libgfxipc_s.a ../../staticlib/libhal_s.a ../../staticlib/libdombindings_s.a ../../staticlib/libxpcom_core.a ../../staticlib/libucvutil_s.a ../../staticlib/libchromium_s.a ../../staticlib/libsnappy_s.a ../../staticlib/libgtkxtbin.a ../../staticlib/libthebes.a ../../staticlib/libgl.a ../../staticlib/libycbcr.a  -L../../dist/bin -L../../dist/lib ../../media/libjpeg/libmozjpeg.a ../../media/libpng/libmozpng.a ../../gfx/qcms/libmozqcms.a /builds/slave/try-lnx/build/obj-firefox/dist/lib/libjs_static.a -L../../dist/bin -L../../dist/lib -lcrmf -lsmime3 -lssl3 -lnss3 -lnssutil3 ../../gfx/cairo/cairo/src/libmozcairo.a  ../../gfx/cairo/libpixman/src/libmozlibpixman.a  -lXrender -lfreetype -lfontconfig ../../gfx/harfbuzz/src/libmozharfbuzz.a  ../../dist/lib/libmozsqlite3.a  ../../gfx/graphite2/src/libmozgraphite2.a ../../modules/zlib/src/libmozz.a ../../dist/lib/libgkmedias.a  -lasound   -lrt ../../gfx/skia/libskia.a -L../../dist/bin -L../../dist/lib  -L/builds/slave/try-lnx/build/obj-firefox/dist/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl ../../dist/lib/libmozalloc.a -L/lib -ldbus-glib-1 -ldbus-1 -lglib-2.0    -lX11  -lXext  -L/lib -lpangoft2-1.0 -lpangocairo-1.0 -lpango-1.0 -lcairo -lgobject-2.0 -lgmodule-2.0 -ldl -lglib-2.0   -L/lib -lgtk-x11-2.0 -latk-1.0 -lgdk-x11-2.0 -lgdk_pixbuf-2.0 -lm -lpangocairo-1.0 -lpango-1.0 -lcairo -lgobject-2.0 -lgmodule-2.0 -ldl -lglib-2.0   -lXt -lgthread-2.0 -lfreetype    -ldl  -lrt    
../../hal/Hal.o: In function `mozilla::hal::EnableSwitchNotifications(mozilla::hal::SwitchDevice)':
/builds/slave/try-lnx/build/obj-firefox/hal/../../hal/Hal.cpp:602: undefined reference to `mozilla::hal_impl::EnableSwitchNotifications(mozilla::hal::SwitchDevice)'
../../hal/Hal.o: In function `mozilla::hal::DisableSwitchNotifications(mozilla::hal::SwitchDevice)':
/builds/slave/try-lnx/build/obj-firefox/hal/../../hal/Hal.cpp:608: undefined reference to `mozilla::hal_impl::DisableSwitchNotifications(mozilla::hal::SwitchDevice)'
../../hal/Hal.o: In function `mozilla::hal::GetCurrentSwitchState(mozilla::hal::SwitchDevice)':
/builds/slave/try-lnx/build/obj-firefox/hal/../../hal/Hal.cpp:614: undefined reference to `mozilla::hal_impl::GetCurrentSwitchState(mozilla::hal::SwitchDevice)'
/usr/bin/ld: libxul.so: hidden symbol `mozilla::hal_impl::GetCurrentSwitchState(mozilla::hal::SwitchDevice)' isn't defined
/usr/bin/ld: final link failed: Nonrepresentable section on output

https://tbpl.mozilla.org/?tree=Try&rev=01d771405be5
Keywords: checkin-needed
Fixed the compiling problem of fallback.
Attachment #616917 - Attachment is obsolete: true
Attachment #617290 - Flags: review+
Keywords: checkin-needed
Blocks: 749080
You need to log in before you can comment on or make changes to this bug.