Convert BatteryManager to WebIDL

RESOLVED FIXED in mozilla21

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 710144 [details] [diff] [review]
patch
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.
(Assignee)

Comment 7

6 years ago
Created attachment 710152 [details] [diff] [review]
patch
Attachment #710144 - Attachment is obsolete: true
Attachment #710152 - Flags: review+
(Assignee)

Comment 8

6 years ago
A new patch with -Idom/battery will be submitted soon.
(Assignee)

Comment 9

6 years ago
Created attachment 710157 [details] [diff] [review]
patch

another quick review for -Ifoo/bar ?
Attachment #710152 - Attachment is obsolete: true
Attachment #710157 - Flags: review?(peterv)
Attachment #710157 - Flags: review?(peterv) → review+
(Assignee)

Updated

6 years ago
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?
(Assignee)

Comment 12

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Assignee)

Updated

6 years ago
Blocks: 838220
Spec has been fixed: no longer using [NoInterfaceObject] and "Navigator implements ..." has been changed to "partial interface Navigator".
You need to log in before you can comment on or make changes to this bug.