[webkit-reviews] review denied: [Bug 126210] [GTK][CMake] Generate GObject DOM bindings .symbols files : [Attachment 220531] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Gustavo Noronha (kov)
<gns at gnome.org>'s request for review:
Bug 126210: [GTK][CMake] Generate GObject DOM bindings .symbols files
https://bugs.webkit.org/show_bug.cgi?id=126210

Attachment 220531: Patch
https://bugs.webkit.org/attachment.cgi?id=220531&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.