Bug 131562 – [GTK] Add HighDPI support for non-accelerated compositing contents

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

We need plumbing to route the scaling factor from GTK+ to WebCore.

[Martin Robinson]

Created attachment 229165 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Owen, I made a few stylistic changes and moved the attachment of the property notify handler to webkitWebViewBaseCreateWebPage. This allowed me to transform the check for pageProxy into an assertion. Do you mind taking a quick look at the patch to see if I inadvertently introduced any non-cosmetic changes? :)

[Owen Taylor]

As far as can tell from comparing the patches, the changes seem to be functionally equivalent.

[Martin Robinson]

Okay. Thanks. I'll land this.

[WebKit Commit Bot]

Comment on attachment 229165 [details]
Patch

Clearing flags on attachment: 229165

Committed r167168: <http://trac.webkit.org/changeset/167168>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

Comment on attachment 229165 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:973
> +    ASSERT(priv->pageProxy);

This shouldn't happen, the signal is connected after creating the page and the page proxy is destroyed on finalize when the private struct is destroyed. If we want to be extra sure, we can save the signal handler and disconnect the signal on dispose since the page is closed there, any further call to page proxy after closed will be simply ignored.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:996
> +    g_signal_connect(object, "notify::scale-factor", G_CALLBACK(scaleFactorChanged), NULL);

What is object here? does this even build? Use nullptr, instead of NULL.

[Martin Robinson]

(In reply to comment #8)
> (From update of attachment 229165 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229165&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:973
> > +    ASSERT(priv->pageProxy);
> 
> This shouldn't happen, the signal is connected after creating the page and the page proxy is destroyed on finalize when the private struct is destroyed. If we want to be extra sure, we can save the signal handler and disconnect the signal on dispose since the page is closed there, any further call to page proxy after closed will be simply ignored.

"This shouldn't happen" is precisely the situation to use an assertion in!

> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:996
> > +    g_signal_connect(object, "notify::scale-factor", G_CALLBACK(scaleFactorChanged), NULL);
> 
> What is object here? does this even build? Use nullptr, instead of NULL.

Ah, I let this one slip through. I can land a fix for it.

[Philippe Normand]

Comment on attachment 229165 [details]
Patch

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

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:270
> +    ASSERT_UNUSED(xScale, 1 == xScale);

Has this been tested in Debug builds? This ASSERT hits in the Debug bot, see http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r167177%20(41643)/results.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 131589

[Carlos Garcia Campos]

(In reply to comment #10)
> (From update of attachment 229165 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229165&action=review
> 
> > Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:270
> > +    ASSERT_UNUSED(xScale, 1 == xScale);
> 
> Has this been tested in Debug builds? This ASSERT hits in the Debug bot, see http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r167177%20(41643)/results.html

The problem is that the tests can also set the device scale factor using WKPageSetCustomBackingScaleFactor, even if we don't set the intrinsic device scale factor to the web page proxy. So we might end up with a scaleFactor > 1 at any moment. We could make sure the custom scale factor is always 1 when hidpi is not supported for GTK+. I'll rebase the patch.

[Carlos Garcia Campos]

Created attachment 232244 [details]
Rebased patch

[Anders Carlsson]

Comment on attachment 232244 [details]
Rebased patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1853
> +    return;

Like we discussed, please add a FIXME saying that this is only temporary.

[Carlos Garcia Campos]

Committed r169445: <http://trac.webkit.org/changeset/169445>