Bug 71518

Created attachment 113572 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

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.

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.

Created attachment 113902 [details] updates per review

Created attachment 113948 [details] update for runtime enable landed

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

Created attachment 113955 [details] update to ToT

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

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

(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)

Created attachment 114308 [details] separate top level types

Created attachment 114345 [details] whitespace

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?

Created attachment 114398 [details] Patch

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

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 on attachment 114398 [details] Patch Clearing flags on attachment: 114398 Committed r100306: <http://trac.webkit.org/changeset/100306>

All reviewed patches have been landed. Closing bug.