Bug 123325 – [GTK][WK2] Build break after r157967 and r157972

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Sergio Villar Senin]

Several changes related to visibility and DrawingAreas

[Csaba Osztrogonác]

Maybe https://bugs.webkit.org/show_bug.cgi?id=123324 will fix GTK too.

[Sergio Villar Senin]

(In reply to comment #1)
> Maybe https://bugs.webkit.org/show_bug.cgi?id=123324 will fix GTK too.

Partially, we need some specific bits from GTK.

[Sergio Villar Senin]

Created attachment 215151 [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

[Sergio Villar Senin]

Created attachment 215153 [details]
Patch

[Sergio Villar Senin]

Replaced gtk_widget_is_visible() by gtk_widget_get_visible()

[Carlos Garcia Campos]

Comment on attachment 215153 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:294
> +    return gtk_widget_get_visible(gtk_widget_get_toplevel(m_viewWidget));

Is this always called once the view is inside a toplevel? because otherwise get_toplevel can return NULL. Should we handle offscreen windows differently? Maybe we can add webkitWebViewBaseIsWindowVisible() and return early if priv->toplevelOnScreenWindow is NULL, and use it instead of calling get_toplevel again.

[Sergio Villar Senin]

Comment on attachment 215153 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:294
>> +    return gtk_widget_get_visible(gtk_widget_get_toplevel(m_viewWidget));
> 
> Is this always called once the view is inside a toplevel? because otherwise get_toplevel can return NULL. Should we handle offscreen windows differently? Maybe we can add webkitWebViewBaseIsWindowVisible() and return early if priv->toplevelOnScreenWindow is NULL, and use it instead of calling get_toplevel again.

This is called in the WebPageProxy when viewStateDidChange() is called with the WindowIsVisible flag. It looks like we are never passing that to the proxy, so we need to send that information to the proxy from the webview in order to properly fix that. Once we track the visibility status of the window we can use it to return the right value in ::isWindowVisible()

[Sergio Villar Senin]

Created attachment 215170 [details]
Patch

[WebKit Commit Bot]

Attachment 215170 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp', u'Source/WebKit2/UIProcess/API/gtk/PageClientImpl.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h', u'Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp', u'Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h', u'Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp']" exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:243:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 215170 [details]
Patch

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

Looks great, please fix the style issues before landing. Thanks!

> Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp:-140
> -}

We are the only ones currently using DrawingAreaProxyImpl, this simply removes code and it's a build fix in the end so I assume we don't need approval from a WebKit2 owner for this.

[Sergio Villar Senin]

(In reply to comment #11)
> (From update of attachment 215170 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215170&action=review
> 
> Looks great, please fix the style issues before landing. Thanks!

It's a false positive because we indent the parameters of g_signal_connect

> > Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp:-140
> > -}
> 
> We are the only ones currently using DrawingAreaProxyImpl, this simply removes code and it's a build fix in the end so I assume we don't need approval from a WebKit2 owner for this.

[Sergio Villar Senin]

Committed r158013: <http://trac.webkit.org/changeset/158013>

[Martin Robinson]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 215170 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=215170&action=review
> > 
> > Looks great, please fix the style issues before landing. Thanks!
> 
> It's a false positive because we indent the parameters of g_signal_connect

We used to align them, but WebKit style specifies the indentation now, so we started following it.