Bug 74595 - [GTK] Add WebKitWindowProperties to WebKit2 GTK+ API
Summary: [GTK] Add WebKitWindowProperties to WebKit2 GTK+ API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2011-12-15 02:03 PST by Carlos Garcia Campos
Modified: 2011-12-15 08:11 PST (History)
3 users (show)

See Also:


Attachments
Patch (57.23 KB, patch)
2011-12-15 02:13 PST, Carlos Garcia Campos
gustavo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-12-15 02:03:20 PST
Now that we have initial UI client implementation that adds create/ready-to-show/close signals, we need an object to represent the WindowFeatures that should be applied in WebKitWebView::ready-to-show. I've used properties instead of features, because I find features a bit confusing, but I'm not opposed to use Features if you guys think it's a better name or for consistency with wk1 API.
Comment 1 Carlos Garcia Campos 2011-12-15 02:13:40 PST
Created attachment 119400 [details]
Patch
Comment 2 WebKit Review Bot 2011-12-15 02:15:58 PST
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 Gustavo Noronha (kov) 2011-12-15 06:06:41 PST
Comment on attachment 119400 [details]
Patch

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

I like the rename to WindowProperties, it certainly caused some misunderstanding before, being called features. I love how good our API test coverage is becoming, WebKit2GTK+ will shine, you're doing a great job Carlos! =)

I was thinking about getWindowFrame. I remember at some point considering, in webkit1, whether we should report the actual window size or the knowledge WebWindowFeatures had of it, for some sites had terrible checks like "while (size != desired_size) request_size_change(desired_size)", or something like that, but I don't think we did anything regarding that, and things seem to be working fine, so this one also looks fine to me.

> Source/WebKit2/UIProcess/API/gtk/WebKitWindowProperties.cpp:452
> + * Get the geometry that the window should have on the screen when shown.

Nit: should be Gets, since we're talking about the function. Also, s/that//, I think.

> Source/WebKit2/UIProcess/API/gtk/WebKitWindowProperties.h:61
> +webkit_window_properties_get_geometry            (WebKitWindowProperties *window_properties,
> +                                                  GdkRectangle           *geometry);

Should we use cairo_rectangle_int_t here, rather than GdkRectangle? GdkRectangle is currently a typedef to the cairo type, and we seem to be gradually moving towards using the cairo types directly. I'll ask the GTK+ people and report back here.
Comment 4 Gustavo Noronha (kov) 2011-12-15 06:19:18 PST
<ebassi> kov: I strongly recommend using cairo types

So I say we should =)
Comment 5 Carlos Garcia Campos 2011-12-15 06:24:16 PST
(In reply to comment #3)
> (From update of attachment 119400 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119400&action=review
> 
> I like the rename to WindowProperties, it certainly caused some misunderstanding before, being called features. I love how good our API test coverage is becoming, WebKit2GTK+ will shine, you're doing a great job Carlos! =)

Thanks! Unit tests are indeed quite useful to find bugs early.

> I was thinking about getWindowFrame. I remember at some point considering, in webkit1, whether we should report the actual window size or the knowledge WebWindowFeatures had of it, for some sites had terrible checks like "while (size != desired_size) request_size_change(desired_size)", or something like that, but I don't think we did anything regarding that, and things seem to be working fine, so this one also looks fine to me.

Well, the window features size is what you should use for the window, while getWindowFrame is the actual size of the window, it might be different. 

> > Source/WebKit2/UIProcess/API/gtk/WebKitWindowProperties.cpp:452
> > + * Get the geometry that the window should have on the screen when shown.
> 
> Nit: should be Gets, since we're talking about the function. Also, s/that//, I think.

I used Get for consistency with WebKitSettings API docs that uses Get/Set everywhere, but I'm not english expert.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWindowProperties.h:61
> > +webkit_window_properties_get_geometry            (WebKitWindowProperties *window_properties,
> > +                                                  GdkRectangle           *geometry);
> 
> Should we use cairo_rectangle_int_t here, rather than GdkRectangle? GdkRectangle is currently a typedef to the cairo type, and we seem to be gradually moving towards using the cairo types directly. I'll ask the GTK+ people and report back here.

I used GdkRecangle here because it's a boxed property and I need to use GDK_TYPE_RECTANGLE, I could use cairo_rectangle in public API but it might be confusing because API docs will say the property is a GdkRectangle
Comment 6 Carlos Garcia Campos 2011-12-15 07:12:18 PST
Committed r102935: <http://trac.webkit.org/changeset/102935>
Comment 7 Martin Robinson 2011-12-15 08:11:12 PST
Comment on attachment 119400 [details]
Patch

Nice patch. Little nit: toolbar, menubar, locationbar and statusbar should probably be tool-bar, menu-bar location-bar and status-bar.