Added non-prefixed navigator.getGamepads() with updated API

Added non-prefixed navigator.getGamepads() with updated API

This CL splits the Gamepad API into prefixed and non-prefixed versions.
The prefixed version continues to operate in the same way as before, though
the types are now named WebKitGamepad and WebKitGamepadList. The non-prefixed
version uses the new GamepadButton type for its button values.

The GamepadEvent type has also been added but the events aren't yet wired up.
When added they will only apply to the the non-prefixed API.

BUG=344556
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169148

[bajones, 2014-02-27 18:41:15]

https://codereview.chromium.org/183313003/diff/1/Source/modules/gamepad/Navig...
File Source/modules/gamepad/NavigatorGamepad.h (right):

https://codereview.chromium.org/183313003/diff/1/Source/modules/gamepad/Navig...
Source/modules/gamepad/NavigatorGamepad.h:39: class NavigatorGamepad FINAL :
public Supplement<Navigator>, public DOMWindowProperty {
DOMWindowProperty isn't being used just yet but it will be necessary to allow
this class to fire events on window when we get to that point. I'm happy to
leave this change out for now if there's any objections.

[bajones, 2014-03-06 00:29:48]

scottmg@: Please review modules/gamepad/

abarth@ or eseidel@: Please review modules.gypi and core/

Removed GamepadEvent and related code from the latest revision since it's a fair
amount of code to on both the Chrome and Blink sides, and as such probably
warrants it's own review. Will add that plumbing in a follow-up CL.

[abarth-chromium, 2014-03-06 01:21:13]

Source/core and modules.gypi LGTM

[eseidel, 2014-03-06 01:56:15]

https://codereview.chromium.org/183313003/diff/40001/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/183313003/diff/40001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:689: return "'navigator.webkitGetGamepads' is
deprecated. Please use the unprefixed 'navigator.getGamepads' method instead.";
You can remove "the unprefixed"  and "method" and the sentence will still convey
your meaning.

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
File Source/modules/gamepad/Gamepad.idl (right):

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
Source/modules/gamepad/Gamepad.idl:26: interface Gamepad : GamepadBase {
I wouldn't do this via inheritance like this.  You expose this inheritance to
the web this way.  Instead I would just have two copies of the IDL, one with
each interface.

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
File Source/modules/gamepad/GamepadBase.idl (right):

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
Source/modules/gamepad/GamepadBase.idl:29: ] interface GamepadBase {
Yeah, just copy the idl into two places, instead of having a base idl.  That
will be least confusing to web developers who happen to walk the prototype
chain. (not that many do).  The new one is the true impl, the old one is just a
copy which will slowly die.

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
File Source/modules/gamepad/GamepadButton.h (right):

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
Source/modules/gamepad/GamepadButton.h:15: class GamepadButton: public
RefCounted<GamepadButton>, public ScriptWrappable {
FINAL?

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/W...
File Source/modules/gamepad/WebKitGamepad.h (right):

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/W...
Source/modules/gamepad/WebKitGamepad.h:12: class WebKitGamepad FINAL: public
GamepadBase {
I'm not even sure we need to use two classes to impelment these.  You'd have to
ask haraken or someone more familiar with teh bindings.  But it's possibel we
can have two idl files, both of which point to the same c++ class?

[Nils Barth (inactive), 2014-03-06 02:14:33]

To concur with Eric:
We can share the implementation w/o adding a base IDL interface,
which is both simpler and more correct.

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
File Source/modules/gamepad/Gamepad.idl (right):

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
Source/modules/gamepad/Gamepad.idl:26: interface Gamepad : GamepadBase {
On 2014/03/06 01:56:16, eseidel wrote:
> I wouldn't do this via inheritance like this.  You expose this inheritance to
> the web this way.  Instead I would just have two copies of the IDL, one with
> each interface.

Exactly, what Eric said:
IDLs are *public*, so they should accord to standards.
Implementation details should be either in:
* (Blink-specific) Extended attributes
* Blink C++ code
In this case, using [ImplementedAs] on the interface seems best.

That is, the C++ class should be Gamepad (= public IDL name),
and we can have the legacy IDL interface as:
[
    ImplementedAs=Gamepad,
] interface WebKitGamepad {
    ...
};

Reference:
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Implem...

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/W...
File Source/modules/gamepad/WebKitGamepad.h (right):

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/W...
Source/modules/gamepad/WebKitGamepad.h:12: class WebKitGamepad FINAL: public
GamepadBase {
On 2014/03/06 01:56:16, eseidel wrote:
> But it's possibel we can have two idl files,
> both of which point to the same c++ class?

You can haz 2 IDL files with t3h same kla55:
interface Foo { ... };
[ImplementedAs=Foo] interface Bar { ... };

[Inactive, 2014-03-06 02:25:12]

On 2014/03/06 02:14:33, Nils Barth wrote:
> To concur with Eric:
> We can share the implementation w/o adding a base IDL interface,
> which is both simpler and more correct.
> 
>
https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
> File Source/modules/gamepad/Gamepad.idl (right):
> 
>
https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
> Source/modules/gamepad/Gamepad.idl:26: interface Gamepad : GamepadBase {
> On 2014/03/06 01:56:16, eseidel wrote:
> > I wouldn't do this via inheritance like this.  You expose this inheritance
to
> > the web this way.  Instead I would just have two copies of the IDL, one with
> > each interface.
> 
> Exactly, what Eric said:
> IDLs are *public*, so they should accord to standards.
> Implementation details should be either in:
> * (Blink-specific) Extended attributes
> * Blink C++ code
> In this case, using [ImplementedAs] on the interface seems best.
> 
> That is, the C++ class should be Gamepad (= public IDL name),
> and we can have the legacy IDL interface as:
> [
>     ImplementedAs=Gamepad,
> ] interface WebKitGamepad {
>     ...
> };
> 
> Reference:
>
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Implem...
> 
>
https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/W...
> File Source/modules/gamepad/WebKitGamepad.h (right):
> 
>
https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/W...
> Source/modules/gamepad/WebKitGamepad.h:12: class WebKitGamepad FINAL: public
> GamepadBase {
> On 2014/03/06 01:56:16, eseidel wrote:
> > But it's possibel we can have two idl files,
> > both of which point to the same c++ class?
> 
> You can haz 2 IDL files with t3h same kla55:
> interface Foo { ... };
> [ImplementedAs=Foo] interface Bar { ... };

Just FYI, you could also use IDL implements statements
(http://www.w3.org/TR/WebIDL/#idl-implements-statements) instead of inheritance
so that the IDL is not duplicated and GamepadBase is not exposed to the
JavaScript. This would be better than duplicating the IDL code in my opinion.

However, in this particular case, I like Adam's proposal better as it makes our
IDL match better the spec.

[Nils Barth (inactive), 2014-03-06 02:53:57]

On 2014/03/06 02:25:12, Chris Dumez wrote:
> Just FYI, you could also use IDL implements statements
> (http://www.w3.org/TR/WebIDL/#idl-implements-statements) instead of
inheritance
> so that the IDL is not duplicated and GamepadBase is not exposed to the
> JavaScript. This would be better than duplicating the IDL code in my opinion.

Good point!


> However, in this particular case, I like Adam's proposal better as it makes
our
> IDL match better the spec.

Agreed (or rather, using [ImplementedAs], correct?).

BTW, note that we can't just have:
interface WebKitGamepad {}; // empty!
WebKitGamepad implements Gamepad;

...as implement*ed* interfaces (target) don't have their own bindings generated:
they're just dependencies, not build targets themselves.

(We'd need to use '... implements GamepadBase' for both, which is what we want
to avoid.)

[Inactive, 2014-03-06 02:54:00]

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
File Source/modules/gamepad/Gamepad.h (right):

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
Source/modules/gamepad/Gamepad.h:43: virtual void buttons(unsigned count, const
blink::WebGamepadButton* data);
OVERRIDE

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
File Source/modules/gamepad/GamepadBase.h (right):

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
Source/modules/gamepad/GamepadBase.h:45: void id(const String& id) { m_id = id;
}
I know this is existing code but I find it odd that the setters don't use "set"
prefix.

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
File Source/modules/gamepad/GamepadButton.cpp (right):

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
Source/modules/gamepad/GamepadButton.cpp:12: RefPtr<GamepadButton> input =
adoptRef(new GamepadButton());
nit: we could "return adoptRef(new GamepadButton());" to avoid ref counting
churn.

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
File Source/modules/gamepad/GamepadButton.h (right):

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
Source/modules/gamepad/GamepadButton.h:21: void value(float val) { m_value =
val; }
setValue?

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/G...
Source/modules/gamepad/GamepadButton.h:24: void pressed(bool val) { m_pressed =
val; }
setPressed?

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/N...
File Source/modules/gamepad/NavigatorGamepad.cpp (right):

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/N...
Source/modules/gamepad/NavigatorGamepad.cpp:37: static void
sampleGamepad(unsigned index, GamepadBase* gamepad, const blink::WebGamepad&
webGamepad)
nit: gamedpad would be passed by reference here to make it clear it cannot be
null.

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/N...
Source/modules/gamepad/NavigatorGamepad.cpp:60: sampleGamepad(i, gamepad.get(),
webGamepad);
nit: *gamepad (see comment above)

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/N...
File Source/modules/gamepad/NavigatorGamepad.idl (right):

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/N...
Source/modules/gamepad/NavigatorGamepad.idl:23: [DeprecateAs=PrefixedGamepad]
nit: I believe we usually put the extended attributes on the same line as the
attribute / method they apply to.

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/W...
File Source/modules/gamepad/WebKitGamepad.h (right):

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/W...
Source/modules/gamepad/WebKitGamepad.h:21: virtual void buttons(unsigned count,
const blink::WebGamepadButton* data);
OVERRIDE

[scottmg, 2014-03-06 18:16:44]

(moved myself to cc as there's other more qualified reviewers looking already)

[bajones, 2014-03-06 18:52:24]

Thank you for the great feedback! New patch is up that addresses everything but
the "ImplementedAs" suggestion.

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/W...
File Source/modules/gamepad/WebKitGamepad.h (right):

https://codereview.chromium.org/183313003/diff/40001/Source/modules/gamepad/W...
Source/modules/gamepad/WebKitGamepad.h:12: class WebKitGamepad FINAL: public
GamepadBase {
On 2014/03/06 02:14:33, Nils Barth wrote:
> On 2014/03/06 01:56:16, eseidel wrote:
> > But it's possibel we can have two idl files,
> > both of which point to the same c++ class?
> 
> You can haz 2 IDL files with t3h same kla55:
> interface Foo { ... };
> [ImplementedAs=Foo] interface Bar { ... };

I would very much like to do this as it seems much cleaner than the code
duplication in the current patch, but adding [ImplementedAs=Gamepad] to the
WebKitGamepad interface gives me some errors compiling the V8 generated code,
along the lines of:

In file included from gen/blink/bindings/V8Window.cpp:488:
gen/blink/bindings/V8WebKitGamepad.h:74:31: error: redefinition of 'wrap'
inline v8::Handle<v8::Object> wrap(Gamepad* impl, v8::Handle<v8::Object>
creationContext, v8::Isolate* isolate)
                              ^
gen/blink/bindings/V8Gamepad.h:74:31: note: previous definition is here
inline v8::Handle<v8::Object> wrap(Gamepad* impl, v8::Handle<v8::Object>
creationContext, v8::Isolate* isolate)

[Inactive, 2014-03-06 19:21:27]

On 2014/03/06 02:53:57, Nils Barth wrote:
> On 2014/03/06 02:25:12, Chris Dumez wrote:
> > Just FYI, you could also use IDL implements statements
> > (http://www.w3.org/TR/WebIDL/#idl-implements-statements) instead of
> inheritance
> > so that the IDL is not duplicated and GamepadBase is not exposed to the
> > JavaScript. This would be better than duplicating the IDL code in my
opinion.
> 
> Good point!
> 
> 
> > However, in this particular case, I like Adam's proposal better as it makes
> our
> > IDL match better the spec.
> 
> Agreed (or rather, using [ImplementedAs], correct?).
> 
> BTW, note that we can't just have:
> interface WebKitGamepad {}; // empty!
> WebKitGamepad implements Gamepad;
> 
> ...as implement*ed* interfaces (target) don't have their own bindings
generated:
> they're just dependencies, not build targets themselves.
> 
> (We'd need to use '... implements GamepadBase' for both, which is what we want
> to avoid.)

Maybe I missed the point. I thought we were trying to avoid having both
interface *inherit* GamepadBase because such change is against spec and exposed
to JS.
I would think that having both WebKitGamepad and Gamepad implement (using IDL
implements statement) GamepadBase would be acceptable as this avoids duplicating
the IDL, GamepadBase would not be exposed the JS (i.e. we would match the spec
behavior-wise).

Anyway, if people prefer duplicating the IDL between WebKitGamepad and Gamepad
(current CL), I don't mind it either. Just saying that implements statements
avoid the duplication and maintain the same web-exposed behavior.

[sof, 2014-03-06 22:33:09]

https://codereview.chromium.org/183313003/diff/60001/Source/modules/gamepad/G...
File Source/modules/gamepad/Gamepad.h (right):

https://codereview.chromium.org/183313003/diff/60001/Source/modules/gamepad/G...
Source/modules/gamepad/Gamepad.h:80: GamepadButtonVector m_buttons;
If you change this typedef to have an Oilpan (transition) type, it will have to
be traced.

https://codereview.chromium.org/183313003/diff/60001/Source/modules/gamepad/G...
File Source/modules/gamepad/GamepadButton.h (right):

https://codereview.chromium.org/183313003/diff/60001/Source/modules/gamepad/G...
Source/modules/gamepad/GamepadButton.h:15: class GamepadButton FINAL : public
RefCounted<GamepadButton>, public ScriptWrappable {
Adding Oilpan transition types to this class also wouldn't be much extra work;
I'll leave it to you to decide if you want to do that here. The following
changes would then be needed:

  - change RefCounted to RefCountedWillBeGarbageCollectedFinalized
  - have create() return PassRefPtrWillBeRawPtr<> instead.
  - add a "void trace(Visitor*) { }" empty method.
  - change GamepadButtonVector's type to
WillBeHeapVector<RefPtrWillBeMember<GamepadButton> >.
  - add a [ WillBeGarbageCollected, ] attribute to the IDL interface decl.

https://codereview.chromium.org/183313003/diff/60001/Source/modules/gamepad/W...
File Source/modules/gamepad/WebKitGamepad.idl (right):

https://codereview.chromium.org/183313003/diff/60001/Source/modules/gamepad/W...
Source/modules/gamepad/WebKitGamepad.idl:5: interface WebKitGamepad {
Add [ WillBeGarbageCollected, ]

https://codereview.chromium.org/183313003/diff/60001/Source/modules/gamepad/W...
File Source/modules/gamepad/WebKitGamepadList.h (right):

https://codereview.chromium.org/183313003/diff/60001/Source/modules/gamepad/W...
Source/modules/gamepad/WebKitGamepadList.h:16: 
This needs to be RefCountedWillBeGarbageCollectedFinalized due to
ScriptWrappable needing to be finalized.

[bajones, 2014-03-07 19:34:35]

On 2014/03/06 22:33:09, sof wrote:
> Adding Oilpan transition types to this class also wouldn't be much extra work;
> I'll leave it to you to decide if you want to do that here.

I've added what I think are the right types. I have no idea what I'm doing when
it comes to Oilpan types, so please verify that I'm not doing anything really
dumb. :)

[sof, 2014-03-07 19:54:57]

Looks close to ready Oilpan-wise, just two details.

(If you want to build with Oilpan enabled, add enable_oilpan=1 to your gyp
defines.)

https://codereview.chromium.org/183313003/diff/80001/Source/modules/gamepad/G...
File Source/modules/gamepad/Gamepad.cpp (left):

https://codereview.chromium.org/183313003/diff/80001/Source/modules/gamepad/G...
Source/modules/gamepad/Gamepad.cpp:62: {
Add visitor->trace(m_buttons);

https://codereview.chromium.org/183313003/diff/80001/Source/modules/gamepad/G...
File Source/modules/gamepad/GamepadButton.cpp (right):

https://codereview.chromium.org/183313003/diff/80001/Source/modules/gamepad/G...
Source/modules/gamepad/GamepadButton.cpp:12: return adoptRef(new
GamepadButton());
This will turn into a no-op once Oilpan becomes default, so change this to
adoptRefWillBeNoop()

[bajones, 2014-03-07 21:16:17]

On 2014/03/07 19:54:57, sof wrote:
> Looks close to ready Oilpan-wise, just two details.

Thanks! Done and done.

[sof, 2014-03-07 21:37:56]

On 2014/03/07 21:16:17, bajones wrote:
> On 2014/03/07 19:54:57, sof wrote:
> > Looks close to ready Oilpan-wise, just two details.
> 
> Thanks! Done and done.

Thanks, it looks good to me now (FloatVector could also move to the heap, but it
can wait.)

haraken: could you double check with a quick review, when there's time?

[bajones, 2014-03-10 19:50:49]

On 2014/03/07 21:37:56, sof wrote:
> On 2014/03/07 21:16:17, bajones wrote:
> > On 2014/03/07 19:54:57, sof wrote:
> > > Looks close to ready Oilpan-wise, just two details.
> > 
> > Thanks! Done and done.
> 
> Thanks, it looks good to me now (FloatVector could also move to the heap, but
it
> can wait.)
> 
> haraken: could you double check with a quick review, when there's time?

Post-weekend review ping!

[bajones, 2014-03-11 23:00:53]

eseidel@, nbarth@: I would appreciate any further feedback you have on this CL
now that the GamepadBase class has been removed.

haraken@: Would you mind giving me a quick Oilpan sanity check as sof@
requested?

[Nils Barth (inactive), 2014-03-12 05:13:51]

A few style points;
currently seeing if we can reuse implementation
(needs CG change).

https://codereview.chromium.org/183313003/diff/110001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/183313003/diff/110001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.h:307: PrefixedGamepad = 286,
(You'll need to sync and update this to 287.)

https://codereview.chromium.org/183313003/diff/110001/Source/modules/gamepad/...
File Source/modules/gamepad/GamepadButton.idl (right):

https://codereview.chromium.org/183313003/diff/110001/Source/modules/gamepad/...
Source/modules/gamepad/GamepadButton.idl:7: WillBeGarbageCollected
Could you add a trailing comma please?

https://codereview.chromium.org/183313003/diff/110001/Source/modules/gamepad/...
File Source/modules/gamepad/WebKitGamepad.idl (right):

https://codereview.chromium.org/183313003/diff/110001/Source/modules/gamepad/...
Source/modules/gamepad/WebKitGamepad.idl:6: WillBeGarbageCollected
Trailing comma please.

https://codereview.chromium.org/183313003/diff/110001/Source/modules/gamepad/...
File Source/modules/gamepad/WebKitGamepadList.idl (right):

https://codereview.chromium.org/183313003/diff/110001/Source/modules/gamepad/...
Source/modules/gamepad/WebKitGamepadList.idl:7: NoInterfaceObject,
Alphabetical order of extended attributes please.
(Also in GamepadList.idl.)

[Nils Barth (inactive), 2014-03-12 06:58:54]

On 2014/03/11 23:00:53, bajones wrote:
> eseidel@, nbarth@: I would appreciate any further feedback you have on this CL
> now that the GamepadBase class has been removed.

Gave a few style feedback points.

Regarding sharing implementation:
I believe this should be possible and pretty easy,
so long as we don't generate duplicate definitions.

I've posted a CL that does the needful at:
Share Gamepad and WebKitGamepad implementation
https://codereview.chromium.org/196503004/

I've posted the CG-only changes (which you can cherry-pick locally) in:
Add [DoNotGenerateClassBindings] extended attribute
https://codereview.chromium.org/196653003/

[bajones, 2014-03-12 22:15:01]

Addressed Nils' style feedback. There's unfortunately some issues with the CG
changes that prevent me from using it right now. I emailed you about it.

[eseidel, 2014-03-12 22:25:25]

https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
File Source/modules/gamepad/GamepadButton.h (right):

https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
Source/modules/gamepad/GamepadButton.h:26: void trace(Visitor*);
Do we need an empty trace method?

https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
File Source/modules/gamepad/NavigatorGamepad.cpp (right):

https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
Source/modules/gamepad/NavigatorGamepad.cpp:61: gamepad = GamepadType::create();
Unfortunate that we need to do a malloc on every sample.  Then again, I suspect
we do several.

https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
Source/modules/gamepad/NavigatorGamepad.cpp:72: 
spurious.

https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
Source/modules/gamepad/NavigatorGamepad.cpp:109:
sampleGamepads<WebKitGamepad>(m_webkitGamepads.get());
Woh, crazy.  So they can sample multiple times per frame or however often they
want I guess.

https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
File Source/modules/gamepad/WebKitGamepad.h (right):

https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
Source/modules/gamepad/WebKitGamepad.h:23: virtual ~WebKitGamepad();
Is virtual needed?

https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
File Source/modules/gamepad/WebKitGamepadList.cpp (right):

https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
Source/modules/gamepad/WebKitGamepadList.cpp:23: return kMaximumGamepads;
The list is not dynamic based on how many gamepads are connected?

https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
File Source/modules/gamepad/WebKitGamepadList.h (right):

https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
Source/modules/gamepad/WebKitGamepadList.h:29: enum { kMaximumGamepads = 4 };
Why an annonymous enum instead of just a const?

[eseidel, 2014-03-12 22:28:19]

https://codereview.chromium.org/183313003/diff/150001/Source/modules/gamepad/...
File Source/modules/gamepad/GamepadList.h (right):

https://codereview.chromium.org/183313003/diff/150001/Source/modules/gamepad/...
Source/modules/gamepad/GamepadList.h:38: class GamepadList : public
RefCountedWillBeGarbageCollectedFinalized<GamepadList>, public ScriptWrappable {
I don't know the rules for oilpan well enough.  I presume someone asked you to
change it to Finalized here?

[bajones, 2014-03-12 22:53:22]

On 2014/03/12 22:25:25, eseidel wrote:
>
https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
> File Source/modules/gamepad/GamepadButton.h (right):
> 
>
https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
> Source/modules/gamepad/GamepadButton.h:26: void trace(Visitor*);
> Do we need an empty trace method?

I'm not sure, I'm very much an Oilpan noob. sof@ didn't call it out in any of
his reviews.

>
https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
> File Source/modules/gamepad/NavigatorGamepad.cpp (right):
> 
>
https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
> Source/modules/gamepad/NavigatorGamepad.cpp:61: gamepad =
GamepadType::create();
> Unfortunate that we need to do a malloc on every sample.  Then again, I
suspect
> we do several.

This only needs to create a new Gamepad object the first time a given index has
a gamepad connected to it.

>
https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
> Source/modules/gamepad/NavigatorGamepad.cpp:72: 
> spurious.

Done.
 
>
https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
> Source/modules/gamepad/NavigatorGamepad.cpp:109:
> sampleGamepads<WebKitGamepad>(m_webkitGamepads.get());
> Woh, crazy.  So they can sample multiple times per frame or however often they
> want I guess.

Yup. And given the nature of the polling the results could actually be different
between two polls in the same frame. Not sure if feature or...
 
>
https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
> File Source/modules/gamepad/WebKitGamepad.h (right):
> 
>
https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
> Source/modules/gamepad/WebKitGamepad.h:23: virtual ~WebKitGamepad();
> Is virtual needed?

No. Fixed.

>
https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
> File Source/modules/gamepad/WebKitGamepadList.cpp (right):
> 
>
https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
> Source/modules/gamepad/WebKitGamepadList.cpp:23: return kMaximumGamepads;
> The list is not dynamic based on how many gamepads are connected?

An unfortunate bug that's been there since the earliest days of gamepad support.
The trick is that the spec requires that we don't change gamepad indices while
they are connected. As such, if I have [Pad1, Pad2] and I disconnect Pad1, the
spec says I must get back [null, Pad2]. This is just an easy way to accommodate
that. I'd like to make it more robust in the near future, but felt like it would
clutter this CL unnecessarily.
 
>
https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
> File Source/modules/gamepad/WebKitGamepadList.h (right):
> 
>
https://codereview.chromium.org/183313003/diff/130001/Source/modules/gamepad/...
> Source/modules/gamepad/WebKitGamepadList.h:29: enum { kMaximumGamepads = 4 };
> Why an annonymous enum instead of just a const?

Fixed to use existing const from WebGamepads. (One const to rule them all!)

>
https://codereview.chromium.org/183313003/diff/150001/Source/modules/gamepad/...
> File Source/modules/gamepad/GamepadList.h (right):
> 
>
https://codereview.chromium.org/183313003/diff/150001/Source/modules/gamepad/...
> Source/modules/gamepad/GamepadList.h:38: class GamepadList : public
> RefCountedWillBeGarbageCollectedFinalized<GamepadList>, public ScriptWrappable
{
> I don't know the rules for oilpan well enough.  I presume someone asked you to
> change it to Finalized here?

Yes, sof@.

[eseidel, 2014-03-12 22:58:17]

lgtm.  Please file a bug for nils about the generator fixes needed to do this w/
less c++ code.

[bajones, 2014-03-12 22:59:19]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-03-12 22:59:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/183313003/170001

[commit-bot: I haz the power, 2014-03-12 22:59:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-12 22:59:38]

Failed to apply patch for Source/core/frame/UseCounter.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/frame/UseCounter.h
  Hunk #1 FAILED at 305.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/frame/UseCounter.h.rej

Patch:       Source/core/frame/UseCounter.h
Index: Source/core/frame/UseCounter.h
diff --git a/Source/core/frame/UseCounter.h b/Source/core/frame/UseCounter.h
index
42dbc8938f2654f036dd464c18a1f71d41f3c60b..6595f5eeb2fad9627ab09535bbdea394884560dc
100644
--- a/Source/core/frame/UseCounter.h
+++ b/Source/core/frame/UseCounter.h
@@ -305,6 +305,7 @@ public:
         PrefixedWorkerURL = 284,
         WindowOrientation = 285,
         DOMStringListContains = 286,
+        PrefixedGamepad = 287,
         // Add new features immediately above this line. Don't change assigned
         // numbers of any item, and don't reuse removed slots.
         NumberOfFeatures, // This enum value must be last.

[bajones, 2014-03-12 23:04:44]

On 2014/03/12 22:58:17, eseidel wrote:
> lgtm.  Please file a bug for nils about the generator fixes needed to do this
w/
> less c++ code.

Requested bug: crbug.com/351943

[bajones, 2014-03-12 23:19:49]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-03-12 23:19:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/183313003/190001

[eseidel, 2014-03-12 23:27:55]

Not that we need to bikeshed specs here, but it seems like it would have been
trivial for the spec to fix that by separating connected gamepad lookup from
lookup of the actual gamepad information.  You'd lookup an id once and then just
asked for the updated gamepad information for that device.

Do modern consoles support more than 4 controllers?  Are we going to care?  I
kinda doubt we do, but the limitation still seems silly.

[bajones, 2014-03-12 23:40:30]

On 2014/03/12 23:27:55, eseidel wrote:
> Not that we need to bikeshed specs here, but it seems like it would have been
> trivial for the spec to fix that by separating connected gamepad lookup from
> lookup of the actual gamepad information.  You'd lookup an id once and then
just
> asked for the updated gamepad information for that device.

Firefox does this, and I don't see anything in the spec that disallows it. I'm
going to be exploring how disruptive it would be to implement that route in
Chrome, because it could be a nice model to work with. Fortunately we can
implement that in the future without breaking backwards compatibility with the
existing code.

> Do modern consoles support more than 4 controllers?  Are we going to care?  I
> kinda doubt we do, but the limitation still seems silly.

The 4 controller limit is a Chrome thing and has no relation to the spec. 4 was
probably chosen because it's all XInput supports, but we are perfectly capable
of doing better. It's yet another thing on my TODO list.

[commit-bot: I haz the power, 2014-03-12 23:53:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-12 23:53:14]

Try jobs failed on following builders:
  tryserver.chromium on mac_blink_rel

[Nils Barth (inactive), 2014-03-13 01:57:40]

Quick LGTM from IDL POV;
I'll see if we can merge the implementations,
but we don't want to block on that.

[Nils Barth (inactive), 2014-03-13 02:40:15]

Failures are due to test needing resetting (noted below).
Also a few minor style issues.

https://codereview.chromium.org/183313003/diff/190001/LayoutTests/gamepad/gam...
File LayoutTests/gamepad/gamepad-polling-access-expected.txt (right):

https://codereview.chromium.org/183313003/diff/190001/LayoutTests/gamepad/gam...
LayoutTests/gamepad/gamepad-polling-access-expected.txt:1: CONSOLE WARNING:
'navigator.webkitGetGamepads' is deprecated. Please use the unprefixed
'navigator.getGamepads' method instead.
This needs a reset: ("unprefixed" was removed).
run_webkit_tests.sh --reset gamepad/gamepad-polling-access.html

https://codereview.chromium.org/183313003/diff/190001/Source/modules/gamepad/...
File Source/modules/gamepad/Gamepad.idl (right):

https://codereview.chromium.org/183313003/diff/190001/Source/modules/gamepad/...
Source/modules/gamepad/Gamepad.idl:27: WillBeGarbageCollected
Add trailing comma.

https://codereview.chromium.org/183313003/diff/190001/Source/modules/gamepad/...
Source/modules/gamepad/Gamepad.idl:37: 
Remove trailing newline.

https://codereview.chromium.org/183313003/diff/190001/Source/modules/gamepad/...
File Source/modules/gamepad/WebKitGamepad.idl (right):

https://codereview.chromium.org/183313003/diff/190001/Source/modules/gamepad/...
Source/modules/gamepad/WebKitGamepad.idl:16: 
Remove trailing newline.

[bajones, 2014-03-13 03:53:37]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-03-13 03:53:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/183313003/210001

[commit-bot: I haz the power, 2014-03-13 04:26:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 04:26:44]

Try jobs failed on following builders:
  tryserver.chromium on mac_blink_rel

[bajones, 2014-03-13 05:20:27]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-03-13 05:20:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/183313003/210001

[commit-bot: I haz the power, 2014-03-13 06:10:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 06:10:20]

Try jobs failed on following builders:
  tryserver.chromium on linux_blink

[bajones, 2014-03-13 16:18:32]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-03-13 16:18:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/183313003/210001

[commit-bot: I haz the power, 2014-03-13 16:19:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 16:19:07]

Failed to apply patch for Source/modules/gamepad/GamepadList.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/gamepad/GamepadList.h
  Hunk #1 FAILED at 29.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/modules/gamepad/GamepadList.h.rej

Patch:       Source/modules/gamepad/GamepadList.h
Index: Source/modules/gamepad/GamepadList.h
diff --git a/Source/modules/gamepad/GamepadList.h
b/Source/modules/gamepad/GamepadList.h
index
78c7e75a5e1e3fcdb79e0543b58c76d1988d5191..b19e6afc5c042d2e2ff471da76bf39ceb9734733
100644
--- a/Source/modules/gamepad/GamepadList.h
+++ b/Source/modules/gamepad/GamepadList.h
@@ -29,13 +29,14 @@
 #include "bindings/v8/ScriptWrappable.h"
 #include "heap/Handle.h"
 #include "modules/gamepad/Gamepad.h"
+#include "public/platform/WebGamepads.h"
 #include "wtf/PassRefPtr.h"
 #include "wtf/RefCounted.h"
 #include "wtf/Vector.h"
 
 namespace WebCore {
 
-class GamepadList : public RefCountedWillBeGarbageCollected<GamepadList>,
public ScriptWrappable {
+class GamepadList : public
RefCountedWillBeGarbageCollectedFinalized<GamepadList>, public ScriptWrappable {
 public:
     static PassRefPtrWillBeRawPtr<GamepadList> create() { return
adoptRefWillBeNoop(new GamepadList); }
     ~GamepadList();
@@ -47,13 +48,12 @@ public:
     void trace(Visitor*);
 
 private:
-    enum { kMaximumGamepads = 4 };
     GamepadList()
     {
         ScriptWrappable::init(this);
     }
 
-    RefPtrWillBeMember<Gamepad> m_items[kMaximumGamepads];
+    RefPtrWillBeMember<Gamepad> m_items[blink::WebGamepads::itemsLengthCap];
 };
 
 } // namespace WebCore

[bajones, 2014-03-13 17:04:49]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-03-13 17:04:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/183313003/230001

[commit-bot: I haz the power, 2014-03-13 21:45:26]

Change committed as 169148