Bug 34779 - Need CA_PRINT_TREE functionality on Windows
Summary: Need CA_PRINT_TREE functionality on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Chris Marrin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-09 17:30 PST by Chris Marrin
Modified: 2010-02-10 16:26 PST (History)
0 users

See Also:


Attachments
Patch (9.05 KB, patch)
2010-02-09 17:36 PST, Chris Marrin
darin: review+
Details | Formatted Diff | Diff
Replacement patch (10.14 KB, patch)
2010-02-10 15:18 PST, Chris Marrin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2010-02-09 17:30:58 PST
CACF does not provide a useable form of this debugging tool. So we need to implement it ourselves to track down some of the layer structure bugs.
Comment 1 Chris Marrin 2010-02-09 17:36:18 PST
Created attachment 48453 [details]
Patch
Comment 2 Darin Adler 2010-02-10 10:16:49 PST
Comment on attachment 48453 [details]
Patch

> +    CFArrayRef sublayers = CACFLayerGetSublayers(layer());
> +    if (CFArrayGetCount(sublayers) <= index)
> +        return 0;

If we're checking the range of index and returning 0 rather than calling CFArrayGetValueAtIndex, then suggest we check for negative numbers too since the index is signed.

> +    return layer(reinterpret_cast<CACFLayerRef>(const_cast<void*>(CFArrayGetValueAtIndex(sublayers, index))));

This should be static_cast instead of reinterpret_cast since it's a cast from void*.

> -    bool isTransformLayer() const { return CACFLayerGetClass(layer()) == kCACFTransformLayer; }
> +    bool isTransformLayer() const;

Why this change? Is there a problem with it being inlined?

r=me
Comment 3 Chris Marrin 2010-02-10 11:09:04 PST
(In reply to comment #2)
> (From update of attachment 48453 [details])
> > +    CFArrayRef sublayers = CACFLayerGetSublayers(layer());
> > +    if (CFArrayGetCount(sublayers) <= index)
> > +        return 0;
> 
> If we're checking the range of index and returning 0 rather than calling
> CFArrayGetValueAtIndex, then suggest we check for negative numbers too since
> the index is signed.

I'll fix it.

> 
> > +    return layer(reinterpret_cast<CACFLayerRef>(const_cast<void*>(CFArrayGetValueAtIndex(sublayers, index))));
> 
> This should be static_cast instead of reinterpret_cast since it's a cast from
> void*.

Will do.

> 
> > -    bool isTransformLayer() const { return CACFLayerGetClass(layer()) == kCACFTransformLayer; }
> > +    bool isTransformLayer() const;
> 
> Why this change? Is there a problem with it being inlined?

I had to change it to use the function call version of the enum since the DLL is delay loaded. Using that function can only be done in the cpp file. I will make a note of that in the changelog

> 
> r=me
Comment 4 Chris Marrin 2010-02-10 15:18:52 PST
Created attachment 48522 [details]
Replacement patch

I realized that it's a bad idea to be calling getenv() on each render to get CA_PRINT_TREE. So I now cache it.
Comment 5 Simon Fraser (smfr) 2010-02-10 15:46:20 PST
Comment on attachment 48522 [details]
Replacement patch

>  void WKCACFLayerRenderer::renderSoon()
> Index: WebCore/platform/graphics/win/WKCACFLayerRenderer.h
> ===================================================================
> --- WebCore/platform/graphics/win/WKCACFLayerRenderer.h	(revision 54620)
> +++ WebCore/platform/graphics/win/WKCACFLayerRenderer.h	(working copy)
> @@ -97,6 +97,10 @@ private:
>      HWND m_hostWindow;
>      Timer<WKCACFLayerRenderer> m_renderTimer;
>      IntRect m_scrollFrame;
> +
> +#ifndef NDEBUG
> +    bool m_cachedPrintTreeFlag;
> +#endif

I don't think the name needs to include 'cached' or 'flag'; both are obvious.
m_printTree is fine.

This stuff is OK, but I'd still like to see a platform-independent layer dump
we can use in DRT.
Comment 6 Chris Marrin 2010-02-10 16:26:13 PST
Landed in http://trac.webkit.org/changeset/54630