Bug 62698 (BatteryStatusAPI)

Summary: Support for Battery Status API
Product: WebKit Reporter: Kihong Kwon <kihong.kwon>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: abarth, ahf, allan.jensen, andersca, anssi.kostiainen, ap, bugs, cshu, darin, dglazkov, donggwan.kim, dwim79, eric, gustavo, gyuyoung.kim, js45.yang, kenneth, laszlo.gombos, leandro, michelangelo, ojan, rakuco, ravi.kasibhatla, rniwa, ryuan.choi, sangseok.lim, s.choi, simon.fraser, tkent, tonikitoo, vimff0, webkit.review.bot, wonsuk11.lee, xan.lopez, yael
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.w3.org/TR/2011/WD-battery-status-20111129/
Bug Depends on: 63376, 79900    
Bug Blocks: 78947, 84386    
Attachments:
Description Flags
Proposed patch
none
Delete blank line and comment line (Fix for comment #3, #5
eric: review-
Modify about comment #8
none
Add mock class and change event name to webkit' prefix added.
none
Add mock class for client and change event name to 'webkit' prefix adding.
none
Add ClientMock class Method implementation
none
Modify for comment #24
none
Add events for the revised spec.
none
Patch.
gustavo: review-, gyuyoung.kim: commit-queue-
Patch.
gyuyoung.kim: commit-queue-
Patch.
none
Modified the patch according to comment #43.
none
Patch.
abarth: review+, abarth: commit-queue-
Patch for landing.
abarth: commit-queue+
Patch for landing. none

Description Kihong Kwon 2011-06-14 23:34:55 PDT
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 Kihong Kwon 2011-06-14 23:51:17 PDT
Created attachment 97242 [details]
Proposed patch
Comment 2 Gyuyoung Kim 2011-06-14 23:57:31 PDT
CC'ing Eric, Darin and Andersca.
Comment 3 Gyuyoung Kim 2011-06-15 00:00:14 PDT
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.
Comment 4 Gyuyoung Kim 2011-06-15 00:03:06 PDT
AFAIK, you should make test cases for new features. Do you prepare test cases for this feature ?
Comment 5 Ryuan Choi 2011-06-15 00:27:30 PDT
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 Kihong Kwon 2011-06-15 01:25:15 PDT
(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 Kihong Kwon 2011-06-15 01:43:19 PDT
Created attachment 97255 [details]
Delete blank line and comment line (Fix for comment #3, #5
Comment 8 Eric Seidel (no email) 2011-06-15 07:07:15 PDT
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.
Comment 9 Kihong Kwon 2011-06-15 23:53:57 PDT
Created attachment 97408 [details]
Modify about comment #8
Comment 10 Kihong Kwon 2011-06-16 00:14:37 PDT
(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.
Comment 11 Olli Pettay (:smaug) 2011-06-18 02:27:13 PDT
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 Kihong Kwon 2011-06-19 21:52:37 PDT
(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 Olli Pettay (:smaug) 2011-06-20 03:10:47 PDT
(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 Kihong Kwon 2011-06-20 23:52:04 PDT
Created attachment 97937 [details]
Add mock class and change event name to webkit' prefix added.
Comment 15 WebKit Review Bot 2011-06-20 23:53:29 PDT
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 Kihong Kwon 2011-06-21 00:01:01 PDT
Created attachment 97938 [details]
Add mock class for client and change event name to 'webkit' prefix adding.
Comment 17 Leandro Pereira 2011-06-27 10:38:50 PDT
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 18 Darin Adler 2011-06-27 11:43:59 PDT
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.
Comment 19 Darin Adler 2011-06-27 11:45:39 PDT
Leandro, are you a WebKit reviewer?
Comment 20 Leandro Pereira 2011-06-27 14:06:06 PDT
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 '?'.
Comment 21 Leandro Pereira 2011-06-27 14:22:25 PDT
(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 Darin Adler 2011-06-27 14:31:41 PDT
(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 Kihong Kwon 2011-06-28 03:37:35 PDT
Created attachment 98894 [details]
Add ClientMock class Method implementation
Comment 24 Kenneth Rohde Christiansen 2011-06-28 03:49:50 PDT
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.
Comment 25 Kenneth Rohde Christiansen 2011-06-28 03:52:39 PDT
> > 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 Kihong Kwon 2011-06-28 05:21:13 PDT
Created attachment 98901 [details]
Modify for comment #24
Comment 27 Kihong Kwon 2011-06-28 05:30:02 PDT
(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 Kihong Kwon 2011-07-06 23:23:36 PDT
Created attachment 99940 [details]
Add events for the revised spec.

http://dev.w3.org/2009/dap/system-info/battery-status.html
Comment 29 Kihong Kwon 2012-03-01 05:13:15 PST
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.
Comment 30 WebKit Review Bot 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 Gyuyoung Kim 2012-03-01 06:26:16 PST
Comment on attachment 129681 [details]
Patch.

Attachment 129681 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11766467
Comment 32 Michelangelo De Simone 2012-03-01 06:34:59 PST
I doubt you want to disable SVG support  for this feature.;)
Comment 33 WebKit Review Bot 2012-03-01 06:56:19 PST
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 34 Gustavo Noronha (kov) 2012-03-01 08:49:57 PST
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.
Comment 35 Gustavo Noronha (kov) 2012-03-01 08:52:25 PST
FWIW, the changes in the svg defaults is breaking the EWS =(
Comment 36 Kihong Kwon 2012-03-01 21:37:21 PST
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 37 Build Bot 2012-03-01 22:04:05 PST
Comment on attachment 129806 [details]
Patch.

Attachment 129806 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11751757
Comment 38 Gyuyoung Kim 2012-03-01 22:18:19 PST
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 39 Gyuyoung Kim 2012-03-01 22:19:48 PST
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 40 Gustavo Noronha (kov) 2012-03-01 22:34:54 PST
Comment on attachment 129806 [details]
Patch.

Attachment 129806 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11776814
Comment 41 Kihong Kwon 2012-03-01 23:20:54 PST
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.
Comment 42 Kihong Kwon 2012-03-02 00:24:46 PST
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 43 Adam Barth 2012-03-05 00:39:18 PST
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.
Comment 44 Kihong Kwon 2012-03-07 00:56:06 PST
Created attachment 130562 [details]
Modified the patch according to comment #43.
Comment 45 Kenneth Rohde Christiansen 2012-03-07 01:23:40 PST
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
Comment 46 Kihong Kwon 2012-03-07 03:53:50 PST
(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.
Comment 47 Kihong Kwon 2012-03-15 04:41:07 PDT
Created attachment 132019 [details]
Patch.
Comment 48 Adam Barth 2012-03-15 11:02:29 PDT
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?
Comment 49 Kihong Kwon 2012-03-15 17:57:06 PDT
(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 50 Adam Barth 2012-03-15 18:12:31 PDT
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.
Comment 51 Kihong Kwon 2012-03-15 19:14:21 PDT
(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.
Comment 52 Kihong Kwon 2012-03-15 21:38:21 PDT
Created attachment 132193 [details]
Patch for landing.
Comment 53 Kihong Kwon 2012-03-15 23:03:57 PDT
Created attachment 132203 [details]
Patch for landing.
Comment 54 WebKit Review Bot 2012-03-16 05:23:27 PDT
Comment on attachment 132203 [details]
Patch for landing.

Clearing flags on attachment: 132203

Committed r110991: <http://trac.webkit.org/changeset/110991>
Comment 55 WebKit Review Bot 2012-03-16 05:23:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 56 Raphael Kubo da Costa (:rakuco) 2012-03-22 11:00:02 PDT
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 Adam Barth 2012-03-22 11:38:49 PDT
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.
Comment 58 Raphael Kubo da Costa (:rakuco) 2012-03-22 15:04:07 PDT
(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>.
Comment 59 Kihong Kwon 2012-03-22 18:42:07 PDT
Thanks for reporting the problem and the patch for it Kubo and Adam.
Comment 60 Simon Fraser (smfr) 2016-11-02 14:39:24 PDT
This was removed in https://trac.webkit.org/changeset/208300