Bug 75435

Summary: [GTK] Use gdk_screen_get_monitor_workarea() when available for screenAvailableRect()
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, gustavo.noronha, gustavo, mrobinson, pnormand, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
mrobinson: review+, gustavo.noronha: commit-queue-
Updated patch
gustavo: commit-queue-
Another attempt to fix the build
gustavo: commit-queue-
Another attempt none

Description Carlos Garcia Campos 2012-01-02 06:43:58 PST
Instead of our own implementation. It was added to GTK+ in version 3.3.6
Comment 1 Carlos Garcia Campos 2012-01-02 06:49:50 PST
Created attachment 120879 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-02 06:52:11 PST
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.
Comment 3 Collabora GTK+ EWS bot 2012-01-02 06:55:41 PST
Comment on attachment 120879 [details]
Patch

Attachment 120879 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11003018
Comment 4 WebKit Review Bot 2012-01-02 07:38:33 PST
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
Comment 5 Martin Robinson 2012-01-02 08:17:41 PST
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.
Comment 6 Carlos Garcia Campos 2012-01-02 08:20:37 PST
(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++.
Comment 7 Martin Robinson 2012-01-02 08:22:49 PST
(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.
Comment 8 Philippe Normand 2012-01-02 08:23:45 PST
What about the EWS failure?
Comment 9 Martin Robinson 2012-01-02 08:26:46 PST
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.
Comment 10 Carlos Garcia Campos 2012-01-02 08:30:00 PST
(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
Comment 11 Carlos Garcia Campos 2012-01-02 08:30:36 PST
(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.
Comment 12 Carlos Garcia Campos 2012-01-02 08:31:11 PST
(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!
Comment 13 Carlos Garcia Campos 2012-01-03 00:19:19 PST
Created attachment 120914 [details]
Updated patch

Updated patch adressing review comments and trying to fix the build on the bot
Comment 14 Gustavo Noronha (kov) 2012-01-03 00:28:35 PST
Comment on attachment 120914 [details]
Updated patch

Attachment 120914 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11075171
Comment 15 Carlos Garcia Campos 2012-01-03 00:40:30 PST
Created attachment 120915 [details]
Another attempt to fix the build

It seems DRT uses GtkVersioning too.
Comment 16 Gustavo Noronha (kov) 2012-01-03 00:43:25 PST
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
Comment 17 Carlos Garcia Campos 2012-01-03 00:54:08 PST
Created attachment 120916 [details]
Another attempt

This time it was WTR
Comment 18 Carlos Garcia Campos 2012-01-03 01:17:25 PST
Committed r103929: <http://trac.webkit.org/changeset/103929>