Bug 75435 – [GTK] Use gdk_screen_get_monitor_workarea() when available for screenAvailableRect()

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Instead of our own implementation. It was added to GTK+ in version 3.3.6

[Carlos Garcia Campos]

Created attachment 120879 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/gtk/GtkVersioning.h:121:  The parameter name "screen" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 4 files


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

[Collabora GTK+ EWS bot]

Comment on attachment 120879 [details]
Patch

Attachment 120879 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11003018

[WebKit Review Bot]

Comment on attachment 120879 [details]
Patch

Attachment 120879 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11046323

New failing tests:
http/tests/inspector/resource-tree/resource-tree-document-url.html

[Martin Robinson]

Comment on attachment 120879 [details]
Patch

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

Nice.

> Source/WebCore/platform/gtk/GtkVersioning.c:355
> +    int result = XGetWindowProperty(display, rootWindow, workArea, 0, 4 * 32 /* Mas len */, False, AnyPropertyType,

Mas len -> max length?

>> Source/WebCore/platform/gtk/GtkVersioning.h:121
>> +void gdk_screen_get_monitor_workarea(GdkScreen *screen, int monitor, GdkRectangle *area);
> 
> The parameter name "screen" adds no information, so it should be removed.  [readability/parameter_name] [5]

Go ahead and fix this one, I'd say.

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 120879 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120879&action=review
> 
> Nice.
> 
> > Source/WebCore/platform/gtk/GtkVersioning.c:355
> > +    int result = XGetWindowProperty(display, rootWindow, workArea, 0, 4 * 32 /* Mas len */, False, AnyPropertyType,
> 
> Mas len -> max length?

oops! :-P

> >> Source/WebCore/platform/gtk/GtkVersioning.h:121
> >> +void gdk_screen_get_monitor_workarea(GdkScreen *screen, int monitor, GdkRectangle *area);
> > 
> > The parameter name "screen" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> Go ahead and fix this one, I'd say.

I didn't fix it because all other prototypes in that header don't follow that rule, and I thought it was because that's C and not C++.

[Martin Robinson]

(In reply to comment #6)

> I didn't fix it because all other prototypes in that header don't follow that rule, and I thought it was because that's C and not C++.

I'm not sure what the rule is for C headers, so you might want to double-check the style rules. Might be good to switch it up, so the style bot doesn't complain. I can't recall exactly why this file is in C any longer.

[Philippe Normand]

What about the EWS failure?

[Martin Robinson]

Ah! I'd totally missed that. Looks like it's just missing some X11 library in the link line. Carlos maybe you could post another patch with a corrected makefile and then let the EWS chew on it. My r+ still holds after that.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #6)
> 
> > I didn't fix it because all other prototypes in that header don't follow that rule, and I thought it was because that's C and not C++.
> 
> I'm not sure what the rule is for C headers, so you might want to double-check the style rules. Might be good to switch it up, so the style bot doesn't complain. I can't recall exactly why this file is in C any longer.

Because it's used by wk1 unit tests, I think

[Carlos Garcia Campos]

(In reply to comment #8)
> What about the EWS failure?

I don't plan to land the patch until I figure out why it fails to build in the bot.

[Carlos Garcia Campos]

(In reply to comment #9)
> Ah! I'd totally missed that. Looks like it's just missing some X11 library in the link line. Carlos maybe you could post another patch with a corrected makefile and then let the EWS chew on it. My r+ still holds after that.

Sure!

[Carlos Garcia Campos]

Created attachment 120914 [details]
Updated patch

Updated patch adressing review comments and trying to fix the build on the bot

[Gustavo Noronha (kov)]

Comment on attachment 120914 [details]
Updated patch

Attachment 120914 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11075171

[Carlos Garcia Campos]

Created attachment 120915 [details]
Another attempt to fix the build

It seems DRT uses GtkVersioning too.

[Gustavo Noronha (kov)]

Comment on attachment 120915 [details]
Another attempt to fix the build

Attachment 120915 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11074204

[Carlos Garcia Campos]

Created attachment 120916 [details]
Another attempt

This time it was WTR

[Carlos Garcia Campos]

Committed r103929: <http://trac.webkit.org/changeset/103929>