[webkit-reviews] review denied: [Bug 70877] [GTK] [WebKit] Replace the gtkdoc autools magic with something more flexible : [Attachment 112797] Patch

[bugzilla-daemon at webkit.org]

Philippe Normand <pnormand at igalia.com> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 70877: [GTK] [WebKit] Replace the gtkdoc autools magic with something more
flexible
https://bugs.webkit.org/show_bug.cgi?id=70877

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

------- Additional Comments from Philippe Normand <pnormand at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=112797&action=review


Looks great Martin, I suggested some small changes, no big deal I think.

> Source/WebKit2/UIProcess/API/gtk/docs/build-gtkdoc:93
> +sys.exit(1 if generator.saw_warnings else 0)

sys.exit(generator.saw_warnings)

should be enough I think.

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:112
> +	   raise_error_if_not_specified('output_dir')
> +	   raise_error_if_not_specified('source_dirs')
> +	   raise_error_if_not_specified('module_name')

If these arguments are not optional they should be made explicit in the
constructor prototype.

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:189
> +	   if not self._has_executable_on_path(args[0]):
> +	       raise Exception('%s not found on path', args[0])
> +
> +	   self.logger.info("Running %s", args[0])
> +	   self.logger.debug("Full command args: %s", str(args))
> +	   process = subprocess.Popen(args, env=env, cwd=cwd,
> +				      stdout=subprocess.PIPE,
> +				      stderr=subprocess.PIPE)

No need for self._has_executable_on_path I think. Just wrap the Popen call in a
try/except block and catch OSError

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:200
> +	   if 'warning' in stderr or 'warning' in stdout:

if 'warning' in stderr + stdout
would work as well. As you prefer :)

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:205
> +

What about returning the stdout contents? So you could use this method instead
of Popen in the sub-class

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:232
> +	   self._create_directory_if_nonexistent(self.output_dir)
> +	   copy_all_files_in_directory(self.doc_dir, self.output_dir)

Maybe use http://docs.python.org/library/shutil.html#shutil.copytree ?

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:243
> +	   self._create_directory_if_nonexistent(html_dest_dir)
> +
> +	   if os.path.exists(html_src_dir):
> +	       copy_all_files_in_directory(html_src_dir, html_dest_dir)

Ditto

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:269
> +	   for source_dir in self.source_dirs:
> +	       args.append('--source-dir=%s' % source_dir)

Just a suggestion, maybe use a list comprehension. They are faster than normal
loops.

args.extend(['--source-dir=%s' % source_dir for source_dir in
self.source_dirs])

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:295
> +	   if self.ldflags:
> +	       env['LDFLAGS'] = ldflags

Shouldn't the test be "if ldflags" ?

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:304
> +	   if 'CFLAGS' in env:
> +	       self.logger.debug('CFLAGS=%s', env['CFLAGS'])
> +	   if 'LDFLAGS' in env:
> +	       self.logger.debug('LDFLAGS %s', env['LDFLAGS'])
> +	   if 'RUN' in env:
> +	       self.logger.debug('RUN=%s', env['RUN'])

Move these debug calls up to avoid the additional ifs?

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:325
> +	   for source_dir in self.source_dirs:
> +	       args.append('--source-dir=%s' % source_dir)

Same remark as in the gtkdoc_scan method ;) Maybe it would make sense to store
the result as an instance attribute?

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:369
> +	   if not 'pkg_config_path' in args:
> +	       raise Exception('pkg_config_path not given')

pkg_config_path should be made explicit.
Now I understand why you used a dictionary in the parent class. I don't have a
strong opinion about it anymore but my personal preference is to make arguments
explicit, even in this case. But up to you, really.

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:389
> +	   self.version = process.communicate()[0].strip()

It'd be nice to use _run_command instead of those Popen calls. Additional bonus
points include logging support