[webkit-reviews] review canceled: [Bug 72589] [GTK] Expose accessibility hierarchy in WebKit2 to ATK/AT-SPI based ATs : [Attachment 121688] Patch proposal

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has canceled Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 72589: [GTK] Expose accessibility hierarchy in WebKit2 to ATK/AT-SPI based
ATs
https://bugs.webkit.org/show_bug.cgi?id=72589

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

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
(In reply to comment #29)
> (From update of attachment 121688 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=121688&action=review
> 
> It looks good to me, great work!. I have just a few more comments, sorry :-P

No worries. Everytime you point something out the patch gets better, so I'm
more than happy to address those issues.

Now attaching a new patch taking care of these new ones. See my comments
below...

> > Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:33
> > +int
> > +main(int argc, char** argv)
> 
> This should be one line.

Fixed.

> > Tools/Scripts/run-gtk-tests:77
> > +	     for path in paths_to_check:
> > +		 filepath = os.path.join(exec_prefix, path, filename)
> > +		 if os.path.isfile(filepath):
> > +		     return filepath
> > +		 return None
> 
> You are only checking the first path, the return should be after the loop.

Argh! Dammed python indentation thing... Fixed.
 
> > Tools/Scripts/run-gtk-tests:97
> > +	     Executive().popen(["xprop", "-root", "-remove", "AT_SPI_BUS"],
> > +			       env=test_env, stderr=subprocess.PIPE,
stdout=subprocess.PIPE)
> 
> Don't use pipes if you are not going to read/write from/to them

Fixed. I also switched to run_command() here since for 'xprop' I won't need the
process later on to kill it (it's not a daemon).

> > Tools/Scripts/run-gtk-tests:105
> > +		     a11y_bus_launcher =
Executive().popen([a11y_bus_launcher_path],
> > +							   env=test_env,
stderr=subprocess.PIPE, stdout=subprocess.PIPE)
> 
> Ditto.

Fixed. In this case I kept using popen() because I need to make sure I kill the
process later on.

> > Tools/Scripts/run-gtk-tests:116
> > +		     a11y_registryd = Executive().popen(["%s/at-spi2-registryd"
% atspi2_exec_path],
> > +							env=test_env,
stderr=subprocess.PIPE, stdout=subprocess.PIPE)
> 
> Ditto.

Fixed. In this case I kept using popen() because I need to make sure I kill the
process later on.

Also, I fixed another bug in there: 

   Bad:
     Executive().popen(["%s/at-spi2-registryd" % atspi2_exec_path] ...

  Good:
     Executive().popen([a11y_registryd_path] ...

Last, I added another modification to this patch, to avoid the following kind
of errors when generating the docs:

  Generating WebKit2 documentation...
  ./webkit2gtk-unused.txt:1: warning: 11 unused declarations.They should be
added to webkit2gtk-sections.txt in the appropriate place.
  ./webkit2gtk-unused.txt:1: warning: 11 unused declarations.They should be
added to webkit2gtk-sections.txt in the appropriate place.

   gtkdoc did not build without warnings

The modification is as follows:

diff --git a/Tools/gtk/generate-gtkdoc b/Tools/gtk/generate-gtkdoc
index db5f83a..0c18a38 100755
--- a/Tools/gtk/generate-gtkdoc
+++ b/Tools/gtk/generate-gtkdoc
@@ -62,6 +62,7 @@ def get_webkit2_options():
			  glob.glob(src_path('PageClientImpl.*')) + \
			  glob.glob(src_path('WebKitUIClient.*')) + \
			  glob.glob(src_path('WebKitWebLoaderClient.*')) + \
+			  glob.glob(src_path('WebKitWebViewBaseAccessible.*'))
+ \
			  glob.glob(src_path('tests/*.h'))
     })
     return (common.build_path('Source', 'WebKit2', 'webkit2gtk-3.0.pc'),
options)


That's all I think. Thanks!