Bug 59728 – Add overlay scrollbar support to Chromium Mac

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

[Sailesh Agrawal]

Add overlay scrollbar support to Chromium Mac

[Sailesh Agrawal]

Created attachment 91540 [details]
Patch

[WebKit Review Bot]

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.

[Nico Weber]

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)

[Sailesh Agrawal]

(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?

[Nico Weber]

sgtm

[Sam Weinig]

This seems to copy a lot of code without refactoring or attribution.  This is not acceptable.

[Nico Weber]

sam: you probably want bug 59741

[Sam Weinig]

(In reply to comment #7)
> sam: you probably want bug 59741

No, this patch contains code copied from ScrollbarAnimatorMac.mm.

[Sam Weinig]

(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.

[Nico Weber]

Comment on attachment 91540 [details]
Patch

Yeah, this is v1. It'll get prettier before it's checked in.

[Sailesh Agrawal]

(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!

[Sailesh Agrawal]

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

[Sailesh Agrawal]

Marking this as fixed since the various sub patches all landed a while ago.