[webkit-reviews] review denied: [Bug 128417] [GTK] generate-gtkdoc should not generate documentation for source files for unbuilt source files : [Attachment 224547] 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 128417: [GTK] generate-gtkdoc should not generate documentation for source
files for unbuilt source files
https://bugs.webkit.org/show_bug.cgi?id=128417

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

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


Thanks for working on this. I have a just a few comments and questions.

> Source/WebCore/bindings/gobject/GNUmakefile.am:606
> +gtkdoc-webkitdom.cfg: $(WebCore)/bindings/gobject/GNUmakefile.am
$(srcdir)/Tools/gtk/GNUmakefile.am
> +	$(AM_V_GEN)echo "[webkitdomgtk]" > $@ && \
> +	echo "pkgconfig_file=$(webkitdom_pkgconfig_file)" >> $@ && \
> +	echo "namespace=webkit_dom" >> $@ && \
> +	echo "doc_dir=DerivedSources/webkitdom/docs" >> $@ && \
> +	echo -e "cflags=-I$(srcdir)/Source $(webkitgtk_gdom_include_dirs)" >>
$@ && \
> +	echo "source_dirs=$(top_builddir)/DerivedSources/webkitdom
$(srcdir)/Source/WebCore/bindings/gobject" >> $@ && \
> +	echo "headers=$(webkitgtk_gdom_built_h_api)
DerivedSources/webkitdom/WebKitDOMDeprecated.h" >> $@
> +BUILT_SOURCES += gtkdoc-webkitdom.cfg

Since we are generating different config files for wk1, wk2 and wkdom, I wonder
if it would be easier to make these config files being python files that can be
just imported. We don't really need to have sections, since the file is
gtkdoc-webkitdom.cfg and the only section it has is webkitdom.

> Tools/gtk/GNUmakefile.am:47
> -docs-build.stamp: $(docs_build_stamp_list)
> +docs-build.stamp: $(docs_build_stamp_list) gtkdoc-webkitdom.cfg
gtkdoc-webkitgtk.cfg gtkdoc-webkit2gtk.cfg

I guess this is a problem if wk1 or wk2 is disabled in build.

> Tools/gtk/generate-gtkdoc:-50
> -def get_gtkdoc_module_paths(xref_dep_packages):
> -    deps = []
> -    html_dir = os.path.join('share', 'gtk-doc', 'html')
> -
> -    for package in xref_dep_packages:
> -	   prefix = common.prefix_of_pkg_config_file(package)
> -	   if prefix is None:
> -	       continue
> -	   for module in xref_dep_packages[package]:
> -	       deps.append(os.path.join(prefix, html_dir, module))
> -
> -    return deps

What is this change exactly? It's not easy to see why we need to change this
looking at the diff.

> Tools/gtk/generate-gtkdoc:86
> +	       if os.path.exists(file) and os.path.isfile(file):

os.path.isfile() returns False if the fiel doesn't exist, so you don't need to
call os.path.exists() too.

> Tools/gtk/generate-gtkdoc:89
> +	   add_file_if_exists(os.path.splitext(header)[0] + ".cpp")
> +	   add_file_if_exists(os.path.splitext(header)[0] + ".c")

You could avoid doing the splitext twice here

> Tools/gtk/generate-gtkdoc:95
> +	   if not os.path.splitext(file)[1] in ['.h', '.c', '.cpp', '.cc']:
> +	       return False # These files are ignored anyway.

I think this is easier to read as if os.path.splitext(file)[1] not in ['.h',
'.c', '.cpp', '.cc']:

You don't need the absolute path for this check, so you can probably move the
previous line after this.

> Tools/gtk/generate-gtkdoc:97
> +	   if not os.path.isfile(file):
> +	       return False

Not sure if you need the abs patch for this check either.

> Tools/gtk/generate-gtkdoc:100
> +	   if file in implementation_files:
> +	       return False
> +	   return True

This could be 

return file not in implementation_files

> Tools/gtk/generate-gtkdoc:108
> +    config = SafeConfigParser()
> +    config.read(config_file)
> +    module_name = config.sections()[0]

What happen if generate-gtkdoc is called and the config files don't exist?

> Tools/gtk/generate-gtkdoc:189
> +    saw_webkit1_warnings =
generate_documentation_for_config(common.build_path('gtkdoc-webkitgtk.cfg'))
> +    saw_webkit2_warnings =
generate_documentation_for_config(common.build_path('gtkdoc-webkit2gtk.cfg'))

I think we should check if the config files actually exist and exit with an
error message if they don't

> Tools/gtk/gtkdoc.py:42
> +			     Required if source_files is not specified.

What is source_files? you mean headers? I think both options should always be
required. Since the config files are now required and they always contain both,
I guess there's no problem and it makes the whole thing easier and safer.

> Tools/gtk/gtkdoc.py:122
> -	   def raise_error_if_not_specified(key):
> -	       if not getattr(self, key):
> -		   raise Exception('%s not specified.' % key)
> -
> -	   raise_error_if_not_specified('output_dir')
> -	   raise_error_if_not_specified('source_dirs')
> -	   raise_error_if_not_specified('module_name')
> +	   if not getattr(self, 'output_dir'):
> +	       raise Exception('output_dir not specified.')
> +	   if not getattr(self, 'module_name'):
> +	       raise Exception('module_name not specified.')

Why?