Bug 187776 - Provide an lldb type summary for WebCore::Color
Summary: Provide an lldb type summary for WebCore::Color
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords:
Depends on: 187855 187856 187872
Blocks:
  Show dependency treegraph
 
Reported: 2018-07-18 11:29 PDT by Dean Jackson
Modified: 2019-06-16 22:14 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.07 KB, patch)
2018-07-18 11:32 PDT, Dean Jackson
dbates: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2018-07-18 11:29:17 PDT
Provide an lldb type summary for WebCore::Color
Comment 1 Dean Jackson 2018-07-18 11:32:04 PDT
Created attachment 345264 [details]
Patch
Comment 2 Simon Fraser (smfr) 2018-07-18 11:34:13 PDT
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?
Comment 3 EWS Watchlist 2018-07-18 11:35:13 PDT
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.
Comment 4 Daniel Bates 2018-07-18 13:23:00 PDT
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.
Comment 5 Daniel Bates 2018-07-18 13:24:29 PDT
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?
Comment 6 Daniel Bates 2018-07-18 13:25:33 PDT
(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.
Comment 7 Dean Jackson 2018-07-18 15:13:57 PDT
(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.
Comment 8 Truitt Savell 2018-07-18 17:16:43 PDT
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.
Comment 9 Truitt Savell 2018-07-18 17:20:12 PDT
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
Comment 10 Simon Fraser (smfr) 2018-07-18 17:32:34 PDT
These tests should never run for iOS simulator.
Comment 11 Truitt Savell 2018-07-18 17:38:32 PDT
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>
Comment 12 Daniel Bates 2018-07-18 18:46:16 PDT
(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.
Comment 13 Ryan Haddad 2018-07-19 08:56:29 PDT
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.
Comment 14 Ryan Haddad 2018-07-19 13:05:54 PDT
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
Comment 15 Ryan Haddad 2018-07-20 08:45:53 PDT
Rolled out this change and all of the follow ups in https://trac.webkit.org/changeset/234040
Comment 16 Daniel Bates 2018-07-20 15:03:50 PDT
(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.
Comment 17 Simon Fraser (smfr) 2019-06-16 21:05:47 PDT
Can this land again?
Comment 18 Daniel Bates 2019-06-16 22:13:09 PDT
(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 😀.
Comment 19 Daniel Bates 2019-06-16 22:14:07 PDT
All other bugs i think we're fixed.