[webkit-reviews] review granted: [Bug 45522] Remove unnecessary constraint in WebCore of choosing either text zoom or full page zoom. : [Attachment 67153] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 45522: Remove unnecessary constraint in WebCore of choosing either text
zoom or full page zoom.
https://bugs.webkit.org/show_bug.cgi?id=45522

Attachment 67153: Patch
https://bugs.webkit.org/attachment.cgi?id=67153&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    float textZoomFactor() const  { return m_textZoomFactor; }

There's an extra space here.

> +	   view->setPageZoomFactor(1.0f);

I suggest just "1" here instead of "1.0f".

> +	   view->setTextZoomFactor(1.0f);

And here.

> +    if (m_zoomsTextOnly != isTextOnly) {
> +	   m_zoomsTextOnly = isTextOnly;
> +	   m_page->setNeedsRecalcStyleInAllFrames();
> +    }

WHy is this call to setNeedsRecalcStyleInAllFrames needed? The calls to
setPageZoomFactor and setTextZoomFactor should handle everything. If that is
not so, then we should consider optimizing the case where the multiplier is 1.
I suspect at least some of the new calls you added to
setNeedsRecalcStyleInAllFrames are unneeded, and I’m not sure you needed to
move the function out of the Settings class at all.

> +    WKBundlePageSetPageZoomFactor(InjectedBundle::shared().page()->page(),
1.0f);

Again, how about 1 instead of 1.0f?

> +    WKBundlePageSetPageZoomFactor(InjectedBundle::shared().page()->page(),
1.0f);

Ditto.

And more of the same.