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?
Does this need to be tracked for fx10?
Created attachment 575451 [details] [diff] [review]
Olli, could you rs+sr the patch?
Comment on attachment 575451 [details] [diff] [review]
>-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
> [scriptable, function, uuid(6dcb803b-e968-4c02-88f5-049a3f2a2efb)]
>-interface nsIDOMBatteryManager : nsIDOMEventTarget
>+interface nsIDOMMozBatteryManager : nsIDOMEventTarget
Could you update the uuid.
Those change, (s)r(s)+
(In reply to Olli Pettay [:smaug] from comment #5)
> Looks like NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper) should be
It seems to be the case already or are you speaking of something else?
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.
(In reply to Masatoshi Kimura [:emk] from comment #8)
> (In reply to Olli Pettay [:smaug] from comment #0)
> > It really should be.
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.
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]
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.
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.)
Does this affect Desktop Firefox at all (ie. Laptops)? If so, is there anything we can do to verify the fix?
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
Hmm, I should precise that on MacOS, you will not have real values because there is no backend yet.
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.