Bug 134325 – Add new platform gamepad abstractions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Add new platform gamepad abstractions

These include a PlatformGamepad object and platform strategies to support either in-process or out-of-process gamepad input.

The code is basically unused here, but goes hand-in-hand with the patch that will be reviewed separately in https://bugs.webkit.org/show_bug.cgi?id=134324

[Brady Eidson]

Created attachment 233864 [details]
Patch v1

[Dean Jackson]

Comment on attachment 233864 [details]
Patch v1

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:7212
> -		07AC46FF1952102100EE9723 /* ISOVTTCue.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = ISOVTTCue.cpp; path = ISOVTTCue.cpp; sourceTree = "<group>"; };
> -		07AC47001952102100EE9723 /* ISOVTTCue.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = ISOVTTCue.h; path = ISOVTTCue.h; sourceTree = "<group>"; };
> +		07AC46FF1952102100EE9723 /* ISOVTTCue.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ISOVTTCue.cpp; sourceTree = "<group>"; };
> +		07AC47001952102100EE9723 /* ISOVTTCue.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ISOVTTCue.h; sourceTree = "<group>"; };

Is this intentional?

> Source/WebCore/platform/GamepadStrategy.h:50
> +#endif // ENABLE(GAMEPAD)

Nit: I think we agreed to not bother with these style comments

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:594
> +#endif // PLATFORM(IOS)
>  
> -#endif
> +#endif // PLATFORM(COCOA)

But here you're adding them, so maybe my memory is incorrect and they should be around :)

[Brady Eidson]

Created attachment 233866 [details]
Patch v2 - Patch that should apply

[Brady Eidson]

(In reply to comment #2)
> (From update of attachment 233864 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233864&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:7212
> > -		07AC46FF1952102100EE9723 /* ISOVTTCue.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = ISOVTTCue.cpp; path = ISOVTTCue.cpp; sourceTree = "<group>"; };
> > -		07AC47001952102100EE9723 /* ISOVTTCue.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = ISOVTTCue.h; path = ISOVTTCue.h; sourceTree = "<group>"; };
> > +		07AC46FF1952102100EE9723 /* ISOVTTCue.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ISOVTTCue.cpp; sourceTree = "<group>"; };
> > +		07AC47001952102100EE9723 /* ISOVTTCue.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ISOVTTCue.h; sourceTree = "<group>"; };
> 
> Is this intentional?

Nope, and it's gone in the updated patch.

> 
> > Source/WebCore/platform/GamepadStrategy.h:50
> > +#endif // ENABLE(GAMEPAD)
> 
> Nit: I think we agreed to not bother with these style comments

I think we agreed to not bother *always* especially when they're short blocks where it would be pointless.

In this case...

> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:594
> > +#endif // PLATFORM(IOS)
> >  
> > -#endif
> > +#endif // PLATFORM(COCOA)
> 
> But here you're adding them, so maybe my memory is incorrect and they should be around :)

I added them because I couldn't follow what these blocks were!

Thanks for the review!

[Brady Eidson]

http://trac.webkit.org/changeset/170459

[Sam Weinig]

We are trying not to add any more PlatformStrategies, and instead are aiming to remove them.  Please back this out and let's discuss other mechanisms you can use to achieve what it is you are trying to do.

[Michal Debski]

(In reply to comment #6)
> We are trying not to add any more PlatformStrategies, and instead are aiming to remove them.  Please back this out and let's discuss other mechanisms you can use to achieve what it is you are trying to do.

I have already updated Gamepad API and did not use PlatformStrategies, please take a look at my patch and tell me what do you think about this change:
https://bugs.webkit.org/show_bug.cgi?id=133850

[Brady Eidson]

(In reply to comment #6)
> We are trying not to add any more PlatformStrategies, and instead are aiming to remove them.  Please back this out and let's discuss other mechanisms you can use to achieve what it is you are trying to do.

I have a good non-platformStrategies based solution for this I'll get in today.

[Brady Eidson]

(In reply to comment #7)
> (In reply to comment #6)
> > We are trying not to add any more PlatformStrategies, and instead are aiming to remove them.  Please back this out and let's discuss other mechanisms you can use to achieve what it is you are trying to do.
> 
> I have already updated Gamepad API and did not use PlatformStrategies, please take a look at my patch and tell me what do you think about this change:
> https://bugs.webkit.org/show_bug.cgi?id=133850

Wow, coincidental!  

Unfortunately there's a lot of needs the work you've done there don't fulfill.  For example, your patch doesn't allow for WebKit2 implementations (which is the very reason I used platform strategies in the first place)

I'll comment more in your bug(s) later today.

But I bet we can use things like your layout test work going forward!

[Brady Eidson]

Filed a new bug to remove the use of PlatformStrategies (https://bugs.webkit.org/show_bug.cgi?id=134348), re-resolving this one.