Bug 61144 – Merge ScrollAnimatorChromiumMac.mm back to ScrollAnimatorMac

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

[Sailesh Agrawal]

This bug is to track the work to merge ScrollAnimatorChromiumMac.mm back to ScrollAnimatorMac. This file was forked for the overlay scrollbar work (see bug 59753).

[Nico Weber]

I believe this was blocked on us enabling smooth scrolling. I enabled smooth scrolling, so this should be good to go now.

Let me know if you want to do it yourself, or if you want me to do it.

[Sailesh Agrawal]

> Let me know if you want to do it yourself, or if you want me to do it.

Feel free to work on this if you like.
Otherwise I'll probably work on this in a month or so.

[Sailesh Agrawal]

Created attachment 118000 [details]
Patch

[Sailesh Agrawal]

bdakin: please review
asvitkine: I tried rubber banding on 10.7 and everything looked ok to me
thakis, jamesr: FYI

So far I've tested the following:
- Chrome on 10.6 and 10.7
- Safari on 10.6

I'll be testing Safari on 10.7 tomorrow. Try bots pending. In particular, I'll keep an eye out on 10.5 bots to make sure I don't regress bug 66577.

> Source/WebCore/platform/mac/NSScrollerImpDetails.mm:3
> + * Copyright (C) 2011 oogle Inc. All Rights Reserved.

oogle Inc. ?

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:48
> +#if !defined(MAC_OS_X_VERSION_10_6) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_6

Should this be using the WebKit macros (BUILDING_ON_*) instead?


Otherwise, LGTM.

[Sailesh Agrawal]

Created attachment 118074 [details]
Patch

[Sailesh Agrawal]

