| Summary: | Content in <iframe> appearing in upper-left corner of WebView on http://webkit.org/blog/386/3d-transforms/ | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Roben (:aroben) <aroben> | ||||||||
| Component: | Layout and Rendering | Assignee: | Chris Marrin <cmarrin> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | cmarrin, dpranke, simon.fraser, webkit.review.bot | ||||||||
| Priority: | P2 | Keywords: | InRadar, PlatformOnly | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows 7 | ||||||||||
| URL: | http://webkit.org/blog/386/3d-transforms/ | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Adam Roben (:aroben)
2009-12-11 14:00:48 PST
Created attachment 54815 [details]
Patch
Attachment 54815 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/win/ChangeLog:5: Line contains tab character. [whitespace/tab] [5]
WebKit/win/ChangeLog:7: Line contains tab character. [whitespace/tab] [5]
WebKit/win/ChangeLog:8: Line contains tab character. [whitespace/tab] [5]
WebKit/win/ChangeLog:9: Line contains tab character. [whitespace/tab] [5]
WebCore/ChangeLog:5: Line contains tab character. [whitespace/tab] [5]
WebCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5]
WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5]
WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5]
WebCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5]
WebCore/ChangeLog:11: Line contains tab character. [whitespace/tab] [5]
WebCore/ChangeLog:12: Line contains tab character. [whitespace/tab] [5]
WebCore/ChangeLog:13: Line contains tab character. [whitespace/tab] [5]
WebCore/ChangeLog:14: Line contains tab character. [whitespace/tab] [5]
WebCore/ChangeLog:15: Line contains tab character. [whitespace/tab] [5]
WebKit/win/WebView.cpp:6096: Tab found; better to use spaces [whitespace/tab] [1]
WebKit/win/WebView.cpp:6097: Tab found; better to use spaces [whitespace/tab] [1]
WebKit/win/WebView.h:886: Tab found; better to use spaces [whitespace/tab] [1]
WebCore/rendering/RenderLayerBacking.cpp:240: Tab found; better to use spaces [whitespace/tab] [1]
WebCore/rendering/RenderLayerBacking.cpp:241: Tab found; better to use spaces [whitespace/tab] [1]
WebCore/rendering/RenderLayerBacking.cpp:242: Tab found; better to use spaces [whitespace/tab] [1]
WebCore/rendering/RenderLayerBacking.cpp:244: Tab found; better to use spaces [whitespace/tab] [1]
WebCore/rendering/RenderLayerBacking.cpp:245: Tab found; better to use spaces [whitespace/tab] [1]
Total errors found: 22 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 54815 [details]
Patch
This patch has tabs.
I'd like to see testcase with an iframe having borders and padding (with pixel results for Mac):
<iframe style="border: 10px inset; padding: 15px src="composited.html>
Created attachment 55034 [details]
Replacement Patch
Comment on attachment 55034 [details] Replacement Patch > Index: WebCore/ChangeLog > =================================================================== > + Made composited iframes work on Windows > + https://bugs.webkit.org/show_bug.cgi?id=32446 > + > + This builds on Simon's work to create compositing I'm flattered that you named me, but it would be more useful for someone reading this in the future to either give a bug number, or svn revision for the relevant changes. > Index: WebCore/platform/graphics/GraphicsLayer.cpp > =================================================================== > writeIndent(ts, indent); > ts << "(" << "GraphicsLayer"; > > - if (behavior & LayerTreeAsTextDebug) { > + if (behavior & LayerTreeAsTextDebug) > ts << " " << static_cast<void*>(const_cast<GraphicsLayer*>(this)); > - ts << " \"" << m_name << "\""; > - } > + > + ts << " \"" << m_name << "\""; > > ts << "\n"; > dumpProperties(ts, indent, behavior); > @@ -418,33 +418,8 @@ void GraphicsLayer::dumpProperties(TextS > ts << "(anchor " << m_anchorPoint.x() << " " << m_anchorPoint.y() << ")\n"; > > writeIndent(ts, indent + 1); > - ts << "(bounds " << m_size.width() << " " << m_size.height() << ")\n"; > - > - writeIndent(ts, indent + 1); > - ts << "(opacity " << m_opacity << ")\n"; > - > - writeIndent(ts, indent + 1); > - ts << "(usingTiledLayer " << m_usingTiledLayer << ")\n"; > - > - writeIndent(ts, indent + 1); > - ts << "(preserves3D " << m_preserves3D << ")\n"; > - > - writeIndent(ts, indent + 1); > - ts << "(drawsContent " << m_drawsContent << ")\n"; > - > - writeIndent(ts, indent + 1); > ts << "(backfaceVisibility " << (m_backfaceVisibility ? "visible" : "hidden") << ")\n"; > > - if (behavior & LayerTreeAsTextDebug) { > - writeIndent(ts, indent + 1); > - ts << "("; > - if (m_client) > - ts << "client " << static_cast<void*>(m_client); > - else > - ts << "no client"; > - ts << ")\n"; > - } > - > writeIndent(ts, indent + 1); > ts << "(backgroundColor "; > if (!m_backgroundColorSet) > @@ -454,16 +429,7 @@ void GraphicsLayer::dumpProperties(TextS > ts << ")\n"; > > writeIndent(ts, indent + 1); > - ts << "(transform "; > - if (m_transform.isIdentity()) > - ts << "identity"; > - else { > - ts << "[" << m_transform.m11() << " " << m_transform.m12() << " " << m_transform.m13() << " " << m_transform.m14() << "] "; > - ts << "[" << m_transform.m21() << " " << m_transform.m22() << " " << m_transform.m23() << " " << m_transform.m24() << "] "; > - ts << "[" << m_transform.m31() << " " << m_transform.m32() << " " << m_transform.m33() << " " << m_transform.m34() << "] "; > - ts << "[" << m_transform.m41() << " " << m_transform.m42() << " " << m_transform.m43() << " " << m_transform.m44() << "]"; > - } > - ts << ")\n"; > + ts << "(bounds " << m_size.width() << " " << m_size.height() << ")\n"; > > writeIndent(ts, indent + 1); > ts << "(childrenTransform "; > @@ -477,6 +443,18 @@ void GraphicsLayer::dumpProperties(TextS > } > ts << ")\n"; > > + writeIndent(ts, indent + 1); > + ts << "(drawsContent " << m_drawsContent << ")\n"; > + > + writeIndent(ts, indent + 1); > + ts << "(masksToBounds " << m_masksToBounds << ")\n"; > + > + writeIndent(ts, indent + 1); > + ts << "(opacity " << m_opacity << ")\n"; > + > + writeIndent(ts, indent + 1); > + ts << "(preserves3D " << m_preserves3D << ")\n"; > + > if (m_replicaLayer) { > writeIndent(ts, indent + 1); > ts << "(replica layer"; > @@ -494,6 +472,31 @@ void GraphicsLayer::dumpProperties(TextS > ts << ")\n"; > } > > + writeIndent(ts, indent + 1); > + ts << "(transform "; > + if (m_transform.isIdentity()) > + ts << "identity"; > + else { > + ts << "[" << m_transform.m11() << " " << m_transform.m12() << " " << m_transform.m13() << " " << m_transform.m14() << "] "; > + ts << "[" << m_transform.m21() << " " << m_transform.m22() << " " << m_transform.m23() << " " << m_transform.m24() << "] "; > + ts << "[" << m_transform.m31() << " " << m_transform.m32() << " " << m_transform.m33() << " " << m_transform.m34() << "] "; > + ts << "[" << m_transform.m41() << " " << m_transform.m42() << " " << m_transform.m43() << " " << m_transform.m44() << "]"; > + } > + ts << ")\n"; > + > + writeIndent(ts, indent + 1); > + ts << "(usingTiledLayer " << m_usingTiledLayer << ")\n"; > + > + if (behavior & LayerTreeAsTextDebug) { > + writeIndent(ts, indent + 1); > + ts << "("; > + if (m_client) > + ts << "client " << static_cast<void*>(m_client); > + else > + ts << "no client"; > + ts << ")\n"; > + } > + > if (m_children.size()) { > writeIndent(ts, indent + 1); > ts << "(children " << m_children.size() << "\n"; These changes should be done in a separate patch. > Index: WebCore/platform/graphics/win/GraphicsLayerCACF.cpp > =================================================================== > void GraphicsLayerCACF::setName(const String& name) > { > String longName = String::format("CALayer(%p) GraphicsLayer(%p) ", m_layer.get(), this) + name; > - GraphicsLayer::setName(longName); > + GraphicsLayer::setName(name); > > m_layer->setName(longName); The name-related changes should be done in a third patch. > Index: WebCore/rendering/RenderLayerBacking.cpp > =================================================================== > -#ifndef NDEBUG > if (renderer()->node()) { > if (renderer()->node()->isDocumentNode()) > m_graphicsLayer->setName("Document Node"); > @@ -107,7 +107,6 @@ void RenderLayerBacking::createGraphicsL > m_graphicsLayer->setName("Reflection"); > else > m_graphicsLayer->setName("Anonymous Node"); > -#endif // NDEBUG More name-related changes here; should be done in a third patch. > m_graphicsLayer->setContentsRect(contentsBox()); > m_graphicsLayer->setDrawsContent(containsPaintedContent()); > + > + // If this is an iframe parent, update the iframe content's box > + if (renderer()->isRenderIFrame()) { > + HTMLIFrameElement* element = static_cast<HTMLIFrameElement*>(renderer()->node()); > + if (Document* contentDocument = element->contentDocument()) { > + if (RenderView* view = contentDocument->renderView()) { > + RenderLayerCompositor* iframeCompositor = view->compositor(); > + iframeCompositor->setContentsBox(contentsBox()); This is only necessary if the iframe contents are composited. I think you should test that here. Maybe add a utility method to get the inner RenderLayerCompositor, given a RenderIFrame. > void RenderLayerBacking::updateInternalHierarchy() > @@ -410,9 +420,7 @@ bool RenderLayerBacking::updateClippingL > if (needsAncestorClip) { > if (!m_ancestorClippingLayer) { > m_ancestorClippingLayer = GraphicsLayer::create(this); > -#ifndef NDEBUG > m_ancestorClippingLayer->setName("Ancestor clipping Layer"); > -#endif > m_ancestorClippingLayer->setMasksToBounds(true); > layersChanged = true; > } > @@ -425,9 +433,7 @@ bool RenderLayerBacking::updateClippingL > if (needsDescendantClip) { > if (!m_clippingLayer) { > m_clippingLayer = GraphicsLayer::create(this); > -#ifndef NDEBUG > m_clippingLayer->setName("Child clipping Layer"); > -#endif > m_clippingLayer->setMasksToBounds(true); > layersChanged = true; > } > @@ -449,9 +455,7 @@ bool RenderLayerBacking::updateForegroun > if (needsForegroundLayer) { > if (!m_foregroundLayer) { > m_foregroundLayer = GraphicsLayer::create(this); > -#ifndef NDEBUG > m_foregroundLayer->setName("Foreground"); > -#endif > m_foregroundLayer->setDrawsContent(true); > m_foregroundLayer->setPaintingPhase(GraphicsLayerPaintForeground); > layerChanged = true; > @@ -474,9 +478,7 @@ bool RenderLayerBacking::updateMaskLayer > if (needsMaskLayer) { > if (!m_maskLayer) { > m_maskLayer = GraphicsLayer::create(this); > -#ifndef NDEBUG > m_maskLayer->setName("Mask"); > -#endif > m_maskLayer->setDrawsContent(true); > m_maskLayer->setPaintingPhase(GraphicsLayerPaintMask); > layerChanged = true; More name-related changes here; should be done in a third patch. > Index: WebCore/rendering/RenderLayerCompositor.cpp > =================================================================== > --- WebCore/rendering/RenderLayerCompositor.cpp (revision 58433) > -void RenderLayerCompositor::parentInRootLayer(RenderLayer* layer) I assume this was unused, but your changelog doesn't mention it. > @@ -764,11 +752,33 @@ void RenderLayerCompositor::rebuildCompo > } > > if (layerBacking) { > - layerBacking->parentForSublayers()->setChildren(layerChildren); > + if (layer->renderer()->isRenderIFrame()) { > + // This is an iframe parent. Make it the parent of the iframe document's root > + layerBacking->parentForSublayers()->removeAllChildren(); Rather than removeAllChildren() followed by addChild(), it's slightly more efficient to just do a setChildren(). > + HTMLIFrameElement* element = static_cast<HTMLIFrameElement*>(layer->renderer()->node()); > + if (Document* contentDocument = element->contentDocument()) { > + if (RenderView* view = contentDocument->renderView()) { > + RenderLayerCompositor* iframeCompositor = view->compositor(); This would be another place to use that utility method. > + iframeCompositor->setContentsBox(layerBacking->contentsBox()); This may be redundant. > +void RenderLayerCompositor::setContentsBox(const IntRect& contentsBox) I don't think the name of this method stands alone. What are the "contents" of a RenderLayerCompositor? What's really happening here is that some outside entity (i.e. the parent iframe) is imposing clipping on the layers owned by this RenderLayerCompositor. That should be reflected in the name. > @@ -930,13 +940,27 @@ void RenderLayerCompositor::didMoveOnscr > if (!m_rootPlatformLayer) > return; > > - Frame* frame = m_renderView->frameView()->frame(); > - Page* page = frame ? frame->page() : 0; > - if (!page) > - return; > + bool attached = false; > + Element* ownerElement = m_renderView->document()->ownerElement(); > + if (ownerElement) { > + RenderObject* renderer = ownerElement->renderer(); > + if (renderer && renderer->isRenderIFrame()) { > + // The layer will get hooked up via RenderLayerBacking::updateGraphicsLayerConfiguration() > + // for the iframe's renderer in the parent document. > + ownerElement->setNeedsStyleRecalc(SyntheticStyleChange); > + attached = true; > + } > + } > > - page->chrome()->client()->attachRootGraphicsLayer(frame, m_rootPlatformLayer.get()); > - m_rootLayerAttached = true; > + if (!attached) { > + Frame* frame = m_renderView->frameView()->frame(); > + Page* page = frame ? frame->page() : 0; > + if (!page) > + return; > + > + page->chrome()->client()->attachRootGraphicsLayer(frame, m_rootPlatformLayer.get()); > + } > + m_rootLayerAttached = true; > } > > void RenderLayerCompositor::willMoveOffscreen() > @@ -944,19 +968,44 @@ void RenderLayerCompositor::willMoveOffs > if (!m_rootPlatformLayer || !m_rootLayerAttached) > return; > > - Frame* frame = m_renderView->frameView()->frame(); > - Page* page = frame ? frame->page() : 0; > - if (!page) > - return; > + bool detached = false; > + Element* ownerElement = m_renderView->document()->ownerElement(); > + if (ownerElement) { > + RenderObject* renderer = ownerElement->renderer(); > + if (renderer->isRenderIFrame()) { > + // The layer will get hooked up via RenderLayerBacking::updateGraphicsLayerConfiguration() > + // for the iframe's renderer in the parent document. > + ownerElement->setNeedsStyleRecalc(SyntheticStyleChange); > + detached = true; Would be nice to share this code in these two methods. > void RenderLayerCompositor::updateRootLayerPosition() > { > - if (m_rootPlatformLayer) > - m_rootPlatformLayer->setSize(FloatSize(m_renderView->rightLayoutOverflow(), m_renderView->bottomLayoutOverflow())); > + if (m_rootPlatformLayer) { > + // FIXME: Adjust the y position of the m_rootPlatformLayer if we are clipping by its top edge > + // Eventually this will be taken care of by scrolling logic > + // https://bugs.webkit.org/show_bug.cgi?id=38518 > + float height = m_renderView->bottomLayoutOverflow(); > + float yOffset = 0; > + > + if (m_clippingLayer && height > m_clippingLayer->size().height()) > + yOffset = m_clippingLayer->size().height() - height; I don't get why we need to do this Y flipping. We don't have Y-flipping code for other "normal" GraphicsLayers, so why do we need it here? It it related to this? > // The root layer does flipping if we need it on this platform. > m_rootPlatformLayer->setGeometryOrientation(GraphicsLayer::compositingCoordinatesOrientation()); Maybe we shouldn't call setGeometryOrientation() on the m_rootPlatformLayer of iframes? > m_rootPlatformLayer = GraphicsLayer::create(0); > + m_rootPlatformLayer->setName("Root"); Part of the name changes. > m_rootPlatformLayer->setSize(FloatSize(m_renderView->rightLayoutOverflow(), m_renderView->bottomLayoutOverflow())); > m_rootPlatformLayer->setPosition(FloatPoint(0, 0)); > + m_rootPlatformLayer->setAnchorPoint(FloatPoint()); This may break Mac. > + // Create a clipping layer if this is an iframe > + Element* ownerElement = m_renderView->document()->ownerElement(); > + if (ownerElement) { Need to call shouldPropagateCompositingToIFrameParent() here (and maybe in some other places too). > Index: LayoutTests/ChangeLog I'd like to see some more Layout Tests for this. 1. One that dynamically switches the iframe contents document into compositing mode after loading. 2. One that dynamically switches the iframe contents document out of compositing mode after loading. 3. One that dynamically resizes a composited iframe. I think I'd like to see one more patch for this. > > m_graphicsLayer->setContentsRect(contentsBox()); > > m_graphicsLayer->setDrawsContent(containsPaintedContent()); > > + > > + // If this is an iframe parent, update the iframe content's box > > + if (renderer()->isRenderIFrame()) { > > + HTMLIFrameElement* element = static_cast<HTMLIFrameElement*>(renderer()->node()); > > + if (Document* contentDocument = element->contentDocument()) { > > + if (RenderView* view = contentDocument->renderView()) { > > + RenderLayerCompositor* iframeCompositor = view->compositor(); > > + iframeCompositor->setContentsBox(contentsBox()); > > This is only necessary if the iframe contents are composited. I think you > should test that here. > > Maybe add a utility method to get the inner RenderLayerCompositor, given a > RenderIFrame. I'm not sure what you mean by that. I've added logic to setContentsBox() to make sure the clippingLayer exists. I believe that if it does and there is a compositor, we have compositing layers, right? If not, let me know how to do more tests to ensure this. > > @@ -764,11 +752,33 @@ void RenderLayerCompositor::rebuildCompo > > } > > > > if (layerBacking) { > > - layerBacking->parentForSublayers()->setChildren(layerChildren); > > + if (layer->renderer()->isRenderIFrame()) { > > + // This is an iframe parent. Make it the parent of the iframe document's root > > + layerBacking->parentForSublayers()->removeAllChildren(); > > Rather than removeAllChildren() followed by addChild(), it's slightly more > efficient to just do a setChildren(). I can't see how that could be. setChildren() calls removeAllChildren() followed by addChild() on each child passed in a Vector. This avoids creating the Vector. We could add a setChild() method, but I don't think that's necessary at this point. > > void RenderLayerCompositor::willMoveOffscreen() > > @@ -944,19 +968,44 @@ void RenderLayerCompositor::willMoveOffs > > if (!m_rootPlatformLayer || !m_rootLayerAttached) > > return; > > > > - Frame* frame = m_renderView->frameView()->frame(); > > - Page* page = frame ? frame->page() : 0; > > - if (!page) > > - return; > > + bool detached = false; > > + Element* ownerElement = m_renderView->document()->ownerElement(); > > + if (ownerElement) { > > + RenderObject* renderer = ownerElement->renderer(); > > + if (renderer->isRenderIFrame()) { > > + // The layer will get hooked up via RenderLayerBacking::updateGraphicsLayerConfiguration() > > + // for the iframe's renderer in the parent document. > > + ownerElement->setNeedsStyleRecalc(SyntheticStyleChange); > > + detached = true; > > Would be nice to share this code in these two methods. Because of the detached flag setting, I don't think a function would make this any more clear. And it's just 2 uses that are right next to each other. I don't think we save enough to make it worth it. > > > void RenderLayerCompositor::updateRootLayerPosition() > > { > > - if (m_rootPlatformLayer) > > - m_rootPlatformLayer->setSize(FloatSize(m_renderView->rightLayoutOverflow(), m_renderView->bottomLayoutOverflow())); > > + if (m_rootPlatformLayer) { > > + // FIXME: Adjust the y position of the m_rootPlatformLayer if we are clipping by its top edge > > + // Eventually this will be taken care of by scrolling logic > > + // https://bugs.webkit.org/show_bug.cgi?id=38518 > > + float height = m_renderView->bottomLayoutOverflow(); > > + float yOffset = 0; > > + > > + if (m_clippingLayer && height > m_clippingLayer->size().height()) > > + yOffset = m_clippingLayer->size().height() - height; > > I don't get why we need to do this Y flipping. We don't have Y-flipping code > for other "normal" GraphicsLayers, so why do we need it here? > > It it related to this? > > // The root layer does flipping if we need it on this platform. > > m_rootPlatformLayer->setGeometryOrientation(GraphicsLayer::compositingCoordinatesOrientation()); > Maybe we shouldn't call setGeometryOrientation() on the m_rootPlatformLayer of > iframes? No. This is a hard fought solution. I think it has to be that other uses are setting the position relative to the lower left edge as a natural fallout of the CG rendering model. I've tried every combination of geometry orientation and anchor point. Nothing has the desired effect. > > > + // Create a clipping layer if this is an iframe > > + Element* ownerElement = m_renderView->document()->ownerElement(); > > + if (ownerElement) { > > Need to call shouldPropagateCompositingToIFrameParent() here (and maybe in some > other places too). I don't feel comfortable adding that call here, unless there's a test case showing that there's a problem. You should open a new bug if you find such a test case. Not having this here may make multiply nested iframes not work. But adding it might make multiply nested iframes crash. We need a new bug so we can decide if this is important enough to fix now or if it can wait. > > > Index: LayoutTests/ChangeLog > > I'd like to see some more Layout Tests for this. > 1. One that dynamically switches the iframe contents document into compositing > mode after loading. > 2. One that dynamically switches the iframe contents document out of > compositing mode after loading. > 3. One that dynamically resizes a composited iframe. At this point, the test cases submitted test the error reported. We need to fix the current bug and deal with additional problems with separate bug reports. A new patch is on its way... Created attachment 55077 [details]
Patch with suggested changes
Landed in http://trac.webkit.org/changeset/58797 Committed r58798: <http://trac.webkit.org/changeset/58798> This caused bug 38565. |