Bug 61144

Summary: Merge ScrollAnimatorChromiumMac.mm back to ScrollAnimatorMac
Product: WebKit Reporter: Sailesh Agrawal <sail>
Component: New BugsAssignee: Sailesh Agrawal <sail>
Status: RESOLVED FIXED    
Severity: Normal CC: asvitkine, bdakin, dglazkov, jamesr, simon.fraser, thakis, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Bug Depends on: 74915    
Bug Blocks: 74375    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Sailesh Agrawal 2011-05-19 14:31:03 PDT
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).
Comment 1 Nico Weber 2011-09-03 18:58:49 PDT
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.
Comment 2 Sailesh Agrawal 2011-09-06 08:53:04 PDT
> 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.
Comment 3 Sailesh Agrawal 2011-12-06 00:09:57 PST
Created attachment 118000 [details]
Patch
Comment 4 Sailesh Agrawal 2011-12-06 00:14:32 PST
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.
Comment 5 asvitkine 2011-12-06 07:03:48 PST
> 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.
Comment 6 Sailesh Agrawal 2011-12-06 11:11:22 PST
Created attachment 118074 [details]
Patch
Comment 7 Sailesh Agrawal 2011-12-06 11:13:12 PST
(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.
Comment 9 Beth Dakin 2011-12-06 11:28:15 PST
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.
Comment 10 James Robinson 2011-12-06 11:36:35 PST
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?
Comment 11 Sailesh Agrawal 2011-12-06 11:36:48 PST
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
Comment 12 Beth Dakin 2011-12-06 13:41:59 PST
(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…
Comment 13 Sailesh Agrawal 2011-12-06 13:45:57 PST
> 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
Comment 14 asvitkine 2011-12-06 13:59:23 PST
> 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).
Comment 15 Sailesh Agrawal 2011-12-07 11:07:26 PST
Created attachment 118235 [details]
Patch
Comment 16 Sailesh Agrawal 2011-12-07 11:08:48 PST
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!
Comment 17 WebKit Review Bot 2011-12-07 11:35:27 PST
Comment on attachment 118235 [details]
Patch

Attachment 118235 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10767147
Comment 18 Sailesh Agrawal 2011-12-14 13:40:38 PST
Ping!
Hey Beth, could you take another look?
The cr-linux failure looks like a flake. Thanks!
Comment 19 Beth Dakin 2011-12-14 18:00:26 PST
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!
Comment 20 WebKit Review Bot 2011-12-14 18:08:05 PST
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
Comment 21 Sailesh Agrawal 2011-12-19 16:29:58 PST
Created attachment 119944 [details]
Patch
Comment 22 Sailesh Agrawal 2011-12-19 16:52:51 PST
Created attachment 119952 [details]
Patch
Comment 23 Sailesh Agrawal 2011-12-19 16:56:00 PST
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.
Comment 24 Nico Weber 2011-12-19 16:57:05 PST
Comment on attachment 119952 [details]
Patch

you already had an r+
Comment 25 WebKit Review Bot 2011-12-19 18:01:32 PST
Comment on attachment 119952 [details]
Patch

Clearing flags on attachment: 119952

Committed r103291: <http://trac.webkit.org/changeset/103291>
Comment 26 WebKit Review Bot 2011-12-19 18:01:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Sailesh Agrawal 2011-12-20 10:18:13 PST
Reverted because this patch broke fast/events/touch/gesture/gesture-scroll.html for Chromium on Mac 10.5.
Comment 28 Sailesh Agrawal 2011-12-20 11:12:27 PST
Created attachment 120046 [details]
Patch
Comment 29 Sailesh Agrawal 2011-12-20 11:15:13 PST
Fixed the 10.5 regression by reapply fix for bug 66577.
Comment 30 WebKit Review Bot 2011-12-20 15:14:15 PST
Comment on attachment 120046 [details]
Patch

Clearing flags on attachment: 120046

Committed r103354: <http://trac.webkit.org/changeset/103354>
Comment 31 WebKit Review Bot 2011-12-20 15:14:22 PST
All reviewed patches have been landed.  Closing bug.
Comment 32 Simon Fraser (smfr) 2011-12-20 15:57:15 PST
This broke the build.
Comment 33 Sailesh Agrawal 2011-12-20 16:26:25 PST
rniwa checked in the following change to fix the build:
http://trac.webkit.org/changeset/103363