Bug 134325 - Add new platform gamepad abstractions
Summary: Add new platform gamepad abstractions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 134076
  Show dependency treegraph
 
Reported: 2014-06-25 19:24 PDT by Brady Eidson
Modified: 2014-06-26 09:53 PDT (History)
9 users (show)

See Also:


Attachments
Patch v1 (35.88 KB, patch)
2014-06-25 19:31 PDT, Brady Eidson
dino: review+
Details | Formatted Diff | Diff
Patch v2 - Patch that should apply (33.85 KB, patch)
2014-06-25 19:41 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2014-06-25 19:24:27 PDT
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
Comment 1 Brady Eidson 2014-06-25 19:31:47 PDT
Created attachment 233864 [details]
Patch v1
Comment 2 Dean Jackson 2014-06-25 19:41:13 PDT
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 :)
Comment 3 Brady Eidson 2014-06-25 19:41:44 PDT
Created attachment 233866 [details]
Patch v2 - Patch that should apply
Comment 4 Brady Eidson 2014-06-25 19:43:23 PDT
(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!
Comment 5 Brady Eidson 2014-06-25 20:59:46 PDT
http://trac.webkit.org/changeset/170459
Comment 6 Sam Weinig 2014-06-25 21:54:03 PDT
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.
Comment 7 Michal Debski 2014-06-26 03:14:49 PDT
(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
Comment 8 Brady Eidson 2014-06-26 08:08:12 PDT
(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.
Comment 9 Brady Eidson 2014-06-26 08:10:13 PDT
(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!
Comment 10 Brady Eidson 2014-06-26 09:53:10 PDT
Filed a new bug to remove the use of PlatformStrategies (https://bugs.webkit.org/show_bug.cgi?id=134348), re-resolving this one.