Bug 25889 - [GTK] scrollbar policy for main frame is not implementable
: [GTK] scrollbar policy for main frame is not implementable
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
: Gtk
: 25646
:
  Show dependency treegraph
 
Reported: 2009-05-20 07:11 PST by
Modified: 2009-08-28 08:35 PST (History)


Attachments
report scrollbar policy (12.38 KB, patch)
2009-05-20 07:31 PST, Gustavo Noronha (kov)
no flags Review Patch | Details | Formatted Diff | Diff
correctly report visible contents rect (2.50 KB, patch)
2009-05-20 07:31 PST, Gustavo Noronha (kov)
no flags Review Patch | Details | Formatted Diff | Diff
report scrollbar policy (12.87 KB, patch)
2009-05-26 12:45 PST, Gustavo Noronha (kov)
no flags Review Patch | Details | Formatted Diff | Diff
report correct visible size (2.90 KB, patch)
2009-05-26 12:57 PST, Gustavo Noronha (kov)
no flags Review Patch | Details | Formatted Diff | Diff
report scrollbar policy (13.90 KB, patch)
2009-06-08 18:23 PST, Gustavo Noronha (kov)
no flags Review Patch | Details | Formatted Diff | Diff
report scrollbar policy (16.45 KB, patch)
2009-06-09 16:15 PST, Gustavo Noronha (kov)
jmalonzo: review-
Review Patch | Details | Formatted Diff | Diff
report scrollbar policy (18.67 KB, patch)
2009-06-27 15:07 PST, Gustavo Noronha (kov)
no flags Review Patch | Details | Formatted Diff | Diff
report scrollbar policy (26.79 KB, patch)
2009-06-29 16:54 PST, Gustavo Noronha (kov)
jmalonzo: review-
Review Patch | Details | Formatted Diff | Diff
report scrollbar policy (28.68 KB, patch)
2009-08-27 14:55 PST, Gustavo Noronha (kov)
zecke: review+
gns: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-05-20 07:11:07 PST
Scrollbar policy for the main frame is currently not implementable, since Scrollview::setScrollbarMode is not forwarded to our view code, nor to the application so that it is able to implement the policy in whatever widget it is using to provide scrolling.

This breaks sites such as orkut.com, which disable the main frame's scrollbars, and implement their own - we get two scrollbars in our current implementation. It also breaks expectations such as css overflow handling forcing the scrollbars to appear/disappear, making some layout tests not work for us.
------- Comment #1 From 2009-05-20 07:31:35 PST -------
Created an attachment (id=30507) [details]
report scrollbar policy

 WebCore/ChangeLog                      |   12 +++
 WebCore/platform/ScrollView.cpp        |    2 +-
 WebCore/platform/gtk/ScrollViewGtk.cpp |   24 ++++++
 WebKit/gtk/ChangeLog                   |   16 ++++
 WebKit/gtk/webkit/webkitwebview.cpp    |  129 +++++++++++++++++++++++++++++++-
 WebKit/gtk/webkit/webkitwebview.h      |   11 +++-
 6 files changed, 191 insertions(+), 3 deletions(-)
------- Comment #2 From 2009-05-20 07:31:41 PST -------
Created an attachment (id=30508) [details]
correctly report visible contents rect

 WebCore/ChangeLog                      |   12 ++++++++++++
 WebCore/platform/ScrollView.cpp        |    2 ++
 WebCore/platform/gtk/ScrollViewGtk.cpp |   15 +++++++++++++++
 3 files changed, 29 insertions(+), 0 deletions(-)
