Bug 45922 – Remove scroll and clip layers for WKCACFLayerRenderer

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

The scroll and clip layers in WKCACFLayerRenderer are redundant now that RenderLayerCompositor has the same things (added for iframes, but also usable for Windows).

[Simon Fraser (smfr)]

Created attachment 67874 [details]
Patch

[WebKit Review Bot]

Attachment 67874 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/win/WebView.cpp:2096:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Marrin]

Comment on attachment 67874 [details]
Patch

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

> WebCore/page/Frame.cpp:883
> +    GraphicsLayer* rootLayer = compositor->layerTreeAsTextRootLayer();

The name of this call is very confusing. I know you're trying to say that this is the root layer you would use when calling layerTreeAsText, but that seems awfully specific. Why not just combine the calls, naming it something like 'rootLayerTreeAsText()' and have it return a string?

[Adam Roben (:aroben)]

Comment on attachment 67874 [details]
Patch

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

> WebCore/rendering/RenderLayerCompositor.cpp:1298
>      // FIXME: eventually we should do this on other platforms too.
>      if (!m_renderView->frameView()->platformWidget())
>          return true;
> -#endif
> +#endif // PLATFORM(MAC)
> +
>      return false;
> +#endif // PLATFORM(WIN)

Is Win vs. Mac the right distinction here? Why isn't the !m_renderView->frameView()->platformWidget() check sufficient?

[Simon Fraser (smfr)]

(In reply to comment #4)
> Is Win vs. Mac the right distinction here? Why isn't the !m_renderView->frameView()->platformWidget() check sufficient?

On Windows we want to always return true.
On Mac we want to return true for iframes in WebKit1, and in WebKit2.
I think other platforms would be like windows.

[Adam Roben (:aroben)]

(In reply to comment #5)
> (In reply to comment #4)
> > Is Win vs. Mac the right distinction here? Why isn't the !m_renderView->frameView()->platformWidget() check sufficient?
> 
> On Windows we want to always return true.
> On Mac we want to return true for iframes in WebKit1, and in WebKit2.
> I think other platforms would be like windows.

Right. What I mean is that we want to do those things due to other choices that have been made on each platform (e.g., Mac returns false for iframes in WebKit1 because it uses NSScrollViews for them (I think)), not because of something intrinsic to each platform. So it seems inappropriate to be using PLATFORM macros. We should instead be detecting whatever the choices were that we've made that require us to return a specific value.

I.e., is there a way we can write this so that if we somehow were to change WebKit1 on Mac not to use NSScrollViews for iframes, the code would do the right thing automatically? And vice-versa for Windows?

[Simon Fraser (smfr)]

Created attachment 67927 [details]
Patch

[WebKit Review Bot]

Attachment 67927 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/win/WebView.cpp:2096:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Marrin]

Comment on attachment 67927 [details]
Patch

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

r=me with style fix

> WebKit/win/WebView.cpp:2096
> +            if (lParam != 0)

Style bot complained about this. Should be if (lParam)

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/67735

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/67735

[Adam Roben (:aroben)]

Comment on attachment 67927 [details]
Patch

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

> WebCore/rendering/RenderLayerCompositor.cpp:1299
> +    // We need to handle our own scrolling if we're:
> +    return !m_renderView->frameView()->platformWidget() // viewless (i.e. non-Mac, or Mac in WebKit2)
> +        || attachment == RootLayerAttachedViaEnclosingIframe; // a composited iframe on Mac

Yay!