Bug 42046 - connect-compositing-iframe2.html test sometimes shows blank iframe content
Summary: connect-compositing-iframe2.html test sometimes shows blank iframe content
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-11 11:57 PDT by Simon Fraser (smfr)
Modified: 2010-07-30 16:02 PDT (History)
3 users (show)

See Also:


Attachments
Patch (7.97 KB, patch)
2010-07-29 21:04 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (9.48 KB, patch)
2010-07-30 15:24 PDT, Simon Fraser (smfr)
darin: review+
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-07-11 11:57:07 PDT
If you reload connect-compositing-iframe2.html after some other test files, sometimes the iframe content is missing.
Comment 1 Simon Fraser (smfr) 2010-07-29 17:03:11 PDT
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.
Comment 2 Simon Fraser (smfr) 2010-07-29 21:04:06 PDT
Created attachment 63024 [details]
Patch
Comment 3 Simon Fraser (smfr) 2010-07-29 21:26:33 PDT
Mitz says that setNeedsStyleRecalc() should ASSERT(changeType != NoStyleChange).
Comment 4 WebKit Review Bot 2010-07-29 22:37:31 PDT
Attachment 63024 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3592609
Comment 5 Darin Adler 2010-07-29 23:30:31 PDT
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
Comment 6 Simon Fraser (smfr) 2010-07-30 08:41:45 PDT
(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.
Comment 7 Simon Fraser (smfr) 2010-07-30 10:42:26 PDT
(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.
Comment 8 Simon Fraser (smfr) 2010-07-30 15:24:24 PDT
Created attachment 63111 [details]
Patch
Comment 9 Simon Fraser (smfr) 2010-07-30 16:02:50 PDT
http://trac.webkit.org/changeset/64383