Bug 131562

Summary: [GTK] Add HighDPI support for non-accelerated compositing contents
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cgarcia, clopez, commit-queue, d-r, gustavo, otaylor
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 131589    
Bug Blocks: 133377    
Attachments:
Description Flags
Patch
none
Rebased patch andersca: review+

Description Martin Robinson 2014-04-11 13:59:57 PDT
We need plumbing to route the scaling factor from GTK+ to WebCore.
Comment 1 Martin Robinson 2014-04-11 15:07:05 PDT
Created attachment 229165 [details]
Patch
Comment 2 WebKit Commit Bot 2014-04-11 15:09:28 PDT
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
Comment 3 Martin Robinson 2014-04-11 15:12:34 PDT
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? :)
Comment 4 Owen Taylor 2014-04-11 15:41:06 PDT
As far as can tell from comparing the patches, the changes seem to be functionally equivalent.
Comment 5 Martin Robinson 2014-04-11 15:42:44 PDT
Okay. Thanks. I'll land this.
Comment 6 WebKit Commit Bot 2014-04-11 16:20:52 PDT
Comment on attachment 229165 [details]
Patch

Clearing flags on attachment: 229165

Committed r167168: <http://trac.webkit.org/changeset/167168>
Comment 7 WebKit Commit Bot 2014-04-11 16:20:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Carlos Garcia Campos 2014-04-12 02:14:07 PDT
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.
Comment 9 Martin Robinson 2014-04-12 08:45:27 PDT
(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.
Comment 10 Philippe Normand 2014-04-13 04:11:26 PDT
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
Comment 11 WebKit Commit Bot 2014-04-13 04:19:10 PDT
Re-opened since this is blocked by bug 131589
Comment 12 Carlos Garcia Campos 2014-05-29 05:06:26 PDT
(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.
Comment 13 Carlos Garcia Campos 2014-05-29 05:40:35 PDT
Created attachment 232244 [details]
Rebased patch
Comment 14 Anders Carlsson 2014-05-29 08:00:52 PDT
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.
Comment 15 Carlos Garcia Campos 2014-05-29 08:11:32 PDT
Committed r169445: <http://trac.webkit.org/changeset/169445>