Bug 66008 - [chromium] Update WebScrollbar so that it works with overlay scrollbars on Lion
Summary: [chromium] Update WebScrollbar so that it works with overlay scrollbars on Lion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Abd-El-Malek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-10 13:20 PDT by John Abd-El-Malek
Modified: 2011-08-10 18:20 PDT (History)
3 users (show)

See Also:


Attachments
Patch (36.70 KB, patch)
2011-08-10 13:24 PDT, John Abd-El-Malek
no flags Details | Formatted Diff | Diff
Patch (37.05 KB, patch)
2011-08-10 13:29 PDT, John Abd-El-Malek
no flags Details | Formatted Diff | Diff
Patch (36.88 KB, patch)
2011-08-10 16:07 PDT, John Abd-El-Malek
no flags Details | Formatted Diff | Diff
Patch (36.88 KB, patch)
2011-08-10 16:24 PDT, John Abd-El-Malek
no flags Details | Formatted Diff | Diff
Patch (36.89 KB, patch)
2011-08-10 16:35 PDT, John Abd-El-Malek
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Abd-El-Malek 2011-08-10 13:20:50 PDT
[chromium] Update WebScrollbar so that it works with overlay scrollbars on Lion
Comment 1 John Abd-El-Malek 2011-08-10 13:24:30 PDT
Created attachment 103526 [details]
Patch
Comment 2 John Abd-El-Malek 2011-08-10 13:25:53 PDT
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.
Comment 3 John Abd-El-Malek 2011-08-10 13:26:24 PDT
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
Comment 4 WebKit Review Bot 2011-08-10 13:27:33 PDT
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.
Comment 5 John Abd-El-Malek 2011-08-10 13:29:09 PDT
Created attachment 103527 [details]
Patch
Comment 6 WebKit Review Bot 2011-08-10 13:30:44 PDT
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.
Comment 7 WebKit Review Bot 2011-08-10 13:33:10 PDT
Comment on attachment 103527 [details]
Patch

Attachment 103527 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9339771
Comment 8 John Abd-El-Malek 2011-08-10 16:07:38 PDT
Created attachment 103549 [details]
Patch
Comment 9 John Abd-El-Malek 2011-08-10 16:24:05 PDT
Created attachment 103552 [details]
Patch
Comment 10 John Abd-El-Malek 2011-08-10 16:35:01 PDT
Created attachment 103554 [details]
Patch
Comment 11 John Abd-El-Malek 2011-08-10 16:36:22 PDT
(latest patch just fixed spacing)

Darin: please have a look
Comment 12 James Robinson 2011-08-10 17:31:30 PDT
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'
Comment 13 John Abd-El-Malek 2011-08-10 18:20:16 PDT
Committed r92811: <http://trac.webkit.org/changeset/92811>