Bug 225556

Summary: Add web process drawing to the focus test in MotionMark
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED WONTFIX    
Severity: Normal CC: cdumez, darin, ews-watchlist, ggaren, jbedard, mjs, rniwa, sabouhallawa, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
simon.fraser: review+
Patch for committing ews-feeder: commit-queue-

Description Myles C. Maxfield 2021-05-07 22:58:21 PDT
Add web process drawing to the focus test in MotionMark
Comment 1 Myles C. Maxfield 2021-05-07 23:24:46 PDT
Created attachment 428079 [details]
Patch
Comment 2 Simon Fraser (smfr) 2021-05-08 10:29:40 PDT
Comment on attachment 428079 [details]
Patch

When we do version bumps, are they normally part of the patch?
Comment 3 Myles C. Maxfield 2021-05-08 17:41:50 PDT
(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 428079 [details]
> Patch
> 
> When we do version bumps, are they normally part of the patch?

I think we batch up multiple patches into a single version bump.

This patch only updates what's in PerformanceTests. When we want to publish, we copy that into Websites/browserbench.org.
Comment 4 Darin Adler 2021-05-08 21:29:56 PDT
Comment on attachment 428079 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428079&action=review

> PerformanceTests/ChangeLog:11
> +        Previously, the MotionMark focus test just animated the blur, opacity, and position of each element.
> +        All of these get mapped to properties on the Core Animation layers, which means that each rAF() tick,
> +        the only thing we were doing was pushing layer state changes to the window server.

This comment talks a lot about window server and Core Animation. But this is a cross-platform, cross-browser benchmark, and window server is specific to Mac, and Core Animation specific to Apple’s macOS/iOS/iPadOS/tvOS/watchOS.

I’m sure that these these improvements help us have a more repeatable test on macOS, but are also helpful on other platforms, including iOS, and to accurately test other web browsers as well. But that’s not stated here and probably should be. We would not want to make the changes if they were harmful in those other configurations.

I’m not sure we need quite so long a change log comment. Maybe there is somewhere else we should leave this information if we are going to need it in the future. In the change log it might or might not be found.
Comment 5 Simon Fraser (smfr) 2021-05-11 10:35:33 PDT
Comment on attachment 428079 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428079&action=review

>> PerformanceTests/ChangeLog:11
>> +        the only thing we were doing was pushing layer state changes to the window server.
> 
> This comment talks a lot about window server and Core Animation. But this is a cross-platform, cross-browser benchmark, and window server is specific to Mac, and Core Animation specific to Apple’s macOS/iOS/iPadOS/tvOS/watchOS.
> 
> I’m sure that these these improvements help us have a more repeatable test on macOS, but are also helpful on other platforms, including iOS, and to accurately test other web browsers as well. But that’s not stated here and probably should be. We would not want to make the changes if they were harmful in those other configurations.
> 
> I’m not sure we need quite so long a change log comment. Maybe there is somewhere else we should leave this information if we are going to need it in the future. In the change log it might or might not be found.

Agreed that we can trim down this changelog.
Comment 6 Myles C. Maxfield 2021-05-11 22:52:43 PDT
Created attachment 428347 [details]
Patch for committing
Comment 7 Radar WebKit Bug Importer 2021-05-14 22:59:19 PDT
<rdar://problem/78047813>
Comment 8 Myles C. Maxfield 2021-07-22 11:01:41 PDT
Focus test has been removed.