Summary: | Provide an lldb type summary for WebCore::Color | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dean Jackson <dino> | ||||
Component: | New Bugs | Assignee: | Dean Jackson <dino> | ||||
Status: | NEW --- | ||||||
Severity: | Normal | CC: | aakash_jain, dbates, ews-watchlist, realdawei, ryanhaddad, simon.fraser, tsavell | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | 187855, 187856, 187872 | ||||||
Bug Blocks: | |||||||
Attachments: |
|
Description
Dean Jackson
2018-07-18 11:29:17 PDT
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. |