Bug 62698 - (BatteryStatusAPI) Support for Battery Status API
(BatteryStatusAPI)
: Support for Battery Status API
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: All All
: P3 Enhancement
Assigned To:
: http://www.w3.org/TR/2011/WD-battery-...
:
: 63376 79900
: 78947 84386
  Show dependency treegraph
 
Reported: 2011-06-14 23:34 PST by
Modified: 2012-04-19 14:46 PST (History)


Attachments
Proposed patch (13.44 KB, patch)
2011-06-14 23:51 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff
Delete blank line and comment line (Fix for comment #3, #5 (13.36 KB, patch)
2011-06-15 01:43 PST, Kihong Kwon
eric: review-
Review Patch | Details | Formatted Diff | Diff
Modify about comment #8 (13.49 KB, patch)
2011-06-15 23:53 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff
Add mock class and change event name to webkit' prefix added. (17.97 KB, patch)
2011-06-20 23:52 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff
Add mock class for client and change event name to 'webkit' prefix adding. (17.93 KB, patch)
2011-06-21 00:01 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff
Add ClientMock class Method implementation (18.25 KB, patch)
2011-06-28 03:37 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff
Modify for comment #24 (21.88 KB, patch)
2011-06-28 05:21 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff
Add events for the revised spec. (21.97 KB, patch)
2011-07-06 23:23 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff
Patch. (70.82 KB, patch)
2012-03-01 05:13 PST, Kihong Kwon
gns: review-
gyuyoung.kim: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch. (84.10 KB, patch)
2012-03-01 21:37 PST, Kihong Kwon
gyuyoung.kim: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch. (91.81 KB, patch)
2012-03-02 00:24 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff
Modified the patch according to comment #43. (89.38 KB, patch)
2012-03-07 00:56 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff
Patch. (69.97 KB, patch)
2012-03-15 04:41 PST, Kihong Kwon
abarth: review+
abarth: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch for landing. (68.69 KB, patch)
2012-03-15 21:38 PST, Kihong Kwon
abarth: commit‑queue+
Review Patch | Details | Formatted Diff | Diff
Patch for landing. (68.66 KB, patch)
2012-03-15 23:03 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2011-06-14 23:34:55 PST
I would like to add battery status event to WebKit.
This property is a new feature that is defined by W3C and it indicates current battery status of device.

Firstable, I'm going to add basic classes about this event.
(BatteryStatusEvent, BatteryStatusController, BatteryStatusClient)

After that I'm going to add codes to the WebKitCore related this event.
Thank you.
------- Comment #1 From 2011-06-14 23:51:17 PST -------
Created an attachment (id=97242) [details]
Proposed patch
------- Comment #2 From 2011-06-14 23:57:31 PST -------
CC'ing Eric, Darin and Andersca.
------- Comment #3 From 2011-06-15 00:00:14 PST -------
(From update of attachment 97242 [details])
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.
------- Comment #4 From 2011-06-15 00:03:06 PST -------
AFAIK, you should make test cases for new features. Do you prepare test cases for this feature ?
------- Comment #5 From 2011-06-15 00:27:30 PST -------
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?
------- Comment #6 From 2011-06-15 01:25:15 PST -------
(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.
------- Comment #7 From 2011-06-15 01:43:19 PST -------
Created an attachment (id=97255) [details]
Delete blank line and comment line (Fix for comment #3, #5
------- Comment #8 From 2011-06-15 07:07:15 PST -------
(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.

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.
------- Comment #9 From 2011-06-15 23:53:57 PST -------
Created an attachment (id=97408) [details]
Modify about comment #8
------- Comment #10 From 2011-06-16 00:14:37 PST -------
(In reply to comment #8)
> (From update of attachment 97255 [details] [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.
------- Comment #11 From 2011-06-18 02:27:13 PST -------
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.
------- Comment #12 From 2011-06-19 21:52:37 PST -------
(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.
------- Comment #13 From 2011-06-20 03:10:47 PST -------
(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.
------- Comment #14 From 2011-06-20 23:52:04 PST -------
Created an attachment (id=97937) [details]
Add mock class and change event name to webkit' prefix added.
------- Comment #15 From 2011-06-20 23:53:29 PST -------
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.
------- Comment #16 From 2011-06-21 00:01:01 PST -------
Created an attachment (id=97938) [details]
Add mock class for client and change event name to 'webkit' prefix adding.
------- Comment #17 From 2011-06-27 10:38:50 PST -------
(From update of attachment 97938 [details])
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 #18 From 2011-06-27 11:43:59 PST -------
(From update of attachment 97938 [details])
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.
------- Comment #19 From 2011-06-27 11:45:39 PST -------
Leandro, are you a WebKit reviewer?
------- Comment #20 From 2011-06-27 14:06:06 PST -------
(From update of attachment 97938 [details])
Per Darin's comments, I'm resetting the flags to '?'.
------- Comment #21 From 2011-06-27 14:22:25 PST -------
(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?
------- Comment #22 From 2011-06-27 14:31:41 PST -------
(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.
------- Comment #23 From 2011-06-28 03:37:35 PST -------
Created an attachment (id=98894) [details]
Add ClientMock class Method implementation
------- Comment #24 From 2011-06-28 03:49:50 PST -------
(From update of attachment 98894 [details])
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.
------- Comment #25 From 2011-06-28 03:52:39 PST -------
> > 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".
------- Comment #26 From 2011-06-28 05:21:13 PST -------
Created an attachment (id=98901) [details]
Modify for comment #24
------- Comment #27 From 2011-06-28 05:30:02 PST -------
(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.
------- Comment #28 From 2011-07-06 23:23:36 PST -------
Created an attachment (id=99940) [details]
Add events for the revised spec.

http://dev.w3.org/2009/dap/system-info/battery-status.html
------- Comment #29 From 2012-03-01 05:13:15 PST -------
Created an attachment (id=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.
------- Comment #30 From 2012-03-01 06:19:21 PST -------
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 #31 From 2012-03-01 06:26:16 PST -------
(From update of attachment 129681 [details])
Attachment 129681 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11766467
------- Comment #32 From 2012-03-01 06:34:59 PST -------
I doubt you want to disable SVG support  for this feature.;)
------- Comment #33 From 2012-03-01 06:56:19 PST -------
(From update of attachment 129681 [details])
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 #34 From 2012-03-01 08:49:57 PST -------
(From update of attachment 129681 [details])
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.
------- Comment #35 From 2012-03-01 08:52:25 PST -------
FWIW, the changes in the svg defaults is breaking the EWS =(
------- Comment #36 From 2012-03-01 21:37:21 PST -------
Created an attachment (id=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 #37 From 2012-03-01 22:04:05 PST -------
(From update of attachment 129806 [details])
Attachment 129806 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11751757
------- Comment #38 From 2012-03-01 22:18:19 PST -------
(From update of attachment 129806 [details])
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 #39 From 2012-03-01 22:19:48 PST -------
(From update of attachment 129806 [details])
There was conflict when I post my comment. Win port buildbot rejected this patch. So, I reject this patch again.
------- Comment #40 From 2012-03-01 22:34:54 PST -------
(From update of attachment 129806 [details])
Attachment 129806 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11776814
------- Comment #41 From 2012-03-01 23:20:54 PST -------
(From update of attachment 129806 [details])
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.
------- Comment #42 From 2012-03-02 00:24:46 PST -------
Created an attachment (id=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 #43 From 2012-03-05 00:39:18 PST -------
(From update of attachment 129837 [details])
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.
------- Comment #44 From 2012-03-07 00:56:06 PST -------
Created an attachment (id=130562) [details]
Modified the patch according to comment #43.
------- Comment #45 From 2012-03-07 01:23:40 PST -------
(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

> 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
------- Comment #46 From 2012-03-07 03:53:50 PST -------
(In reply to comment #45)
> (From update of attachment 130562 [details] [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.
------- Comment #47 From 2012-03-15 04:41:07 PST -------
Created an attachment (id=132019) [details]
Patch.
------- Comment #48 From 2012-03-15 11:02:29 PST -------
(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?
------- Comment #49 From 2012-03-15 17:57:06 PST -------
(In reply to comment #48)
> (From update of attachment 132019 [details] [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 #50 From 2012-03-15 18:12:31 PST -------
(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!

> 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.
------- Comment #51 From 2012-03-15 19:14:21 PST -------
(In reply to comment #50)
> (From update of attachment 132019 [details] [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.
------- Comment #52 From 2012-03-15 21:38:21 PST -------
Created an attachment (id=132193) [details]
Patch for landing.
------- Comment #53 From 2012-03-15 23:03:57 PST -------
Created an attachment (id=132203) [details]
Patch for landing.
------- Comment #54 From 2012-03-16 05:23:27 PST -------
(From update of attachment 132203 [details])
Clearing flags on attachment: 132203

Committed r110991: <http://trac.webkit.org/changeset/110991>
------- Comment #55 From 2012-03-16 05:23:50 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #56 From 2012-03-22 11:00:02 PST -------
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 #57 From 2012-03-22 11:38:49 PST -------
(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.
------- Comment #58 From 2012-03-22 15:04:07 PST -------
(In reply to comment #57)
> (From update of attachment 132203 [details] [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>.
------- Comment #59 From 2012-03-22 18:42:07 PST -------
Thanks for reporting the problem and the patch for it Kubo and Adam.