Bug 147867

Summary: Search fields should scale when rendering while zoomed
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, kondapallykalyan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch dbates: review+

Description Wenson Hsieh 2015-08-10 22:17:02 PDT
When adjusting page scale factor or zoom factor, search fields should be scaled up when rendering.
Comment 1 Wenson Hsieh 2015-08-10 22:22:36 PDT
Created attachment 258705 [details]
Patch
Comment 2 Wenson Hsieh 2015-08-13 10:36:01 PDT
Created attachment 258897 [details]
Patch
Comment 3 Daniel Bates 2015-08-13 14:37:08 PDT
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.
Comment 4 Daniel Bates 2015-08-13 14:40:45 PDT
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?
Comment 5 Wenson Hsieh 2015-08-13 14:56:34 PDT
Good point. I'll pull both pieces of drawing code (search fields and selects) out to a common function in RenderThemeMac.
Comment 6 Wenson Hsieh 2015-08-13 16:30:12 PDT
Created attachment 258952 [details]
Patch
Comment 7 Wenson Hsieh 2015-08-13 16:35:27 PDT
Created attachment 258953 [details]
Patch
Comment 8 Wenson Hsieh 2015-08-13 16:49:55 PDT
Created attachment 258955 [details]
Patch
Comment 9 Wenson Hsieh 2015-08-13 16:52:19 PDT
<rdar://problem/10155575>
Comment 10 Wenson Hsieh 2015-08-13 18:18:02 PDT
Created attachment 258965 [details]
Patch
Comment 11 Wenson Hsieh 2015-08-13 20:50:03 PDT
Created attachment 258973 [details]
Patch
Comment 12 Wenson Hsieh 2015-08-14 07:03:06 PDT
Created attachment 258999 [details]
Patch
Comment 13 Daniel Bates 2015-08-14 17:05:09 PDT
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?
Comment 14 Wenson Hsieh 2015-08-15 10:50:32 PDT
Committed r188510: <http://trac.webkit.org/changeset/188510>