(In reply to comment #5)
> > Source/WebCore/platform/mac/NSScrollerImpDetails.mm:3
> > + * Copyright (C) 2011 oogle Inc. All Rights Reserved.
> 
> oogle Inc. ?

Fixed.

> > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:48
> > +#if !defined(MAC_OS_X_VERSION_10_6) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_6
> 
> Should this be using the WebKit macros (BUILDING_ON_*) instead?

Yep, fixed.

[Sailesh Agrawal]

Pending try jobs:
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/125
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds/54

[Beth Dakin]

Comment on attachment 118000 [details]
Patch

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

I have to r- because I really don't understand why you are getting rid of the #if USE(SCROLLBAR_PAINTER) statement. It is the precedent to keep code related to new APIs from running at all on old platforms.

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.h:34
> +class ScrollbarThemeChromiumMac : public ScrollbarThemeMac {

I am taking your word for it that this is the right thing to do. As you know, I am not a Chromium platform expert, so I have a hard time evaluating this particular part of the patch, so I am just taking it for granted that this is correct. If a Chromium person could weigh in on this part, that would be ideal.

> Source/WebCore/platform/mac/NSScrollerImpDetails.h:32
> +#if (defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD))

This confuses me, and is the reason for the r-. Why are you defining these things on Leopard and SnowLeopard? They should definitely not be defined there. Overlay scrollbars are a concept new to Lion, and the old platforms should not be aware of these things. The precedent in the code is to only do new stuff on new platforms, not to define dummy versions for all platforms.

> Source/WebCore/platform/mac/NSScrollerImpDetails.mm:33
> +bool isScrollbarOverlayAPIAvailable()

This function isn't necessary. The #if USE(SCROLLBAR_PAINTER) is definitely the way to do this, in my opinion.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:48
> +#if !defined(MAC_OS_X_VERSION_10_6) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_6

Again, this shouldn't be necessary. None of this code should be running on pre-Lion systems.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:-560
> -#if USE(SCROLLBAR_PAINTER)

Why is it okay to just remove some of these #if USE statements and replace others with if (isScrollbarOverlayAPIAvailable())? But again, I seriously object to the removal of the #is USE anyway.

[James Robinson]

For Chromium we don't build a different binary for Leopard/Snow Leopard/Lion, instead we build the same code and use runtime checks to decide the behavior - for example draw overlay scrollbars when running on Lion.

In other bits of the code with this issue we've added helper functions that on Apple Mac builds compile down to a simple 'return true' or 'return false' depending on the BUILDING_ON_... macro and on chromium do the runtime check. Would that be appropriate here?

[Sailesh Agrawal]

Thanks for the quick review Beth!

> > Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.h:34
> > +class ScrollbarThemeChromiumMac : public ScrollbarThemeMac {
> 
> I am taking your word for it that this is the right thing to do. As you know, I am not a Chromium platform expert, so I have a hard time evaluating this particular part of the patch, so I am just taking it for granted that this is correct. If a Chromium person could weigh in on this part, that would be ideal.

Sounds good. I'll get a Chromium person to review all the Chromium specific bits.

> > Source/WebCore/platform/mac/NSScrollerImpDetails.h:32
> > +#if (defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD))
> 
> This confuses me, and is the reason for the r-. Why are you defining these things on Leopard and SnowLeopard? They should definitely not be defined there. Overlay scrollbars are a concept new to Lion, and the old platforms should not be aware of these things. The precedent in the code is to only do new stuff on new platforms, not to define dummy versions for all platforms.

Sorry for the confusion, I should have put this in the ChangeLog.

For Chromium we have a single build that runs on all 3 platforms. Instead of using #define's we check for existence of APIs at runtime. I agree that this is slightly more code but it's not that much worse. We cache the existence of the API in a static variable (see NSScrollerImpDetails.mm:isScrollbarOverlayAPIAvailable()). Afterwards everything is exactly the same as with the #ifdef code.

If you would like I could change isScrollbarOverlayAPIAvailable() to be something like this:
#if !PLATFORM(CHROMIUM)
#define isScrollbarOverlayAPIAvailable() (true)
#else
#define isScrollbarOverlayAPIAvailable() (/* do runtime check here */)
#endif

Thanks!
Sailesh

[Beth Dakin]

(In reply to comment #10)
> For Chromium we don't build a different binary for Leopard/Snow Leopard/Lion, instead we build the same code and use runtime checks to decide the behavior - for example draw overlay scrollbars when running on Lion.
> 

Ah! Okay! Thank you for explaining. I have never come across this before, though I suppose that seems rather surprising now that I understand.

> In other bits of the code with this issue we've added helper functions that on Apple Mac builds compile down to a simple 'return true' or 'return false' depending on the BUILDING_ON_... macro and on chromium do the runtime check. Would that be appropriate here?

I do think this would be more appropriate here. 

Maybe this is not a good idea, but something else that comes to mind is this…would it be possible to keep the ifdef and have it always set to true on Chromium so that the code is compiled in appropriately, and then use a runtime check to know whether to opt into it? I haven't fully fleshed out this though, and maybe it just doesn't make sense…

[Sailesh Agrawal]

> Maybe this is not a good idea, but something else that comes to mind is this…would it be possible to keep the ifdef and have it always set to true on Chromium so that the code is compiled in appropriately, and then use a runtime check to know whether to opt into it? I haven't fully fleshed out this though, and maybe it just doesn't make sense…

I think this would be difficult in places where we do things like
  #if USE(SCROLLBAR_PAINTER)
      // do overlay scrollbar work
  #else
      // do non-overlay scrollbar work
  #endif

> If you would like I could change isScrollbarOverlayAPIAvailable() to be something like this:
> #if !PLATFORM(CHROMIUM)
> #define isScrollbarOverlayAPIAvailable() (true)
> #else
> #define isScrollbarOverlayAPIAvailable() (/* do runtime check here */)
> #endif

This should be equivalent to the ifdef for modern compilers, which will be able to optimize the other branch away.

Though, you probably want to have the !PLATFORM(CHROMIUM) branch be based on USE(SCROLLBAR_PAINTER).

[Sailesh Agrawal]

Created attachment 118235 [details]
Patch

[Sailesh Agrawal]

I've replaced the runtime checks for !PLATFORM(CHROMIUM) with a simpler return true/false statement.

I tested Chromium and Safari on 10.6 and 10.7. New try jobs pending.

Please take another look. Thanks!

[WebKit Review Bot]

Comment on attachment 118235 [details]
Patch

Attachment 118235 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10767147

[Sailesh Agrawal]

Ping!
Hey Beth, could you take another look?
The cr-linux failure looks like a flake. Thanks!

[Beth Dakin]

Comment on attachment 118235 [details]
Patch

Hi Sailesh, sorry for the delay. I was trying to figure out a way to avoid having ScrollerPainter-related member variables in pre-Lion builds. I guess there's no way around that though. Looks good!

[WebKit Review Bot]

Comment on attachment 118235 [details]
Patch

Rejecting attachment 118235 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
245 (offset 9 lines).
Hunk #11 succeeded at 264 (offset 9 lines).
Hunk #12 succeeded at 273 (offset 9 lines).
Hunk #13 succeeded at 415 (offset 9 lines).
Hunk #14 succeeded at 435 (offset 9 lines).
Hunk #15 succeeded at 455 (offset 9 lines).
Hunk #16 FAILED at 471.
2 out of 16 hunks FAILED -- saving rejects to file Source/WebCore/platform/mac/ScrollbarThemeMac.mm.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Beth Dakin', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10910048

[Sailesh Agrawal]

Created attachment 119944 [details]
Patch

[Sailesh Agrawal]

Created attachment 119952 [details]
Patch

[Sailesh Agrawal]

Beth could you take another look, had to fix merge conflict in xcode project.

The previous patch also resolved merge conflicts due to all the scroll animator / theme code changes that happened 2 weeks ago.

Thanks.

[Nico Weber]

Comment on attachment 119952 [details]
Patch

you already had an r+

[WebKit Review Bot]

Comment on attachment 119952 [details]
Patch

Clearing flags on attachment: 119952

Committed r103291: <http://trac.webkit.org/changeset/103291>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sailesh Agrawal]

Reverted because this patch broke fast/events/touch/gesture/gesture-scroll.html for Chromium on Mac 10.5.

[Sailesh Agrawal]

Created attachment 120046 [details]
Patch

[Sailesh Agrawal]

Fixed the 10.5 regression by reapply fix for bug 66577.

[WebKit Review Bot]

Comment on attachment 120046 [details]
Patch

Clearing flags on attachment: 120046

Committed r103354: <http://trac.webkit.org/changeset/103354>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This broke the build.

[Sailesh Agrawal]

rniwa checked in the following change to fix the build:
http://trac.webkit.org/changeset/103363