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

[bugzilla-daemon at webkit.org]

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
text
https://bugs.webkit.org/show_bug.cgi?id=234707

Attachment 448007: Patch

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




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

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

>>> Source/WebCore/ChangeLog:8
>>> +	     Test:
imported/w3c/web-platform-tests/css/css-writing-modes/text-combine-upright-deco
rations-001.html
>> 
>> 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)

>>>
LayoutTests/imported/w3c/web-platform-tests/css/css-writing-modes/text-combine-
upright-decorations-001.html:28
>>> +	 -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:
https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/web-platf
orm-tests/wpt+-webkit-text-combine&patternType=literal
> 
> Once bug 150821 lands, I'll remove usages of -webkit-text-combine in the WPT
repo.
> 
> ---
> 
> 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.