Bug 14857 – [gtk] ScrollView and WebKitGtkPage changes to make multiple frames possible

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Holger Freyther]

To implement FrameLoaderClientGdk::createFrame (#14729) we will need support for possible overlapping frames. The Windows port of WebKit has an approach to just use one native window resource for the whole FrameView. This approach was adopted to Gdk/Gtk. We will have one GdkWindow for the whole FrameView, this GdkWindow is called bin_window (like with GtkLayout) and will be owned and managaed by WebKitGtkPage (which is a GtkContainer just like GtkLayout).

This patch makes WebKitGtkPage a true GtkContainer, implementing the virtual methods/default handlers, on this way killing WebCore/platform/gdk/FrameGdk and changing the Platform*Event to take specifc GdkEvent* structs insteads of the generic GdkEvent one.

The FrameView is created on the fly in FrameLoaderClientGdk::makeDocumentView

The ScrollView and Widget methods from the Windows port where used with one exception. To implement the Gtk+ scrollable protocol the FrameView may get GtkAdjustment's set from the outside. Whenever this happens the internal PlatformScrollBar will not be used! The other change is within Widget due the usage of PlatformScrollBar. PlatformScrollBar is a GtkWidget without an associated window, it will be used by the RenderLayer and by the ScrollView. It will be positioned by the geometryChanged() method of Widget. To not make the PlatformScrollbar scroll when the view is scrolled a method was added to Widget to select between convertToContainingWindow and contentsToWindow. The other options would have been to subclass PlatformScrollbar and use that subclass in ScrollView, or to move that method/selection to PlatformScrollBar. But I think it is nice to have it in Widget.

Adjust RenderTheme to draw the Theme at the right position and add IntPoint translation to the GraphicsContext to support that as Gdk/Gtk drawing does not know about our translation (we just share the same surface).

Implement PlatformScrollbar.

Patches need to be applied in the following order:

1.) Make WebKitGtkPage a GtkContainer
2.) Implement ScrollView
3.) On-The-Fly FrameView creation
4.) Use GtkAdjustment
5.) ScrollBar changes
6.) RenderTheme

[Holger Freyther]

Created attachment 15794 [details]
Make WebKitGtkPage a GtkContainer

Make WebKitGtkPage a GtkContainer, kill FrameGdk.cpp

[Holger Freyther]

Created attachment 15795 [details]
Implement ScrollView

Implement ScrollView and Widget by following the Windows port design.

[Holger Freyther]

Created attachment 15796 [details]
On-The-Fly FrameView creation

Create the FrameView from FrameLoaderClientGdk::makeDocumentView

[Holger Freyther]

Created attachment 15797 [details]
Use GtkAdjustment

Use GtkAdjustment when they are set from the outside instead of the PlatformScrollBar

[Holger Freyther]

Created attachment 15798 [details]
ScrollBar changes

Implement PlatformScrollBar.

[Holger Freyther]

Created attachment 15799 [details]
RenderTheme

Gtk+ does not know about the translation of GraphicsContext which is coming from ScrollView::paint. As we don't scale using the painter I can simply ask cairo to give us the real x,y position and keep width and height the same.

This approach might (as documented in Widget) work for the Gtk{H,V}ScrollBar as long as the width and height are right but it was not tested.

[Adam Treat]

I don't understand the reasoning for the 'On-The-Fly FrameView creation'

Why is this done in the mac and gdk ports, but not the windows port?

[Holger Freyther]

