Closed Bug 701517 Opened 13 years ago Closed 13 years ago

Battery API isn't Moz-prefixed

Categories

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

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla11
Tracking Status
firefox8 --- unaffected
firefox9 --- unaffected
firefox10 - affected
firefox11 --- verified

People

(Reporter: smaug, Assigned: mounir)

Details

(Whiteboard: [needs branch][qa!])

Attachments

(1 file)

It really should be.
("NavigatorBattery" in window) shouldn't be true.

Other option is to not use nsIDOM* prefix with the interfaces. Then they don't
automatically get added to global scope.
I guess ("BatteryManager" in window) shouldn't be true too.

Why do we want all interfaces name to be in the global scope?
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Does this need to be tracked for fx10?
Target Milestone: --- → mozilla10
Attached patch Patch v1Splinter Review
Olli, could you rs+sr the patch?
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #575451 - Flags: superreview?(bugs)
Whiteboard: [needs review]
Comment on attachment 575451 [details] [diff] [review]
Patch v1


>-class BatteryManager : public nsIDOMBatteryManager
>+class BatteryManager : public nsIDOMMozBatteryManager
>                      , public nsDOMEventTargetWrapperCache
>                      , public BatteryObserver
Hmm, nsDOMEventTargetWrapperCache is not first in the inheritance chain :/
I'm trying to make all the eventtarget to inherit nsIDOMEvent target as the first interface, since
that could eventually allow some optimizations.
Could you move nsDOMEventTargetWrapperCache to be the first one, especially because
BatteryManager relies on nsDOMEventTarget to do the QI for nsISupports.

Looks like NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper) should be
NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetWrapperCache)

 
> [scriptable, function, uuid(6dcb803b-e968-4c02-88f5-049a3f2a2efb)]
>-interface nsIDOMBatteryManager : nsIDOMEventTarget
>+interface nsIDOMMozBatteryManager : nsIDOMEventTarget
Could you update the uuid.

Those change, (s)r(s)+
Attachment #575451 - Flags: superreview?(bugs) → superreview+
(In reply to Olli Pettay [:smaug] from comment #5)
> Looks like NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper) should be
> NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetWrapperCache)

It seems to be the case already or are you speaking of something else?
Flags: in-testsuite+
Whiteboard: [needs review]
Attachment #575451 - Flags: checkin+
Attachment #575451 - Flags: approval-mozilla-aurora?
This should go to Aurora to make sure we do not ship un-prefixed interfaces. There is nearly no risk to do that.
(In reply to Olli Pettay [:smaug] from comment #0)
> It really should be.
Why?
http://hsivonen.iki.fi/vendor-prefixes/
(In reply to Masatoshi Kimura [:emk] from comment #8)
> (In reply to Olli Pettay [:smaug] from comment #0)
> > It really should be.
> Why?
> http://hsivonen.iki.fi/vendor-prefixes/

You already have to use navigator.mozBattery to access the BatteryManager object. We need to be consistent and prefix the interfaces too. If we want to unprefix everything that's another debate. As a side note, I had the feeling Henri's post was mostly about CSS it seems to me that prefixed DOM features are less hurting (because less used I guess?).
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #9)
> As a side note, I had the
> feeling Henri's post was mostly about CSS it seems to me that prefixed DOM
> features are less hurting (because less used I guess?).

My post was about vendor prefixing in general. WebKit has been prefixing CSS more, so we see that as a problem more clearly. We've been prefixing APIs more, so chances are that the problems caused by API prefixing are felt more by others.

Also, Mozilla has been prefixing APIs that allow performance or memory optimizations (requestAnimationFrame and chunked XHR response types). It's less obvious to a site user when a Web app doesn't take those optimized paths and ends up burning more memory or CPU, so the lack of optimal behavior is less obvious than when CSS eye candy fails to take effect.
In this case we're talking about a highly experimental API, AFAIK. So some kind of
prefixing or not enabling in release builds are the possible options, IMO.
(In reply to Olli Pettay [:smaug] from comment #11)
> In this case we're talking about a highly experimental API, AFAIK. So some
> kind of
> prefixing or not enabling in release builds are the possible options, IMO.

Not enabling it in non-B2G release builds has the advantage that it leaves a better pressure to turn the API into a non-experimental one than prefixing.
Whiteboard: [needs branch]
https://hg.mozilla.org/mozilla-central/rev/4882413755d3
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: mozilla10 → mozilla11
Target Milestone: mozilla11 → mozilla10
I think the questions of disabling WebAPI work in releases, enabling them only in B2G or not prefixing them should be sent to the mozilla.dev-webapi newsgroup/mailing-list. I'm not willing to do something that would apply to Battery API only, so until a decision has been taken, I prefer to follow the rules that seem to have been used until now.
Comment on attachment 575451 [details] [diff] [review]
Patch v1

[Triage Comment]
Per previous agreement, the battery API is pref'd off in 10 (see bug 703568). This bug is fixed in 11 and will therefore be corrected when the pref is on by default.
Attachment #575451 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Even if preffed off, the interfaces are still going to be visible, aren't they? Olli?
Depends on how you've have exposed the interfaces.
IIRC, using DOM_CLASSINFO_MAP_BEGIN_MAYBE_DISABLE in DOMClassInfo allows one to hide the interface.

(Desktop FF doesn't have TouchEvent, but it is in Fennec.)
Target Milestone: mozilla10 → mozilla11
Does this affect Desktop Firefox at all (ie. Laptops)? If so, is there anything we can do to verify the fix?
Whiteboard: [needs branch] → [needs branch][qa?]
Using nightly and http://robnyman.github.com/battery/ on my laptop has the battery info reported.
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #18)
> Does this affect Desktop Firefox at all (ie. Laptops)? If so, is there
> anything we can do to verify the fix?

Yes, you can try it on laptops (even desktops, you will just get charged/100%).
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #20)
> Yes, you can try it on laptops (even desktops, you will just get
> charged/100%).

Hmm, I should precise that on MacOS, you will not have real values because there is no backend yet.
Whiteboard: [needs branch][qa?] → [needs branch][qa+]
Verified on the 20120305181207 builds using http://robnyman.github.com/battery/:
Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101 Firefox/11.0

The verification was done on desktops so the results were:
Battery level is 100%
Battery status is charging
Battery will be fully charged in 0 minutes.
Battery will be discharged in Infinity minutes.
Status: RESOLVED → VERIFIED
Whiteboard: [needs branch][qa+] → [needs branch][qa!]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.