[webkit-reviews] review denied: [Bug 32446] Content in <iframe> appearing in upper-left corner of WebView on http://webkit.org/blog/386/3d-transforms/ : [Attachment 55034] Replacement Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 4 13:56:04 PDT 2010
Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 32446: Content in <iframe> appearing in upper-left corner of WebView on
http://webkit.org/blog/386/3d-transforms/
https://bugs.webkit.org/show_bug.cgi?id=32446
Attachment 55034: Replacement Patch
https://bugs.webkit.org/attachment.cgi?id=55034&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> 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::compositingCoordinat
esOrientation());
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.
More information about the webkit-reviews
mailing list