Bug 147867 - Search fields should scale when rendering while zoomed
Summary: Search fields should scale when rendering while zoomed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-08-10 22:17 PDT by Wenson Hsieh
Modified: 2015-08-15 10:50 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.55 KB, patch)
2015-08-10 22:22 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (2.52 KB, patch)
2015-08-13 10:36 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (5.44 KB, patch)
2015-08-13 16:30 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (5.33 KB, patch)
2015-08-13 16:35 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (5.34 KB, patch)
2015-08-13 16:49 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (5.31 KB, patch)
2015-08-13 18:18 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (5.36 KB, patch)
2015-08-13 20:50 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (5.27 KB, patch)
2015-08-14 07:03 PDT, Wenson Hsieh
dbates: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>