701517 - Battery API isn't Moz-prefixed

Status: VERIFIED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

It really should be.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

("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.

Comment 2

[Mounir Lamouri (:mounir)]

I guess ("BatteryManager" in window) shouldn't be true too.

Why do we want all interfaces name to be in the global scope?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Does this need to be tracked for fx10?

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Olli, could you rs+sr the patch?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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)+

Comment 6

[Mounir Lamouri (:mounir)]

(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?

Comment 7

[Mounir Lamouri (:mounir)]

This should go to Aurora to make sure we do not ship un-prefixed interfaces. There is nearly no risk to do that.

Comment 8

[Masatoshi Kimura [:emk]]

(In reply to Olli Pettay [:smaug] from comment #0)
> It really should be.
Why?
http://hsivonen.iki.fi/vendor-prefixes/

Comment 9

[Mounir Lamouri (:mounir)]

(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?).

Comment 10

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

(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.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 12

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

(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.

Comment 13

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/4882413755d3

Comment 14

[Mounir Lamouri (:mounir)]

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 15

[Alex Keybl [:akeybl]]

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.

Comment 16

[Mounir Lamouri (:mounir)]

Even if preffed off, the interfaces are still going to be visible, aren't they? Olli?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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.)

Comment 18

[u279076]

Does this affect Desktop Firefox at all (ie. Laptops)? If so, is there anything we can do to verify the fix?

Comment 19

[Kevin Brosnan [Ex-Mozilla]]

Using nightly and http://robnyman.github.com/battery/ on my laptop has the battery info reported.

Comment 20

[Mounir Lamouri (:mounir)]

(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%).

Comment 21

[Mounir Lamouri (:mounir)]

(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.

Comment 22

[Ioana (away)]

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.