Summary: | Add overlay scrollbar support to Chromium Mac | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sailesh Agrawal <sail> | ||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | bdakin, jamesr, mihaip, rsesek, sam, thakis, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Other | ||||||
OS: | OS X 10.5 | ||||||
Bug Depends on: | 59741, 59753, 59756 | ||||||
Bug Blocks: | |||||||
Attachments: |
|
Description
Sailesh Agrawal
2011-04-28 12:46:02 PDT
Created attachment 91540 [details]
Patch
Attachment 91540 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.h:31: Alphabetical sorting problem. [build/include_order] [4]
Source/WebCore/platform/chromium/ScrollbarOverlayUtilitiesMac.h:65: The parameter name "painter" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/JavaScriptCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.h:30: Alphabetical sorting problem. [build/include_order] [4]
Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.h:34: Alphabetical sorting problem. [build/include_order] [4]
Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Total errors found: 6 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 91540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91540&action=review This CL does too many things at once. > Source/JavaScriptCore/ChangeLog:3 > + Reviewed by NOBODY (OOPS!). why are you changing an existing an existing changelog entry instead of adding a new one? (Tools/Scripts/webkit-patch should do this automatically for you) (In reply to comment #3) > (From update of attachment 91540 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91540&action=review > > This CL does too many things at once. There are basically 4 parts: - enable WTF_USE_WK_SCROLLBAR_PAINTER - update ScrollbarThemeChromiumMac.mm - fork ScrollAnimatorMac.mm - add the ScrollbarOverlayUtilities Should I split it that way? Does each part get its own bug? sgtm This seems to copy a lot of code without refactoring or attribution. This is not acceptable. (In reply to comment #7) > sam: you probably want bug 59741 No, this patch contains code copied from ScrollbarAnimatorMac.mm. (In reply to comment #6) > This seems to copy a lot of code without refactoring or attribution. This is not acceptable. Sorry, the attribution is there, but this a simply silly amount of copying. Comment on attachment 91540 [details]
Patch
Yeah, this is v1. It'll get prettier before it's checked in.
(In reply to comment #9) > (In reply to comment #6) > > This seems to copy a lot of code without refactoring or attribution. This is not acceptable. > > Sorry, the attribution is there, but this a simply silly amount of copying. I'll work on trying to use ScrollAnimatorMac.* without forking. Thanks for the feedback! I've split this into three separate reviews. I've also removed the forking of ScrollAnimatorMac.mm. https://bugs.webkit.org/show_bug.cgi?id=59741 https://bugs.webkit.org/show_bug.cgi?id=59753 https://bugs.webkit.org/show_bug.cgi?id=59756 Marking this as fixed since the various sub patches all landed a while ago. |