Bug 25889 - [GTK] scrollbar policy for main frame is not implementable
Summary: [GTK] scrollbar policy for main frame is not implementable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 25646
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-20 07:11 PDT by Gustavo Noronha (kov)
Modified: 2009-08-28 08:35 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2009-05-20 07:11:07 PDT
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 Gustavo Noronha (kov) 2009-05-20 07:31:35 PDT
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(-)
Comment 2 Gustavo Noronha (kov) 2009-05-20 07:31:41 PDT
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(-)
Comment 3 Gustavo Noronha (kov) 2009-05-20 07:33:10 PDT
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.
Comment 4 Gustavo Noronha (kov) 2009-05-20 07:33:46 PDT
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.
Comment 5 Gustavo Noronha (kov) 2009-05-20 16:25:56 PDT
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.
Comment 6 Jaroslav Klaus 2009-05-22 21:18:57 PDT
(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 Holger Freyther 2009-05-23 05:44:24 PDT
(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 Holger Freyther 2009-05-25 19:07:29 PDT
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 Gustavo Noronha (kov) 2009-05-25 20:07:04 PDT
(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 Gustavo Noronha (kov) 2009-05-26 12:45:39 PDT
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(-)
Comment 11 Gustavo Noronha (kov) 2009-05-26 12:46:50 PDT
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.
Comment 12 Gustavo Noronha (kov) 2009-05-26 12:57:01 PDT
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(-)
Comment 13 Gustavo Noronha (kov) 2009-05-26 13:04:48 PDT
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.
Comment 14 Gustavo Noronha (kov) 2009-06-08 18:23:00 PDT
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(-)
Comment 15 Gustavo Noronha (kov) 2009-06-08 18:24:39 PDT
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.
Comment 16 Holger Freyther 2009-06-09 07:53:41 PDT
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... :)
Comment 17 Holger Freyther 2009-06-09 08:31:54 PDT
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.
Comment 18 Christian Dywan 2009-06-09 10:41:51 PDT
+ 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 Gustavo Noronha (kov) 2009-06-09 10:45:10 PDT
(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 Gustavo Noronha (kov) 2009-06-09 16:15:08 PDT
(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 Gustavo Noronha (kov) 2009-06-09 16:15:39 PDT
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(-)
Comment 22 Gustavo Noronha (kov) 2009-06-09 16:17:31 PDT
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 Gustavo Noronha (kov) 2009-06-09 16:44:51 PDT
(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 =/.
Comment 24 Christian Dywan 2009-06-10 12:48:49 PDT
(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 Gustavo Noronha (kov) 2009-06-12 10:29:45 PDT
(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 Gustavo Noronha (kov) 2009-06-12 11:28:20 PDT
(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 Gustavo Noronha (kov) 2009-06-15 14:05:54 PDT
(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 Holger Freyther 2009-06-22 00:03:36 PDT
I think you want to add _() to the properties names?
Comment 29 Gustavo Noronha (kov) 2009-06-22 08:46:52 PDT
(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 Jan Alonzo 2009-06-25 04:35:01 PDT
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.
Comment 31 Eric Seidel (no email) 2009-06-26 01:41:03 PDT
This has been sitting in the commit queue for a month. :(  Kov?  What's your plan here...
Comment 32 Gustavo Noronha (kov) 2009-06-27 14:42:09 PDT
Comment on attachment 30677 [details]
report correct visible size

I got this one committed as r45313, now.
Comment 33 Gustavo Noronha (kov) 2009-06-27 15:07:16 PDT
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(-)
Comment 34 Gustavo Noronha (kov) 2009-06-27 15:09:40 PDT
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 =).
Comment 35 Jan Alonzo 2009-06-27 22:35:59 PDT
Comment on attachment 30677 [details]
report correct visible size

clearing review flag
Comment 36 Gustavo Noronha (kov) 2009-06-29 16:54:51 PDT
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(-)
Comment 37 Gustavo Noronha (kov) 2009-06-29 16:56:01 PDT
Comment on attachment 32028 [details]
report scrollbar policy

Adding a test, and more changelogs.
Comment 38 Jan Alonzo 2009-06-30 07:06:02 PDT
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 :)
Comment 39 Gustavo Noronha (kov) 2009-07-14 03:54:18 PDT
(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 <= =)
Comment 40 Holger Freyther 2009-07-20 03:35:51 PDT
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 Jan Alonzo 2009-07-31 21:08:06 PDT
Comment on attachment 32028 [details]
report scrollbar policy

r- for now as this needs to be updated per Holger's comment.
Comment 42 Gustavo Noronha (kov) 2009-08-27 14:55:12 PDT
Created attachment 38691 [details]
report scrollbar policy
Comment 43 Holger Freyther 2009-08-28 07:27:11 PDT
Looks fine, no objections from my point of view.
Comment 44 Holger Freyther 2009-08-28 07:30:29 PDT
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.
Comment 45 Gustavo Noronha (kov) 2009-08-28 08:35:20 PDT
Landed as r47866, thanks!