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: | Mac | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Wenson Hsieh
2015-08-10 22:17:02 PDT
Created attachment 258705 [details]
Patch
Created attachment 258897 [details]
Patch
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 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? Good point. I'll pull both pieces of drawing code (search fields and selects) out to a common function in RenderThemeMac. Created attachment 258952 [details]
Patch
Created attachment 258953 [details]
Patch
Created attachment 258955 [details]
Patch
Created attachment 258965 [details]
Patch
Created attachment 258973 [details]
Patch
Created attachment 258999 [details]
Patch
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? Committed r188510: <http://trac.webkit.org/changeset/188510> |