838124 - Convert BatteryManager to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Mounir Lamouri (:mounir)]

Comment on attachment 710144 [details] [diff] [review]
patch

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

::: dom/battery/BatteryManager.cpp
@@ +11,5 @@
>  #include "nsDOMEvent.h"
>  #include "mozilla/Preferences.h"
>  #include "nsDOMEventTargetHelper.h"
>  
> +#include "mozilla/dom/BatteryManagerBinding.h"

nit: please, don't leave an empty line before that #include.

::: dom/battery/Makefile.in
@@ +19,5 @@
>  
>  EXPORTS_NAMESPACES = mozilla/dom/battery
>  
>  EXPORTS_mozilla/dom/battery = \
> +  BatteryManager.h \

Why are you doing that? BatteryManager.h wasn't exported on purpose.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 710144 [details] [diff] [review]
patch

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

::: dom/battery/BatteryManager.cpp
@@ +65,2 @@
>  {
> +  return mCharging;

I'd inline this.

@@ +68,5 @@
>  
> +double
> +BatteryManager::Level() const
> +{
> +  return mLevel;

I'd inline this.

::: dom/battery/BatteryManager.h
@@ +21,5 @@
>  class BatteryInformation;
>  } // namespace hal
>  
>  namespace dom {
>  namespace battery {

It would be nice to remove the battery namespace, it would allow us to not need the change to Bindings.conf. In general we put everything directly in the mozilla::dom namespace. Either do that here or file a new bug.

Comment 4

[Peter Van der Beken [:peterv]]

(In reply to Mounir Lamouri (:mounir) from comment #2)
> Why are you doing that? BatteryManager.h wasn't exported on purpose.

Because the binding needs access to the header.

Comment 5

[Mounir Lamouri (:mounir)]

(In reply to Peter Van der Beken [:peterv] from comment #3)
> It would be nice to remove the battery namespace, it would allow us to not
> need the change to Bindings.conf. In general we put everything directly in
> the mozilla::dom namespace. Either do that here or file a new bug.

That would be nice, indeed.

(In reply to Peter Van der Beken [:peterv] from comment #4)
> (In reply to Mounir Lamouri (:mounir) from comment #2)
> > Why are you doing that? BatteryManager.h wasn't exported on purpose.
> 
> Because the binding needs access to the header.

Sure, but can't we simply -Idom/battery/ for bindings? exporting BatteryManager.h makes it usable by anyone using the Mozilla Platform (if such thing still exists).

Comment 6

[Peter Van der Beken [:peterv]]

> Sure, but can't we simply -Idom/battery/ for bindings? exporting
> BatteryManager.h makes it usable by anyone using the Mozilla Platform (if
> such thing still exists).

In general we've just exported stuff. I don't really care either way I think.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 8

[Andrea Marchesini [:baku]]

A new patch with -Idom/battery will be submitted soon.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch

another quick review for -Ifoo/bar ?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dda25400b9d7

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 710157 [details] [diff] [review]
patch

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

::: dom/webidl/BatteryManager.webidl
@@ +9,5 @@
> + * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
> + * liability, trademark and document use rules apply.
> + */
> +
> +[NoInterfaceObject]

Why would you do that?

Comment 12

[Andrea Marchesini [:baku]]

This is part of the webIDL in the specs: http://www.w3.org/TR/battery-status/

Comment 13

[Boris Zbarsky [:bzbarsky]]

Wait.  That spec makes no sense.  Why is it not just "partial interface Navigator"?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Oh, wait.  NavigatorBattery shouldn't exist.  It should just be "partial interface Navigator".  No opinion on BatteryManager per se, though I agree that it's odd to mark it as NoInterfaceObject.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dda25400b9d7

Comment 16

[Mounir Lamouri (:mounir)]

Spec has been fixed: no longer using [NoInterfaceObject] and "Navigator implements ..." has been changed to "partial interface Navigator".