Bug 59728 - Add overlay scrollbar support to Chromium Mac
Summary: Add overlay scrollbar support to Chromium Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 59741 59753 59756
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-28 12:46 PDT by Sailesh Agrawal
Modified: 2011-08-18 12:15 PDT (History)
7 users (show)

See Also:


Attachments
Patch (85.37 KB, patch)
2011-04-28 12:48 PDT, Sailesh Agrawal
thakis: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sailesh Agrawal 2011-04-28 12:46:02 PDT
Add overlay scrollbar support to Chromium Mac
Comment 1 Sailesh Agrawal 2011-04-28 12:48:12 PDT
Created attachment 91540 [details]
Patch
Comment 2 WebKit Review Bot 2011-04-28 12:51:10 PDT
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 3 Nico Weber 2011-04-28 13:07:45 PDT
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)
Comment 4 Sailesh Agrawal 2011-04-28 13:11:16 PDT
(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?
Comment 5 Nico Weber 2011-04-28 13:12:50 PDT
sgtm
Comment 6 Sam Weinig 2011-04-28 14:47:11 PDT
This seems to copy a lot of code without refactoring or attribution.  This is not acceptable.
Comment 7 Nico Weber 2011-04-28 14:48:53 PDT
sam: you probably want bug 59741
Comment 8 Sam Weinig 2011-04-28 14:49:50 PDT
(In reply to comment #7)
> sam: you probably want bug 59741

No, this patch contains code copied from ScrollbarAnimatorMac.mm.
Comment 9 Sam Weinig 2011-04-28 14:51:38 PDT
(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 10 Nico Weber 2011-04-28 14:53:04 PDT
Comment on attachment 91540 [details]
Patch

Yeah, this is v1. It'll get prettier before it's checked in.
Comment 11 Sailesh Agrawal 2011-04-28 14:53:46 PDT
(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!
Comment 12 Sailesh Agrawal 2011-04-28 16:19:24 PDT
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
Comment 13 Sailesh Agrawal 2011-08-18 12:15:21 PDT
Marking this as fixed since the various sub patches all landed a while ago.