Bug 13890 – [gdk] Use a GtkLayout for the ScrollView implementation to get real scrollbars

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

[Holger Freyther]

Currently FrameGdk.cpp simulates scrolling by intercepting key events and manipulating the RenderLayer. To have proper ScrollBars one needs to use GtkAdjustment and probably be able to be included in a GtkScrolledWindow.

This patch removes the key event intercepting from FrameGdk, makes ScrollView operate on a GtkLayout which can be embedded into a GtkScrolledWindow.

Issues that this patch doesn't solve yet:
  - The FrameGdk WheelEvent handling needs some checking, I suspect that this is unreached code.
  - ScrollView currently doesn't implement the ScrollBar policy.
  - Resource usage must be checked (valgrind)
  - I need to make sure to not have mixed up width/height

[Holger Freyther]

Created attachment 14745 [details]
Make ScrollView a scrollable view

Without the ChangeLog entries one would see that less lines of code are needed to implement scrollbars properly. Please review, meanwhile I plan to valgrind and search for CodingStyle issues myself as well.

 WebCore/ChangeLog                           |   49 +++++++++
 WebCore/platform/ScrollView.h               |   12 +-
 WebCore/platform/gdk/FrameGdk.cpp           |   87 +---------------
 WebCore/platform/gdk/ScrollViewGdk.cpp      |  147 +++++++++++++---------------
 WebCore/platform/gdk/TemporaryLinkStubs.cpp |    8 -
 WebCore/platform/gdk/WidgetGdk.cpp          |   52 ++++++++-
 WebKitTools/ChangeLog                       |   14 ++
 WebKitTools/GdkLauncher/main.cpp            |   53 ++++++----
 8 files changed, 231 insertions(+), 191 deletions(-)

[Alp Toker]

(In reply to comment #1)
> Created an attachment (id=14745) [edit]
> Make ScrollView a scrollable view

 FloatRect ScrollView::visibleContentRect() const
 {
-    FloatRect contentRect = FloatRect(m_data->viewportArea);
-    contentRect.move(m_data->scrollOffset);
-    return contentRect;
+    return FloatRect(contentsX(), contentsY(), visibleHeight(), visibleWidth());
 }

visibleHeight() and visibleWidth() are the wrong way round. Switching them makes the patch work better.

I haven't had the opportunity to review it for correctness yet though.

[Alp Toker]

(In reply to comment #1)
> Created an attachment (id=14745) [edit]
> Make ScrollView a scrollable view

I noticed that the map area on http://maps.google.com/ does not resize vertically with this patch.

[Holger Freyther]

(In reply to comment #3)
> (In reply to comment #1)
> > Created an attachment (id=14745) [edit]
> > Make ScrollView a scrollable view
> 
> I noticed that the map area on http://maps.google.com/ does not resize
> vertically with this patch.
> 
-    gFrame->sendResizeEvent();
+    gFrame->view()->adjustViewSize();

Sending the Resize Event is still needed. This seems to fix maps.google.com here. 

[Holger Freyther]

Created attachment 14750 [details]
Make ScrollView a scrollable view

-in GdkLauncher/main.cpp call Frame::sendResizeEvent on viewport resizes.
-Fix the width/height swap spotted by Alp
-There is no need to move the contentsUpdate IntRect. It is correct.

[Maciej Stachowiak]

Will this GtkLayout thing work for subframe scrolling, and will it let the view make its own scroll view if it is in the window with other contents? I am not sure how GtkLayout works.

[Holger Freyther]

(In reply to comment #6)
> Will this GtkLayout thing work for subframe scrolling, and will it let the view
> make its own scroll view if it is in the window with other contents? I am not
> sure how GtkLayout works.
> 

Honest answer is I don't know. But it is unlikely that it will work. Currently the Gdk FrameLoaderClient is returning 0 on the request to create a new Frame.

From what I can see is that we only have one FrameView at a time? So the SubFrame's are within the FrameTree of the 'initial'/main Frame? How is the Mac port handling scrolling of SubFrames? Is it using PlatformScrollbar + RenderLayout::setScrollOffsetX,Y for that? Or do you have more than one NSView?

Looking forward to chat about this, I will try to understand how the Mac port is doing this. And I don't understand the second part of the question. Do you mean having another ScrollView inside the FrameView?

[Anders Carlsson]

Comment on attachment 14750 [details]
Make ScrollView a scrollable view

r=me

[Mark Rowe (bdash)]

Landed in r21886.