Bug 45922 - Remove scroll and clip layers for WKCACFLayerRenderer
Summary: Remove scroll and clip layers for WKCACFLayerRenderer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on: 45931
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-16 15:12 PDT by Simon Fraser (smfr)
Modified: 2010-09-20 06:46 PDT (History)
3 users (show)

See Also:


Attachments
Patch (15.80 KB, patch)
2010-09-16 18:30 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (16.92 KB, patch)
2010-09-17 11:45 PDT, Simon Fraser (smfr)
cmarrin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-09-16 15:12:41 PDT
The scroll and clip layers in WKCACFLayerRenderer are redundant now that RenderLayerCompositor has the same things (added for iframes, but also usable for Windows).
Comment 1 Simon Fraser (smfr) 2010-09-16 18:30:39 PDT
Created attachment 67874 [details]
Patch
Comment 2 WebKit Review Bot 2010-09-16 18:33:41 PDT
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.
Comment 3 Chris Marrin 2010-09-17 04:53:44 PDT
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?
Comment 4 Adam Roben (:aroben) 2010-09-17 06:04:52 PDT
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?
Comment 5 Simon Fraser (smfr) 2010-09-17 08:49:18 PDT
(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.
Comment 6 Adam Roben (:aroben) 2010-09-17 08:52:25 PDT
(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?
Comment 7 Simon Fraser (smfr) 2010-09-17 11:45:45 PDT
Created attachment 67927 [details]
Patch
Comment 8 WebKit Review Bot 2010-09-17 11:47:03 PDT
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.
Comment 9 Chris Marrin 2010-09-17 11:56:08 PDT
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)
Comment 10 Simon Fraser (smfr) 2010-09-17 12:01:17 PDT
http://trac.webkit.org/changeset/67735
Comment 11 Simon Fraser (smfr) 2010-09-17 12:02:16 PDT
http://trac.webkit.org/changeset/67735
Comment 12 Adam Roben (:aroben) 2010-09-20 06:46:08 PDT
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!