Bug 14729 – [gtk] Implement FrameLoaderClientGdk::createFrame

• Component: New Bugs
• Status: RESOLVED FIXED

[Holger Freyther]

Due to CSS Frame's can overlap. This means one would have to make sure to deliver expose events in the same z-order as the one of the Render Tree and to get alpha between overlapping frames right.

The planned solution to this issue is the following:
   -Make Frame's not use any Gdk/Gtk resources
   -Create the FrameView on demand from within the FrameLoaderClientGdk
   -Make ScrollView not use any Gdk/Gtk resources and follow the Windows and Qt design which involves fixing PlatformScrollbar and properly implementing the HitEvent in a way robust across different styles/themes. The biggest issue with FrameView's will be handling GtkAdjustment to the outside and drawing scrollbars. E.g. the WebKitGtkPage will get gtk_widget_set_scroll_adjustments called and these need to be forwarded to the FrameView (everytime the FrameView gets created).
    -Make WebKitGtkPage something like a GtkDrawable and have a GdkWindow of the size of the viewport. All ScrollView's need to track there current scroll offset and relative position in their parent. ScrollView::update and the other invalidate methods need to invalidate the right rect of the outer GtkDrawable of the WebKitGtkPage.
     -WebKitGtkPage should behave like a GtkLayout and be embedable in a GtkScrolledWindow
     -ScrollView::addWidget should continue to work to allow embedding plugins at one point in time.
     -Big question of addWidget for ScrollView's is who is going to create the scrollbars? Who is going to forceLayout when we show/hide scrollbars?

[Holger Freyther]

Add the Gtk keyword, even to the curl bugs.

[Holger Freyther]

Created attachment 15731 [details]
Implement ScrollView similiar to win/qt

Mostly follow the Windows design (which works great):
The most controversal change is with Widget::setScrollViewControl. PlatformScrollbar uses a GtkWidget. In order to draw it correctly we need to have a proper GtkAllocation set on the widget. We set one when PlatformScrollbar::setRect gets called.

Now PlatformScrollbar is used by RenderFrame and ScrollView. In the case of ScrollView scrolling should not change the position of the ScrollBar. In Win/Qt this is achieved by translating the context before painting. As the PlatformScrollbar in Gtk+ is a GtkWidget I would like to avoid changing the GtkAllocation on every paint and treat it more like WebCore/plugins/win and handle it like/similiar geometryChanged to position it.
This leads to the controversial change in Widget to select which coordinate translation to use. Other solutions considered where to subclass PlatformScrollbar for ScrollView to change the placing behaviour.

This is just a current diff of the work in progress implementation and comments would be appreciated.

[Adam Roben (:aroben)]

CCing Hyatt so he can look at the PlatformScrollBar changes

[Holger Freyther]

Created attachment 15748 [details]
Implement ScrollView similiar to win/qt

It has the same controversal change as the previous attachment. This patch changes the following things:

  -It should apply against ToT
  -It handles getting embedded into a GtkScrolledWindow nicely by using GtkAdjustment

Known issues:
  -On destructing it seems I get ref counting of the PlatformScrollbar wrong.
  -We might create and destroy PlatformScrollbar too often (same as on Windows and not introduced by this patch)

[Holger Freyther]

Created attachment 15754 [details]
Implement ScrollView similiar to win/qt

Compared to the previous patch:

-Make it compile on case-sensitive filesystems (PlatformScrollbar.h vs. PlatformScrollBar.h)
-Add (again) refcounting to PlatformScrollbar and ScrollView
-Implement forall and unrealize in WebKitGtkPage to make issues on exit go away.
-Fixup some comments.

The biggest part of the patch is copy and paste from the windows port. The controversal change is in Widget to use one of the two possible IntRect conversion methods. I think this change is fine, other approaches would be to move this code to PlatformScrollbar or to subclass PlatformScrollbar and use that subclss in ScrollView.

I think this is commit-worthy.

[Holger Freyther]

Created attachment 15755 [details]
Implement ScrollView similiar to win/qt

Compared to the previous patch:

-Make it compile on case-sensitive filesystems (PlatformScrollbar.h vs. PlatformScrollBar.h)
-Add (again) refcounting to PlatformScrollbar and ScrollView
-Implement forall and unrealize in WebKitGtkPage to make issues on exit go away.
-Fixup some comments.

The biggest part of the patch is copy and paste from the windows port. The controversal change is in Widget to use one of the two possible IntRect conversion methods. I think this change is fine, other approaches would be to move this code to PlatformScrollbar or to subclass PlatformScrollbar and use that subclss in ScrollView.

I think this is commit-worthy.

[Holger Freyther]

Created attachment 15756 [details]
Implement ScrollView similiar to win/qt

Compared to the previous patch:

-Make it compile on case-sensitive filesystems (PlatformScrollbar.h vs. PlatformScrollBar.h)
-Add (again) refcounting to PlatformScrollbar and ScrollView
-Implement forall and unrealize in WebKitGtkPage to make issues on exit go away.
-Fixup some comments.

The biggest part of the patch is copy and paste from the windows port. The controversal change is in Widget to use one of the two possible IntRect conversion methods. I think this change is fine, other approaches would be to move this code to PlatformScrollbar or to subclass PlatformScrollbar and use that subclss in ScrollView.

I think this is commit-worthy.

[Holger Freyther]

Comment on attachment 15754 [details]
Implement ScrollView similiar to win/qt

Duplicate attachment.

[Holger Freyther]

Comment on attachment 15755 [details]
Implement ScrollView similiar to win/qt

Duplicate attachment

[Holger Freyther]

I have one report that the Xserver hogs memory when scrolling. But I have no information beyond this.

[Holger Freyther]

Created attachment 15784 [details]
Implement ScrollView similiar to win/qt to support createFrame

Changes due the Xserver leakage report:
The z-order painting patch introduced thig bug. The gdk_window_end_painting call was removed. As Xan pointed out in another bug report this window is double buffered any so we can remove the begin_paint call as well.

Interdiff:
 +    gdk_region_get_clipbox(event->region, &clip);
-+    gdk_window_begin_paint_region(event->window, event->region);

[Christian Dywan]

I tested the patch 15755 which caused the X server to eat memory when scrolling.

I can also confirm that patch 15784 is fine now.

[Adam Roben (:aroben)]

Comment on attachment 15784 [details]
Implement ScrollView similiar to win/qt to support createFrame

Clearing review flag as the patches in bug 14857 should be reviewed instead.

[Holger Freyther]

Thanks to the reviewing effort of Niko and especially Adam this method and everything needed to implement it is in ToT. There is one known issue/race with GtkLayout and the positioning of ScrollViewScrollbar which will be addressed in another patch.