Bug 71518 - [Chromium] gamepad changes to the public interface of Chromium port
: [Chromium] gamepad changes to the public interface of Chromium port
Status: RESOLVED FIXED
: WebKit
WebKit API
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: http://dvcs.w3.org/hg/webevents/raw-f...
:
:
: 71753
  Show dependency treegraph
 
Reported: 2011-11-03 16:24 PST by
Modified: 2011-11-15 12:05 PST (History)


Attachments
Patch (7.25 KB, patch)
2011-11-03 16:28 PST, Scott Graham
no flags Review Patch | Details | Formatted Diff | Diff
updates per review (7.66 KB, patch)
2011-11-07 11:44 PST, Scott Graham
no flags Review Patch | Details | Formatted Diff | Diff
update for runtime enable landed (7.66 KB, patch)
2011-11-07 16:01 PST, Scott Graham
no flags Review Patch | Details | Formatted Diff | Diff
update to ToT (7.62 KB, patch)
2011-11-07 16:35 PST, Scott Graham
no flags Review Patch | Details | Formatted Diff | Diff
separate top level types (9.55 KB, patch)
2011-11-09 10:07 PST, Scott Graham
no flags Review Patch | Details | Formatted Diff | Diff
whitespace (9.55 KB, patch)
2011-11-09 13:05 PST, Scott Graham
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.49 KB, patch)
2011-11-09 17:06 PST, Scott Graham
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-11-03 16:24:37 PST
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
------- Comment #1 From 2011-11-03 16:28:21 PST -------
Created an attachment (id=113572) [details]
Patch
------- Comment #2 From 2011-11-03 16:29:54 PST -------
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
------- Comment #3 From 2011-11-05 14:56:48 PST -------
(From update of attachment 113572 [details])
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.
------- Comment #4 From 2011-11-07 11:44:21 PST -------
(From update of attachment 113572 [details])
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.
------- Comment #5 From 2011-11-07 11:44:41 PST -------
Created an attachment (id=113902) [details]
updates per review
------- Comment #6 From 2011-11-07 16:01:41 PST -------
Created an attachment (id=113948) [details]
update for runtime enable landed
------- Comment #7 From 2011-11-07 16:29:30 PST -------
(From update of attachment 113948 [details])
Attachment 113948 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10332790
------- Comment #8 From 2011-11-07 16:35:38 PST -------
Created an attachment (id=113955) [details]
update to ToT
------- Comment #9 From 2011-11-08 11:33:34 PST -------
(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.
------- Comment #10 From 2011-11-08 13:45:25 PST -------
(From update of attachment 113955 [details])
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.
------- Comment #11 From 2011-11-08 14:16:45 PST -------
(From update of attachment 113955 [details])
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).
------- Comment #12 From 2011-11-09 09:25:47 PST -------
(In reply to comment #11)
> (From update of attachment 113955 [details] [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)
------- Comment #13 From 2011-11-09 10:07:47 PST -------
Created an attachment (id=114308) [details]
separate top level types
------- Comment #14 From 2011-11-09 13:05:37 PST -------
Created an attachment (id=114345) [details]
whitespace
------- Comment #15 From 2011-11-09 17:00:31 PST -------
(From update of attachment 114345 [details])
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?
------- Comment #16 From 2011-11-09 17:06:43 PST -------
Created an attachment (id=114398) [details]
Patch
------- Comment #17 From 2011-11-09 17:08:47 PST -------
(From update of attachment 114345 [details])
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.)
------- Comment #18 From 2011-11-15 08:53:50 PST -------
Hey Darin, you had previously r+'d with comments, but I fat fingered when uploading the tweaks. Could I get you to r+ again?
------- Comment #19 From 2011-11-15 12:05:49 PST -------
(From update of attachment 114398 [details])
Clearing flags on attachment: 114398

Committed r100306: <http://trac.webkit.org/changeset/100306>
------- Comment #20 From 2011-11-15 12:05:54 PST -------
All reviewed patches have been landed.  Closing bug.