Bug 42046 – connect-compositing-iframe2.html test sometimes shows blank iframe content

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

[Simon Fraser (smfr)]

If you reload connect-compositing-iframe2.html after some other test files, sometimes the iframe content is missing.

[Simon Fraser (smfr)]

The SyntheticStyleChange is getting clobbered by a FullStyleChange when the class attribute changes, with the result that we don't call styleChanged on the RenderLayer.

[Simon Fraser (smfr)]

Created attachment 63024 [details]
Patch

[Simon Fraser (smfr)]

Mitz says that setNeedsStyleRecalc() should ASSERT(changeType != NoStyleChange).

[WebKit Review Bot]

Attachment 63024 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3592609

[Darin Adler]

Comment on attachment 63024 [details]
Patch

Two thoughts on how to refine this further:

    1) setNeedsStyleRecalc no longer seems like a great name to me since this doesn't set a flag named "setNeedsStyleRecalc". It's still a bit like setNeedsDisplayInRect, so I guess we can leave the name along. But maybe we can find a better name.

    2) setNeedsStyleRecalc should assert that changeType is != NoStyleChange, in case we missed any callers who need to call the new function.

The setStyleChange function can't be called outside of Node.cpp because it's defined inside the file and marked inline. Thus if you want to call it from clearNeedsStyleRecalc you need to either move Node::setStyleChange to the header or move the body of clearNeedsStyleRecalc into the .cpp file and make it non-inline.

Or you could write this:

    void clearNeedsStyleRecalc() { m_nodeFlags &= ~StyleChangeMask; }

Which would have the added benefit of being more efficient.

review- since it breaks the build as-is

[Simon Fraser (smfr)]

(In reply to comment #5)

> The setStyleChange function can't be called outside of Node.cpp because it's defined inside the file and marked inline. Thus if you want to call it from clearNeedsStyleRecalc you need to either move Node::setStyleChange to the header or move the body of clearNeedsStyleRecalc into the .cpp file and make it non-inline.

I added
+    void clearNeedsStyleRecalc() { setStyleChange(NoStyleChange); }

to Node.h. This patch builds just fine (all the tests pass with it). I agree with the rest of your feedback.

[Simon Fraser (smfr)]

(In reply to comment #6)
>  This patch builds just fine (all the tests pass with it). I agree with the rest of your feedback.

Ah, but I didn't build Release, so the inline didn't kick in. I see now.

[Simon Fraser (smfr)]

Created attachment 63111 [details]
Patch

[Simon Fraser (smfr)]

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