Bug 37878 - Hook compositing layers together across iframes
Summary: Hook compositing layers together across iframes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-20 11:50 PDT by Simon Fraser (smfr)
Modified: 2010-07-08 22:39 PDT (History)
4 users (show)

See Also:


Attachments
Patch (12.86 KB, patch)
2010-04-20 12:43 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-04-20 11:50:32 PDT
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.
Comment 1 Simon Fraser (smfr) 2010-04-20 12:43:23 PDT
Created attachment 53867 [details]
Patch
Comment 2 Chris Marrin 2010-04-20 12:49:35 PDT
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 3 mitz 2010-04-20 14:19:16 PDT
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.
Comment 4 Simon Fraser (smfr) 2010-04-20 14:24:46 PDT
Comment on attachment 53867 [details]
Patch

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

Keeping bug open for more work.
Comment 5 Eric Seidel (no email) 2010-04-20 22:38:05 PDT
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?
Comment 6 Eric Seidel (no email) 2010-04-20 22:38:28 PDT
I'm not sure why SheriffBot never complained here.
Comment 7 Simon Fraser (smfr) 2010-04-20 22:42:31 PDT
Ah, I should have fixed this result via bug 37885.
Comment 8 Eric Seidel (no email) 2010-04-20 22:44:52 PDT
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?
Comment 9 Simon Fraser (smfr) 2010-04-20 22:49:14 PDT
Fixed that test result in http://trac.webkit.org/changeset/57953 .
Comment 10 Eric Seidel (no email) 2010-04-20 22:55:02 PDT
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)
Comment 11 Simon Fraser (smfr) 2010-04-20 23:05:24 PDT
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?
Comment 12 Eric Seidel (no email) 2010-04-20 23:06:33 PDT
http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r57952%20(6190)/results.html

Shows the crash and that's a build of r57952.
Comment 13 Eric Seidel (no email) 2010-04-20 23:06:56 PDT
Thank you for fixing the test results, btw.
Comment 14 Simon Fraser (smfr) 2010-04-20 23:13:29 PDT
Please file a new bug on the transitions/transition-end-event-destroy-iframe.html assertion.
Comment 15 Eric Seidel (no email) 2010-04-20 23:17:55 PDT
(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?
Comment 16 Eric Seidel (no email) 2010-04-21 18:13:16 PDT
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.
Comment 17 Simon Fraser (smfr) 2010-04-23 18:01:41 PDT
This caused regressions. See bug 38072.
Comment 18 Simon Fraser (smfr) 2010-07-08 22:39:09 PDT
This is done now.