[webkit-reviews] review granted: [Bug 215428] [macOS] Zoomed-in search field is clipped out : [Attachment 406488] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Aditya Keerthi <akeerthi at apple.com>'s
request for review:
Bug 215428: [macOS] Zoomed-in search field is clipped out
https://bugs.webkit.org/show_bug.cgi?id=215428

Attachment 406488: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 406488
  --> https://bugs.webkit.org/attachment.cgi?id=406488
Patch

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

> Source/WebCore/rendering/RenderThemeMac.mm:1999
> +    NSControlSize controlSize = controlSizeForFont(style);
> +    setFontFromControlSize(style, controlSize);

I know you just moved this, but I suggest doing this in one line without a
local variable.

> LayoutTests/ChangeLog:15
> +	   * fast/forms/search/search-zoom-rendering-expected.txt: Added.
> +	   * fast/forms/search/search-zoom-rendering.html: Added.
> +	   *
platform/ios-wk2/fast/forms/search/search-zoom-rendering-expected.txt: Added.
> +	   *
platform/mac-mojave/fast/forms/search/search-zoom-rendering-expected.txt:
Added.
> +	   * platform/win/fast/forms/search/search-zoom-rendering-expected.txt:
Added.

It’s really undesirable to have another render-tree-dumping test like this. Is
there any way we can test this without a render tree dump?

One thing to think about is that the render tree dump doesn’t actually check
the clipping, only the geometry. Since that’s all we need to check, maybe we
can do a reftest that explicitly sets the size? Or perhaps we can do a
dumpAsText test that explicitly queries the height?

We can land like this, but these are really hard to maintain over time. I’m
particularly puzzled by the seemingly random set of expectation files. For
example, why does legacy WebKit testing on iOS use the shared master
expectation, but modern WebKit testing on iOS use its own. Maybe that file
should be in platform/ios instead of in platform/ios-wk2?