Bug 92447

Summary: nrwt: move image diffing code to a separate module
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: New BugsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, ojan, rniwa, tony
Priority: P2 Keywords: NRWT
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 92450    
Attachments:
Description Flags
Patch
none
address review comments none

Description Dirk Pranke 2012-07-26 19:00:30 PDT
nrwt: move image diffing code to a separate module
Comment 1 Dirk Pranke 2012-07-26 19:04:58 PDT
Created attachment 154808 [details]
Patch
Comment 2 Ryosuke Niwa 2012-07-26 20:15:47 PDT
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.
Comment 3 Ryosuke Niwa 2012-07-26 20:17:59 PDT
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?
Comment 4 Dirk Pranke 2012-07-26 21:59:05 PDT
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.
Comment 5 Dirk Pranke 2012-07-26 22:01:04 PDT
Created attachment 154838 [details]
address review comments
Comment 6 Ryosuke Niwa 2012-07-26 22:42:54 PDT
(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.
Comment 7 Dirk Pranke 2012-07-27 11:21:19 PDT
Committed r123893: <http://trac.webkit.org/changeset/123893>