Bug 126210 – [GTK][CMake] Generate GObject DOM bindings .symbols files

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

These files are used to check the ABI of the DOM bindings as well as to help generate documentation for the bindings.

[Gustavo Noronha (kov)]

Created attachment 220531 [details]
Patch

[Martin Robinson]

Comment on attachment 220531 [details]
Patch

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

Nice! I have a couple small concerns, but it's great to see this patch.

> Source/PlatformGTK.cmake:7
> +    fake-gdom-gen-symbols

Perhaps this target could be called generate-gdom-symbol-file?

> Source/WebCore/PlatformGTK.cmake:716
> +    file(GLOB GObjectDOMBindingsCheckDependencies

Maybe GObjectDOMBindingsSymbolFiles. I'm a little worried that this isn't going to work properly for a clean build though, where the symbols files don't exist yet. If I understand correctly this GLOB is run during the cmake phase. Instead you probably need to iterate through the class list.

> Source/WebCore/PlatformGTK.cmake:718
> +        "${WEBCORE_DIR}/bindings/gobject/WebKitDOM*.symbols"
> +            "${DERIVED_SOURCES_GOBJECT_DOM_BINDINGS_DIR}/WebKit*.symbols"

The indentation is a little funky here.

> Source/WebCore/PlatformGTK.cmake:725
> +            OUTPUT ${DERIVED_SOURCES_GOBJECT_DOM_BINDINGS_DIR}/webkitdom.symbols
> +            DEPENDS ${GObjectDOMBindingsCheckDependencies} ${CMAKE_SOURCE_DIR}/Tools/gtk/check-gdom-symbols
> +    COMMAND ln -n -s -f ${WEBCORE_DIR}/bindings/gobject/WebKitDOM*.symbols ${DERIVED_SOURCES_GOBJECT_DOM_BINDINGS_DIR}
> +    COMMAND ${CMAKE_SOURCE_DIR}/Tools/gtk/check-gdom-symbols

Here as well.

> Tools/gtk/check-gdom-symbols:30
> +derived_sources_path = common.build_path('DerivedSources')
> +gdom_source_path = common.top_level_path('Source', 'WebCore', 'bindings')
> +bindings_scripts_path = os.path.join(gdom_source_path, 'scripts')
> +api_break_test_path = os.path.join(bindings_scripts_path, 'gobject-run-api-break-test')
> +generated_gdom_symbols_path = os.path.join(derived_sources_path, 'webkitdom', 'webkitdom.symbols')

Do you mind declaring these right before you use them?

> Tools/gtk/check-gdom-symbols:53
> +

You should probably do the if __name__ == "__main__": dance here.

> Tools/gtk/check-gdom-symbols:57
> +    for file_path in symbols_files:
> +        with open(file_path) as file_handle:
> +            tmp.write(file_handle.read())

I think it might be a little clearer to do something like:
    for file_path in glob.glob(os.path.join(derived_sources_path, 'webkitdom', '*.h')):
        if not os.path.basename(file_path).startswith('WebKit') or h.endswith('Private.h'):
            continue
        file_path = file_path.replace('.h', '.symbols')
        ...

> Tools/gtk/check-gdom-symbols:66
> +    if not should_update_symbols_file(tmp.name, generated_gdom_symbols_path):
> +        source = open(tmp.name, 'r')
> +        destination = open(generated_gdom_symbols_path, 'w')
> +        destination.write(source.read())
> +        destination.close()

Instead of writing to a temporary file, why not do the should_update_symbols_file check (against the unwritten contents of the temporary file) before calling gobject-run-api-break-test. Then you can always pass the other script generated_gdom_symbols_path and avoid using the temporary file at all.

[Gustavo Noronha (kov)]

(In reply to comment #2)
> Instead of writing to a temporary file, why not do the should_update_symbols_file check (against the unwritten contents of the temporary file) before calling gobject-run-api-break-test. Then you can always pass the other script generated_gdom_symbols_path and avoid using the temporary file at all.

gobject-run-api-break-test needs to be run before should_update_symbols_file is executed, because it tests the tmp file against the file that gets generated by the other script, not the one available in the sources. I guess it could be changed to get the contents of what currently is being written to a tmpfile through stdin, though, but I'd prefer to do the same we were doing in the make target first and doing that in a follow-up patch if that's ok.

[Gustavo Noronha (kov)]

Created attachment 223109 [details]
Patch

[Martin Robinson]

Comment on attachment 223109 [details]
Patch

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

Okay! Please look into the dependencies for generate-gdom-symbols-file before landing though.

> Source/WebCore/PlatformGTK.cmake:735
> +    add_custom_target(generate-gdom-symbols-file
> +        DEPENDS GObjectDOMBindings
> +    )

It seems like this needs to also depend on all the individual *.symbol files. in the output.

[Gustavo Noronha (kov)]

Created attachment 223851 [details]
Patch

[Gustavo Noronha (kov)]

I had broken the dependency chain by mistake when I updated the patch, this one gets the target actually called. That uncovered an issue: our bindings were not being generated properly for StorageQuota and StorageInfo, because their IDL files were not listed in the supplemental file even though we listed them in the classes to be generated. This version also fixes that. Believe it's good to go, running it by you again =)

[Martin Robinson]

Comment on attachment 223851 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:2753
> -if (ENABLE_QUOTA)
> +if (ENABLE_QUOTA OR PORT STREQUAL "GTK")

Probably want to stick a little comment here explaining the situation.

[Gustavo Noronha (kov)]

Comment on attachment 223851 [details]
Patch

Committed: http://trac.webkit.org/changeset/164076