Bug 37878 – Hook compositing layers together across iframes

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

When an iframe gains composited content, we should toggle the enclosing frame into compositing mode, both to preserve layering (since main frame content can overlay iframes), and so that 3d-transformed iframes work.

[Simon Fraser (smfr)]

Created attachment 53867 [details]
Patch

[Chris Marrin]

Comment on attachment 53867 [details]
Patch

> diff --git a/WebCore/rendering/RenderIFrame.h b/WebCore/rendering/RenderIFrame.h
> index 60e6fdc..0fe22f5 100644
> --- a/WebCore/rendering/RenderIFrame.h
> +++ b/WebCore/rendering/RenderIFrame.h
> @@ -34,18 +34,43 @@ class RenderIFrame : public RenderFrameBase {
>  public:
>      RenderIFrame(Element*);
>  
> +#if USE(ACCELERATED_COMPOSITING)
> +    bool requiresAcceleratedCompositing() const;
> +#endif
> +
>  private:
>      virtual void calcHeight();
>      virtual void calcWidth();
>  
>      virtual void layout();
>  
> +#if USE(ACCELERATED_COMPOSITING)
> +    virtual bool requiresLayer() const;
> +#endif
> +    virtual bool isRenderIFrame() const { return true; }
> +
>      virtual const char* renderName() const { return "RenderPartObject"; } // Lying for now to avoid breaking tests
>  
>      bool flattenFrame();
>  
>  };
>  
> +inline RenderIFrame* toRenderIFrame(RenderObject* object)
> +{
> +    ASSERT(!object || !strcmp(object->renderName(), "RenderPartObject")); // FIXME: fix the string when we fix renderName().
> +    return static_cast<RenderIFrame*>(object);
> +}

I don't get why you have to do a string compare here. Why can't you use isRenderIFrame()? I understand that's just in the ASSERT, but why isn't the isRenderIFrame() test sufficient?

Comment on attachment 53867 [details]
Patch

> +        (WebCore::RenderLayerCompositor::enableCompositingMode): Call out to the RenderView when
> +        the compositing mode changes, so that the parent document can updates its compositing status.

Typo: “updates”

Like Chris said, the assertions in the to* methods should just use is*, not awkward string compare.

[Simon Fraser (smfr)]

Comment on attachment 53867 [details]
Patch

http://trac.webkit.org/changeset/57919

Keeping bug open for more work.

[Eric Seidel (no email)]

This has been failing on SL since it landed:

http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r57919%20(8634)/results.html

Looks like tabs vs. spaces, possibly?

[Eric Seidel (no email)]

I'm not sure why SheriffBot never complained here.

[Simon Fraser (smfr)]

Ah, I should have fixed this result via bug 37885.

[Eric Seidel (no email)]

I think http://trac.webkit.org/changeset/57913 is actually causing the failure, but the question is why did it not show up until 57919?

[Simon Fraser (smfr)]

Fixed that test result in http://trac.webkit.org/changeset/57953 .

[Eric Seidel (no email)]

I think this may also have caused a test to start failing in debug:
http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r57921%20(6175)/transitions/transition-end-event-destroy-iframe-stderr.txt

ASSERTION FAILED: !needsLayout()
(/Volumes/Data/WebKit-BuildSlave/snowleopard-intel-leaks/build/WebCore/page/FrameView.cpp:1801 virtual void WebCore::FrameView::paintContents(WebCore::GraphicsContext*, const WebCore::IntRect&))

At least it's the only painting related change in the range (r57916-r57921)

[Simon Fraser (smfr)]

The !needsLayout() assertion should have been fixed by this commit (rolling out an earlier change of mine):
http://trac.webkit.org/changeset/57895

Maybe the bots are just very behind?

[Eric Seidel (no email)]

http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r57952%20(6190)/results.html

Shows the crash and that's a build of r57952.

[Eric Seidel (no email)]

Thank you for fixing the test results, btw.

[Simon Fraser (smfr)]

Please file a new bug on the transitions/transition-end-event-destroy-iframe.html assertion.

[Eric Seidel (no email)]

(In reply to comment #14)
> Please file a new bug on the
> transitions/transition-end-event-destroy-iframe.html assertion.

I'm confused.  Do we believe this change is the cause?  If so, why would we leave the bot failing the ASSERT?

[Eric Seidel (no email)]

Comment on attachment 53867 [details]
Patch

Cleared Dan Bernstein's review+ from obsolete attachment 53867 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Simon Fraser (smfr)]

This caused regressions. See bug 38072.

[Simon Fraser (smfr)]

This is done now.