[Webkit-unassigned] [Bug 62698] Support for Battery Status API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=62698





--- Comment #51 from Kihong Kwon <kihong.kwon at samsung.com>  2012-03-15 19:14:21 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.