[webkit-reviews] review granted: [Bug 234707] REGRESSION(r286955): Fix painting text-decorations with combined text : [Attachment 448007] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Dec 30 14:08:21 PST 2021

Dean Jackson <dino at apple.com> has granted Tim Nguyen (:ntim) <ntim at apple.com>'s
request for review:
Bug 234707: REGRESSION(r286955): Fix painting text-decorations with combined

Attachment 448007: Patch


--- Comment #9 from Dean Jackson <dino at apple.com> ---
Comment on attachment 448007
  --> https://bugs.webkit.org/attachment.cgi?id=448007

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

>>> Source/WebCore/ChangeLog:8
>>> +	     Test:
>> Has this test been failing? If not, then how is this test covering the bug?
>> We need a test that can detect the bug, right?
> It hasn't been failing because it uses text-combine-upright (and we support
-webkit-text-combine instead). If you add `-webkit-text-combine`, the test
starts failing. 
> Bug 150821 aliases `-webkit-text-combine: horizontal` to
`text-combine-upright: all` (and the test does fail there).

OK, so 150821 marks this test as ImageOnlyFailure. But this patch doesn't
remove that expectation?

> Source/WebCore/rendering/TextBoxPainter.cpp:397
> +	   GraphicsContext& context = m_paintInfo.context();

Is there value in having the local variable here? (and below)

>>> +	 -webkit-text-combine: horizontal;
>> Why are we making this change to a WPT test? Is this being upstreamed? Do
the other browser vendors want this test to have a "-webkit-" prefixed property
in it?
>> Change log doesn’t explain any of this.
> I'm planning to upstream it:
https://github.com/web-platform-tests/wpt/pull/32204 (this PR should get
approved automatically when this patch gets r+). It's not uncommon practice to
add vendor prefixes if the main thing being tested isn't the syntax itself, see
current usages:
> Once bug 150821 lands, I'll remove usages of -webkit-text-combine in the WPT
> ---
> Alternatively, we can land bug 150821 first, and then remove the
ImageOnlyFailure test expectation in this bug.

Now I get it. Yeah, I think this patch should land after 150821 and change the
expectation to pass.

More information about the webkit-reviews mailing list