Bug 59753

Summary: Chromium Mac: Fork ScrollAnimatorMac to ScrollAnimatorChromiumMac for overlay scrollbar support
Product: WebKit Reporter: Sailesh Agrawal <sail>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, commit-queue, dglazkov, jamesr, mihai, morrita, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 61347    
Bug Blocks: 59728    
Attachments:
Description Flags
Patch
none
Fixed Objective-C symbol clash error.
jamesr: review-
Fixed patch to not include changes from another branch.
none
Fixed some bugs
none
Fixed change log
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Sailesh Agrawal 2011-04-28 15:56:28 PDT
This bug is to track the work to use ScrollAnimatorMac.mm in Chromium Mac.
Comment 1 Sailesh Agrawal 2011-04-28 16:05:18 PDT
Created attachment 91578 [details]
Patch
Comment 2 James Robinson 2011-04-28 16:38:49 PDT
Please mark the patch review? when you want it reviewed.
Comment 3 Sailesh Agrawal 2011-04-28 16:52:06 PDT
Created attachment 91589 [details]
Fixed Objective-C symbol clash error.
Comment 4 James Robinson 2011-04-28 16:59:38 PDT
Comment on attachment 91589 [details]
Fixed Objective-C symbol clash error.

View in context: https://bugs.webkit.org/attachment.cgi?id=91589&action=review

The changes to ScrollbarOverlayUtilitiesMac.h/mm appear to be changes from WebKit style braces to non WebKit style braces, except for one typo.  I'd suggest not changing those files at all.

Why is the include guard on ScrollAnimatorMac changed?  Do we want to use ScrollAnimator on chromium mac even if ENABLE(SMOOTH_SCROLLING) is off?  That seems wrong, why not just turn SMOOTH_SCROLLING on if that's the behavior we want?  Is there code in ScrollAnimatorMac.mm currently that is not related to the SMOOTH_SCROLLING enable that should be refactored?

It seems you have some unintended changes in ScrollAnimatorMac.mm (like reodering the elasticDelta/reboundDelta functions for no obvious reason).

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

you can't do this, it will fail a presubmit check. replace this with some text explaining why this patch does not have layout tests.
Comment 5 Sailesh Agrawal 2011-04-28 17:15:41 PDT
Created attachment 91598 [details]
Fixed patch to not include changes from another branch.
Comment 6 Sailesh Agrawal 2011-04-28 17:16:33 PDT
Comment on attachment 91589 [details]
Fixed Objective-C symbol clash error.

View in context: https://bugs.webkit.org/attachment.cgi?id=91589&action=review

>> Source/WebCore/ChangeLog:10
>> +        No new tests. (OOPS!)
> 
> you can't do this, it will fail a presubmit check. replace this with some text explaining why this patch does not have layout tests.

Fixed.
Comment 7 Sailesh Agrawal 2011-04-28 17:18:40 PDT
> The changes to ScrollbarOverlayUtilitiesMac.h/mm appear to be changes from WebKit style braces to non WebKit style braces, except for one typo.  I'd suggest not changing those files at all.

Ooops, sorry. Forgot to rebase my branch. Fixed.

> Why is the include guard on ScrollAnimatorMac changed?  Do we want to use ScrollAnimator on chromium mac even if ENABLE(SMOOTH_SCROLLING) is off?  That seems wrong, why not just turn SMOOTH_SCROLLING on if that's the behavior we want?  Is there code in ScrollAnimatorMac.mm currently that is not related to the SMOOTH_SCROLLING enable that should be refactored?
So I think enabling SMOOTH_SCROLLING should be separate from enabling overlay scrollbar. As far as I can tell there's no reason we have to have smooth scrolling to use ScrollAnimatorMac.mm

