Summary: | Merge ScrollAnimatorChromiumMac.mm back to ScrollAnimatorMac | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sailesh Agrawal <sail> | ||||||||||||||
Component: | New Bugs | Assignee: | 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
Sailesh Agrawal
2011-05-19 14:31:03 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. > 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.
Created attachment 118000 [details]
Patch
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. Created attachment 118074 [details]
Patch
(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. 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 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. 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? 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 (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… > 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).
Created attachment 118235 [details]
Patch
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 on attachment 118235 [details] Patch Attachment 118235 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10767147 Ping! Hey Beth, could you take another look? The cr-linux failure looks like a flake. Thanks! 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 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 Created attachment 119944 [details]
Patch
Created attachment 119952 [details]
Patch
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 on attachment 119952 [details]
Patch
you already had an r+
Comment on attachment 119952 [details] Patch Clearing flags on attachment: 119952 Committed r103291: <http://trac.webkit.org/changeset/103291> All reviewed patches have been landed. Closing bug. Reverted because this patch broke fast/events/touch/gesture/gesture-scroll.html for Chromium on Mac 10.5. Created attachment 120046 [details]
Patch
Fixed the 10.5 regression by reapply fix for bug 66577. Comment on attachment 120046 [details] Patch Clearing flags on attachment: 120046 Committed r103354: <http://trac.webkit.org/changeset/103354> All reviewed patches have been landed. Closing bug. This broke the build. rniwa checked in the following change to fix the build: http://trac.webkit.org/changeset/103363 |