[webkit-reviews] review denied: [Bug 71518] [Chromium] gamepad changes to the public interface of Chromium port : [Attachment 113572] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Scott Graham
<scottmg at chromium.org>'s request for review:
Bug 71518: [Chromium] gamepad changes to the public interface of Chromium port
https://bugs.webkit.org/show_bug.cgi?id=71518

Attachment 113572: Patch
https://bugs.webkit.org/attachment.cgi?id=113572&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=113572&action=review


> Source/WebKit/chromium/public/WebGamepads.h:29
> +const int maximumGamepadIdLength = 128;

we normally define these within the class so that they are scoped to the class
name.

also you might consider following WebInputEvent as a guide here.  it uses the
term
fooLengthCap instead of maximumFooLength.  i think it would make sense for
WebGamepad
to define axesLengthCap and buttonsLengthCap.

> Source/WebKit/chromium/public/WebGamepads.h:35
> +public:

how about defining a default constructor?  i know you intend to treat this
as POD data, and POD types technically should not have any methods, but in
practice a constructor is OK.  see WebInputEvent, which works similarly.

> Source/WebKit/chromium/public/WebGamepads.h:50
> +    unsigned numAxes;

uber-nit: we usually spell out "numberOf" instead of using the common "num"
abbreviation.  however, in this case, had you considered using a Length suffix?
 it might be nice for it to be axes and axesLength instead of axes and
numberOfAxes (just as descriptive, but starts with axes so that the first
letter is also lower-case).  this way you would have axes, axesLength, and
axesLengthCap.

> Source/WebKit/chromium/public/WebGamepads.h:64
> +class WebGamepads {

one top-level type per file is the rule for webkit APIs.

nit: when a class's name is the same as one of its member variables, i usually
look to
rename one of them.  that way you avoid code having confusing expressions like
"foo.foo"
In this case you could rename gamepads to items, or you could rename
WebGamepads to
WebGamepad{Array,List,Vector}.

> Source/WebKit/chromium/public/WebGamepads.h:67
> +    int numGamepads;

above, you use unsigned for numAxes and numButtons.  should be unsigned here
too?

> Source/WebKit/chromium/public/WebKitPlatformSupport.h:112
> +    // Gamepad -------------------------------------------------------------


nit: add a new line after separator comment.

> Source/WebKit/chromium/public/WebKitPlatformSupport.h:113
> +    virtual void pollGamepads(WebGamepads& pads) { pads.numGamepads = 0; }

question unrelated to this patch: would there be any benefit to knowing which
gamepad the application has selected?

i don't know what's better, but it also occurred to me that "sample" might be a
nice prefix for this method.
sampleGamepads or sampleAvailableGamepads.  another idea is query:
queryGamepads, queryAvailableGamepads.
i'm just mentioning the "Available" modifier, as it might be useful.  not sure.