[Chromium] gamepad changes to the public interface of Chromium port

• Component: WebKit API
• Status: RESOLVED FIXED

[Scott Graham]

Includes only the gamepad changes that touch the public interface of the Chromium port. (There's no "Component: WebKit Chromium"?)

See also https://bugs.webkit.org/show_bug.cgi?id=69451 and http://codereview.chromium.org/8345027

[Scott Graham]

Created attachment 113572 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 113572 [details]
Patch

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.

[Scott Graham]

Comment on attachment 113572 [details]
Patch

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.

Renamed and made more local following style in WebInputEvent.

>> 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.

Done.

>> 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.

Done.

>> 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}.

I put WebGamepad inside WebGamepads to avoid multiple top level. It seemed sort of heavy to make a new file for just the container, and it would separate the pieces that need to be one POD as a unit, so I thought they felt better together. Happy to move to separate files if that's preferred though.

Went with .items and .length on top level structure.

>> Source/WebKit/chromium/public/WebGamepads.h:67
>> +    int numGamepads;
> 
> above, you use unsigned for numAxes and numButtons.  should be unsigned here too?

Done.

>> Source/WebKit/chromium/public/WebKitPlatformSupport.h:112
>> +    // Gamepad -------------------------------------------------------------
> 
> nit: add a new line after separator comment.

Done.

>> 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.

I don't understand the question; the application can use all of the gamepads simultaneously so there's no real selection to be done. We could potentially introduce the concept of a "primary" gamepad but I'm not sure if it's well-defined.

Changed to sampleGamepads as that sounds nicer. At the moment "Available" feels redundant to me, but I could be convinced otherwise.

[Scott Graham]

Created attachment 113902 [details]
updates per review

[Scott Graham]

Created attachment 113948 [details]
update for runtime enable landed

[WebKit Review Bot]

Comment on attachment 113948 [details]
update for runtime enable landed

Attachment 113948 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10332790

[Scott Graham]

Created attachment 113955 [details]
update to ToT

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
> I don't understand the question; the application can use all of the gamepads simultaneously so there's no real selection to be done. We could potentially introduce the concept of a "primary" gamepad but I'm not sure if it's well-defined.
> 
> Changed to sampleGamepads as that sounds nicer. At the moment "Available" feels redundant to me, but I could be convinced otherwise.

Ah, I hadn't considered an app taking input from multiple gamepads at once.  That makes complete sense though!  Thanks for the clarification.

[Darin Fisher (:fishd, Google)]

Comment on attachment 113955 [details]
update to ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=113955&action=review

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

nit: we usually reserve the Web-prefix for top-level types.  Inner types drop the prefix.

I guess this is bike-shedding territory, but all things considered, how about moving
WebGamepad back to the top-level, and then eliminate WebGamepads in favor of just having
the sampleGamepads function return a |const WebGamepad*| that is terminated by an empty
WebGamepad structure or return the length as an out parameter?

  virtual const WebGamepad* sampleGamepads(); // Terminated by "empty" WebGamepad
  virtual const WebGamepad* sampleGamepads(unsigned* length);
  virtual unsigned sampleGamepads(const WebGamepad**);
  virtual unsigned sampleGamepads(WebGamepad* buffer, unsigned bufferLength); // Returns number of WebGamepad elements, up to bufferLength, that are filled in.

Finally, you could also go with:

  virtual WebVector<WebGamepad> sampleGamepads();

^^^ Perhaps the cost of copying is worth the improved code simplicity?

If you don't like such options, then my next suggestion is WebGamepad as a top-level type
with WebGamepads as its own top-level type.

[Scott Graham]

Comment on attachment 113955 [details]
update to ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=113955&action=review

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

> 
> nit: we usually reserve the Web-prefix for top-level types.  Inner types drop the prefix.
> 
> I guess this is bike-shedding territory, but all things considered, how about moving
> WebGamepad back to the top-level, and then eliminate WebGamepads in favor of just having
> the sampleGamepads function return a |const WebGamepad*| that is terminated by an empty
> WebGamepad structure or return the length as an out parameter?
> 
>   virtual const WebGamepad* sampleGamepads(); // Terminated by "empty" WebGamepad
>   virtual const WebGamepad* sampleGamepads(unsigned* length);
>   virtual unsigned sampleGamepads(const WebGamepad**);
>   virtual unsigned sampleGamepads(WebGamepad* buffer, unsigned bufferLength); // Returns number of WebGamepad elements, up to bufferLength, that are filled in.
> 
> Finally, you could also go with:
> 
>   virtual WebVector<WebGamepad> sampleGamepads();
> 
> ^^^ Perhaps the cost of copying is worth the improved code simplicity?
> 
> If you don't like such options, then my next suggestion is WebGamepad as a top-level type
> with WebGamepads as its own top-level type.

