Bug 84929 – Add building gamepad module to CMake&GNUMake&QMake

• Component: WebKit API
• Status: RESOLVED WORKSFORME

[Sean Wang]

The gamepad files are only built in Chromium porting currently. Since it is the module of WebKit, so it should be added to other portings.

[Sean Wang]

Created attachment 138935 [details]
Patch

[Sean Wang]

The gamepad module uses "float[]" syntax in its Gamepad.idl. The JSC binding generator doesn't compile "float[]" well now, the issue is being discussed in the bugs 80696 84540. So this bug depends on the bugs 80696 84540.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 138935 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:2376
> +IF (ENABLE_GAMEPAD)

You also need to add an entry to cmakeconfig.h.cmake.

> Source/WebCore/CMakeLists.txt:2388
> +        Modules/gamepad/NavigatorGamepad.cpp
> +        Modules/gamepad/Gamepad.cpp
> +        Modules/gamepad/GamepadList.cpp

Please keep this list sorted.

[Sean Wang]

Created attachment 139122 [details]
Patch

[Philippe Normand]

Comment on attachment 139122 [details]
Patch

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

Just a missing change, looks good otherwise.

> Source/WebCore/GNUmakefile.list.am:1042
>  

IDL_PATH should also be updated, it's in GNUmakefile.am.

[Philippe Normand]

CCing Zan, who should be interested for the GTK port side of this :)

[Sean Wang]

Created attachment 143725 [details]
Patch:add changing Source/WebCore/GNUmakefile.am

Adding gamepad to IDL_PATH

[Zan Dobersek]

Comment on attachment 143725 [details]
Patch:add changing Source/WebCore/GNUmakefile.am

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

Other than the following comments this looks OK for the GNUmakefiles.

> Source/WebCore/GNUmakefile.am:807
> +    $(WebCore)/Modules/gamepad \

A few more changes are required in GNUmakefile.am:
- '-I$(srcdir)/Source/WebCore/Modules/gamepad \' include path should be added to webcore_cppflags
- '$(shell ls $(srcdir)/Source/WebCore/Modules/gamepad/*.idl) \' file listing should be added to EXTRA_DIST

> Source/WebCore/GNUmakefile.list.am:1057
> +	Source/WebCore/Modules/gamepad/Gamepad.cpp \
> +	Source/WebCore/Modules/gamepad/Gamepad.h\
> +	Source/WebCore/Modules/gamepad/GamepadList.cpp \
> +	Source/WebCore/Modules/gamepad/GamepadList.h\
> +	Source/WebCore/Modules/gamepad/NavigatorGamepad.cpp \
> +	Source/WebCore/Modules/gamepad/NavigatorGamepad.h

A few more changes are required in GNUmakefile.list.am:
- 'DerivedSources/WebCore/JSGamepad.(cpp|h)' and 'DerivedSources/WebCore/JSGamepadList.(cpp|h)' should be added to webcore_built_sources
- '$(WebCore)/Modules/gamepad/(Gamepad|GamepadList|NavigatorGamepad).idl' entries should be added to dom_binding_idls

[Sean Wang]

Created attachment 143750 [details]
Patch

[Philippe Normand]

(In reply to comment #9)
> Created an attachment (id=143750) [details]
> Patch

What's the difference with previous patch? Zan's review doesn't seem to be adressed in this new patch.

[Sean Wang]

Created attachment 144009 [details]
Patch

Sorry, I uploaded the wrong patch, and didn't check after uploading.

[Philippe Normand]

Comment on attachment 144009 [details]
Patch

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

> Source/WebCore/GNUmakefile.list.am:1064
> +	Source/WebCore/Modules/gamepad/NavigatorGamepad.h

Missing backslash here

[Zan Dobersek]

Since the work has somewhat ceased in here I went on and added necessary files and directory inclusions to the patch that's up for review in bug #87503. Because of that I'm also removing the block flag for that bug.

Other than that, the latest patch should just remove any modifications to GNUmakefiles and it'd probably be ready to land.

[Gyuyoung Kim]

Comment on attachment 144009 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:2402
> +IF (ENABLE_GAMEPAD)

I don't like to use macro both CMake and files. Files related to Gamepad are already guarded by #if ENABLE(GAMEPAD). Isn't this duplicating macro usage ?

[Martin Robinson]

Looks like this has been implemented through various other bugs.