Provide an lldb type summary for WebCore::Color
Created attachment 345264 [details] Patch
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?
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 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 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?
(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.
(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.
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.
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
These tests should never run for iOS simulator.
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>
(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.
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.
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
Rolled out this change and all of the follow ups in https://trac.webkit.org/changeset/234040
(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.
Can this land again?
(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 😀.
All other bugs i think we're fixed.