Bug 66008 – [chromium] Update WebScrollbar so that it works with overlay scrollbars on Lion

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

[John Abd-El-Malek]

[chromium] Update WebScrollbar so that it works with overlay scrollbars on Lion

[John Abd-El-Malek]

Created attachment 103526 [details]
Patch

[John Abd-El-Malek]

This is the patch you reviewed in http://codereview.chromium.org/7538006/.

There are some style errors because I've separated the chromium and non-chromium headers in the cpp files. This seemed to be the style of the newer chrome WebKit files, so I've followed it, although I don't care either way.

[John Abd-El-Malek]

also, I can't roll this until the chromium side patch lands. So as soon as that does, I'll update this with a DEPS roll for chrome

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/src/ScrollbarGroup.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/chromium/src/WebScrollbarImpl.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/chromium/src/WebScrollbarImpl.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 11 files


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

[John Abd-El-Malek]

Created attachment 103527 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/src/ScrollbarGroup.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/chromium/src/WebScrollbarImpl.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/chromium/src/WebScrollbarImpl.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 12 files


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

[WebKit Review Bot]

Comment on attachment 103527 [details]
Patch

Attachment 103527 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9339771

[John Abd-El-Malek]

Created attachment 103549 [details]
Patch

[John Abd-El-Malek]

Created attachment 103552 [details]
Patch

[John Abd-El-Malek]

Created attachment 103554 [details]
Patch

[John Abd-El-Malek]

(latest patch just fixed spacing)

Darin: please have a look

[James Robinson]

Comment on attachment 103554 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103554&action=review

R=me

> Source/WebKit/chromium/src/ScrollbarGroup.cpp:8
> + *     * Redistributions of source code must retain the above copyright

fyi, this is the wrong copyright header format.  we're supposed to use the 2-clause version for new stuff (like in http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp, for instance)

> Source/WebKit/chromium/src/ScrollbarGroup.cpp:46
> +    : m_page(page),
> +      m_horizontalScrollbar(0)

nit: comma goes on the next line

> Source/WebKit/chromium/src/ScrollbarGroup.cpp:79
> +    } else {
> +        willRemoveVerticalScrollbar(scrollbar->scrollbar());

might as well ASSERT() this is the vertical scrollbar

> Source/WebKit/chromium/src/ScrollbarGroup.h:48
> +    ScrollbarGroup(WebCore::Page*);

add 'explicit'

[John Abd-El-Malek]

Committed r92811: <http://trac.webkit.org/changeset/92811>