Bug 25889 – [GTK] scrollbar policy for main frame is not implementable

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

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.

[Gustavo Noronha (kov)]

Created attachment 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(-)

[Gustavo Noronha (kov)]

Created attachment 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(-)

[Gustavo Noronha (kov)]

Comment on attachment 30507 [details]
report scrollbar policy

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.

[Gustavo Noronha (kov)]

Comment on attachment 30508 [details]
correctly report visible contents rect

This is not strictly needed to fix this specific problem, but size reporting is off without it.

[Gustavo Noronha (kov)]

Comment on attachment 30508 [details]
correctly report visible contents rect

Actually nevermind looking at this patch. It's broken. I'll investigate a bit more.

[Jaroslav Klaus]

(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

[Holger Freyther]

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

[Holger Freyther]

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?

[Gustavo Noronha (kov)]

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

[Gustavo Noronha (kov)]

Created attachment 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(-)

[Gustavo Noronha (kov)]

Comment on attachment 30675 [details]
report scrollbar policy

This time reporting policy only for the main frame, since it is the only that matters from a WebView point of view.

[Gustavo Noronha (kov)]

Created attachment 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(-)

[Gustavo Noronha (kov)]

Comment on attachment 30677 [details]
report correct visible size

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.

[Gustavo Noronha (kov)]

Created attachment 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(-)

[Gustavo Noronha (kov)]

Comment on attachment 31076 [details]
report scrollbar policy

More changes due to talking to zecke on IRC. We now report policy changes to any frame that has an adjustment.

[Holger Freyther]

Comment on attachment 30677 [details]
report correct visible size

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

[Holger Freyther]

Comment on attachment 31076 [details]
report scrollbar policy


>  
> +#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.

[Christian Dywan]

+ 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?

[Gustavo Noronha (kov)]

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

[Gustavo Noronha (kov)]

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

[Gustavo Noronha (kov)]

Created attachment 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(-)

[Gustavo Noronha (kov)]

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

[Gustavo Noronha (kov)]

(In reply to comment #21)
> Created an attachment (id=31111) [review]
> report scrollbar policy

Btw, I forgot about the i18n markers again, but I will add them when landing =/.

[Christian Dywan]

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

[Gustavo Noronha (kov)]

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

[Gustavo Noronha (kov)]

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

[Gustavo Noronha (kov)]

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

[Holger Freyther]

I think you want to add _() to the properties names?

[Gustavo Noronha (kov)]

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

[Jan Alonzo]

Comment on attachment 31111 [details]
report scrollbar policy

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

[Eric Seidel (no email)]

This has been sitting in the commit queue for a month. :(  Kov?  What's your plan here...

[Gustavo Noronha (kov)]

Comment on attachment 30677 [details]
report correct visible size

I got this one committed as r45313, now.

[Gustavo Noronha (kov)]

Created attachment 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(-)

[Gustavo Noronha (kov)]

Comment on attachment 31980 [details]
report scrollbar policy

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

[Jan Alonzo]

Comment on attachment 30677 [details]
report correct visible size

clearing review flag

[Gustavo Noronha (kov)]

Created attachment 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(-)

[Gustavo Noronha (kov)]

Comment on attachment 32028 [details]
report scrollbar policy

Adding a test, and more changelogs.

[Jan Alonzo]

Comment on attachment 32028 [details]
report scrollbar policy

> +        (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 :)

[Gustavo Noronha (kov)]

(In reply to comment #38)
> (From update of attachment 32028 [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 <= =)

[Holger Freyther]

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.

[Jan Alonzo]

Comment on attachment 32028 [details]
report scrollbar policy

r- for now as this needs to be updated per Holger's comment.

[Gustavo Noronha (kov)]

Created attachment 38691 [details]
report scrollbar policy

[Holger Freyther]

Looks fine, no objections from my point of view.

[Holger Freyther]

Comment on attachment 38691 [details]
report scrollbar policy


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

[Gustavo Noronha (kov)]

Landed as r47866, thanks!