WebPageCreationParameters::encode doesn’t encode the suppressScrollbarAnimations member.
<rdar://problem/54043967>
WebPageCreationParameters.suppressScrollbarAnimations seems to be unused. It was added in https://trac.webkit.org/changeset/128134/
Created attachment 376564 [details] Patch
Attachment 376564 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WebKit/WebPageCreationParametersEncodeDecodeTest.cpp:31: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WebKit/WebPageCreationParametersEncodeDecodeTest.cpp:87: One space before end of line comments [whitespace/comments] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKit/WebPageCreationParametersEncodeDecodeTest.cpp:190: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/WebKit/WebPageCreationParametersEncodeDecodeTest.cpp:194: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 4 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 376564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376564&action=review > Source/WebCore/platform/graphics/DashArray.h:28 > +#include <CoreGraphics/CoreGraphics.h> Seems strange to include a platform-specific header from this cross-platform header. > Source/WebKit/Configurations/DebugRelease.xcconfig:43 > +OTHER_CFLAGS = $(inherited) -DWK_ENGINEERING_BUILD=1; Preprocessor definitions should go in the GCC_PEEPROCESSOR_DEFINITIONS build settings rather than appear as compiler flags, so that the rest of the build system can reason about them. Also, isn’t there already an ENGINEERING_BUILD macro used for this?
Comment on attachment 376564 [details] Patch This patch does not actually fix the bug. Yes you are now encoding / decoding it. However, WebPage is still not reading it upon creation.
(In reply to Chris Dumez from comment #6) > Comment on attachment 376564 [details] > Patch > > This patch does not actually fix the bug. Yes you are now encoding / > decoding it. However, WebPage is still not reading it upon creation. Note that if the API test was a real test, it would have probably caught this. An API test for low level encoding / decoding of WebPageCreationParameters is low value IMHO. I'd rather have an API test that shows the actual user-facing bug which would start passing with your change.
(In reply to Chris Dumez from comment #7) > (In reply to Chris Dumez from comment #6) > > Comment on attachment 376564 [details] > > Patch > > > > This patch does not actually fix the bug. Yes you are now encoding / > > decoding it. However, WebPage is still not reading it upon creation. > > Note that if the API test was a real test, it would have probably caught > this. An API test for low level encoding / decoding of > WebPageCreationParameters is low value IMHO. I'd rather have an API test > that shows the actual user-facing bug which would start passing with your > change. You likely want something like this in the WebPage constructor: setSuppressScrollbarAnimations(parameters.suppressScrollbarAnimations); And then you'd need to audit the code to figure out how to write a test for it. You'd likely need to create a WebView, then before doing any load (as this would start the WebContent process on macOS), change the value of suppressScrollbarAnimations, then do a load. When the WebContent process launches and the WebPage is created, it should now pick up this flag properly.
(In reply to Chris Dumez from comment #7) > (In reply to Chris Dumez from comment #6) > > Comment on attachment 376564 [details] > > Patch > > > > This patch does not actually fix the bug. Yes you are now encoding / > > decoding it. However, WebPage is still not reading it upon creation. > > Note that if the API test was a real test, it would have probably caught > this. An API test for low level encoding / decoding of > WebPageCreationParameters is low value IMHO. I'd rather have an API test > that shows the actual user-facing bug which would start passing with your > change. +1, despite Brady's insistence that this is a valuable way to test
(In reply to Chris Dumez from comment #8) > (In reply to Chris Dumez from comment #7) > > (In reply to Chris Dumez from comment #6) > > > Comment on attachment 376564 [details] > > > Patch > > > > > > This patch does not actually fix the bug. Yes you are now encoding / > > > decoding it. However, WebPage is still not reading it upon creation. > > > > Note that if the API test was a real test, it would have probably caught > > this. An API test for low level encoding / decoding of > > WebPageCreationParameters is low value IMHO. I'd rather have an API test > > that shows the actual user-facing bug which would start passing with your > > change. > > You likely want something like this in the WebPage constructor: > setSuppressScrollbarAnimations(parameters.suppressScrollbarAnimations); > > And then you'd need to audit the code to figure out how to write a test for > it. > You'd likely need to create a WebView, then before doing any load (as this > would start the WebContent process on macOS), change the value of > suppressScrollbarAnimations, then do a load. When the WebContent process > launches and the WebPage is created, it should now pick up this flag > properly. (or test that it comes back from a forced crash, which is my favorite way)
(In reply to Chris Dumez from comment #7) > Note that if the API test was a real test, it would have probably caught > this. An API test for low level encoding / decoding of > WebPageCreationParameters is low value IMHO. I'd rather have an API test > that shows the actual user-facing bug which would start passing with your > change. I agree. It seems weird to add the complexity of WK_TESTSUPPORT_EXPORT to support a means of testing that we don't really want to encourage.