Closed Bug 838124 Opened 7 years ago Closed 7 years ago

Convert BatteryManager to WebIDL

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Attached patch patch (obsolete) — Splinter Review
Attachment #710144 - Flags: review?(peterv)
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 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.
Attachment #710144 - Flags: review?(peterv) → review+
(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.
(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).
> 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.
Attached patch patch (obsolete) — Splinter Review
Attachment #710144 - Attachment is obsolete: true
Attachment #710152 - Flags: review+
A new patch with -Idom/battery will be submitted soon.
Attached patch patchSplinter Review
another quick review for -Ifoo/bar ?
Attachment #710152 - Attachment is obsolete: true
Attachment #710157 - Flags: review?(peterv)
Attachment #710157 - Flags: review?(peterv) → review+
Keywords: checkin-needed
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?
This is part of the webIDL in the specs: http://www.w3.org/TR/battery-status/
Wait.  That spec makes no sense.  Why is it not just "partial interface Navigator"?
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.
https://hg.mozilla.org/mozilla-central/rev/dda25400b9d7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Blocks: 838220
Spec has been fixed: no longer using [NoInterfaceObject] and "Navigator implements ..." has been changed to "partial interface Navigator".
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.