[webkit-reviews] review denied: [Bug 129571] [GTK] Simplify the GObject DOM bindings API break check into one step : [Attachment 225721] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 129571: [GTK] Simplify the GObject DOM bindings API break check into one
step
https://bugs.webkit.org/show_bug.cgi?id=129571

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

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225721&action=review


I think this breaks distcheck. I think the patch could be split, one patch to
move all the logic to a single script (like it was originally) and another one
to move it to make check. I really want to make sure I'm not going to miss the
api breaks.

> Source/WebCore/bindings/gobject/GNUmakefile.am:-503
> -DerivedSources/webkitdom/webkitdom.symbols: $(gdom_symbol_files)
$(WebCore)/bindings/gobject/webkitdom.symbols
$(WebCore)/bindings/scripts/gobject-run-api-break-test
$(srcdir)/Tools/gtk/check-gdom-symbols
> -	$(AM_V_GEN)$(srcdir)/Tools/gtk/check-gdom-symbols
> -

I didn't even know this script existed. Anyway, you should also remove the
webkitdom.symbols file from EXTRA_DIST

> Source/WebCore/bindings/scripts/gobject-run-api-break-test:-1
> -#!/usr/bin/env python

You should also remove the script from EXTRA_DIST in
Source/WebCore/GNUmakefile.am

> Tools/ChangeLog:9
> +	   * gtk/check-for-webkitdom-api-breaks: Added. A combination of the
two removed scripts.

Why the rename? what was wrong with gobject-run-api-break-test? I really don't
mind the name, I'm just curious.

> Tools/GNUmakefile.am:270
> +	$(top_srcdir)/Tools/gtk/check-for-webkitdom-api-breaks

You should add it to EXTRA_DIST too. Are you sure this is run by the bots? I'm
afraid I'm going to miss all api breaks from now on. Would it be possible to
add a specific step in the bots for this particular test, like the other tests?


> Tools/gtk/check-for-webkitdom-api-breaks:44
> +    missing_api = expected_api.difference(built_api)
> +    new_api = built_api.difference(expected_api)

I love this approach of using sets and get the difference, it's much better
than having to parse diffs :-)

> Tools/gtk/check-for-webkitdom-api-breaks:52
> +	   sys.stderr.write("New API detected in GObject DOM bindings\n")
> +	   sys.stderr.write("	 %s\n" % "    ".join(new_api))

This is not an error, we used to write this to stdout instead.

> Tools/gtk/webkitdom.py:1
> +#!/usr/bin/env python

This new file should be added to EXTRA_DIST as well

> Tools/gtk/webkitdom.py:46
> +	   if header_name == "webkitdom.h" or header_name ==
"webkitdomdefines.h":

if header_name in ("webkitdom.h", "webkitdomdefines.h"):