Comment on attachment 572398 [details] [diff] [review] wip Review of attachment 572398 [details] [diff] [review]: ----------------------------------------------------------------- Is that working with all supported versions of Windows? (Windows XP to Windows 7) By the way, it's awesome you jumped on this! I'm looking forward for a final patch ready to be landed! :) ::: hal/windows/WindowsHal.cpp @@ +55,5 @@ > +UpdateHandler(nsITimer* aTimer, void* aClosure) { > + static hal::BatteryInformation sLastInfo; > + hal::BatteryInformation currentInfo; > + > + hal_impl::GetCurrentBatteryInformation(&currentInfo); I don't think you need hal_impl. @@ +71,5 @@ > + if (sUpdateTimer) { > + sUpdateTimer->InitWithFuncCallback(UpdateHandler, > + nsnull, > + Preferences::GetInt("dom.battery.timer", 5000), > + nsITimer::TYPE_REPEATING_SLACK); There is no event system for this? In that case, 5000 ms update seems a bit too much, 30s would be better.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #2) > Comment on attachment 572398 [details] [diff] [review] [diff] [details] [review] > wip > > Review of attachment 572398 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > Is that working with all supported versions of Windows? (Windows XP to > Windows 7) It should work on all windows that we support. > By the way, it's awesome you jumped on this! I'm looking forward for a final > patch ready to be landed! :) > > ::: hal/windows/WindowsHal.cpp > @@ +55,5 @@ > > +UpdateHandler(nsITimer* aTimer, void* aClosure) { > > + static hal::BatteryInformation sLastInfo; > > + hal::BatteryInformation currentInfo; > > + > > + hal_impl::GetCurrentBatteryInformation(&currentInfo); > > I don't think you need hal_impl. > > @@ +71,5 @@ > > + if (sUpdateTimer) { > > + sUpdateTimer->InitWithFuncCallback(UpdateHandler, > > + nsnull, > > + Preferences::GetInt("dom.battery.timer", 5000), > > + nsITimer::TYPE_REPEATING_SLACK); > > There is no event system for this? > In that case, 5000 ms update seems a bit too much, 30s would be better. Yes for Windows 2000 and XP. But, I think that we can use RegisterPowerSettingNotification for Vista or later.

- If Vista or later, using new power API - If XP, using timer-based callback. Also, if we can get HWND (or nsIWidget) from hal code, we don't create own window.

Comment on attachment 573134 [details] [diff] [review] fix v1 I'm ok to review the Battery API specific stuff but Jim should review the Windows specific stuff. Note that I'm going to land today some patches that add two new properties to BatteryManager (how long before charging/discharging) which is going to require some new stuff in this backend. I will be fine to have the backend implemented in two times though.

We currently have code related to power broadcast msg handling for sleep and resume notification in the main widget event procedure. Interestingly enough I'm also working on removing that code and placing the functionality into an object (which creates it's own window on Windows, similar to this) and incorporates a bunch of mac code related to sleep/resume notifications, but this code is located over in xre. At some point it seems like this code will need to be blended together, as we are ending up with a lot of different functionality that is closely related in various parts of our code base. Is this code part of xul lib and will the notifications be available to both parent and child processes?

Also, if this is dom event related, why isn't it in mozilla/ dom/ system/ windows/ ?

(In reply to Jim Mathies [:jimm] from comment #6) > Is this code part of xul lib and will the notifications be available to both > parent and child processes? This code is the backend of the Battery API which is a DOM API so available in the child and parent processes. (In reply to Jim Mathies [:jimm] from comment #7) > Also, if this is dom event related, why isn't it in mozilla/ dom/ system/ > windows/ ? hal/ is for Hardware Abstraction Layer. The idea is to put all device interaction code there without poluting widget/ or dom/ which are not really the best places for that. Would that be interesting to have other consumers to use the DOM API and have this code live in hal/?

General platform C++ code can call into hal/. It's intended to be a simple C++ layer over system interfaces, much lighter-weight than XPCOM-backend-per-OS, that's set up from the beginning for content processes (nested, even). JS code should use the DOM interface.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #9) > General platform C++ code can call into hal/. It's intended to be a simple > C++ layer over system interfaces, much lighter-weight than > XPCOM-backend-per-OS, that's set up from the beginning for content processes > (nested, even). JS code should use the DOM interface. Should a bug be filed on moving everything in dom/system into hal/system? Looking there I see geo location, motion sensing, and haptic feedback, all of which seem to fit well with hal.

I would like to do that eventually, but it's not at the top of anyone's list right now.

Comment on attachment 573134 [details] [diff] [review] fix v1 Review of attachment 573134 [details] [diff] [review]: ----------------------------------------------------------------- Those are mostly nits. Though, I would like to understand why you are calling GetCurrentBatteryInformation and change those after in Windows Vista+ callback. Could you re-ask a review with the nits fixed and the call explained? f=mounir for the DOM parts (I can't say anything about the Windows API parts) Thanks for working on this, that would be pretty cool to have a support on Windows :) ::: hal/Makefile.in @@ +74,5 @@ > ifdef MOZ_ENABLE_DBUS > CPPSRCS += UPowerClient.cpp > endif > else > +ifeq (WINNT,$(OS_TARGET)) nit: I think |else ifeq| would have been more readable. ::: hal/windows/WindowsHal.cpp @@ +46,5 @@ > +namespace hal_impl { > + > +static nsCOMPtr<nsITimer> sUpdateTimer = nsnull; > + > +#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN As far as I know, we require a SDK that handles Vista and later to compile Gecko. No need for this I think. @@ +49,5 @@ > + > +#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN > +/* Power Event API is Vista or later */ > +typedef HPOWERNOTIFY (WINAPI *REGISTERPOWERSETTINGNOTIFICATION) (HANDLE, LPCGUID, DWORD); > +typedef BOOL (WINAPI *UNREGISTERPOWERSETTINGNOTIFICATION) (HPOWERNOTIFY); Those types are not defined by the Windows API? @@ +71,5 @@ > +UpdateHandler(nsITimer* aTimer, void* aClosure) { > + static hal::BatteryInformation sLastInfo; > + hal::BatteryInformation currentInfo; > + > + hal_impl::GetCurrentBatteryInformation(&currentInfo); You don't need the hal_impl here. @@ +85,5 @@ > +LRESULT CALLBACK > +BatteryWindowProc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam) { > + switch (msg) { > + case WM_POWERBROADCAST: > + if (wParam == PBT_POWERSETTINGCHANGE) { I would do something like: if (msg != WM_POWERBROADCAST || wParam != PBT_POWERSETTINGCHANGE) { return DefWindowProc(hwnd, msg, wParam, lParam); } /* the code we want */ @@ +98,5 @@ > + GUID_BATTERY_PERCENTAGE_REMAINING)) { > + currentInfo.level() = (float)(*(DWORD*)setting->Data) / 100.0f; > + } else if (IsEqualGUID(setting->PowerSetting, GUID_ACDC_POWER_SOURCE)) { > + currentInfo.charging() = (*(DWORD*)setting->Data) == 0; > + } I'm not sure why you call GetCurrentBatteryInformation and then change them. @@ +111,5 @@ > + > +void > +EnableBatteryNotifications() > +{ > +#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN You should replace this by a runtime check. @@ +162,5 @@ > + > + // for Windows 2000 and Windwos XP. If we remove Windows XP support, > + // we should remove timer-based power notification > + sUpdateTimer = do_CreateInstance(NS_TIMER_CONTRACTID); > + if (sUpdateTimer) { I think you should add a runtime check to make sure we a running Windows XP or 2000 when running the timer. No need to do that otherwise. @@ +166,5 @@ > + if (sUpdateTimer) { > + sUpdateTimer->InitWithFuncCallback(UpdateHandler, > + nsnull, > + Preferences::GetInt("dom.battery.timer", > + 30 * 1000 /* 30s */), Write 30000 instead of 30*1000. @@ +174,5 @@ > + > +void > +DisableBatteryNotifications() > +{ > +#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN You should replace this by a runtime check. @@ +194,5 @@ > + > + if (sUpdateTimer) { > + sUpdateTimer->Cancel(); > + sUpdateTimer = nsnull; > + } Same thing for the timer: only if Windows XP or 2000. @@ +203,5 @@ > +{ > + SYSTEM_POWER_STATUS status; > + if (!GetSystemPowerStatus(&status)) { > + aBatteryInfo->level() = dom::battery::kDefaultLevel; > + aBatteryInfo->charging() = dom::battery::kDefaultCharging; Add |using namespace dom::battery;| in mozilla namespace. @@ +209,5 @@ > + } > + > + aBatteryInfo->level() = status.BatteryLifePercent == 255 ? > + dom::battery::kDefaultLevel : > + ((float)status.BatteryLifePercent) / 100.0f; Please, do something like: condition ? foo : bar ::: modules/libpref/src/init/all.js @@ +3378,5 @@ > // Battery API > pref("dom.battery.enabled", true); > +#ifdef XP_WIN > +pref("dom.battery.timer", 30000); > +#endif Do we have to add the pref here? By adding it here, it will show up in about:config. I think it would be better to not show it: playing wrongly with it might have bad consequences for user experience.

Also, I think it would be nice to have a separate file for battery stuff. You can just create a WindowsHal.cpp file defining Vibrate() methods and a WindowsBattery.cpp (or name it like you want) that defines battery related stuff. That way, we are going to prevent files with thousands of line and we compartment stuff that are different.

Makoto, is there anything I can do to help you with that patch?

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #14) > Makoto, is there anything I can do to help you with that patch? I will work next week. >> +#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN >> +/* Power Event API is Vista or later */ >> +typedef HPOWERNOTIFY (WINAPI *REGISTERPOWERSETTINGNOTIFICATION) (HANDLE, LPCGUID, DWORD); >> +typedef BOOL (WINAPI *UNREGISTERPOWERSETTINGNOTIFICATION) (HPOWERNOTIFY); > > Those types are not defined by the Windows API? Yes. Although functions are defined, we cannot use it to support 2000/XP. So we have to loading dynamically. >@@ +46,5 @@ >> +namespace hal_impl { >> + >> +static nsCOMPtr<nsITimer> sUpdateTimer = nsnull; >> + >> +#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN > > As far as I know, we require a SDK that handles Vista and later to compile Gecko. No > need for this I think. This will be necessary for mingw32 support.

>@@ +71,5 @@ >> +UpdateHandler(nsITimer* aTimer, void* aClosure) { >> + static hal::BatteryInformation sLastInfo; >> + hal::BatteryInformation currentInfo; >> + >> + hal_impl::GetCurrentBatteryInformation(&currentInfo); > >You don't need the hal_impl here. We cannot remove it. VC causes compile error (error C2668: 'mozilla::hal::GetCurrentBatteryInformation' : ambiguous call to overloaded function) due to conflict of hal vs hal_impl

Comment on attachment 576885 [details] [diff] [review] fix Review of attachment 576885 [details] [diff] [review]: ----------------------------------------------------------------- A general nit: you are using NULL but I think you should use nsnull instead. r=me with all those nits fixed. That's a pretty nice work, thank you! :) (I will let Jim review more deeply the Windows API usage.) ::: hal/windows/WindowsBattery.cpp @@ +73,5 @@ > +} > + > +static void > +UpdateHandler(nsITimer* aTimer, void* aClosure) { > + static hal::BatteryInformation sLastInfo; I think an assert here wouldn't hurt. Like: NS_ASSERTION(!IsVistaOrLater(), "We shouldn't call this function for Vista or later versions!"); @@ +91,5 @@ > +LRESULT CALLBACK > +BatteryWindowProc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam) { > + switch (msg) { > + case WM_POWERBROADCAST: > + if (wParam == PBT_POWERSETTINGCHANGE) { I think you could make this more readable with a condition like: if (msg != WM_POWERBROADCAST || wParam != PBT_POWERSETTINGCHANGE) { return DefWindowProc(hwnd, msg, wParam, lParam); } // get battery info and do what you have to do @@ +121,5 @@ > + sUnregisterPowerSettingNotification = (UNREGISTERPOWERSETTINGNOTIFICATION) > + GetProcAddress(hUser32, "UnregisterPowerSettingNotification"); > + > + if (sRegisterPowerSettingNotification && > + sUnregisterPowerSettingNotification) { I believe you should call MOZ_ASSERT or NS_ASSERTION if sRegister..Notification and sUnregister...Notification are still null here. They should not, right? Sure thing is you should leave the method because you do not seem to do anything if those are null. In addition, it will make the code more readable because it will look like: if (!condition) { ASSERT(); return; } // what we have to do @@ +143,5 @@ > + 0, 0, 0, 0, 0, > + NULL, NULL, hSelf, NULL); > + } > + > + if (sHWnd != NULL) { As above, you might want to assert if sHwnd == nsnull and you should do: if (sHWnd == nsnull) { return; } @@ +206,5 @@ > + SYSTEM_POWER_STATUS status; > + if (!GetSystemPowerStatus(&status)) { > + aBatteryInfo->level() = kDefaultLevel; > + aBatteryInfo->charging() = kDefaultCharging; > + aBatteryInfo->remainingTime() = kUnknownRemainingTime; Could you change this to 0? The specs have changed and if there is no battery, the returned value should be 0.

What patches do I need to apply and build this? Everything from bug 678694 I suppose? If so, could someone post a roll up there of all that work?

(In reply to Jim Mathies [:jimm] from comment #19) > What patches do I need to apply and build this? Everything from bug 678694 I > suppose? If so, could someone post a roll up there of all that work? Everything has been pushed in mozilla-central. You just need to apply this patch to get the Windows backend.

Comment on attachment 576885 [details] [diff] [review] fix Review of attachment 576885 [details] [diff] [review]: ----------------------------------------------------------------- Not much to say beyond the comments by Mounir. Code builds fine. Do we have tests for all this? As far as bug 679240 goes, I'll file a follow up on trying to blend the functionality tied to these windows together. ::: hal/windows/WindowsBattery.cpp @@ +46,5 @@ > + > +namespace mozilla { > +namespace hal_impl { > + > +static nsCOMPtr<nsITimer> sUpdateTimer = nsnull; Isn't this against the rules? I think nsCOMPtrs should always be member variables. Something to do with leak detection. @@ +61,5 @@ > +#endif > + > + > +static bool IsVistaOrLater() > +{ Minor nits - your using different forms of paren positioning on various functions and methods. If you could streamline that into a single standard for the file that would be great. @@ +90,5 @@ > +static > +LRESULT CALLBACK > +BatteryWindowProc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam) { > + switch (msg) { > + case WM_POWERBROADCAST: You probably want to add a deferred message handler in ipc/glue/WindowsMessageLoop for WM_POWERBROADCAST. http://mxr.mozilla.org/mozilla-central/source/ipc/glue/WindowsMessageLoop.cpp#189 Otherwise these events might occasionally get dropped.

(In reply to Jim Mathies [:jimm] from comment #21) > Do we have tests for all this? We have some simple automatic tests and we have some manual testings from QA. Some more complex testing my come with a project named Marionette to test WebAPI work in general (with qemu and other fun stuff). > > +static nsCOMPtr<nsITimer> sUpdateTimer = nsnull; > > Isn't this against the rules? I think nsCOMPtrs should always be member > variables. Something to do with leak detection. I believe this is ok as long as it is always nullified before exiting which should be the case, here, right? > @@ +61,5 @@ > > +#endif > > + > > + > > +static bool IsVistaOrLater() > > +{ > > Minor nits - your using different forms of paren positioning on various > functions and methods. If you could streamline that into a single standard > for the file that would be great. Actually, Makoto, Mozilla coding style is usually something like: returnType className::methodName() { } I guess we only need to have this patch cleaned-up before landing! :)

(In reply to Jim Mathies [:jimm] from comment #21) > You probably want to add a deferred message handler in > ipc/glue/WindowsMessageLoop for WM_POWERBROADCAST. For this, all you need to do is add WM_POWERBROADCAST to this fall through set of case statements: http://mxr.mozilla.org/mozilla-central/source/ipc/glue/WindowsMessageLoop.cpp#219 The default deferred message wrapper covers all the standard event arguments.

Actually, you should use the second set. I guess this message expects an unconventional true response. http://mxr.mozilla.org/mozilla-central/source/ipc/glue/WindowsMessageLoop.cpp#225