Bug 36782 - Add Layer Tree output capability to DRT
Summary: Add Layer Tree output capability to DRT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (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-03-29 14:23 PDT by Chris Marrin
Modified: 2010-04-01 14:59 PDT (History)
3 users (show)

See Also:


Attachments
Patch with test case (22.01 KB, patch)
2010-03-29 20:33 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Replacement patch (25.91 KB, patch)
2010-03-31 14:36 PDT, 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-03-29 14:23:53 PDT
Today, it's difficult to do meaningful tests of composited content. There is no way to dump the render tree used for composited content (the GraphicsLayers) like there is for non-composited content. Adding this would give us much better test coverage.

Steps to implement:

1) Implement cross-platform dump format output for the GraphicsLayer tree.
2) Add a layerTreeAsText call to DRT to get this formatted output (with all the plumbing through the system)
3) Add initial tests to use the functionality

This gets us most of the way there, but leaves off one important testing area. Under the GraphicsLayers are platform specific objects which are used to do the actual rendering. On Mac this is based on CALayer, and on Windows it's based on CACFLayer. There are often multiple platform specific layers for each GraphicsLayer, and these layers have their own hierarchy. The GraphicsLayer and platform specific layer hierarchies must stay in sync, but there is currently no consistency checking going on. We should add such checking to the platform specific subclasses of GraphicsLayer (GraphicsLayerCA and GraphicsLayerCACF) and then assert when the checks fail. That way DRT will assert when testing the debug builds and the consistency checking won't get in the way of release performance.
Comment 1 Chris Marrin 2010-03-29 20:33:16 PDT
Created attachment 51997 [details]
Patch with test case

Please do not review this patch yet. It is the Mac side. I need to add the Windows side to it. This is just a placeholder so I can put the patch on Windows and finish the work.
Comment 2 Chris Marrin 2010-03-29 20:35:10 PDT
Actually, please do look the patch over. The general structure and test case are final. I just need to add the WebKit and DRT parts of the plumbing for Windows. Please comment on what's there so far.
Comment 3 Simon Fraser (smfr) 2010-03-29 21:22:26 PDT
Comment on attachment 51997 [details]
Patch with test case

> Index: WebCore/page/Frame.cpp
> ===================================================================

> +String Frame::layerTreeAsText()
> +{
> +    if (!contentRenderer())
> +        return String();
> +
> +    GraphicsLayer* rootLayer = contentRenderer()->compositor()->rootPlatformLayer();
> +    if (!rootLayer)
> +        return String();
> +        
> +    return rootLayer->layerTreeAsText();
> +}

This needs to have #if ENABLE() guards inside it.


> Index: WebCore/page/Frame.h
> ===================================================================

> +        String layerTreeAsText();

This method should be |const|.

> Index: WebCore/platform/graphics/GraphicsLayer.cpp
> ===================================================================

