Bug 147867 – Search fields should scale when rendering while zoomed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Wenson Hsieh]

When adjusting page scale factor or zoom factor, search fields should be scaled up when rendering.

[Wenson Hsieh]

Created attachment 258705 [details]
Patch

[Wenson Hsieh]

Created attachment 258897 [details]
Patch

[Daniel Bates]

Comment on attachment 258897 [details]
Patch

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

In theory we could test this change by writing a pixel test. I'm unclear of the value of adding such a test given that the build.webkit.org tester do not run pixel tests :(

> Source/WebCore/rendering/RenderThemeMac.mm:1617
> +    Page *page = o.document().page();

Nit: The * should be on the left:

Page* page = o.document().page();

> Source/WebCore/rendering/RenderThemeMac.mm:1618
> +    float pageScaleFactor = page->pageScaleFactor();

This variable is referenced exactly once in this function (line 1621) and its name does not add anything that cannot be inferred from its right hand side expression. I suggest that we remove this variable and inline its value on line 1621.

> Source/WebCore/rendering/RenderThemeMac.mm:1619
> +    float deviceScaleFactor = page->deviceScaleFactor();

By a similar argument, I suggest that we remove this variable and inline its value into line 1623.

[Daniel Bates]

Comment on attachment 258897 [details]
Patch

On another note, this code is almost identical to the code that you added to RenderThemeMac::paintMenuList() in the patch for bug #147868. Can we share more code?

[Wenson Hsieh]

Good point. I'll pull both pieces of drawing code (search fields and selects) out to a common function in RenderThemeMac.

[Wenson Hsieh]

Created attachment 258952 [details]
Patch

[Wenson Hsieh]

Created attachment 258953 [details]
Patch

[Wenson Hsieh]

Created attachment 258955 [details]
Patch

[Wenson Hsieh]

<rdar://problem/10155575>

[Wenson Hsieh]

Created attachment 258965 [details]
Patch

[Wenson Hsieh]

Created attachment 258973 [details]
Patch

[Wenson Hsieh]

Created attachment 258999 [details]
Patch

[Daniel Bates]

Comment on attachment 258999 [details]
Patch

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

Thank you for iterating on the patch.

> Source/WebCore/ChangeLog:14
> +        (WebCore::RenderThemeMac::paintMenuList): Refactored to use paintCellAndSetFocusedElementNeedsRepaintIfNecessary

Nit: Missing period at the end of this sentence.

> Source/WebCore/rendering/RenderThemeMac.mm:1462
> +    bool shouldUseImageBuffer = renderer.style().effectiveZoom() != 1.0f || page->pageScaleFactor() != 1.0f;

It's not necessary to use the .0f suffix as per the WebKit Code Style guidelines.

> Source/WebCore/rendering/RenderThemeMac.mm:1464
> +    if (ThemeMac::drawCellOrFocusRingWithViewIntoContext(cell, paintInfo.context, rect, documentViewFor(renderer), shouldDrawCell, shouldDrawFocusRing, shouldUseImageBuffer, page->deviceScaleFactor()))

I know this is not part of your patch. I noticed that ThemeMac::drawCellOrFocusRingWithViewIntoContext() takes its first argument as a RetainPtr. Can we have it take a raw pointer?

[Wenson Hsieh]

Committed r188510: <http://trac.webkit.org/changeset/188510>