> It seems you have some unintended changes in ScrollAnimatorMac.mm (like reodering the elasticDelta/reboundDelta functions for no obvious reason).
Fixed. Shouldn't have been in the patch.
Comment 8 WebKit Review Bot 2011-04-28 17:22:26 PDT
Attachment 91598 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8520542
Comment 9 Sailesh Agrawal 2011-04-29 14:00:57 PDT
Created attachment 91728 [details]
Fixed some bugs
Comment 10 Sailesh Agrawal 2011-04-29 14:04:34 PDT
Comment on attachment 91728 [details]
Fixed some bugs

Oops, this patch has some problems. Removing.
Comment 11 Sailesh Agrawal 2011-04-29 14:10:46 PDT
Created attachment 91733 [details]
Fixed change log
Comment 12 Sam Weinig 2011-05-01 13:27:44 PDT
Comment on attachment 91733 [details]
Fixed change log

View in context: https://bugs.webkit.org/attachment.cgi?id=91733&action=review

> Source/WebCore/platform/mac/ScrollAnimatorMac.h:29
> +#if ENABLE(SMOOTH_SCROLLING) || (PLATFORM(CHROMIUM) && OS(DARWIN))

Why doesn't Chromium on Mac just enable SMOOTH_SCROLLING if you want to take this code path.  Adding additional defines defeats the purpose of have fine grained enabled macros.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:54
> +static NSPoint IntPointToNSPoint(WebCore::IntPoint point) {
> +    return NSMakePoint(point.x(), point.y());
> +}
> +
> +static WebCore::IntPoint NSPointToIntPoint(NSPoint point) {
> +    return WebCore::IntPoint(point.x, point.y);
> +}

