Bug 62698 (BatteryStatusAPI)

Created attachment 97242 [details] Proposed patch

CC'ing Eric, Darin and Andersca.

Comment on attachment 97242 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=97242&action=review > Source/WebCore/dom/BatteryStatusClient.h:20 > + * Remove blank line. > Source/WebCore/dom/BatteryStatusController.cpp:20 > + * ditto. > Source/WebCore/dom/BatteryStatusController.h:20 > + * ditto. > Source/WebCore/dom/BatteryStatusEvent.cpp:20 > + * ditto. > Source/WebCore/dom/BatteryStatusEvent.h:20 > + * ditto. > Source/WebCore/dom/BatteryStatusEvent.idl:20 > + * ditto.

AFAIK, you should make test cases for new features. Do you prepare test cases for this feature ?

View in context: https://bugs.webkit.org/attachment.cgi?id=97242&action=review > Source/WebCore/dom/BatteryStatusController.h:45 > +// bool isActive() { return !m_listeners.isEmpty(); } Is it comment?

(In reply to comment #4) > AFAIK, you should make test cases for new features. Do you prepare test cases for this feature ? Sure, I'm almost finished implementation this. After finishing this, absolutely I'll make test cases for this.

Created attachment 97255 [details] Delete blank line and comment line (Fix for comment #3, #5

Comment on attachment 97255 [details] Delete blank line and comment line (Fix for comment #3, #5 View in context: https://bugs.webkit.org/attachment.cgi?id=97255&action=review It looks like you based this on the location services stuff. I'm not sure if that's the best model or not for this design. We need tests for this. Even if you use a Mock like Location services does. > Source/WebCore/ChangeLog:14 > + No new tests. (OOPS!) This will prevent using the commit-queue. Normally this line is replaced by a list of tests or reasons why testing is impossible/impractical. > Source/WebCore/dom/BatteryStatusClient.h:29 > +class BatteryStatusClient { Why should this be a WebKit client as opposed to a WebCore platform interface? I'm not sure that it matters, but I'm curious why you made the choice to go up through the WebKit layer instead of down through WebCore platform. > Source/WebCore/dom/BatteryStatusEvent.cpp:29 > +BatteryStatusEvent::BatteryStatusEvent() Why do we need an event subclass for this event? Does it carry any interesting payload? Many other APIs just send a named event w/o any subclass. Then clients know to go look at some property on the Window object when they get the event. > Source/WebCore/dom/BatteryStatusEvent.cpp:30 > + : Event(eventNames().batterystatusEvent, false, true) These false/true are meaningless. Not your fault, but Event should eventually be changed to take enums instead. > Source/WebCore/dom/BatteryStatusEvent.cpp:31 > + , m_isPlugged(false), m_level(0) Normally we do these one per line.

Created attachment 97408 [details] Modify about comment #8

(In reply to comment #8) > (From update of attachment 97255 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97255&action=review > > It looks like you based this on the location services stuff. I'm not sure if that's the best model or not for this design. > I referenced device orientation event for this. Bacause they are almost same operation (transfer device info. to pages) I'm not sure this is a best choice also, but I want to make a coherence of these kind of events) > We need tests for this. Even if you use a Mock like Location services does. > > > Source/WebCore/ChangeLog:14 > > + No new tests. (OOPS!) > > This will prevent using the commit-queue. Normally this line is replaced by a list of tests or reasons why testing is impossible/impractical. > I'll add a testing source for this, and I fix a changlog also. > > Source/WebCore/dom/BatteryStatusClient.h:29 > > +class BatteryStatusClient { > > Why should this be a WebKit client as opposed to a WebCore platform interface? I'm not sure that it matters, but I'm curious why you made the choice to go up through the WebKit layer instead of down through WebCore platform. > Client class is a interface for all platform port. So I think this interface have to be out of platform. (This can be made all platform port reference a client class) > > Source/WebCore/dom/BatteryStatusEvent.cpp:29 > > +BatteryStatusEvent::BatteryStatusEvent() > > Why do we need an event subclass for this event? Does it carry any interesting payload? Many other APIs just send a named event w/o any subclass. Then clients know to go look at some property on the Window object when they get the event. > Battery Status Event have to send a battery status to webpages. It have to have attributes for battery info. That's why I made this is a Event subclass. > > Source/WebCore/dom/BatteryStatusEvent.cpp:30 > > + : Event(eventNames().batterystatusEvent, false, true) > > These false/true are meaningless. Not your fault, but Event should eventually be changed to take enums instead. > OK. I'will fix after enums are made. > > Source/WebCore/dom/BatteryStatusEvent.cpp:31 > > + , m_isPlugged(false), m_level(0) > > Normally we do these one per line. It's fixed.

Note, the patch doesn't match the current draft (missed the additions to window and workers), and since the draft may still change (based on some comments in the mailing list), things should be webkit prefixed.

(In reply to comment #11) > Note, the patch doesn't match the current draft (missed the additions to window > and workers), and since the draft may still change (based on some comments in the mailing list), things should be webkit prefixed. I'm going to add window part patch very soon. And could you explain what prefix mean? I understand that is "-webkit-" prefix, but I'm not sure because I haven't seen events have a prefix. Did you mean prefix is like "-webkit-onbatterystatus" and "-webkit-batterystatus"? Thank you for your comment.

(In reply to comment #12) > I understand that is "-webkit-" prefix, but I'm not sure because I haven't seen events have a prefix. > Did you mean prefix is like "-webkit-onbatterystatus" and "-webkit-batterystatus"? Events do have prefixes. http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#extending_events-prefixes So, if you add support for onfoo listener, it would be, I think, onwebkitbatterystatus and the event type would be webkitbatterystatus.

Created attachment 97937 [details] Add mock class and change event name to webkit' prefix added.

Attachment 97937 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/mock/BatteryStatusClientMock.h:33: Missing space inside { }. [whitespace/braces] [5] Source/WebCore/platform/mock/BatteryStatusClientMock.cpp:23: You should add a blank line after implementation file's own header. [build/include_order] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 97938 [details] Add mock class for client and change event name to 'webkit' prefix adding.

Comment on attachment 97938 [details] Add mock class for client and change event name to 'webkit' prefix adding. View in context: https://bugs.webkit.org/attachment.cgi?id=97938&action=review > Source/WebCore/dom/BatteryStatusClient.h:34 > + virtual void setBatteryStatus(bool isPlugged, float level) = 0; Prefer enums to booleans. > Source/WebCore/dom/BatteryStatusController.cpp:74 > + Vector<RefPtr<DOMWindow> > listenersVector; Stray space after <DOMWindow>. > Source/WebCore/dom/BatteryStatusController.cpp:78 > + for (size_t i = 0; i < listenersVector.size(); ++i) > + listenersVector[i]->dispatchEvent(event); You should use a better name than 'i'. > Source/WebCore/dom/BatteryStatusController.h:43 > + void didChangeBatteryStatus(bool isPlugged, float level); Prefer enums to booleans. Also, no parameter names on header files. > Source/WebCore/dom/BatteryStatusController.h:50 > + typedef HashCountedSet<RefPtr<DOMWindow> > ListenersCountedSet; Stray space after <DOMWindow>.

Comment on attachment 97938 [details] Add mock class for client and change event name to 'webkit' prefix adding. View in context: https://bugs.webkit.org/attachment.cgi?id=97938&action=review Every one of the review comments here is incorrect! >> Source/WebCore/dom/BatteryStatusClient.h:34 >> + virtual void setBatteryStatus(bool isPlugged, float level) = 0; > > Prefer enums to booleans. We only prefer enums if callers are likely to be passing constants. >> Source/WebCore/dom/BatteryStatusController.cpp:74 >> + Vector<RefPtr<DOMWindow> > listenersVector; > > Stray space after <DOMWindow>. That is a required space, not a stray space. Otherwise older C++ compilers get confused and think it’s the >> operation. >> Source/WebCore/dom/BatteryStatusController.cpp:78 >> + listenersVector[i]->dispatchEvent(event); > > You should use a better name than 'i'. No, i is the standard name for a simple loop index in a case like this, and this is correct style for WebKit. >> Source/WebCore/dom/BatteryStatusController.h:43 >> + void didChangeBatteryStatus(bool isPlugged, float level); > > Prefer enums to booleans. Also, no parameter names on header files. That’s wrong. We do want parameter names on header files when the type alone does not make the parameter clear. >> Source/WebCore/dom/BatteryStatusController.h:50 >> + typedef HashCountedSet<RefPtr<DOMWindow> > ListenersCountedSet; > > Stray space after <DOMWindow>. Same as above. This is right, not wrong.

Leandro, are you a WebKit reviewer?

Comment on attachment 97938 [details] Add mock class for client and change event name to 'webkit' prefix adding. Per Darin's comments, I'm resetting the flags to '?'.

(In reply to comment #19) > Leandro, are you a WebKit reviewer? I'm not. However, I'm trying to help out and informally reviewing EFL-related patches; this one got in the mix because I happen to be watching people that often submit patches to that port. I'm setting r- as a suggestion from eseidel, so that these patches do not appear on http://webkit.org/pending-review and as a precaution so that reviewers unfamiliar with EFL don't review/rubber-stamp patches that might end up having to be rolled out. For these reasons, from now on I'll only r- EFL-related patches. I've already reset the flags to '?'. Regarding your comments, thanks -- I didn't know about old compilers getting confused with <T1<T2>>, the boolean/enum choice, and 'i' being an OK identifier for simple loops. I usually try to copy the style found on other files, but I've seen some violations (specially on older code) in the past so I try to refer only to the guidelines. Maybe I should file bugs to add notes about these on the coding style guidelines?

(In reply to comment #21) > Regarding your comments, thanks -- I didn't know about old compilers getting confused with <T1<T2>>, the boolean/enum choice, and 'i' being an OK identifier for simple loops. Sure, would be good to have those in the style guidelines. Except probably for the <X, <Y>> one, because that’s more of a C++ FAQ rather than something WebKit-specific. When I say “older compilers”, I think that means almost all compilers with only a few exceptions of quite-new ones. But we should keep an eye on which compilers we have to compile with. Would be nice to get rid of that once we require support only for the new ones.

Created attachment 98894 [details] Add ClientMock class Method implementation

Comment on attachment 98894 [details] Add ClientMock class Method implementation View in context: https://bugs.webkit.org/attachment.cgi?id=98894&action=review Generally looks quite good, comments: Where are you handling suspend/resume? When the document is suspended you dont want to keep listening/polling. Also what if I have two batteries, ie a reserve one as well? How is that handled? > Source/WebCore/ChangeLog:8 > + Add New Classes to the WebCore for new Event(BatteryStatusEvnet) "new classes"* > Source/WebCore/ChangeLog:9 > + - BatteryStatusClient is a interface class for each port. "an interface" > Source/WebCore/ChangeLog:10 > + - BatteryStatusController is a control class between client and WebCore. controller? > Source/WebCore/ChangeLog:11 > + this class has a event listeners for BatteryStatusEvent handling This* listener without s > Source/WebCore/ChangeLog:13 > + - BatteryStatusClientMock is a mock class for clinet port client* > Source/WebCore/ChangeLog:16 > + Test cases will be added as soon as after the whole feature is implemented Add dot at the end as we use proper sentenses > Source/WebCore/dom/BatteryStatusClient.h:32 > +public: > + > + virtual void setController(BatteryStatusController*) = 0; I would remove that newline > Source/WebCore/dom/BatteryStatusClient.h:35 > + virtual void setBatteryStatus(bool isPlugged, float level) = 0; Might this grow in the future? I was wondering if you should split it into two methods: setBatteryLevel() setIsCharging() > Source/WebCore/dom/BatteryStatusEvent.cpp:50 > + m_isPlugged = isPlugged; Does this mean that it is charging as well? > Source/WebCore/platform/mock/BatteryStatusClientMock.cpp:54 > + if (m_level == level || m_isPlugged == isPlugged) shouldnt that be an && ? > Source/WebCore/platform/mock/BatteryStatusClientMock.h:52 > + float m_level; > + > +}; One newline too many there.

> > Source/WebCore/dom/BatteryStatusClient.h:29 > > +class BatteryStatusClient { > > Why should this be a WebKit client as opposed to a WebCore platform interface? I'm not sure that it matters, but I'm curious why you made the choice to go up through the WebKit layer instead of down through WebCore platform. For the other clients it is done to that it can be sandboxed. Not sure if that is important here. It also might make our WebKit2 code a bit more nicely separates as there are multiply ports of that "port".

Created attachment 98901 [details] Modify for comment #24

(In reply to comment #25) > > > Source/WebCore/dom/BatteryStatusClient.h:29 > > > +class BatteryStatusClient { > > > > Why should this be a WebKit client as opposed to a WebCore platform interface? I'm not sure that it matters, but I'm curious why you made the choice to go up through the WebKit layer instead of down through WebCore platform. > > For the other clients it is done to that it can be sandboxed. Not sure if that is important here. It also might make our WebKit2 code a bit more nicely separates as there are multiply ports of that "port". I almost fix according to your comment. And, In my opinion, a client interface have to be referenced by all port. So, core part is a right position for client interface to me. Don't you think about that? Thank you for your comment.

Created attachment 99940 [details] Add events for the revised spec. http://dev.w3.org/2009/dap/system-info/battery-status.html

Created attachment 129681 [details] Patch. I have an issue about this patch. Battery Status API has to be added under the Navigator class, but there is no event handling logic in the navigator. So I implemented event listener handling logic to the BatteryManager, because I didn't have other choice, but I think this is not good to add event listener to the Battery Status API. I think navigator needs event listener handling logic like a DOMWindow. Leave some comments about this please. Thank you.

Attachment 129681 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/dom/BatteryStatus/add-lis..." exit_code: 1 Source/WebCore/Modules/battery/BatteryController.cpp:89: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WebCore/Modules/battery/BatteryController.cpp:89: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/Modules/battery/BatteryController.h:46: The parameter name "batteryStatus" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.h:52: One space before end of line comments [whitespace/comments] [5] Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.h:53: One space before end of line comments [whitespace/comments] [5] Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:21: You should add a blank line after implementation file's own header. [build/include_order] [4] Total errors found: 6 in 55 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 129681 [details] Patch. Attachment 129681 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11766467

I doubt you want to disable SVG support for this feature.;)

Comment on attachment 129681 [details] Patch. Attachment 129681 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11751432 New failing tests: fast/dom/BatteryStatus/updates.html fast/dom/BatteryStatus/multiple-frames.html fast/dom/BatteryStatus/add-listener-from-callback.html fast/dom/BatteryStatus/no-page-cache.html fast/dom/BatteryStatus/event-after-navigation.html fast/dom/BatteryStatus/window-property.html fast/dom/BatteryStatus/basic-operation.html

Comment on attachment 129681 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=129681&action=review > Tools/Scripts/build-webkit:320 > - define => "ENABLE_SVG", default => 1, value => \$svgSupport }, > + define => "ENABLE_SVG", default => 0, value => \$svgSupport }, You should not change these defaults, specially not in an unrelated patch. > Tools/Scripts/build-webkit:326 > - define => "ENABLE_SVG_FONTS", default => 1, value => \$svgFontsSupport }, > + define => "ENABLE_SVG_FONTS", default => 0, value => \$svgFontsSupport }, Ditto. > Tools/Scripts/build-webkit:344 > - define => "ENABLE_VIDEO_TRACK", default => (isAppleWebKit() || isGtk()), value => \$videoTrackSupport }, > + define => "ENABLE_VIDEO_TRACK", default => (isAppleWebKit() || isGtk() || isEfl()), value => \$videoTrackSupport }, Unrelated, better have it in a separate patch.

FWIW, the changes in the svg defaults is breaking the EWS =(

Created attachment 129806 [details] Patch. I updated the test patch yesterday - sorry about the delay in doing so. I would need your comments for the issue mentioned below. Battery Status API needs to be added under the Navigator class, but there is not any event handling logic there. Therefore, because I have no other choice, I have implemented the necessary event handling logic in the BatteryManager. I, however, think it may not look good to add the event listener in the Batter Status API itself. IMHO, the Navigator class would need some sort of event listener handling logic similar to DOMWindow. I would like to have your comments how you think this. Thank you.

Comment on attachment 129806 [details] Patch. Attachment 129806 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11751757

Comment on attachment 129806 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=129806&action=review > Source/WebCore/Modules/battery/BatteryStatus.cpp:38 > + Unneeded line. > Source/WebCore/dom/Document.cpp:1960 > + return; Need to add an empty line. > Source/WebCore/dom/EventNames.h:61 > + macro(dischargingtimechange) \ Alphabetical sorting nit. > Source/WebKit/efl/ewk/ewk_view.cpp:780 > + smartData->_priv = _ewk_view_priv_new(smartData); Why do you change location of private data creation ? > Source/WebKit/efl/ewk/ewk_view.h:125 > + Eina_Bool useMock; We are using efl coding style in public header. > Source/WebKit/efl/ewk/ewk_view.h:2368 > +EAPI void ewk_view_dump_render_tree_mode_enable(Evas_Object* ewkView, Eina_Bool flag); variable name is wrong. Please read WebKit EFL coding style first. http://trac.webkit.org/wiki/EFLWebKitCodingStyle

Comment on attachment 129806 [details] Patch. There was conflict when I post my comment. Win port buildbot rejected this patch. So, I reject this patch again.

Comment on attachment 129806 [details] Patch. Attachment 129806 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11776814

Comment on attachment 129806 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=129806&action=review Thank you for your kind review. >> Source/WebCore/Modules/battery/BatteryStatus.cpp:38 >> + > > Unneeded line. OK. >> Source/WebCore/dom/Document.cpp:1960 >> + return; > > Need to add an empty line. You are right. >> Source/WebCore/dom/EventNames.h:61 >> + macro(dischargingtimechange) \ > > Alphabetical sorting nit. OK. >> Source/WebKit/efl/ewk/ewk_view.cpp:780 >> + smartData->_priv = _ewk_view_priv_new(smartData); > > Why do you change location of private data creation ? The smartData->api have to set before calling _ewk_view_priv_new(). Because use_mock which is in the api is used in the _ewk_view_priv_new(), >> Source/WebKit/efl/ewk/ewk_view.h:125 >> + Eina_Bool useMock; > > We are using efl coding style in public header. OK. I will be fix. >> Source/WebKit/efl/ewk/ewk_view.h:2368 >> +EAPI void ewk_view_dump_render_tree_mode_enable(Evas_Object* ewkView, Eina_Bool flag); > > variable name is wrong. Please read WebKit EFL coding style first. > http://trac.webkit.org/wiki/EFLWebKitCodingStyle OK.

Created attachment 129837 [details] Patch. I would need your comments for the issue mentioned below. Battery Status API needs to be added under the Navigator class, but there is not any event handling logic there. Therefore, because I have no other choice, I have implemented the necessary event handling logic in the BatteryManager. I, however, think it may not look good to add the event listener in the Batter Status API itself. IMHO, the Navigator class would need some sort of event listener handling logic similar to DOMWindow. I would like to have your comments how you think this. Thank you.

Comment on attachment 129837 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=129837&action=review > Source/WebCore/dom/Document.cpp:1964 > +#if ENABLE(BATTERY_STATUS) > + if (!page()) > + return; > + > + if (BatteryController* batteryController = BatteryController::from(page())) > + batteryController->removeAllListeners(); > +#endif This change shouldn't be needed. We should find a way for BatteryController to observe this condition and do this work itself.

Created attachment 130562 [details] Modified the patch according to comment #43.

Comment on attachment 130562 [details] Modified the patch according to comment #43. View in context: https://bugs.webkit.org/attachment.cgi?id=130562&action=review > LayoutTests/fast/dom/BatteryStatus/no-page-cache-expected.txt:1 > +Tests that pages that use BatteryStatus are not put in the page cache. Really? That is pretty sad. Why not do the extra work to make it possible to have them in the cache. If any spec changes are needed for that (shouldn't be needed), I have contacts > Source/WebCore/ChangeLog:6 > + This API is implemented under the Navigator class. If so, it would be really good if you made sure this works properly when suspending and resuming, like there is no reason to send updates if all browser windows subscribed are in the background, nor if the document is suspended for panning/scaling or any other kind of animation. > Source/WebCore/ChangeLog:11 > + There are four types of events: onchargingchange, onchargingtimechange, ondischargingtimechange, onlevelchange. > + All events are operated based on a callback mechanism. > + When battery event has raised, the event listener watching battery status has to be called. > + The battery status can be accessed using BatteryManager(navigator.webkitBattery). > + http://www.w3.org/TR/battery-status/ This changelog could be formatted a bit better to make it more readable > Source/WebCore/Modules/battery/BatteryController.cpp:41 > + for (size_t i = 0; i < m_listeners.size(); ++i) > + m_listeners[i]->batteryControllerDestroyed(); Why not use iterators? > Source/WebKit/efl/ChangeLog:6 > + Add BatteryClientEfl and DumpRenerTreeSupportEfl class implementation for the layout tests. Spelling issue with Render* > Tools/ChangeLog:6 > + Add setMockBatteryStatus API to layoutTestController for testing the Battery Status API. Why is this not in the internals object instead? This is not something anyone would implement with a public API anyway. And there is talk about moving more things to the internals object. Maybe you could talk to rniwa

(In reply to comment #45) > (From update of attachment 130562 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130562&action=review > > > LayoutTests/fast/dom/BatteryStatus/no-page-cache-expected.txt:1 > > +Tests that pages that use BatteryStatus are not put in the page cache. > > Really? That is pretty sad. Why not do the extra work to make it possible to have them in the cache. If any spec changes are needed for that (shouldn't be needed), I have contacts I'm sorry about this is not proper test for the Battery Status API There is no issue between page-cache and battery status API. But I think if the Battery Status API inherit ActiveDOMObject for suspend and resume, page cache will be working well. > > > Source/WebCore/ChangeLog:6 > > + This API is implemented under the Navigator class. > > If so, it would be really good if you made sure this works properly when suspending and resuming, like there is no reason to send updates if all browser windows subscribed are in the background, nor if the document is suspended for panning/scaling or any other kind of animation. ditto. > > > Source/WebCore/ChangeLog:11 > > + There are four types of events: onchargingchange, onchargingtimechange, ondischargingtimechange, onlevelchange. > > + All events are operated based on a callback mechanism. > > + When battery event has raised, the event listener watching battery status has to be called. > > + The battery status can be accessed using BatteryManager(navigator.webkitBattery). > > + http://www.w3.org/TR/battery-status/ > > This changelog could be formatted a bit better to make it more readable > OK. I will do this. > > Source/WebCore/Modules/battery/BatteryController.cpp:41 > > + for (size_t i = 0; i < m_listeners.size(); ++i) > > + m_listeners[i]->batteryControllerDestroyed(); > > Why not use iterators? I will change this. > > > Source/WebKit/efl/ChangeLog:6 > > + Add BatteryClientEfl and DumpRenerTreeSupportEfl class implementation for the layout tests. > > Spelling issue with Render* > OK, It will be fixed. > > Tools/ChangeLog:6 > > + Add setMockBatteryStatus API to layoutTestController for testing the Battery Status API. > > Why is this not in the internals object instead? This is not something anyone would implement with a public API anyway. And there is talk about moving more things to the internals object. Maybe you could talk to rniwa OK, I will try to talk to rniwa about this. Thanks a lot.

Created attachment 132019 [details] Patch.

Comment on attachment 132019 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=132019&action=review > Source/WebCore/dom/Document.cpp:213 > +#if ENABLE(BATTERY_STATUS) > +#include "BatteryController.h" > +#include "NavigatorBattery.h" > +#endif Why are these headers needed if there are no other changes to Document?

(In reply to comment #48) > (From update of attachment 132019 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132019&action=review > > > Source/WebCore/dom/Document.cpp:213 > > +#if ENABLE(BATTERY_STATUS) > > +#include "BatteryController.h" > > +#include "NavigatorBattery.h" > > +#endif > > Why are these headers needed if there are no other changes to Document? This is my mistake. I will fix it.

Comment on attachment 132019 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=132019&action=review This looks good. I've got a couple nits below, but once those are addressed we can land this patch. Thanks! > LayoutTests/ChangeLog:11 > + * fast/dom/BatteryStatus/add-listener-from-callback-expected.txt: Added. I might have added these tests to a top-level directory in LayoutTests called battery-status, but the location of the tests aren't super important. > Source/WebCore/Modules/battery/BatteryController.cpp:60 > + if (pos == WTF::notFound) > + return; Should it be an error to remove a listener that not in m_listeners? > Source/WebCore/Modules/battery/BatteryController.h:49 > + BatteryController(BatteryClient*); Please add the explicit keyword to single-argument constructors. > Source/WebCore/Modules/battery/BatteryController.h:52 > + typedef Vector<BatteryManager*> ListenerVector; Normally we'd put this declaration right below "private:" with a blank line between it and the constructor.

(In reply to comment #50) > (From update of attachment 132019 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132019&action=review > > This looks good. I've got a couple nits below, but once those are addressed we can land this patch. Thanks! > Thank you for your review comments. After updating my patch, I will address this patch at webkit-dev mailing list. > > LayoutTests/ChangeLog:11 > > + * fast/dom/BatteryStatus/add-listener-from-callback-expected.txt: Added. > > I might have added these tests to a top-level directory in LayoutTests called battery-status, but the location of the tests aren't super important. > OK, I will move location to top-level directory. > > Source/WebCore/Modules/battery/BatteryController.cpp:60 > > + if (pos == WTF::notFound) > > + return; > > Should it be an error to remove a listener that not in m_listeners? > I think there is no check for WTF::notFound in the Vector::remove but I will make sure about this by re-checking, after that if I can remove this without any error, I will remove it. > > Source/WebCore/Modules/battery/BatteryController.h:49 > > + BatteryController(BatteryClient*); > > Please add the explicit keyword to single-argument constructors. > OK, I will add it. > > Source/WebCore/Modules/battery/BatteryController.h:52 > > + typedef Vector<BatteryManager*> ListenerVector; > > Normally we'd put this declaration right below "private:" with a blank line between it and the constructor. OK. I will change position of typedef.

Created attachment 132193 [details] Patch for landing.

Created attachment 132203 [details] Patch for landing.

Comment on attachment 132203 [details] Patch for landing. Clearing flags on attachment: 132203 Committed r110991: <http://trac.webkit.org/changeset/110991>

All reviewed patches have been landed. Closing bug.

This patch seems to make the EFL port crash in fast/dom/navigator-detached-nocrash.html -- see bug 81773. BatteryManager::BatteryManager is always considering Navigator::frame() returns non-NULL. It'd be good to have some response from Kihong, otherwise I'm tempted to roll it out.

Comment on attachment 132203 [details] Patch for landing. View in context: https://bugs.webkit.org/attachment.cgi?id=132203&action=review > Source/WebCore/Modules/battery/NavigatorBattery.cpp:41 > +{ I think you just need to add a null check for navigator->frame() here and return 0.

(In reply to comment #57) > (From update of attachment 132203 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132203&action=review > > > Source/WebCore/Modules/battery/NavigatorBattery.cpp:41 > > +{ > > I think you just need to add a null check for navigator->frame() here and return 0. That works fine indeed, thanks. Fix landed in <http://trac.webkit.org/changeset/111770>.

Thanks for reporting the problem and the patch for it Kubo and Adam.

This was removed in https://trac.webkit.org/changeset/208300