Bug 100062 – nrwt: truncate meter lines properly on windows

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt: truncate meter lines properly on windows

[Dirk Pranke]

Created attachment 170046 [details]
Patch

[Dirk Pranke]

Tony, this modifies the change you made to truncate lines to the terminal width (ninja-style) so that it works on windows as well. I'm not entirely happy with this patch - the layering is a bit weird - so I'm open to feedback. 

Maybe I should just leave the code in MeteredStream and not add a platforminfo object until some other class needs it?

[Tony Chang]

Comment on attachment 170046 [details]
Patch

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

Putting the method in platforminfo seems fine.  I would probably pass in the number of columns rather than the whole platforminfo object (Makes it easier to test? I think I read this in a TotT.).

> Tools/Scripts/webkitpy/common/system/platforminfo.py:92
> +            if self.is_win():

Looks like cygwin is also considered win, but I think we want cygwin to take the 'else' code path.

> Tools/Scripts/webkitpy/common/system/platforminfo.py:95
> +                h = windll.kernel32.GetStdHandle(-12)  # handle for stderr

Nit: h -> handle

> Tools/Scripts/webkitpy/common/system/platforminfo.py:96
> +                csbi = create_string_buffer(22)

csbi -> console_screen_buffer_info

> Tools/Scripts/webkitpy/common/system/platforminfo.py:98
> +                res = windll.kernel32.GetConsoleScreenBufferInfo(h, csbi)
> +                if res:

Nit: Remove the temp variable?

> Tools/Scripts/webkitpy/common/system/platforminfo.py:100
> +                    (_, _, _, _, _, left, _, right, _, _, _) = struct.unpack("hhhhHhhhhhh", csbi.raw)

Nit: Don't need () on the left side of the assignment.

> Tools/Scripts/webkitpy/layout_tests/views/printing.py:333
> +        # We don't write into the rightmost column of a terminal since this may cause lines to wrap on Windows.
> +        max_line_length = self._meter.number_of_columns() - 1

Can we subtract the extra column only no Windows?  I would be annoyed by the wasted column on Linux.  Maybe we can do the -1 in number_of_columns to make the testing easier.

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 170046 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170046&action=review
> 
> Putting the method in platforminfo seems fine.  I would probably pass in the number of columns rather than the whole platforminfo object (Makes it easier to test? I think I read this in a TotT.).
> 

It's a bit of a tossup, since platform info is so small, but will do.

> > Tools/Scripts/webkitpy/common/system/platforminfo.py:92
> > +            if self.is_win():
> 
> Looks like cygwin is also considered win, but I think we want cygwin to take the 'else' code path.
>

Will verify.
 
> > Tools/Scripts/webkitpy/common/system/platforminfo.py:95
> > +                h = windll.kernel32.GetStdHandle(-12)  # handle for stderr
> 
> Nit: h -> handle
>

Will do.
 
> > Tools/Scripts/webkitpy/common/system/platforminfo.py:96
> > +                csbi = create_string_buffer(22)
> 
> csbi -> console_screen_buffer_info
> 
> > Tools/Scripts/webkitpy/common/system/platforminfo.py:98
> > +                res = windll.kernel32.GetConsoleScreenBufferInfo(h, csbi)
> > +                if res:
> 
> Nit: Remove the temp variable?
> 

What temp variable?

> > Tools/Scripts/webkitpy/common/system/platforminfo.py:100
> > +                    (_, _, _, _, _, left, _, right, _, _, _) = struct.unpack("hhhhHhhhhhh", csbi.raw)
> 
> Nit: Don't need () on the left side of the assignment.
> 

Done.

> > Tools/Scripts/webkitpy/layout_tests/views/printing.py:333
> > +        # We don't write into the rightmost column of a terminal since this may cause lines to wrap on Windows.
> > +        max_line_length = self._meter.number_of_columns() - 1
> 
> Can we subtract the extra column only no Windows?  I would be annoyed by the wasted column on Linux.  Maybe we can do the -1 in number_of_columns to make the testing easier.

Yeah, I waffled on this. I will push it inside number_of_columns().

[Dirk Pranke]

Created attachment 170487 [details]
update w/ review feedback

[Tony Chang]

Comment on attachment 170487 [details]
update w/ review feedback

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

> Tools/Scripts/webkitpy/common/system/platforminfo.py:96
> +                console_screen_buffer_info = create_string_buffer(22)

Maybe make a variable for 22 explaining what it is? sizeof_console_screen_buffer_info, I assume.

> Tools/Scripts/webkitpy/common/system/platforminfo.py:98
> +                res = windll.kernel32.GetConsoleScreenBufferInfo(handle, console_screen_buffer_info)
> +                if res:

I meant you don't need |res|. This could just be:
if windll.kernel32.GetConsoleScreenBufferInfo(handle, console_screen_buffer_info):

[Dirk Pranke]

Created attachment 170496 [details]
fix typos, verify works on cygwin as well

[Dirk Pranke]

Created attachment 170499 [details]
update w/ more review feedback

[Dirk Pranke]

(In reply to comment #6)
> (From update of attachment 170487 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170487&action=review
> 
> > Tools/Scripts/webkitpy/common/system/platforminfo.py:96
> > +                console_screen_buffer_info = create_string_buffer(22)
> 
> Maybe make a variable for 22 explaining what it is? sizeof_console_screen_buffer_info, I assume.
> 

Added a comment; a variable seemed like overkill.

> > Tools/Scripts/webkitpy/common/system/platforminfo.py:98
> > +                res = windll.kernel32.GetConsoleScreenBufferInfo(handle, console_screen_buffer_info)
> > +                if res:
> 
> I meant you don't need |res|. This could just be:
> if windll.kernel32.GetConsoleScreenBufferInfo(handle, console_screen_buffer_info):

Ah. Done.

[Dirk Pranke]

Committed r132423: <http://trac.webkit.org/changeset/132423>