(In reply to comment #7)
> I don't understand the reasoning for the 'On-The-Fly FrameView creation'
> 
> Why is this done in the mac and gdk ports, but not the windows port?

This is interesting specially if you look at David's comment in #13913.

[Alp Toker]

Some of these patches introduce slightly strange scrolling behaviour (the content is slow at keeping up with the position of the scrollbar), and possibly an impact on performance.

[Holger Freyther]

(In reply to comment #9)
> Some of these patches introduce slightly strange scrolling behaviour (the
> content is slow at keeping up with the position of the scrollbar), and possibly
> an impact on performance.

Sure. This is the nature of the "Implement ScrollView" change. In contrast to GtkLayout we can not create this big GdkWindow anymore and can't use gdk_window_move which was so good for scrolling. As described when originally posting this patch there are hooks to ChromeClient to update the backing store and to do the scrolling. It is matter of using gdk_window_scroll_by or similiar method and reducing the exposing region to accelerate scrolling again but these are not yet implemented. For now to always keep up with scrolling you could force gdk to deliver the expose events (gdk_window_process_updates), probably in ScrollView::updateScrollbars or in the methods calling it (ScrollViewPrivate::valueChanged, ScrollViewPrivate::adjustmentChanged).

[Adam Roben (:aroben)]

Comment on attachment 15796 [details]
On-The-Fly FrameView creation

I'm not sure why this patch is needed. The previous implementation looked pretty much identical to the Windows implementation in WebKit/win/WebFrame.cpp.

[Holger Freyther]

(In reply to comment #11)
> (From update of attachment 15796 [details] [edit])
> I'm not sure why this patch is needed. The previous implementation looked
> pretty much identical to the Windows implementation in WebKit/win/WebFrame.cpp.
> 

From bug number #13913 and darin's (IIRC) comments on IRC are FrameView's not meant to be recycled. I found out about this wondering why after visitng w3c.org TR's and leaving torwards another site ScrollView::setStaticBackground(true) was called even if the site has no static background. So either the patch from #13913 should be finished (by taking mitz comments into acount), or I need to recreate a Frame everytime I surf to another patch (we might do that when implementing the b/f cache), or I miss something else.

At least this patch follows the mac port in regard to creating the FrameView, so some enligthenment would be appreciated.

[Adam Roben (:aroben)]

Comment on attachment 15794 [details]
Make WebKitGtkPage a GtkContainer

This patch looks like it should be N separate checkins:

1. FrameGdk move and changes
2. Changes to the various *Event classes
3. WebKitGtkPage changes

It would have been nice if this patch could have been broken up into those pieces before putting it up for review. It would have made it much easier to review.

     switch (event->type) {
-        case GDK_MOTION_NOTIFY:
-            m_eventType = MouseEventMoved;
-            m_button = NoButton;
+    case GDK_BUTTON_PRESS:
+    case GDK_2BUTTON_PRESS:
+    case GDK_3BUTTON_PRESS:
+    case GDK_BUTTON_RELEASE:

   Please keep the cases indented from the switch.

+    switch (motion->type) {
+    case GDK_MOTION_NOTIFY:

   Ditto here.

+    g_object_ref(event->expose.window);

   Do you need to release this ref?

+    gdk_event_free (event);

   Please remove the space before the (.

+ * TODO: We should follow Qt and move that to webkitgtksettings.

   Typo: that -> this

+GObject* webkit_gtk_frame_new(WebKitGtkPage* web_page)

Since we want to move towards the WebKit coding style, the parameter name should be webPage (and other variables in this method should be named similarly).

+#include "PlatformMouseEvent.h"
+#include "PlatformKeyboardEvent.h"
+#include "PlatformWheelEvent.h"
+#include "GraphicsContext.h"
+#include "EventHandler.h"
+#include "HitTestRequest.h"
+#include "HitTestResult.h"

   Please keep #includes ordered alphabetically.

+    frame->view()->wheelEvent(wheelEvent);

I believe this should go through EventHandler::handleWheelEvent.

+    HitTestRequest hitTestRequest(true, true);
+    HitTestResult hitTestResult(wheelEvent.pos());
+    frame->renderer()->layer()->hitTest(hitTestRequest, hitTestResult);
+    Node* node = hitTestResult.innerNode();
+    if (!node)
+        return FALSE;
+
+    return TRUE;

I'm not sure why this code is needed.

+    frame->forceLayout();
+    frame->view()->adjustViewSize();

   These two lines aren't needed on Windows. Why are they needed here?

+ * it is a GtkContainer to implement ScrollView. And we need to make it similiar
+ * to GtkLayout to have sane painting.

   It would be good to say in what way this code makes WebKitGtkPage similar to GtkLayout, and what "sane" means.

+            gtk_widget_map((*current));

   Looks like you've got some extra parentheses here.

+    if (private_data->children.contains(widget)) {

   You should just return early if this condition is false.

+    G_OBJECT_CLASS (webkit_gtk_page_parent_class)->finalize (object);

   You've got extra spaces before your parentheses.

+    WebKitGtkPagePrivate *private_data = WEBKIT_GTK_PAGE_GET_PRIVATE(WEBKIT_GTK_PAGE(page));

   Please use camel case.

+    /*
+     * TODO: move that over to use webkitgtksettings 
+     */

   This should be a FIXME.

   This looks fine overall, though again I'm no Gtk expert.

[Holger Freyther]

Comment on attachment 15799 [details]
RenderTheme

Applied on the 8th of August 2007.

[Holger Freyther]

Comment on attachment 15798 [details]
ScrollBar changes

Besides the removal of gtk_object_sink this patch has been applied.

[Mark Rowe (bdash)]

Comment on attachment 15794 [details]
Make WebKitGtkPage a GtkContainer

Clearing review flag to get this out of the commit queue.

[Alp Toker]

This was fixed some time ago, right?

[Holger Freyther]

"On-The-Fly FrameView" still waits for Adam and David Hyatt to test/agree. See comment #8 of this bug report.

[Jan Alonzo]

Hi Holger, I think you've already fixes this a week ago iirc right? For reference: http://trac.webkit.org/projects/webkit/changeset/31877.

Ok to close?

[Gustavo Noronha (kov)]

Well, I am closing it now. If needed please reopen.