[webkit-reviews] review granted: [Bug 196338] [SimpleLineLayout] Disable SLL when text-underline-position is not auto. : [Attachment 366153] Patch

Wed Mar 27 22:49:28 PDT 2019

Daniel Bates <dbates at webkit.org> has granted zalan <zalan at apple.com>'s request
for review:
Bug 196338: [SimpleLineLayout] Disable SLL when text-underline-position is not
auto.
https://bugs.webkit.org/show_bug.cgi?id=196338

Attachment 366153: Patch

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

--- Comment #3 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 366153
  --> https://bugs.webkit.org/attachment.cgi?id=366153
Patch

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

> Source/WebCore/ChangeLog:7
> +	   Disable simple line layout unconditionally on non-auto
text-underline-position content. We don's support it yet. 

Don?

Changelog order does not look correct. This line should be under the next line.
See other change logs.

>
LayoutTests/fast/text/simple-line-layout-with-text-underline-position-expected.
html:1
> +<html>

Ok as-is. Bothers me there is no doctype, but that is just me and no quirks to
trip over.

>
LayoutTests/fast/text/simple-line-layout-with-text-underline-position-expected.
html:5
> +<body></body>

Haha. Got it, SLL disabled = no output just like a page with an empty body. 😀

> LayoutTests/fast/text/simple-line-layout-with-text-underline-position.html:1
> +<html>

Ok as-is. Test uses quirks mode, but this seems unnecessary.

> LayoutTests/fast/text/simple-line-layout-with-text-underline-position.html:3
> +<title>This tests that simple line layout is disabled for
text-underline-position: under</title>

Ok as-is. I personally don't like long web page titles. WPT like it. I don't.
Want to know more then ask me. No need to appease me though.

> LayoutTests/fast/text/simple-line-layout-with-text-underline-position.html:10
> +    overflow:hidden;

Ok as-is. Inconsistent style. (Alexey would say this adds randomness, but to me
it just bothers me though no need to do anything; ok as-is.) Look at other
lines if you want to.

> LayoutTests/fast/text/simple-line-layout-with-text-underline-position.html:24
> +<div class=first>Pass if after selecting these 2 lines</div>

Ok as-is. Text does not make sense to me. Test does not look like it
programmatically selects anything.