{ needs to be on the next line. Function can't start with a capital.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:221
> +    return IntPointToNSPoint(_animator->scrollableArea()->currentMousePosition());

Why is this necessary, IntPoint has a conversion to NSPoint.
Comment 13 Sailesh Agrawal 2011-05-01 13:38:53 PDT
> Why doesn't Chromium on Mac just enable SMOOTH_SCROLLING if you want to take this code path.  Adding additional defines defeats the purpose of have fine grained enabled macros.

I'm not sure where we are when it comes to implementing smooth scrolling. I'll ask around but it would be nice to decouple these two features. http:://crbug.com/61140 tracks the smooth scrolling work.

I was talking to jamesr and he recommended refactoring ScrollAnimatorMac.mm so that the overlay code was in a separate file. What do you think about that idea?

> Why is this necessary, IntPoint has a conversion to NSPoint.

Will fix.
Comment 14 Beth Dakin 2011-05-02 12:02:53 PDT
(In reply to comment #13)
> I was talking to jamesr and he recommended refactoring ScrollAnimatorMac.mm so that the overlay code was in a separate file. What do you think about that idea?
> 

Right now there are a lot of delegates implemented in ScrollAnimatorMac.mm, and I think it would be reasonable to move those into a different file. However, I definitely do not think that all of the overlay-related code should be moved to a different file. It's very much relevant to the animator, and I think the animator should continue to hold member variables that point to the delegates whether the delegates are implemented elsewhere or not.
Comment 15 James Robinson 2011-05-03 14:31:32 PDT
Without having a ton of knowledge about this code, I think it should be possible to turn overlays on without enabling the scroll animator (or at least the bits associated with smooth scrolling).  I think it's reasonable that the smooth scrolling logic here depend on overlay scrollbars being enabled.
Comment 16 Sailesh Agrawal 2011-05-10 10:50:30 PDT
> However, I definitely do not think that all of the overlay-related code should be moved to a different file. It's very much relevant to the animator, and I think the animator should continue to hold member variables that point to the delegates whether the delegates are implemented elsewhere or not.

Hi Beth and Sam. It sounds like the best thing would be to temporarily fork ScrollAnimatorMac to ScrollAnimatorChromiumMac. I'll file a bug on myself to merge it back when smooth scrolling is enabled for Chromium (Peter is working on this right now).

Does this seem reasonable?
Comment 17 Sailesh Agrawal 2011-05-13 16:06:38 PDT
Created attachment 93526 [details]
Patch
Comment 18 Sailesh Agrawal 2011-05-13 16:11:25 PDT
Hi all, this latest patch addresses review comments and goes back to forking ScrollAnimatorMac.

I think this is much cleaner than using #ifdefs everywhere. This feature needs to make it into the next version fo Google Chrome so I'd appreciate any help getting change moving forward.

I'll file a bug to track the work to merge it back to ScrollAnimatorMac once smooth scrolling is enabled for Google Chrome Mac.

Thanks!
Comment 19 Hajime Morrita 2011-05-15 18:56:14 PDT
How about to make ScrollAnimatorChromiumMac a subclass of  ScrollAnimatorMac
and factor out chromium specific parts into a method and override it? 
Forked code is really hard to maintain and I hope we didn't have more....
Comment 20 Sailesh Agrawal 2011-05-15 19:13:44 PDT
(In reply to comment #19)
> How about to make ScrollAnimatorChromiumMac a subclass of  ScrollAnimatorMac
> and factor out chromium specific parts into a method and override it? 
> Forked code is really hard to maintain and I hope we didn't have more....

Currently ScrollAnimatorMac is guarded by an #if ENABLE(SMOOTH_SCROLLING). To subclass it I would have to change that to:
    #if ENABLE(SMOOTH_SCROLLING) || (PLATFORM(CHROMIUM) && OS(DARWIN))
As per comment 12 we want to avoid that.

At this point the lack of smooth scrolling is the only real difference between the Mac and Chrome version. Once we enable smooth scrolling I wouldn't even need a subclass.
Comment 21 Hajime Morrita 2011-05-15 19:24:54 PDT
Ah I see. My comment was pointless... I'm sorry for the interruption.
Comment 22 Beth Dakin 2011-05-19 14:06:12 PDT
Sailesh and I talked about this on IRC. We agreed that this is okay for a temporary solution, but a bug should be filed to un-fork asap.
Comment 23 Sailesh Agrawal 2011-05-19 14:32:40 PDT
Filed bug 61144 to track the work to merge ScrollAnimatorChromiumMac.mm back to ScrollAnimatorMac.

Beth and Sam, could I commit this change?

Thanks!
Comment 24 Beth Dakin 2011-05-19 14:58:45 PDT
(In reply to comment #23)
> Filed bug 61144 to track the work to merge ScrollAnimatorChromiumMac.mm back to ScrollAnimatorMac.
> 
> Beth and Sam, could I commit this change?
> 
> Thanks!

Yes, you can. You should get a Chromium person to r+ since in the end this patch will only affect Chromium. But yes, you can commit it.
Comment 25 Sailesh Agrawal 2011-05-23 13:28:34 PDT
Created attachment 94473 [details]
Patch
Comment 26 Sailesh Agrawal 2011-05-23 13:30:19 PDT
Removed some unnecessary changes to ScrollAnimatorChromiumMac.mm. At this point the only changes to the forked code is renaming ScrollAnimatorMac to ScrollAnimatorChromiumMac and renaming ScrollbarThemeMac to ScrollbarThemeChromiumMac. Added this to the ChangeLog.

James could you take another look? Thanks!
Comment 27 James Robinson 2011-05-23 13:36:08 PDT
Comment on attachment 94473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94473&action=review

Seems OK

> Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.h:55
> +    ScrollAnimatorChromiumMac(ScrollableArea*);

should be explicit

> Source/WebCore/platform/graphics/IntPoint.h:40
> +#if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && OS(DARWIN))

should this just be #if OS(DARWIN)?

> Source/WebCore/platform/graphics/IntPoint.h:122
> +#if (PLATFORM(MAC) || (PLATFORM(CHROMIUM) && OS(DARWIN))) && !defined(NSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES)

should this just be #if OS(DARWIN) && !defined(NSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES) ?
Comment 28 Sailesh Agrawal 2011-05-23 14:05:59 PDT
Created attachment 94485 [details]
Patch
Comment 29 Sailesh Agrawal 2011-05-23 14:06:47 PDT
Comment on attachment 94473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94473&action=review

>> Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.h:55
>> +    ScrollAnimatorChromiumMac(ScrollableArea*);
> 
> should be explicit

Done.

>> Source/WebCore/platform/graphics/IntPoint.h:40
>> +#if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && OS(DARWIN))
> 
> should this just be #if OS(DARWIN)?

Done.

>> Source/WebCore/platform/graphics/IntPoint.h:122
>> +#if (PLATFORM(MAC) || (PLATFORM(CHROMIUM) && OS(DARWIN))) && !defined(NSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES)
> 
> should this just be #if OS(DARWIN) && !defined(NSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES) ?

Done.
Comment 30 WebKit Commit Bot 2011-05-23 19:04:14 PDT
Comment on attachment 94485 [details]
Patch

Clearing flags on attachment: 94485

Committed r87118: <http://trac.webkit.org/changeset/87118>
Comment 31 WebKit Commit Bot 2011-05-23 19:04:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 32 Sailesh Agrawal 2011-05-26 13:54:10 PDT
Re-opening bug since my change got backed out. Uploading a new patch in a second.
Comment 33 Sailesh Agrawal 2011-05-26 13:57:56 PDT
Created attachment 95034 [details]
Patch
Comment 34 Sailesh Agrawal 2011-05-26 14:02:31 PDT
This is a new patch that passes layout tests. See:
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds/194
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/633
For comparison here's the output from the try bot with an empty change list:
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds/191
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/631

The differences between this patch and the one that got reverted are:
 - disable smooth scrolling by adding and #ifdef in ScrollAnimatorChromiumMac::scroll()
 - adding IntPointMac.mm. This is used by some ScrollAnimatorChromiuMac code that's currently #ifdef'ed out.
  - removed exceptions from test_expectations.txt that were added when my change was reverted

I'm running browser tests and ui_tests locally and will update this bug with the results. Thanks.
Comment 35 Sailesh Agrawal 2011-05-26 17:51:43 PDT
> I'm running browser tests and ui_tests locally and will update this bug with the results. Thanks.

browser_tests passed. There were some errors in the ui_tests but I don't think they're related to my change.
Comment 36 Mihai Parparita 2011-06-07 12:01:19 PDT
Comment on attachment 95034 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95034&action=review

> Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.mm:926
> +    m_haveScrolledSincePageLoad = true;

Nit: Is this necessary? smoothScrollWithEvent is only called by handleWheelEvent, and you already set m_haveScrolledSincePageLoad to true there.
Comment 37 Sailesh Agrawal 2011-06-07 12:11:03 PDT
Comment on attachment 95034 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95034&action=review

>> Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.mm:926
>> +    m_haveScrolledSincePageLoad = true;
> 
> Nit: Is this necessary? smoothScrollWithEvent is only called by handleWheelEvent, and you already set m_haveScrolledSincePageLoad to true there.

This is all copy paste from the platform/mac/ScrollAnimatorMac.mm code. My goal is to keep the two files as similar as possible so that I can re-merge them once we have smooth scrolling on chromium mac.
Comment 38 WebKit Review Bot 2011-06-07 12:56:26 PDT
Comment on attachment 95034 [details]
Patch

Rejecting attachment 95034 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2

Last 500 characters of output:
(offset -4 lines).
patching file Source/WebCore/platform/ScrollAnimator.cpp
patching file Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.h
patching file Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.mm
patching file Source/WebCore/platform/graphics/IntPoint.h
Hunk #1 succeeded at 38 (offset 1 line).
Hunk #2 succeeded at 126 (offset 6 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Mihai Parparita', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/8811198
Comment 39 Sailesh Agrawal 2011-06-07 13:26:35 PDT
Created attachment 96292 [details]
Patch
Comment 40 WebKit Review Bot 2011-06-07 16:57:43 PDT
Comment on attachment 96292 [details]
Patch

Clearing flags on attachment: 96292

Committed r88286: <http://trac.webkit.org/changeset/88286>
Comment 41 WebKit Review Bot 2011-06-07 16:57:50 PDT
All reviewed patches have been landed.  Closing bug.