Summary: | Add Layer Tree output capability to DRT | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Marrin <cmarrin> | ||||||
Component: | Tools / Tests | Assignee: | Chris Marrin <cmarrin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | eric, simon.fraser, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Chris Marrin
2010-03-29 14:23:53 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.
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 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? Created attachment 52212 [details]
Replacement patch
This makes changes from Simon's review and adds Windows side of patch.
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 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 Landed in http://trac.webkit.org/changeset/56934 http://trac.webkit.org/changeset/56934 appears to have broken Chromium Win Release, Chromium Mac Release, and Chromium Linux Release 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. :( |