Bug 59753 – Chromium Mac: Fork ScrollAnimatorMac to ScrollAnimatorChromiumMac for overlay scrollbar support

• Component: New Bugs
• Status: RESOLVED FIXED

[Sailesh Agrawal]

This bug is to track the work to use ScrollAnimatorMac.mm in Chromium Mac.

[Sailesh Agrawal]

Created attachment 91578 [details]
Patch

[James Robinson]

Please mark the patch review? when you want it reviewed.

[Sailesh Agrawal]

Created attachment 91589 [details]
Fixed Objective-C symbol clash error.

[James Robinson]

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.

[Sailesh Agrawal]

Created attachment 91598 [details]
Fixed patch to not include changes from another branch.

[Sailesh Agrawal]

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.

[Sailesh Agrawal]

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

[WebKit Review Bot]

Attachment 91598 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8520542

[Sailesh Agrawal]

Created attachment 91728 [details]
Fixed some bugs

[Sailesh Agrawal]

Comment on attachment 91728 [details]
Fixed some bugs

Oops, this patch has some problems. Removing.

[Sailesh Agrawal]

Created attachment 91733 [details]
Fixed change log

[Sam Weinig]

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.

[Sailesh Agrawal]

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

[Beth Dakin]

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

[James Robinson]

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.

[Sailesh Agrawal]

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

[Sailesh Agrawal]

Created attachment 93526 [details]
Patch

[Sailesh Agrawal]

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!

[Hajime Morrita]

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

[Sailesh Agrawal]

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

[Hajime Morrita]

Ah I see. My comment was pointless... I'm sorry for the interruption.

[Beth Dakin]

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.

[Sailesh Agrawal]

Filed bug 61144 to track the work to merge ScrollAnimatorChromiumMac.mm back to ScrollAnimatorMac.

Beth and Sam, could I commit this change?

Thanks!

[Beth Dakin]

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

[Sailesh Agrawal]

Created attachment 94473 [details]
Patch

[Sailesh Agrawal]

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!

[James Robinson]

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

[Sailesh Agrawal]

Created attachment 94485 [details]
Patch

[Sailesh Agrawal]

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.

[WebKit Commit Bot]

Comment on attachment 94485 [details]
Patch

Clearing flags on attachment: 94485

Committed r87118: <http://trac.webkit.org/changeset/87118>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sailesh Agrawal]

Re-opening bug since my change got backed out. Uploading a new patch in a second.

[Sailesh Agrawal]

Created attachment 95034 [details]
Patch

[Sailesh Agrawal]

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.

[Sailesh Agrawal]

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

[Mihai Parparita]

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.

[Sailesh Agrawal]

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.

[WebKit Review Bot]

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

[Sailesh Agrawal]

Created attachment 96292 [details]
Patch

[WebKit Review Bot]

Comment on attachment 96292 [details]
Patch

Clearing flags on attachment: 96292

Committed r88286: <http://trac.webkit.org/changeset/88286>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.