Bug 14857

Created attachment 15794 [details] Make WebKitGtkPage a GtkContainer Make WebKitGtkPage a GtkContainer, kill FrameGdk.cpp

Created attachment 15795 [details] Implement ScrollView Implement ScrollView and Widget by following the Windows port design.

Created attachment 15796 [details] On-The-Fly FrameView creation Create the FrameView from FrameLoaderClientGdk::makeDocumentView

Created attachment 15797 [details] Use GtkAdjustment Use GtkAdjustment when they are set from the outside instead of the PlatformScrollBar

Created attachment 15798 [details] ScrollBar changes Implement PlatformScrollBar.

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.

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?

(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.

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.

(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).

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.

(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.

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.

Comment on attachment 15799 [details] RenderTheme Applied on the 8th of August 2007.

Comment on attachment 15798 [details] ScrollBar changes Besides the removal of gtk_object_sink this patch has been applied.

Comment on attachment 15794 [details] Make WebKitGtkPage a GtkContainer Clearing review flag to get this out of the commit queue.

This was fixed some time ago, right?

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

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?

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