Battery API isn't Moz-prefixed

VERIFIED FIXED in Firefox 11

Status

()

Core
DOM
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: smaug, Assigned: mounir)

Tracking

Trunk
mozilla11
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox8 unaffected, firefox9 unaffected, firefox10- affected, firefox11 verified)

Details

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

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
It really should be.
(Reporter)

Comment 1

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

Comment 2

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

Updated

6 years ago
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox8: --- → unaffected
status-firefox9: --- → unaffected
tracking-firefox10: --- → ?
Target Milestone: --- → mozilla10
(Assignee)

Comment 4

6 years ago
Created attachment 575451 [details] [diff] [review]
Patch v1

Olli, could you rs+sr the patch?
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #575451 - Flags: superreview?(bugs)
(Assignee)

Updated

6 years ago
Whiteboard: [needs review]
(Reporter)

Comment 5

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

Comment 6

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

Updated

6 years ago
Flags: in-testsuite+
Whiteboard: [needs review]
(Assignee)

Updated

6 years ago
Attachment #575451 - Flags: checkin+
Attachment #575451 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 7

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

Comment 9

6 years ago
(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.
(Reporter)

Comment 11

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

Updated

6 years ago
Whiteboard: [needs branch]
https://hg.mozilla.org/mozilla-central/rev/4882413755d3
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla10 → mozilla11
(Assignee)

Updated

6 years ago
status-firefox11: affected → fixed
(Assignee)

Updated

6 years ago
Target Milestone: mozilla11 → mozilla10
(Assignee)

Comment 14

6 years ago
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-

Updated

6 years ago
tracking-firefox10: ? → -
(Assignee)

Comment 16

6 years ago
Even if preffed off, the interfaces are still going to be visible, aren't they? Olli?
(Reporter)

Comment 17

6 years ago
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.)

Updated

5 years ago
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.
(Assignee)

Comment 20

5 years ago
(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%).
(Assignee)

Comment 21

5 years ago
(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+]

Comment 22

5 years ago
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
status-firefox11: fixed → verified
Whiteboard: [needs branch][qa+] → [needs branch][qa!]
You need to log in before you can comment on or make changes to this bug.