Refactor paintOverhangAreas to allow non-Mac Chromium platforms to reuse code
Created attachment 108380 [details] Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=108380&action=review > Source/WebCore/platform/chromium/ScrollbarThemeChromium.cpp:47 > + if (!initialized && PlatformSupport::enableScrollOverhang()) { So we never set |initialized| to true if PlatformSupport::enableScrollOverhang() always returns false? Maybe you want an inner if statement here.. > Source/WebCore/platform/chromium/ScrollbarThemeChromium.cpp:63 > + if (!PlatformSupport::enableScrollOverhang()) { Can you just call the super class version of this method, in this case? > Source/WebCore/rendering/RenderLayer.cpp:-1717 > - return m_scrollOrigin + m_scrollSize - visibleContentRect(true).size(); Can you give some context on this change? Why is it not needed on the Mac for rubber banding?
> > Source/WebCore/rendering/RenderLayer.cpp:-1717 > > - return m_scrollOrigin + m_scrollSize - visibleContentRect(true).size(); > > Can you give some context on this change? Why is it not needed on the Mac for rubber banding? This is a bug fix to a problem I encountered while scrolling beyond the edge of the page. This change shouldn't be in this patch. I'll remove it. I've addressed the other two points you mentioned as well. I'm uploading a new patch now.
Created attachment 108385 [details] Patch
(In reply to comment #4) > Created an attachment (id=108385) [details] > Patch This patch must land first on the chromium side: http://codereview.chromium.org/8003001/
This is only for non-accelerated pages, right? See https://bugs.webkit.org/show_bug.cgi?id=66969 . It's weird that the hw part is gated on a compile-time guard but the software part is gated on a PlatformSupport function call.
Nico is right. Instead of a platform check, can you base it on the state of ENABLE(RUBBER_BANDING)?
Done. (In reply to comment #7) > Nico is right. Instead of a platform check, can you base it on the state of ENABLE(RUBBER_BANDING)?
Created attachment 108398 [details] Patch
Comment on attachment 108398 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108398&action=review > Source/WebCore/platform/chromium/ScrollbarThemeChromium.cpp:62 > + // Does this platform support overhang? useless comment > Source/WebCore/platform/chromium/ScrollbarThemeChromium.cpp:63 > +#if ENABLE(RUBBER_BANDING) Shouldn't this be `#if !ENABLE(RUBBER_BANDING`?
> Source/WebCore/platform/chromium/ScrollbarThemeChromium.cpp:64 > + ScrollbarThemeComposite::paintOverhangAreas(view, context, horizontalOverhangRect, verticalOverhangRect, dirtyRect); This check is reversed. Instead of doing this, can you put the whole method inside an ENABLE(RUBBER_BANDING) block (and in the header also). That way, if its not enabled, the superclass version will be used already. > Source/WebCore/platform/chromium/ScrollbarThemeChromium.h:62 > + RefPtr<Pattern> m_overhangPattern; Surround this with #if ENABLE(RUBBER_BANDING)?
Created attachment 108409 [details] Patch
Comment on attachment 108409 [details] Patch Looks good to me. I haven't looked to closely on the code that's been moved around, I assume you didn't change it.
(In reply to comment #13) > (From update of attachment 108409 [details]) > Looks good to me. I haven't looked to closely on the code that's been moved around, I assume you didn't change it. Just style changes according to the style checker. The code is the same.
dglazkov: Can you honor this with an r+ please?
Comment on attachment 108409 [details] Patch Should this be USE_RUBBER_BANDING?
Adam ^^^?
Ah, I see -- Fady tells me that rubber-banding is an existing define. Nevermind :)
Comment on attachment 108409 [details] Patch Rejecting attachment 108409 [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: o-playing-and-pause.html = MISSING PASS Regressions: Unexpected image and text mismatch : (1) svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT Regressions: Unexpected image mismatch : (5) fast/text/atsui-multiple-renderers.html = IMAGE fast/text/international/danda-space.html = IMAGE fast/text/international/thai-baht-space.html = IMAGE fast/text/international/thai-line-breaks.html = IMAGE platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE Full output: http://queues.webkit.org/results/9826003
Comment on attachment 108409 [details] Patch I suspect some of these tests are flaky. Will try again, if it fails again, I will investigate further.
Comment on attachment 108409 [details] Patch Clearing flags on attachment: 108409 Committed r95860: <http://trac.webkit.org/changeset/95860>
All reviewed patches have been landed. Closing bug.
This change caused the following tests to have pixel diffs on the mac: fast/events/scale-and-scroll-iframe-body.html fast/events/scale-and-scroll-iframe-window.html platform/chromium/fast/repaint/fixed-layout-360x240.html The overhang area is no longer being painted. On one hand, the claim was that this CL should have no visible effect. On the other hand, the new results are closed to the non-Chromium baselines. Any preference?
(In reply to comment #23) > This change caused the following tests to have pixel diffs on the mac: > > fast/events/scale-and-scroll-iframe-body.html > fast/events/scale-and-scroll-iframe-window.html > platform/chromium/fast/repaint/fixed-layout-360x240.html > > The overhang area is no longer being painted. On one hand, the claim was that this CL should have no visible effect. On the other hand, the new results are closed to the non-Chromium baselines. Any preference? Ah, it's because you moved the function to ScrollbarThemeChromium.cpp, but that is excluded from the Mac build: http://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/WebCore.gyp/WebCore.gyp&q=scrollbarthemechromium%20file:gyp&exact_package=chromium&l=1468 I'm going to roll back this change.
Reverted r95860 for reason: Breaks overhang rendering on Chromium Mac Committed r95889: <http://trac.webkit.org/changeset/95889>
ScrollbarThemeChromium is pretty small, so it might be okay to inject into the inheritance hierarchy between ScrollbarThemeChromiumMac and ScrollbarThemeComposite. I think ScrollbarThemeChromiumMac will override all the interesting bits anyway. The other possibility is to move overhang painting into a separate class - e.g. ScrollOverhangPainterChromium.cpp which we can use from both ScrollbarThemeChromiumMac and ScrollbarThemeChromium. The changes for that would be more substantial, but it might be a better design long-term (e.g. if we decide to have different such painters).
(In reply to comment #26) > ScrollbarThemeChromium is pretty small, so it might be okay to inject into the inheritance hierarchy between ScrollbarThemeChromiumMac and ScrollbarThemeComposite. I think ScrollbarThemeChromiumMac will override all the interesting bits anyway. > > The other possibility is to move overhang painting into a separate class - e.g. ScrollOverhangPainterChromium.cpp which we can use from both ScrollbarThemeChromiumMac and ScrollbarThemeChromium. The changes for that would be more substantial, but it might be a better design long-term (e.g. if we decide to have different such painters). Sorry about the breakage, I assumed an inheritance relationship for ScrollbarThemeChromiumMac without verifying. Thank you Mihai for the revert! I like the idea of moving overhang painting code out of the ScrollTheme code. I'll look into doing that. Thanks.