[webkit-reviews] review granted: [Bug 187776] Provide an lldb type summary for WebCore::Color : [Attachment 345264] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Dean Jackson <dino at apple.com>'s
request for review:
Bug 187776: Provide an lldb type summary for WebCore::Color
https://bugs.webkit.org/show_bug.cgi?id=187776

Attachment 345264: Patch

https://bugs.webkit.org/attachment.cgi?id=345264&action=review




--- Comment #4 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 345264
  --> https://bugs.webkit.org/attachment.cgi?id=345264
Patch

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

Please fix the style errors, pick one quoting style, and fix up the names of
variables to conform to the PEP8 style guide.

> Tools/lldb/lldb_webkit.py:328
> +	   return (rgbaAndFlags & 0x2)

This returns the result of the bitwise-and and may not be in [0, 1]. The name
of this function implies that this function will return a boolean. I suggest we
write this line as:

return bool(rgbaAndFlags & 0x2)

This will convert the result of the bitwise-and into a boolean.

> Tools/lldb/lldb_webkit.py:332
> +	   return rgbaAndFlags & 0x4

By a similar argument I would write this as:

return bool(rgbaAndFlags & 0x4)

> Tools/lldb/lldb_webkit.py:335
> +	   extendedColor =
self.valobj.GetChildMemberWithName('m_colorData').GetChildMemberWithName('exten
dedColor').Dereference()

We seem to alternate between single quoted string literals and double quoted
string literals in this patch. I suggest we pick one style of quotes and stick
with it throughout this patch.

For Python code we follow PEP8 style; => variable names should use underscore
for separating words and they should be written in lowercase. So, extendedColor
=> extended_color.

> Tools/lldb/lldb_webkit.py:350
> +

Please remove this line. I do not feel that it improves the readability of this
code.

> Tools/lldb/lldb_webkit_unittest.py:160
> +	   self.assertIsNotNone(variable)

I know that you are just mimicing the same assert in
serial_test_WTFVectorProvider_vector_size_and_capacity() and
serial_test_WTFVectorProvider_empty_vector(). I do not see much value in these
asserts as Python would die with an exception that indicates a None was passed
when the summary provider code executes for any non-trivial summary provider.
The asserts were added in <https://trac.webkit.org/changeset/233330>. I suspect
this was done to try to debug inconsistent results we were seeing on the bots
at the time, which was fixed in bug #187229.