Expose the width and height scroll bars add to a view to chromium.

• Component: WebKit API
• Status: RESOLVED WONTFIX

[Sam Kerner]

Chromium allows extensions to create popups that are sized to fit the popup's content.  Browser action popups are an example.  In order to compute the proper size, chromium needs to know the width and height scroll bars add to a window.  Add a method to expose this information.

This change will allow the following bug to be fixed:  http://code.google.com/p/chromium/issues/detail?id=31494

[Sam Kerner]

Created attachment 51307 [details]
Patch to add WebKit::WebFrameImpl::scrollbarsSize().

[WebKit Review Bot]

Attachment 51307 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Kerner]

Created attachment 51311 [details]
Style fix from previous patch.

[Darin Fisher (:fishd, Google)]

Could you deduce this information from WebFrame::contentsSize() if you also had a WebFrame::visibleSize()?  Maybe we should just add a WebFrame::visibleSize() method.

[David Levin]

Comment on attachment 51311 [details]
Style fix from previous patch.

This really needs Darin Fisher's review since you are changing the public api, so these are just some drive by comments.

> Index: WebKit/chromium/public/WebFrame.h
> @@ -141,6 +141,11 @@ public:
>      // height required to display the document without scrollbars.
>      virtual int documentElementScrollHeight() const = 0;
>  
> +    // Return the size scrollbars add to the frame.  This is the width of the

One space before . in comments is the standard WebKit style.

> +    // horizontal scrollbar, and the height of the vertical scrollbar.  A
> +    // nonexistent scrollbar has a width and height of zero.
> +    virtual WebSize scrollbarsSize() const = 0;


> Index: WebKit/chromium/src/WebFrameImpl.cpp
> +WebSize WebFrameImpl::scrollbarsSize() const
> +{
...
> +    if (m_frame) {
> +        WebCore::FrameView *view = m_frame->view();

Incorrect * placement. It should look like this:
WebCore::FrameView* view = m_frame->view();

[Darin Fisher (:fishd, Google)]

Comment on attachment 51311 [details]
Style fix from previous patch.

R- since there are open questions and i'd like to get this out of the review queue.

[Sam Kerner]

Sorry to not look at this for a while.  Milestone-5 issues distracted me.

(In reply to comment #4)
> Could you deduce this information from WebFrame::contentsSize() if you also had
> a WebFrame::visibleSize()?  Maybe we should just add a WebFrame::visibleSize()
> method.

That could work if WebFrame::contentsSize() gave the size of the contents drawn on screen.  However, it seems to give the size of the contents, including content you would need to scroll to see.

I could add WebFrame::visibleSize() and WebFrame::visibleSizeIncludingScrollbars(), and subtract them.  Or I could create WebFrame::visibleSize(bool includeScrollbarSize).  Would one of those options be preferred to adding WebFrame:scrollbarsSize()?

Sam

[Darin Fisher (:fishd, Google)]

[Sorry for the delayed reply!]

How about adding the following methods:

  int verticalScrollbarWidth() const;
  int horizontalScrollbarHeight() const;

These can return 0 if the scrollbar does not exist.

[Peter Kasting]

Given that it's not clear how to fix the underlying bug here from the Chrome side, I'm closing this.