Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Only send one drawing command per list marker #6842

Merged
merged 1 commit into from Nov 30, 2022
Merged

Only send one drawing command per list marker #6842

merged 1 commit into from Nov 30, 2022

Conversation

Ahmad-S792
Copy link
Contributor

@Ahmad-S792 Ahmad-S792 commented Nov 27, 2022 •
edited by webkit-early-warning-system

5e81d33

Only send one drawing command per list marker

Only send one drawing command per list marker
https://bugs.webkit.org/show_bug.cgi?id=248378

Reviewed by Tim Nguyen.

Merge - https://src.chromium.org/viewvc/blink?view=revision&;revision=151161

Previously, some list markers (notably list-style-type: disc, circle, square)
would send two drawing commands per marker: one to fill the marker, and
then a second one to fill it. This constructs unnecessary paints and causes
a weird visual effect if the color is translucent.

This should have a minor visible effect on these list markers, but it's not
identical: since the stroke area isn't drawn again, the antialiasing varies.

* Source/WebCore/rendering/RenderListMarker.cpp:
(RenderListMarker::paint): Update draw to fill or stroke for "disc", "circle" and "square"
* LayoutTests/fast/lists/list-type-translucent-color.html: Added Test Case
* LayoutTests/fast/lists/list-type-translucent-color-expected.html: Added Test Case Expectation

Canonical link: https://commits.webkit.org/257175@main

de1364b

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk 💥 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 api-mac   🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk1
✅ 🛠 tv-sim ✅ 🧪 mac-wk2
✅ 🛠 🧪 merge ✅ 🛠 watch ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 5583788)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 api-mac ❌ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk1
✅ 🛠 tv-sim ✅ 🧪 mac-wk2
✅ 🛠 watch ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress

@Ahmad-S792 Ahmad-S792 self-assigned this Nov 28, 2022
@Ahmad-S792 Ahmad-S792 added the Layout and Rendering For bugs with layout and rendering of Web pages. label Nov 28, 2022
@Ahmad-S792 Ahmad-S792 marked this pull request as ready for review November 28, 2022 13:39
smfr
smfr previously requested changes Nov 28, 2022
View reviewed changes
Copy link
Contributor

@smfr smfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be possible to make a ref test for this.

@Ahmad-S792 Ahmad-S792 force-pushed the one-drawing-command-per-list-marker branch from 5583788 to dc96715 Compare November 28, 2022 17:31
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash dc96715)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl   🧪 ios-wk2 ✅ 🛠 mac-AS-debug   🧪 gtk-wk2
  🧪 api-ios   🧪 api-mac   🧪 api-gtk
✅ 🛠 tv   🧪 mac-wk1
✅ 🛠 tv-sim   🧪 mac-wk2
✅ 🛠 watch   🧪 mac-AS-debug-wk2
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress

@Ahmad-S792 Ahmad-S792 force-pushed the one-drawing-command-per-list-marker branch from dc96715 to 5b558cb Compare November 28, 2022 19:37
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 5b558cb)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 api-mac ✅ 🧪 api-gtk
✅ 🛠 tv ❌ 🧪 mac-wk1
✅ 🛠 tv-sim ❌ 🧪 mac-wk2
✅ 🛠 watch ❌ 🧪 mac-AS-debug-wk2
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress

@Ahmad-S792 Ahmad-S792 dismissed smfr’s stale review November 28, 2022 20:02

Added Test case and Tim reviewed it.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 29, 2022
@Ahmad-S792 Ahmad-S792 force-pushed the one-drawing-command-per-list-marker branch from 5b558cb to de1364b Compare November 29, 2022 13:41
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash de1364b)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk 💥 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 api-mac   🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk1
✅ 🛠 tv-sim ✅ 🧪 mac-wk2
✅ 🛠 🧪 merge ✅ 🛠 watch ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress

@Ahmad-S792
Copy link
Contributor Author

@nt1m - I had to add tolerance / fuzzy to testcase since it had 0.01% difference on macOS, do you want to have quick review or if it passes EWS, I can commit it?

@Ahmad-S792 Ahmad-S792 added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Nov 30, 2022
@Ahmad-S792
Only send one drawing command per list marker
5e81d33 
Only send one drawing command per list marker
https://bugs.webkit.org/show_bug.cgi?id=248378

Reviewed by Tim Nguyen.

Merge - https://src.chromium.org/viewvc/blink?view=revision&revision=151161

Previously, some list markers (notably list-style-type: disc, circle, square)
would send two drawing commands per marker: one to fill the marker, and
then a second one to fill it. This constructs unnecessary paints and causes
a weird visual effect if the color is translucent.

This should have a minor visible effect on these list markers, but it's not
identical: since the stroke area isn't drawn again, the antialiasing varies.

* Source/WebCore/rendering/RenderListMarker.cpp:
(RenderListMarker::paint): Update draw to fill or stroke for "disc", "circle" and "square"
* LayoutTests/fast/lists/list-type-translucent-color.html: Added Test Case
* LayoutTests/fast/lists/list-type-translucent-color-expected.html: Added Test Case Expectation

Canonical link: https://commits.webkit.org/257175@main
@webkit-commit-queue
Copy link
Collaborator

Committed 257175@main (5e81d33): https://commits.webkit.org/257175@main

Reviewed commits have been landed. Closing PR #6842 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit 5e81d33 into WebKit:main Nov 30, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 30, 2022
@Ahmad-S792 Ahmad-S792 deleted the one-drawing-command-per-list-marker branch December 3, 2022 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nt1m nt1m nt1m approved these changes

@smfr smfr smfr left review comments

@alanbaradlay alanbaradlay Awaiting requested review from alanbaradlay

Assignees

@Ahmad-S792 Ahmad-S792

Labels
Layout and Rendering For bugs with layout and rendering of Web pages.
Projects
None yet
Milestone
No milestone
6 participants
@Ahmad-S792 @webkit-early-warning-system @webkit-commit-queue @smfr @nt1m @webkit-ews-buildbot