Bug 68648 – Refactor paintOverhangAreas to allow non-Mac Chromium platforms to reuse code

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Fady Samuel]

Refactor paintOverhangAreas to allow non-Mac Chromium platforms to reuse code

[Fady Samuel]

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?

[Fady Samuel]

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

[Fady Samuel]

Created attachment 108385 [details]
Patch

[Fady Samuel]

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

[Nico Weber]

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

[Fady Samuel]

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

[Fady Samuel]

Created attachment 108398 [details]
Patch

[Nico Weber]

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

[Fady Samuel]

Created attachment 108409 [details]
Patch

[Nico Weber]

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.

[Fady Samuel]

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

[Nico Weber]

dglazkov: Can you honor this with an r+ please?

[Dimitri Glazkov (Google)]

Comment on attachment 108409 [details]
Patch

Should this be USE_RUBBER_BANDING?

[Dimitri Glazkov (Google)]

Adam ^^^?

[Dimitri Glazkov (Google)]

Ah, I see -- Fady tells me that rubber-banding is an existing define. Nevermind :)

[WebKit Review Bot]

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

[Fady Samuel]

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.

[WebKit Review Bot]

Comment on attachment 108409 [details]
Patch

Clearing flags on attachment: 108409

Committed r95860: <http://trac.webkit.org/changeset/95860>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mihai Parparita]

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?

[Mihai Parparita]

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

[Mihai Parparita]

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

[Fady Samuel]

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