OK. I prefer

virtual unsigned sampleGamepads(WebGamepad* buffer, unsigned bufferLength);

if that's agreeable. That seems worth it to get rid of one of the types.

I will avoid "empty" termination because when gamepads are removed, the others that are still connected must not change indices. That means we'd have to have both "disconnected" and "end-of-list" which seems confusing.

Using that prototype should also still allow a fixed size one-time allocation of this structure on the Chromium side.

Given the localized usage and limited number of calls (i.e. one) to this API, the minor improvement in simplicity doesn't seem worth the copy (to me).

[Scott Graham]

(In reply to comment #11)
> (From update of attachment 113955 [details])

> > I guess this is bike-shedding territory, but all things considered, how about moving
> > WebGamepad back to the top-level, and then eliminate WebGamepads in favor of just having
> > the sampleGamepads function return a |const WebGamepad*| that is terminated by an empty
> > WebGamepad structure or return the length as an out parameter?
> > 
> >   virtual const WebGamepad* sampleGamepads(); // Terminated by "empty" WebGamepad
> >   virtual const WebGamepad* sampleGamepads(unsigned* length);
> >   virtual unsigned sampleGamepads(const WebGamepad**);
> >   virtual unsigned sampleGamepads(WebGamepad* buffer, unsigned bufferLength); // Returns number of WebGamepad elements, up to bufferLength, that are filled in.
> > 
> > Finally, you could also go with:
> > 
> >   virtual WebVector<WebGamepad> sampleGamepads();
> > 
> > ^^^ Perhaps the cost of copying is worth the improved code simplicity?
> > 
> > If you don't like such options, then my next suggestion is WebGamepad as a top-level type
> > with WebGamepads as its own top-level type.
>
>I prefer
> 
> virtual unsigned sampleGamepads(WebGamepad* buffer, unsigned bufferLength);
> 
> if that's agreeable. That seems worth it to get rid of one of the types.

Having gone through and changed this, I kind of don't like it now. It adds noise down through the glue code that has to track the maximum and actual lengths in function signatures, and those lengths are disconnected from the buffer.

So, I intend to go with the "two top-level types" option now, as I think it's a bit tidier, when considering the consumers of this API.

(Apologies for dithering on this relatively minor point)

[Scott Graham]

Created attachment 114308 [details]
separate top level types

[Scott Graham]

Created attachment 114345 [details]
whitespace

[Darin Fisher (:fishd, Google)]

Comment on attachment 114345 [details]
whitespace

View in context: https://bugs.webkit.org/attachment.cgi?id=114345&action=review

> Source/WebKit/chromium/public/WebGamepad.h:31
> +// This structure is intentionally POD and fixed size so that it can be shared

nit: "can be [stored in] shared"

> Source/WebKit/chromium/public/WebGamepad.h:53
> +    unsigned index;

i'm not entirely sure why this is needed.  why wouldn't you just leave holes in WebGamepads?  or, is this so that you can avoid wasting storage space for the holes?

[Scott Graham]

Created attachment 114398 [details]
Patch

[Scott Graham]

Comment on attachment 114345 [details]
whitespace

View in context: https://bugs.webkit.org/attachment.cgi?id=114345&action=review

>> Source/WebKit/chromium/public/WebGamepad.h:31
>> +// This structure is intentionally POD and fixed size so that it can be shared
> 
> nit: "can be [stored in] shared"

Done.

>> Source/WebKit/chromium/public/WebGamepad.h:53
>> +    unsigned index;
> 
> i'm not entirely sure why this is needed.  why wouldn't you just leave holes in WebGamepads?  or, is this so that you can avoid wasting storage space for the holes?

Agreed, removed. (It was needed in a previous iteration that attempted to send each pad separately so to know where to slot it.)

[Scott Graham]

Hey Darin, you had previously r+'d with comments, but I fat fingered when uploading the tweaks. Could I get you to r+ again?

[WebKit Review Bot]

Comment on attachment 114398 [details]
Patch

Clearing flags on attachment: 114398

Committed r100306: <http://trac.webkit.org/changeset/100306>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.