Bug 187776 – Provide an lldb type summary for WebCore::Color

• Component: New Bugs
• Status: NEW

[Dean Jackson]

Provide an lldb type summary for WebCore::Color

[Dean Jackson]

Created attachment 345264 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 345264 [details]
Patch

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

> Tools/lldb/lldb_webkit_unittest.py:180
> +    def serial_test_WebCoreColorProvider_rgba_color(self):
> +        variable = self._sbFrame.FindVariable('rgbaColor');
> +        self.assertIsNotNone(variable)
> +        summary = lldb_webkit.WebCoreColorProvider_SummaryProvider(variable, {})
> +        self.assertEqual(summary, "{ rgba(255, 128, 64, 0.50) }")

No test for semantic colors?

[EWS Watchlist]

Attachment 345264 [details] did not pass style-queue:


ERROR: Tools/lldb/lldbWebKitTester/main.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/lldb/lldb_webkit_unittest.py:159:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Tools/lldb/lldb_webkit_unittest.py:165:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Tools/lldb/lldb_webkit_unittest.py:171:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Tools/lldb/lldb_webkit_unittest.py:177:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Tools/lldb/lldb_webkit_unittest.py:181:  blank line at end of file  [pep8/W391] [5]
ERROR: Tools/lldb/lldb_webkit_unittest.py:161:  [TestSummaryProviders.serial_test_WebCoreColorProvider_invalid_color] Module 'lldb_webkit' has no 'WebCoreColorProvider_SummaryProvider' member  [pylint/E1101] [5]
ERROR: Tools/lldb/lldb_webkit_unittest.py:167:  [TestSummaryProviders.serial_test_WebCoreColorProvider_extended_color] Module 'lldb_webkit' has no 'WebCoreColorProvider_SummaryProvider' member  [pylint/E1101] [5]
ERROR: Tools/lldb/lldb_webkit_unittest.py:173:  [TestSummaryProviders.serial_test_WebCoreColorProvider_rgb_color] Module 'lldb_webkit' has no 'WebCoreColorProvider_SummaryProvider' member  [pylint/E1101] [5]
ERROR: Tools/lldb/lldb_webkit_unittest.py:179:  [TestSummaryProviders.serial_test_WebCoreColorProvider_rgba_color] Module 'lldb_webkit' has no 'WebCoreColorProvider_SummaryProvider' member  [pylint/E1101] [5]
Total errors found: 10 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 345264 [details]
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('extendedColor').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.

[Daniel Bates]

Comment on attachment 345264 [details]
Patch

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

> Tools/lldb/lldbWebKitTester/main.cpp:74
> +    extendedColor.alpha(); // Use the variable after the break point so that it doesn't get derefed.

Can you please elaborate on why this is needed?

[Daniel Bates]

(In reply to Daniel Bates from comment #5)
> Comment on attachment 345264 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345264&action=review
> 
> > Tools/lldb/lldbWebKitTester/main.cpp:74
> > +    extendedColor.alpha(); // Use the variable after the break point so that it doesn't get derefed.
> 
> Can you please elaborate on why this is needed?

I mean, we run the unit tests once we hit the breakpoint and I would have expected that extendedColor (and all the other locals) would still be on the stack at the time.

[Dean Jackson]

(In reply to Daniel Bates from comment #6)
> 
> I mean, we run the unit tests once we hit the breakpoint and I would have
> expected that extendedColor (and all the other locals) would still be on the
> stack at the time.

This was something I ran into while testing which ended up being a user error. It can be removed.

[Truitt Savell]

looks like revision <https://trac.webkit.org/changeset/233934/webkit> 

has caused 5 webkitpy errors:

<https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20%28Tests%29/builds/6785/steps/webkitpy-test/logs/stdio>

A few of them are the new tests that were added.

[Truitt Savell]

Also webkitpy test runner fails completely on iOS 11 Simulator. 

Output:
https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20%28Tests%29/builds/5470/steps/webkitpy-test/logs/stdio

[Simon Fraser (smfr)]

These tests should never run for iOS simulator.

[Truitt Savell]

Reverted r233934 for reason:

Revision caused 5 webkitpy failures on Mac and broke webkitpy testing on iOS simulator

Committed r233942: <https://trac.webkit.org/changeset/233942>

[Daniel Bates]

(In reply to Truitt Savell from comment #8)
> looks like revision <https://trac.webkit.org/changeset/233934/webkit> 
> 
> has caused 5 webkitpy errors:
> 
> <https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Release%20WK1%20%28Tests%29/builds/6785/steps/
> webkitpy-test/logs/stdio>
> 
> A few of them are the new tests that were added.

We need to teach the webkitpy driver code and/or build-lldbwebkittesster to only build lldbWebKitTester on Mac.

[Ryan Haddad]

This change was re-landed in https://trac.webkit.org/changeset/233943/webkit.

webkitpy tests are passing on macOS release bots, but lldbWebKitTester is failing to build on macOS debug bots as well as iOS simulator bots:
https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20%28Tests%29/builds/7313/steps/webkitpy-test/logs/stdio

I tried clearing the WebkitBuild directory on one of the bots and running the tests manually, but hit the same error. I was able to run the tests locally without issue.

[Ryan Haddad]

A fix attempted was landed in https://trac.webkit.org/changeset/233988, but the issue persists:
https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20(Tests)/builds/7317

[Ryan Haddad]

Rolled out this change and all of the follow ups in https://trac.webkit.org/changeset/234040

[Daniel Bates]

(In reply to Ryan Haddad from comment #13)
> This change was re-landed in https://trac.webkit.org/changeset/233943/webkit.
> 
> webkitpy tests are passing on macOS release bots, but lldbWebKitTester is
> failing to build on macOS debug bots as well as iOS simulator bots:
> https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20%28Tests%29/
> builds/7313/steps/webkitpy-test/logs/stdio

Looking at this output this debug tester thinks that it should use a relwase-built lldbWebKitTester. But this bot will only have a debug built lldbWebKitTester. So, it tries to build it and bad things happen. Filed bug #187872 to fix this issue.

[Simon Fraser (smfr)]

Can this land again?

[Daniel Bates]

(In reply to Simon Fraser (smfr) from comment #17)
> Can this land again?

I thought Dean landed just the code change.... anyway, I don't care if we land this change with the tests either as I always have a WebKit build and I have a fast machine 😀 so any slowdown is likely not noticeable. But there may be some slowdown  on the EWS or for tools-only folks. If it significantly slows downs the EWS bots then that might motivate Aakash to work faster and fix bug #189205 and bug #189206 😀.

[Daniel Bates]

All other bugs i think we're fixed.