[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 #18 from Darin Adler <darin at apple.com>  2011-06-27 11:43:59 PST ---
(From update of attachment 97938)
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.

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