Last Comment Bug 758849 - UPower isnt linux-only
: UPower isnt linux-only
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Hardware Abstraction Layer (HAL) (show other bugs)
: Trunk
: Other OpenBSD
: -- normal (vote)
: mozilla15
Assigned To: Landry Breuil (:gaston)
:
Mentors:
Depends on: 696041
Blocks: 788414
  Show dependency treegraph
 
Reported: 2012-05-25 23:51 PDT by Landry Breuil (:gaston)
Modified: 2012-09-04 23:34 PDT (History)
1 user (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Buld upowerclient on BSDs too if dbus is enabled (1.24 KB, patch)
2012-05-26 01:38 PDT, Landry Breuil (:gaston)
no flags Details | Diff | Review
Buld upowerclient on BSDs too if dbus is enabled (1.36 KB, patch)
2012-05-28 03:40 PDT, Landry Breuil (:gaston)
mounir: review+
Details | Diff | Review
Same patch without EOF change (1001 bytes, patch)
2012-05-29 01:47 PDT, Landry Breuil (:gaston)
mounir: checkin+
Details | Diff | Review

Description Landry Breuil (:gaston) 2012-05-25 23:51:40 PDT
While looking for other things under hal/ i noticed the following chunk in hal?Makefile.in, within ifeq (Linux,$(OS_TARGET))


79679: ifdef MOZ_ENABLE_DBUS
79679: CPPSRCS += UPowerClient.cpp
92976: else
92976: CPPSRCS += FallbackBattery.cpp
79679: endif

That's a bit wrong, since upower is not linux only. It also builds and runs fine on OpenBSD and FreeBSD (also probably builds on NetBSD but there's no dedicated backend yet, see http://cgit.freedesktop.org/upower/tree/src). I dont know if MOZ_ENABLE_DBUS can be set on osx (i guess so), so maybe the same ifdef MOZ_ENABLE_DBUS -then-enable-upower chunk should be added to the last else chunk, so that poor other platforms get a chance to talk to upower too ?

It doesnt seem to me that this battery api is used for anything else on desktop firefox, but who knows, in the future it might ?
Comment 1 Landry Breuil (:gaston) 2012-05-26 01:38:56 PDT
Created attachment 627453 [details] [diff] [review]
Buld upowerclient on BSDs too if dbus is enabled

That patch correctly builds UPowerClient.o here if dbus is enabled.
Comment 2 Mounir Lamouri (:mounir) 2012-05-27 02:58:00 PDT
Landry, what about just adding a line for BSD's instead?
Comment 3 Landry Breuil (:gaston) 2012-05-28 03:40:23 PDT
Created attachment 627656 [details] [diff] [review]
Buld upowerclient on BSDs too if dbus is enabled

Something like that ?
Comment 4 Mounir Lamouri (:mounir) 2012-05-29 01:29:12 PDT
Comment on attachment 627656 [details] [diff] [review]
Buld upowerclient on BSDs too if dbus is enabled

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

r=me but avoid the change at the end if unnecessary.

::: hal/Makefile.in
@@ +135,5 @@
>  ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
>  # So that we can call nsScreenManagerGonk::GetConfiguration().
>  LOCAL_INCLUDES += -I$(topsrcdir)/widget/gonk
>  LOCAL_INCLUDES += -I$(topsrcdir)/widget/xpwidgets
> +endif

What are you changing here?
Comment 5 Landry Breuil (:gaston) 2012-05-29 01:46:20 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4)
> Comment on attachment 627656 [details] [diff] [review]
> Buld upowerclient on BSDs too if dbus is enabled
> 
> Review of attachment 627656 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me but avoid the change at the end if unnecessary.
> 
> ::: hal/Makefile.in
> @@ +135,5 @@
> >  ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
> >  # So that we can call nsScreenManagerGonk::GetConfiguration().
> >  LOCAL_INCLUDES += -I$(topsrcdir)/widget/gonk
> >  LOCAL_INCLUDES += -I$(topsrcdir)/widget/xpwidgets
> > +endif
> 
> What are you changing here?

Hmm. i guess that's mercurial/vim that added that, dunno if there's a policy wrt newline at EOF in the codebase..

-endif
\ No newline at end of file
+endif
Comment 6 Landry Breuil (:gaston) 2012-05-29 01:47:56 PDT
Created attachment 627881 [details] [diff] [review]
Same patch without EOF change

New patch without that chunk, setting checkin-needed.
Comment 8 Ed Morley [:emorley] 2012-05-29 10:15:25 PDT
https://hg.mozilla.org/mozilla-central/rev/ce3d4fd7033b

Note You need to log in before you can comment on or make changes to this bug.