Bug 92447 – nrwt: move image diffing code to a separate module

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt: move image diffing code to a separate module

[Dirk Pranke]

Created attachment 154808 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 154808 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154808&action=review

> Tools/ChangeLog:11
> +        ImageDiff after a single invocation, and thus we subsequent

Nit: thus we subsequent

> Tools/Scripts/webkitpy/layout_tests/port/image_diff.py:48
> +        self._proc = None

Please spell out process. "proc" may refer to procedure, which I initially thought it was referring to.

> Tools/Scripts/webkitpy/layout_tests/port/image_diff.py:62
> +                            len(actual_contents), actual_contents,
> +                            len(expected_contents), expected_contents))

Ditto about aligning indentation at (.

> Tools/Scripts/webkitpy/layout_tests/port/image_diff.py:64
> +        except IOError as e:

Please spell out exception.

[Ryosuke Niwa]

Comment on attachment 154808 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154808&action=review

Somehow my earlier comment wasn't posted :(

> Tools/Scripts/webkitpy/layout_tests/port/server_process.py:106
> +                                                stdout=self._host.executive.PIPE,
> +                                                stderr=self._host.executive.PIPE,
> +                                                close_fds=close_fds,
> +                                                env=self._env,
> +                                                universal_newlines=self._universal_newlines)

We don't normally align subsequent lines like this elsewhere in WebKit.
Elsewhere, we indent by 4 spaces to the right of self._proc.
Is it really absolutely required for PEP8?

[Dirk Pranke]

Thanks for the review!

(In reply to comment #3)
> > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:106
> > +                                                stdout=self._host.executive.PIPE,
> > +                                                stderr=self._host.executive.PIPE,
> > +                                                close_fds=close_fds,
> > +                                                env=self._env,
> > +                                                universal_newlines=self._universal_newlines)
> 
> We don't normally align subsequent lines like this elsewhere in WebKit.
> Elsewhere, we indent by 4 spaces to the right of self._proc.
> Is it really absolutely required for PEP8?

PEP8 recommends either aligning to the paren or an 8-char indent (to distinguish that it is a continuation line rather than a new, indented line), but we don't typically enforce this and I'd be surprised if there was a strong tendency for one or the other in the python code.

That said, done :).

(In reply to comment #2)
> (From update of attachment 154808 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154808&action=review
> 
> > Tools/ChangeLog:11
> > +        ImageDiff after a single invocation, and thus we subsequent
> 
> Nit: thus we subsequent
> 

Fixed.

> > Tools/Scripts/webkitpy/layout_tests/port/image_diff.py:48
> > +        self._proc = None
> 
> Please spell out process. "proc" may refer to procedure, which I initially thought it was referring to.
>

Fixed.
 
> > Tools/Scripts/webkitpy/layout_tests/port/image_diff.py:62
> > +                            len(actual_contents), actual_contents,
> > +                            len(expected_contents), expected_contents))
> 
> Ditto about aligning indentation at (.
>

Fixed.
 
> > Tools/Scripts/webkitpy/layout_tests/port/image_diff.py:64
> > +        except IOError as e:
> 
> Please spell out exception.

Fixed.

[Dirk Pranke]

Created attachment 154838 [details]
address review comments

[Ryosuke Niwa]

(In reply to comment #4)
> Thanks for the review!
> 
> (In reply to comment #3)
> > > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:106
> > > +                                                stdout=self._host.executive.PIPE,
> > > +                                                stderr=self._host.executive.PIPE,
> > > +                                                close_fds=close_fds,
> > > +                                                env=self._env,
> > > +                                                universal_newlines=self._universal_newlines)
> > 
> > We don't normally align subsequent lines like this elsewhere in WebKit.
> > Elsewhere, we indent by 4 spaces to the right of self._proc.
> > Is it really absolutely required for PEP8?
> 
> PEP8 recommends either aligning to the paren or an 8-char indent (to distinguish that it is a continuation line rather than a new, indented line), but we don't typically enforce this and I'd be surprised if there was a strong tendency for one or the other in the python code.

If it's not mandatory, we should probably stick with the WebKit style guide.

[Dirk Pranke]

Committed r123893: <http://trac.webkit.org/changeset/123893>