Bug 180904 – WebPageCreationParameters doesn’t encode suppressScrollbarAnimations

• Component: WebKit2
• Status: NEW

WebPageCreationParameters::encode doesn’t encode the suppressScrollbarAnimations member.

[Radar WebKit Bug Importer]

<rdar://problem/54043967>

[Simon Fraser (smfr)]

WebPageCreationParameters.suppressScrollbarAnimations seems to be unused. It was added in https://trac.webkit.org/changeset/128134/

[Kate Cheney]

Created attachment 376564 [details]
Patch

[EWS Watchlist]

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?

[Chris Dumez]

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.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Tim Horton]

(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

[Tim Horton]

(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)

[Simon Fraser (smfr)]

(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.