Add overlay scrollbar support to Chromium Mac
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.
sam: you probably want bug 59741
(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.