------- Comment #3 From 2009-05-20 07:33:10 PST -------
(From update of attachment 30507 [details])
This patch is WIP, and built on top of a patch by zecke which is still to be reviewed. What is missing from this patch is basically making the properties read/write, but I would like comments.
------- Comment #4 From 2009-05-20 07:33:46 PST -------
(From update of attachment 30508 [details])
This is not strictly needed to fix this specific problem, but size reporting is off without it.
------- Comment #5 From 2009-05-20 16:25:56 PST -------
(From update of attachment 30508 [details])
Actually nevermind looking at this patch. It's broken. I'll investigate a bit more.
------- Comment #6 From 2009-05-22 21:18:57 PST -------
(In reply to comment #3)
> (From update of attachment 30507 [details] [review])
> This patch is WIP, and built on top of a patch by zecke which is still to be
> reviewed. What is missing from this patch is basically making the properties
> read/write, but I would like comments.
> 

Thanks for the patch. It works fine. There is additional problem probably related to other bug. Adobe flash plugin overlays WebView scrollbars (horizontal and vertical) and sometimes shifts a content of the view with horizontal scrollbar.

How to repeat:
- disable ads blocking if you have
- enable flash
- go to http://www.ynet.co.il
- shrink the window the horizontal scrollbar appears
- watch the rss ticker and ads (flash) that overlays scrollbars 
- horizontal scrollbar starts after flash content on the left side of the page
------- Comment #7 From 2009-05-23 05:44:24 PST -------
(In reply to comment #6)
> (In reply to comment #3)
> > (From update of attachment 30507 [details] [review] [review])
> > This patch is WIP, and built on top of a patch by zecke which is still to be
> > reviewed. What is missing from this patch is basically making the properties
> > read/write, but I would like comments.
> > 
> 
> Thanks for the patch. It works fine. There is additional problem probably
> related to other bug. Adobe flash plugin overlays WebView scrollbars
> (horizontal and vertical) and sometimes shifts a content of the view with
> horizontal scrollbar.
> 
> How to repeat:
> - disable ads blocking if you have
> - enable flash
> - go to http://www.ynet.co.il
> - shrink the window the horizontal scrollbar appears
> - watch the rss ticker and ads (flash) that overlays scrollbars 
> - horizontal scrollbar starts after flash content on the left side of the page

please file a separate bug for flash bugs. Yes, the flash window will overlap with various things. This includes popups (see youtube search) and as you mention scrollbars...
------- Comment #8 From 2009-05-25 19:07:29 PST -------
Found a bit of time to look into. I think you will want to move this to WebKitWebFrame. With sites like images.google.com (actually when clicking on a search result). Otherwise you will get the signal (assuming hostWindow() will be != 0) for every frame of the frameset?
------- Comment #9 From 2009-05-25 20:07:04 PST -------
(In reply to comment #8)
> Found a bit of time to look into. I think you will want to move this to
> WebKitWebFrame. With sites like images.google.com (actually when clicking on a
> search result). Otherwise you will get the signal (assuming hostWindow() will
> be != 0) for every frame of the frameset?
> 

The scrollbars that show up in the frames in images.google.com results are drawn by webkit, and the policy changes are already respected correctly. We may want to notify the frame about such changes for informational, but it doesn't really matter that much to the application or to the scrolling widget that is WebView's parent, I think. So what we want is to only emit that signal on the WebView for the main frame. Does that make sense to you?
------- Comment #10 From 2009-05-26 12:45:39 PST -------
Created an attachment (id=30675) [details]
report scrollbar policy

 WebCore/ChangeLog                      |   12 +++
 WebCore/platform/ScrollView.cpp        |    2 +-
 WebCore/platform/gtk/ScrollViewGtk.cpp |   36 +++++++++
 WebKit/gtk/ChangeLog                   |   16 ++++
 WebKit/gtk/webkit/webkitwebview.cpp    |  129 +++++++++++++++++++++++++++++++-
 WebKit/gtk/webkit/webkitwebview.h      |   11 +++-
 6 files changed, 203 insertions(+), 3 deletions(-)
------- Comment #11 From 2009-05-26 12:46:50 PST -------
(From update of attachment 30675 [details])
This time reporting policy only for the main frame, since it is the only that matters from a WebView point of view.
------- Comment #12 From 2009-05-26 12:57:01 PST -------
Created an attachment (id=30677) [details]
report correct visible size

 WebCore/ChangeLog                      |   12 ++++++++++++
 WebCore/platform/ScrollView.cpp        |    2 ++
 WebCore/platform/gtk/ScrollViewGtk.cpp |   21 +++++++++++++++++++++
 3 files changed, 35 insertions(+), 0 deletions(-)
------- Comment #13 From 2009-05-26 13:04:48 PST -------
(From update of attachment 30677 [details])
Ok, after investigating the other porblem, I also reworked this one to only apply the special-cased code to the main frame, as it should.
------- Comment #14 From 2009-06-08 18:23:00 PST -------
Created an attachment (id=31076) [details]
report scrollbar policy

 WebCore/ChangeLog                             |   16 ++++
 WebCore/page/ChromeClient.h                   |    4 +
 WebCore/platform/ScrollView.cpp               |    2 +-
 WebCore/platform/ScrollView.h                 |    4 +
 WebCore/platform/gtk/ScrollViewGtk.cpp        |   26 ++++++
 WebKit/gtk/ChangeLog                          |   18 ++++
 WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp |   26 ++++++
 WebKit/gtk/WebCoreSupport/ChromeClientGtk.h   |    1 +
 WebKit/gtk/webkit/webkitwebframe.cpp          |  112 ++++++++++++++++++++++++-
 WebKit/gtk/webkit/webkitwebframe.h            |    7 ++
 10 files changed, 214 insertions(+), 2 deletions(-)
------- Comment #15 From 2009-06-08 18:24:39 PST -------
(From update of attachment 31076 [details])
More changes due to talking to zecke on IRC. We now report policy changes to any frame that has an adjustment.
------- Comment #16 From 2009-06-09 07:53:41 PST -------
(From update of attachment 30677 [details])
Okay, it is a bit sad to have more platform specific code, but such is life... after all the troubles I still think the GtkAdjustment thing is the right way to do it... :)
------- Comment #17 From 2009-06-09 08:31:54 PST -------
(From update of attachment 31076 [details])

>  
> +#if PLATFORM(GTK)
> +        virtual void scrollbarsPolicyDidChange(Frame*) const { };
> +#endif

You will have to bite the bullet and add it to EmptyClients, Qt, Win, Mac, Wx... all of *Client.h should be pure virtual to get the analogy of an interface. I was recently reinforcing this.



>  void ScrollView::scrollbarModes(ScrollbarMode& horizontalMode, ScrollbarMode& verticalMode) const
>  {
> diff --git a/WebCore/platform/ScrollView.h b/WebCore/platform/ScrollView.h
> index 4d5bed1..179b916 100644
> --- a/WebCore/platform/ScrollView.h
> +++ b/WebCore/platform/ScrollView.h
> @@ -40,6 +40,9 @@
>  #endif
>  
>  #if PLATFORM(GTK)
> +namespace WebCore {
> +class Frame;
> +}

>      GtkAdjustment* m_verticalAdjustment;
>      void setScrollOffset(const IntSize& offset) { m_scrollOffset = offset; }
> +    virtual Frame* frame() const = 0;
>  #endif

we can't do this, it is a layering violation and hyatt will deep fry us. more on this later.




> +
> +    // For frames that do have adjustments attached, we want to report
> +    // policy changes, so that they may be applied to the widget to
> +    // which the WebView has been added, for instance.
> +    Page* page = frame() ? frame()->page() : 0;
> +    if (page)
> +        page->chrome()->client()->scrollbarsPolicyDidChange(frame());
> +}
> +

You can look at the "scrollRectIntoView". It is used by the mac to send information into the ChromeClient. I think we can adopt a strategy like that. What you can do either in the method above, or preferable in the ChromeClient implementation itself, is ask the ScrollView if it is a FrameView (isFrameView) and then cast it to a FrameView.


> +    g_object_class_install_property(objectClass, PROP_HORIZONTAL_SCROLLBAR_POLICY,
> +                                    g_param_spec_enum("horizontal-scrollbar-policy",
> +                                                      "Horizontal Scrollbar Policy",
> +                                                      "Determines the current policy for the horizontal scrollbar of the frame.",
> +                                                      GTK_TYPE_POLICY_TYPE,
> +                                                      GTK_POLICY_AUTOMATIC,
> +                                                      WEBKIT_PARAM_READABLE));
> +
> +    /**
> +     * WebKitWebFrame:vertical-scrollbar-policy:
> +     *
> +     * Determines the current policy for the vertical scrollbar of
> +     * the frame. For the main frame, you should follow this policy on
> +     * the widget that surrounds the #WebKitWebView to make some
> +     * content behave as expected.
> +     *
> +     * Since: 1.1.9
> +     */
> +    g_object_class_install_property(objectClass, PROP_VERTICAL_SCROLLBAR_POLICY,
> +                                    g_param_spec_enum("vertical-scrollbar-policy",
> +                                                      "Vertical Scrollbar Policy",
> +                                                      "Determines the current policy for the vertical scrollbar of the frame.",
> +                                                      GTK_TYPE_POLICY_TYPE,
> +                                                      GTK_POLICY_AUTOMATIC,
> +                                                      WEBKIT_PARAM_READABLE));
>

i18n in the attributes? :)


looks fine otherwise, sorry that we need another iteration.
------- Comment #18 From 2009-06-09 10:41:51 PST -------
+ For the main frame, you should follow this policy on
+     * the widget that surrounds the #WebKitWebView to make some
+     * content behave as expected.

Just a small point: the wording is a little unclear. Does this mean that one should set the same policy on the scrolled window containing the web view?
------- Comment #19 From 2009-06-09 10:45:10 PST -------
(In reply to comment #18)
> + For the main frame, you should follow this policy on
> +     * the widget that surrounds the #WebKitWebView to make some
> +     * content behave as expected.
> 
> Just a small point: the wording is a little unclear. Does this mean that one
> should set the same policy on the scrolled window containing the web view?

Yes =). Do you have a suggestion on how to make it clearer?
------- Comment #20 From 2009-06-09 16:15:08 PST -------
(In reply to comment #17)
> You will have to bite the bullet and add it to EmptyClients, Qt, Win, Mac,
> Wx... all of *Client.h should be pure virtual to get the analogy of an
> interface. I was recently reinforcing this.

No problem (although PLATFORM(MAC) has some methods that go against this, are those just historical artifacts?).

> >      GtkAdjustment* m_verticalAdjustment;
> >      void setScrollOffset(const IntSize& offset) { m_scrollOffset = offset; }
> > +    virtual Frame* frame() const = 0;
> >  #endif
> 
> we can't do this, it is a layering violation and hyatt will deep fry us. more
> on this later.
[...]
> > +    Page* page = frame() ? frame()->page() : 0;
> > +    if (page)
> > +        page->chrome()->client()->scrollbarsPolicyDidChange(frame());
> > +}
> > +
> 
> You can look at the "scrollRectIntoView". It is used by the mac to send
> information into the ChromeClient. I think we can adopt a strategy like that.
> What you can do either in the method above, or preferable in the ChromeClient
> implementation itself, is ask the ScrollView if it is a FrameView (isFrameView)
> and then cast it to a FrameView.

I had to make that cast in the ScrollView code, because I needed a way to get to the chrome client, anyway. Let's see if it looks good.
------- Comment #21 From 2009-06-09 16:15:39 PST -------
Created an attachment (id=31111) [details]
report scrollbar policy

 WebCore/ChangeLog                             |   16 ++++
 WebCore/loader/EmptyClients.h                 |    1 +
 WebCore/page/ChromeClient.h                   |    1 +
 WebCore/platform/ScrollView.cpp               |    2 +-
 WebCore/platform/gtk/ScrollViewGtk.cpp        |   31 +++++++
 WebKit/gtk/ChangeLog                          |   18 ++++
 WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp |   26 ++++++
 WebKit/gtk/WebCoreSupport/ChromeClientGtk.h   |    1 +
 WebKit/gtk/webkit/webkitwebframe.cpp          |  112 ++++++++++++++++++++++++-
 WebKit/gtk/webkit/webkitwebframe.h            |    7 ++
 WebKit/mac/WebCoreSupport/WebChromeClient.h   |    1 +
 WebKit/qt/WebCoreSupport/ChromeClientQt.h     |    1 +
 WebKit/win/WebCoreSupport/WebChromeClient.h   |    1 +
 WebKit/wx/WebKitSupport/ChromeClientWx.h      |    1 +
 14 files changed, 217 insertions(+), 2 deletions(-)
------- Comment #22 From 2009-06-09 16:17:31 PST -------
btw, I'm not committing the r+'ed one because I'm lazy, and want to get them in in the same order as they are in my branch, but if anyone gets in a hurry let me know =D
------- Comment #23 From 2009-06-09 16:44:51 PST -------
(In reply to comment #21)
> Created an attachment (id=31111) [details] [review]
> report scrollbar policy

Btw, I forgot about the i18n markers again, but I will add them when landing =/.
------- Comment #24 From 2009-06-10 12:48:49 PST -------
(In reply to comment #19)
> (In reply to comment #18)
> > + For the main frame, you should follow this policy on
> > +     * the widget that surrounds the #WebKitWebView to make some
> > +     * content behave as expected.
> > 
> > Just a small point: the wording is a little unclear. Does this mean that one
> > should set the same policy on the scrolled window containing the web view?
> 
> Yes =). Do you have a suggestion on how to make it clearer?

I would say "make sure to set the same policy on the scrollable widget containing the web view", to avoid the ambiguity with "following".
------- Comment #25 From 2009-06-12 10:29:45 PST -------
(In reply to comment #24)
> > Yes =). Do you have a suggestion on how to make it clearer?
> 
> I would say "make sure to set the same policy on the scrollable widget
> containing the web view", to avoid the ambiguity with "following".

That's perfect, I'll make this change for landing, thanks! 
------- Comment #26 From 2009-06-12 11:28:20 PST -------
(In reply to comment #25)
> (In reply to comment #24)
> > > Yes =). Do you have a suggestion on how to make it clearer?
> > 
> > I would say "make sure to set the same policy on the scrollable widget
> > containing the web view", to avoid the ambiguity with "following".
> 
> That's perfect, I'll make this change for landing, thanks! 
> 

Actually, there's a test that fails with my code. Since it is not failing with windows, it may be because of that check. Changing the code to this makes the failing test pass, and the one I enable with the patch, as well:

+    if (gLayoutTestController->dumpStatusCallbacks()) {
+        if (message && strcmp(message, ""))
+            printf("UI DELEGATE STATUS CALLBACK: setStatusText:%s\n", message);
+    }

r=you on going like that?
------- Comment #27 From 2009-06-15 14:05:54 PST -------
(In reply to comment #26)
> (In reply to comment #25)
> Actually, there's a test that fails with my code. Since it is not failing with
> windows, it may be because of that check. Changing the code to this makes the
> failing test pass, and the one I enable with the patch, as well:
> 
> +    if (gLayoutTestController->dumpStatusCallbacks()) {
> +        if (message && strcmp(message, ""))
> +            printf("UI DELEGATE STATUS CALLBACK: setStatusText:%s\n",
> message);
> +    }

Nevermind this comment, it was meant for a different bug report!
------- Comment #28 From 2009-06-22 00:03:36 PST -------
I think you want to add _() to the properties names?
------- Comment #29 From 2009-06-22 08:46:52 PST -------
(In reply to comment #28)
> I think you want to add _() to the properties names?
> 

Yeah, I was planning to do that when committing, as well as the doc change suggested by Christian in #24. I just wanted to avoid uploading a new patch. You want me to do it?
------- Comment #30 From 2009-06-25 04:35:01 PST -------
(From update of attachment 31111 [details])
(In reply to comment #29)
> (In reply to comment #28)
> > I think you want to add _() to the properties names?
> > 
> 
> Yeah, I was planning to do that when committing, as well as the doc change
> suggested by Christian in #24. I just wanted to avoid uploading a new patch.
> You want me to do it?

Please update the patch with regards to Christian's and Holger's comments, + update "Since".

Also, can we add a test for this (if we don't have one for it yet - layout, unit tests)?

I'll r- this for now based on the these.
------- Comment #31 From 2009-06-26 01:41:03 PST -------
This has been sitting in the commit queue for a month. :(  Kov?  What's your plan here...
------- Comment #32 From 2009-06-27 14:42:09 PST -------
(From update of attachment 30677 [details])
I got this one committed as r45313, now.
------- Comment #33 From 2009-06-27 15:07:16 PST -------
Created an attachment (id=31980) [details]
report scrollbar policy

 WebCore/ChangeLog                             |   19 ++++
 WebCore/loader/EmptyClients.h                 |    1 +
 WebCore/page/ChromeClient.h                   |    1 +
 WebCore/platform/ScrollView.cpp               |    2 +-
 WebCore/platform/gtk/ScrollViewGtk.cpp        |   31 +++++++
 WebKit/gtk/ChangeLog                          |   18 ++++
 WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp |   26 ++++++
 WebKit/gtk/WebCoreSupport/ChromeClientGtk.h   |    1 +
 WebKit/gtk/webkit/webkitwebframe.cpp          |  112 ++++++++++++++++++++++++-
 WebKit/gtk/webkit/webkitwebframe.h            |    7 ++
 WebKit/mac/ChangeLog                          |    9 ++
 WebKit/mac/WebCoreSupport/WebChromeClient.h   |    1 +
 WebKit/qt/ChangeLog                           |    9 ++
 WebKit/qt/WebCoreSupport/ChromeClientQt.h     |    1 +
 WebKit/win/ChangeLog                          |    9 ++
 WebKit/win/WebCoreSupport/WebChromeClient.h   |    1 +
 WebKit/wx/ChangeLog                           |    9 ++
 WebKit/wx/WebKitSupport/ChromeClientWx.h      |    1 +
 18 files changed, 256 insertions(+), 2 deletions(-)
------- Comment #34 From 2009-06-27 15:09:40 PST -------
(From update of attachment 31980 [details])
(In reply to comment #31)
> This has been sitting in the commit queue for a month. :(  Kov?  What's your
> plan here...

I was under the impression that both patches would receive an r+ quickly, and since they were related, planned to commit them together, but I was clearly wrong =(. So I got out of my laziness today, and actually updated the second patch with the small issues that were pointed out, so that it can get a review, and decided to commit the other =).
------- Comment #35 From 2009-06-27 22:35:59 PST -------
(From update of attachment 30677 [details])
clearing review flag
------- Comment #36 From 2009-06-29 16:54:51 PST -------
Created an attachment (id=32028) [details]
report scrollbar policy

 ChangeLog                                     |   12 +++
 GNUmakefile.am                                |    6 +
 WebCore/ChangeLog                             |   38 ++++++++
 WebCore/loader/EmptyClients.h                 |    1 +
 WebCore/page/ChromeClient.h                   |    1 +
 WebCore/platform/ScrollView.cpp               |    2 +-
 WebCore/platform/gtk/ScrollViewGtk.cpp        |   31 ++++++
 WebKit/gtk/ChangeLog                          |   26 +++++
 WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp |   26 +++++
 WebKit/gtk/WebCoreSupport/ChromeClientGtk.h   |    1 +
 WebKit/gtk/tests/testwindow.c                 |  125 +++++++++++++++++++++++++
 WebKit/gtk/webkit/webkitwebframe.cpp          |  112 ++++++++++++++++++++++-
 WebKit/gtk/webkit/webkitwebframe.h            |    7 ++
 WebKit/mac/ChangeLog                          |   12 +++
 WebKit/mac/WebCoreSupport/WebChromeClient.h   |    1 +
 WebKit/qt/ChangeLog                           |   12 +++
 WebKit/qt/WebCoreSupport/ChromeClientQt.h     |    1 +
 WebKit/win/ChangeLog                          |   12 +++
 WebKit/win/WebCoreSupport/WebChromeClient.h   |    1 +
 WebKit/wx/ChangeLog                           |   12 +++
 WebKit/wx/WebKitSupport/ChromeClientWx.h      |    1 +
 21 files changed, 438 insertions(+), 2 deletions(-)
------- Comment #37 From 2009-06-29 16:56:01 PST -------
(From update of attachment 32028 [details])
Adding a test, and more changelogs.
------- Comment #38 From 2009-06-30 07:06:02 PST -------
(From update of attachment 32028 [details])
> +        (WebCore::ScrollView::setScrollbarModes):
> +>>>>>>> report scrollbar policy:WebCore/ChangeLog

Something went wrong here.

> +    GtkWidget* parent = gtk_widget_get_parent(GTK_WIDGET(webView));
> +    if (!parent || !GTK_IS_SCROLLED_WINDOW(parent))
> +        return;

Should this be an &&?

> +    /* Never */

Never?

> +    /* Always */

Is it ok to be more descriptive? :)

> +    /* Auto */

Ditto.

> +     * @web_view: the object which received the signal
> +     *
> +     * Signal emitted when policy for one or both of the scrollbars of
> +     * the view has changed. The default handler will apply the new
> +     * policy to the container that holds the #WebKitWebFrame if it is
> +     * a #GtkScrolledWindow. If you do not want this to be handled
> +     * automatically, you need to handle this signal.

What does the last sentence mean? 

> +     * Determines the current policy for the horizontal scrollbar of
> +     * the frame. For the main frame, make sure to set the same policy
> +     * on the scrollable widget containing the #WebKitWebView, unless
> +     * you know what you are doing.

What happens if they don't set the same policy in the widget containing the WebView? I think it should mention it here too.

The rest looks fine. I'll let Holger do the r+ as he's more familiar with it (and it's in his todo list :)
------- Comment #39 From 2009-07-14 03:54:18 PST -------
(In reply to comment #38)
> (From update of attachment 32028 [details] [details])
> > +        (WebCore::ScrollView::setScrollbarModes):
> > +>>>>>>> report scrollbar policy:WebCore/ChangeLog
> 
> Something went wrong here.

/me hates ChangeLogs

> > +    GtkWidget* parent = gtk_widget_get_parent(GTK_WIDGET(webView));
> > +    if (!parent || !GTK_IS_SCROLLED_WINDOW(parent))
> > +        return;
> 
> Should this be an &&?

No. We want to return early in the case where we have no parent, or in the case we have a parent, and it is not a scrolled window.

> > +    /* Never */
> 
> Never?

Those are the names of the scrolled window policies - never display scrollbars, in this case. I guess I can be more descriptive.

> > +     * Determines the current policy for the horizontal scrollbar of
> > +     * the frame. For the main frame, make sure to set the same policy
> > +     * on the scrollable widget containing the #WebKitWebView, unless
> > +     * you know what you are doing.
> 
> What happens if they don't set the same policy in the widget containing the
> WebView? I think it should mention it here too.

It depends on if/how web applications code work. If, say, a hackish code depends on the visible area being made smaller by adding a scrollbar, for some reason, it will break if the scrollbar does not appear, making the visible area smaller. I am not sure adding more info here will help makes this any clearer, but I'm up for suggestions.

> The rest looks fine. I'll let Holger do the r+ as he's more familiar with it
> (and it's in his todo list :)

Cool. Need to track him, and make this not slip one more release =P.

http://bugzilla.gnome.org/show_bug.cgi?id=588130 <= =)
------- Comment #40 From 2009-07-20 03:35:51 PST -------
We should not do the upcast to FrameView in the ScrollView. The road taken before (even inside ScrollView) is the following.

ScrollView::yourMethod()
{
  if (hostWindow()) hostWindow()->scrollPolicyChange(param);
}

HostWindow.h
 virtual void scrollPolicyChange(param) = 0;

Chrome.h
  virtual void scrollPolicyChange(param);

Chrome.cpp

Chrome::scrollPolicyChange(param)
{
   client->scrollPolicyChange
}


and this should work as the mac is doing it for scrolling into a rect. And we should go this route as dhyatt spent quite some time getting the upcasting out of ScrollView and I think we should not sneak it in again.
------- Comment #41 From 2009-07-31 21:08:06 PST -------
(From update of attachment 32028 [details])
r- for now as this needs to be updated per Holger's comment.
------- Comment #42 From 2009-08-27 14:55:12 PST -------
Created an attachment (id=38691) [details]
report scrollbar policy
------- Comment #43 From 2009-08-28 07:27:11 PST -------
Looks fine, no objections from my point of view.
------- Comment #44 From 2009-08-28 07:30:29 PST -------
(From update of attachment 38691 [details])

>  
> +void ScrollView::setScrollbarModes(ScrollbarMode horizontalMode, ScrollbarMode verticalMode)
> +{
> +    if (horizontalMode == m_horizontalScrollbarMode && verticalMode == m_verticalScrollbarMode)
> +        return;
> +
> +    m_horizontalScrollbarMode = horizontalMode;
> +    m_verticalScrollbarMode = verticalMode;
> +
> +    // We don't really care about reporting policy changes on frames
> +    // that have no adjustments attached to them.
> +    if (!m_horizontalAdjustment) {
> +        updateScrollbars(scrollOffset());
> +        return;
> +    }
> +
> +    if (!isFrameView())
> +        return;

okay, I don't think we need this here, but it does not really hurt. Thanks a lot for finishing it.
------- Comment #45 From 2009-08-28 08:35:20 PST -------
Landed as r47866, thanks!