> -void GraphicsLayer::dumpLayer(TextStream& ts, int indent) const
> +void GraphicsLayer::dumpLayer(TextStream& ts, int indent, bool debug) const
>  {
>      writeIndent(ts, indent);
> -    ts << "(" << "GraphicsLayer" << " " << static_cast<void*>(const_cast<GraphicsLayer*>(this));
> -    ts << " \"" << m_name << "\"\n";
> -    dumpProperties(ts, indent);
> +    if (debug)
> +        ts << "(" << "GraphicsLayer" << " " << static_cast<void*>(const_cast<GraphicsLayer*>(this));
> +    else
> +        ts << "(" << "GraphicsLayer";

I'd prefer this as:

  ts << "(" << "GraphicsLayer";
  if (debug)
    ts << " " << static_cast<void*>(const_cast<GraphicsLayer*>(this));

> -void GraphicsLayer::dumpProperties(TextStream& ts, int indent) const
> +void GraphicsLayer::dumpProperties(TextStream& ts, int indent, bool debug) const

> +    if (debug) {
> +        if (m_client)
> +            ts << static_cast<void*>(m_client);
> +        else
> +            ts << "none";

Better as "(no client)" I think.

> +        m_replicaLayer->dumpLayer(ts, indent+2, debug);

Spaces around the "+" please.

> -            m_children[i]->dumpLayer(ts, indent+2);
> +            m_children[i]->dumpLayer(ts, indent+2, debug);

Ditto.

> Index: WebCore/platform/graphics/GraphicsLayer.h
> ===================================================================
> +    void dumpLayer(TextStream&, int indent = 0, bool debug = false) const;

I can imagine that we want finer granularity than "debug"/"not debug" in the output in future. I'd like to see this modeled after RenderAsTextBehavior in RenderTreeAsText.h. That also has the advantage of making the callsite easier to read (no mysterious "true" or "false").

Another nice addition would be a debug-only method that can be called from gdb, just like showLayerTree() in RenderObject.h.


> Index: LayoutTests/compositing/geometry/preserve-3d-switching.html
> ===================================================================

I think this test should be committed separately. It would also be a little neater if the test dumped the layer output into a <pre> element.


> Index: LayoutTests/platform/mac/compositing/geometry/preserve-3d-switching-expected.txt
> ===================================================================
> +  (position 0.00 0.00)
> +  (anchor 0.50 0.50)
> +  (bounds 800.00 600.00)
> +  (opacity 1.00)
> +  (usingTiledLayer 0)
> +  (m_preserves3D 0)

Odd that this has m_

> +  (drawsContent 0)
> +  (m_backfaceVisibility visible)

Ditto. Maybe just avoid dumping properties that have default values.

> +    (GraphicsLayer "CALayer(0x105216350) GraphicsLayer(0x105215fe0) Document Node"

This looks like debug output, which should never end up in a test result.

> +          (childrenTransform [1.00 0.00 0.00 0.00] [0.00 1.00 0.00 0.00] [0.00 0.00 1.00 -0.00] [0.00 0.00 0.00 1.00])

What is the story with outputting floating point. Can we make sure we don't get precision issues between platforms?
Comment 4 Chris Marrin 2010-03-31 14:36:42 PDT
Created attachment 52212 [details]
Replacement patch

This makes changes from Simon's review and adds Windows side of patch.
Comment 5 WebKit Review Bot 2010-03-31 14:41:37 PDT
Attachment 52212 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/GraphicsLayer.cpp:395:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebCore/platform/graphics/GraphicsLayer.cpp:489:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Simon Fraser (smfr) 2010-03-31 14:47:22 PDT
Comment on attachment 52212 [details]
Replacement patch

> Index: WebCore/page/Frame.cpp
> ===================================================================

> +#if USE(ACCELERATED_COMPOSITING)
> +String Frame::layerTreeAsText() const

This should be inside the method, so that the method is still present in non-accel-comp builds (which avoids breaking the exports).

The method can return an empty string if ACCELERATED_COMPOSITING is off.

> Index: WebCore/page/Frame.h
> ===================================================================

> +#if USE(ACCELERATED_COMPOSITING)
> +        String layerTreeAsText() const;
> +#endif

This would not be wrapped here.

> Index: WebCore/platform/graphics/GraphicsLayer.cpp
> ===================================================================

> +#ifndef NDEBUG
> +void showGraphicsLayerTree(const WebCore::GraphicsLayer* layer)
> +{
> +    if (!layer)
> +        return;
> +
> +    WebCore::String output = layer->layerTreeAsText(LayerTreeAsTextDebug);

Doesn't LayerTreeAsTextDebug needs to be WebCore::LayerTreeAsTextDebug ?

> Index: WebKit/win/WebFrame.cpp
> ===================================================================

> +#if USE(ACCELERATED_COMPOSITING)
> +    String text = frame->layerTreeAsText();
> +#else
> +    String text;
> +#endif

You wouldn't need the #ifdef here.

r=me
Comment 7 Chris Marrin 2010-04-01 13:16:20 PDT
Landed in http://trac.webkit.org/changeset/56934
Comment 8 WebKit Review Bot 2010-04-01 14:05:18 PDT
http://trac.webkit.org/changeset/56934 appears to have broken Chromium Win Release, Chromium Mac Release, and Chromium Linux Release
Comment 9 Eric Seidel (no email) 2010-04-01 14:59:17 PDT
Looks like those were just the first builders to break.  Looks like basically all builders broke from this change.

Sad that this got an r+ when the EWS had warned that this broke builds. :(