Bug 45522 – Remove unnecessary constraint in WebCore of choosing either text zoom or full page zoom.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Sam Weinig]

We should remove the unnecessary constraint in WebCore of having to choose between either using text zoom or full page zoom. There is no reason they can't both be used independently of each other.  That behavior should be up to the port API layer.

[Sam Weinig]

Created attachment 67153 [details]
Patch

[Sam Weinig]

It would be really great if people from the different ports could take a look at the changes I made to the WebKit layer, and make sure that it is all up to grade.

[Xan Lopez]

(In reply to comment #2)
> It would be really great if people from the different ports could take a look at the changes I made to the WebKit layer, and make sure that it is all up to grade.

In GTK+ you just changed things so that they keep working as they are now right? ie, faking that full page zoom and text zoom are still mutually exclusive. In that case it looks good, although of course we should change the APIs to take into account the new situation. Thanks for taking care of things!

[Darin Adler]

Comment on attachment 67153 [details]
Patch

> +    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.

[Darin Fisher (:fishd, Google)]

Comment on attachment 67153 [details]
Patch

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

> WebKit/chromium/src/FrameLoaderClientImpl.cpp:1401
> +    bool zoomTextOnly = m_webFrame->viewImpl();
viewImpl is of type WebViewImpl*, but you are storing it in a 'bool'.  i think this is going to force zoomTextOnly to always be true.

i think you meant to write m_webFrame->viewImpl()->zoomTextOnly().

> WebKit/chromium/src/WebViewImpl.cpp:1494
> +        m_page->setNeedsRecalcStyleInAllFrames();
why isn't this a byproduct of calling set{Text,Page}ZoomFactor?  isn't unfortunate for WebKit layers to need to understand when to call setNeedsRecalcStyleInAllFrames?

[Darin Adler]

(In reply to comment #5)
> > WebKit/chromium/src/FrameLoaderClientImpl.cpp:1401
> > +    bool zoomTextOnly = m_webFrame->viewImpl();
> viewImpl is of type WebViewImpl*, but you are storing it in a 'bool'.  i think this is going to force zoomTextOnly to always be true.
> 
> i think you meant to write m_webFrame->viewImpl()->zoomTextOnly().

Good catch! I missed that one!

> > WebKit/chromium/src/WebViewImpl.cpp:1494
> > +        m_page->setNeedsRecalcStyleInAllFrames();
> why isn't this a byproduct of calling set{Text,Page}ZoomFactor?  isn't unfortunate for WebKit layers to need to understand when to call setNeedsRecalcStyleInAllFrames?

Not sure if you noticed, but I made the same comment.

[Peter Kasting]

Comment on attachment 67153 [details]
Patch

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

> WebKit/chromium/src/WebViewImpl.cpp:1485
> +        view->setPageZoomFactor(1.0f);
It seems slightly strange to me that we're removing a constraint on "only use text zoom or page zoom at one time" and yet this still forces the other zoom level to 1 every time one zoom level is set.  It seems like it would be better to either rely on the port to do this if it wants zooming one thing to reset the other, or make the APIs take both a text and page zoom value simultaneously (so that ports must specify what they mean, and we avoid doing two style recalcs in a row).

[Darin Adler]

(In reply to comment #7)
> It seems slightly strange to me that we're removing a constraint on "only use text zoom or page zoom at one time" and yet this still forces the other zoom level to 1 every time one zoom level is set. It seems like it would be better to either rely on the port to do this if it wants zooming one thing to reset the other, or make the APIs take both a text and page zoom value simultaneously (so that ports must specify what they mean, and we avoid doing two style recalcs in a row).

The code you quoted is from inside the Chromium port, and we do rely on the port to do this!

This patch preserves the WebKit API for the various ports. You suggestions point to a direction for improving Chromium’s WebKit API and could be done in a different patch.

[Darin Adler]

(In reply to comment #7)
> we avoid doing two style recalcs in a row

There will only be one style recalc. The code sets a flag saying recalc is needed, and doing that twice is relatively inexpensive.

[Sam Weinig]

Created attachment 67229 [details]
Updated patch

Thanks for the feedback. Here is an updated patch. It turns out the call to setNeedsRecalcStyleInAllFrames was unnecessary and that we would probably end up doing a little too much work if we called both setPageZoomFactor and setTextZoomFactor in a row, so I have added a function which can change both at once if that is what is wanted.

[Darin Adler]

(In reply to comment #10)
> we would probably end up doing a little too much work if we called both setPageZoomFactor and setTextZoomFactor in a row, so I have added a function which can change both at once if that is what is wanted.

Really? Are you sure?

There are other changes someone might make at the same time too, so I don’t see why we have to gang these two together. I think having a function to change both at once is not worthwhile, and poor factoring. Unless it’s measurably better for performance.

[Darin Adler]

To put it another way: If setting both at once is really more efficient, then I suggest including that in WebKit2’s public API too.

[Sam Weinig]

(In reply to comment #11)
> (In reply to comment #10)
> > we would probably end up doing a little too much work if we called both setPageZoomFactor and setTextZoomFactor in a row, so I have added a function which can change both at once if that is what is wanted.
> 
> Really? Are you sure?
> 
> There are other changes someone might make at the same time too, so I don’t see why we have to gang these two together. I think having a function to change both at once is not worthwhile, and poor factoring. Unless it’s measurably better for performance.

Well, both end up calling document->recalcStyle(Node::Force);, which Dan tells me can be expensive.

[Sam Weinig]

(In reply to comment #12)
> To put it another way: If setting both at once is really more efficient, then I suggest including that in WebKit2’s public API too.

I think for the UIProcess side, I will add something for that, for the bundle side (for the test runner) I don't see a compelling reason to optimize this at this point.

[Darin Fisher (:fishd, Google)]

Comment on attachment 67229 [details]
Updated patch

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

> WebKit/chromium/src/WebViewImpl.cpp:1489
> +    if (oldZoomFactor != zoomFactor && textOnly != m_zoomTextOnly) {
shouldn't this be an OR?  if either the zoom factor or the textOnly flag changes, we need to notify the plugin.

[Sam Weinig]

(In reply to comment #15)
> (From update of attachment 67229 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67229&action=prettypatch
> 
> > WebKit/chromium/src/WebViewImpl.cpp:1489
> > +    if (oldZoomFactor != zoomFactor && textOnly != m_zoomTextOnly) {
> shouldn't this be an OR?  if either the zoom factor or the textOnly flag changes, we need to notify the plugin.

(In reply to comment #15)
> (From update of attachment 67229 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67229&action=prettypatch
> 
> > WebKit/chromium/src/WebViewImpl.cpp:1489
> > +    if (oldZoomFactor != zoomFactor && textOnly != m_zoomTextOnly) {
> shouldn't this be an OR?  if either the zoom factor or the textOnly flag changes, we need to notify the plugin.

Yeah. It should. Is the rest of the patch ok?

[Sam Weinig]

Created attachment 67275 [details]
Patch 3

[Darin Adler]

Comment on attachment 67275 [details]
Patch 3

> +    float textZoomFactor() const  { return m_textZoomFactor; }

Extra space here after const.

Longer term you should find a way to make it efficient to set both zoom factors separately, presumably by making sure the style recalculation is deferred, and eliminate the function to do both at once.

r=me

[WebKit Review Bot]

http://trac.webkit.org/changeset/67274 might have broken Qt Linux Release minimal

[Sam Weinig]

Landed in http://trac.webkit.org/changeset/67274.