Last Comment Bug 701517 - Battery API isn't Moz-prefixed
: Battery API isn't Moz-prefixed
Status: VERIFIED FIXED
[needs branch][qa!]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Mounir Lamouri (:mounir)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-10 14:09 PST by Olli Pettay [:smaug]
Modified: 2012-03-07 02:54 PST (History)
12 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
-
affected
verified


Attachments
Patch v1 (11.15 KB, patch)
2011-11-18 07:40 PST, Mounir Lamouri (:mounir)
bugs: superreview+
akeybl: approval‑mozilla‑aurora-
mounir: checkin+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2011-11-10 14:09:14 PST
It really should be.
Comment 1 Olli Pettay [:smaug] 2011-11-10 14:11:34 PST
("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) 2011-11-10 15:11:55 PST
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 [:bz] (still a bit busy) 2011-11-12 20:35:45 PST
Does this need to be tracked for fx10?
Comment 4 Mounir Lamouri (:mounir) 2011-11-18 07:40:20 PST
Created attachment 575451 [details] [diff] [review]
Patch v1

Olli, could you rs+sr the patch?
Comment 5 Olli Pettay [:smaug] 2011-11-18 08:13:52 PST
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) 2011-11-19 02:43:02 PST
(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) 2011-11-20 10:12:34 PST
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] 2011-11-20 21:38:38 PST
(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) 2011-11-21 01:21:41 PST
(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) 2011-11-21 02:51:00 PST
(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] 2011-11-21 03:13:38 PST
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) 2011-11-21 03:50:44 PST
(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) 2011-11-21 09:03:07 PST
https://hg.mozilla.org/mozilla-central/rev/4882413755d3
Comment 14 Mounir Lamouri (:mounir) 2011-11-22 02:07:54 PST
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] 2011-12-04 16:34:50 PST
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) 2011-12-04 18:39:59 PST
Even if preffed off, the interfaces are still going to be visible, aren't they? Olli?
Comment 17 Olli Pettay [:smaug] 2011-12-05 02:10:51 PST
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-05 16:35:16 PST
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 [:kbrosnan] 2012-03-05 18:17:11 PST
Using nightly and http://robnyman.github.com/battery/ on my laptop has the battery info reported.
Comment 20 Mounir Lamouri (:mounir) 2012-03-06 06:13:01 PST
(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) 2012-03-06 06:13:59 PST
(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) 2012-03-07 02:54:27 PST
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.

Note You need to log in before you can comment on or make changes to this bug.