[Webkit-unassigned] [Bug 62698] [DOM] Support for battery status event

[bugzilla-daemon at webkit.org]

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





--- Comment #24 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2011-06-28 03:49:50 PST ---
(From update of attachment 98894)
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.

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