Bug 45365

Summary: Remove support for non-lazy attach and fix frames to load from insertedIntoDocument instead of attach
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ayao, commit-queue, jamesr, jeffcz
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 45737    
Attachments:
Description Flags
Patch
none
Patch
none
Updated with Dave and Dimitri's comments none

Eric Seidel (no email)
Reported 2010-09-07 22:40:09 PDT
Remove support for non-lazy attach and fix frames to load from insertedIntoDocument instead of attach
Attachments
Patch (12.60 KB, patch)
2010-09-07 22:43 PDT, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Patch (21.86 KB, patch)
2010-09-09 00:36 PDT, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Updated with Dave and Dimitri's comments (22.74 KB, patch)
2010-09-09 10:32 PDT, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2010-09-07 22:43:54 PDT
Created attachment 66842 [details] Patch
Eric Seidel (no email)
Comment 2 2010-09-07 22:45:33 PDT
Comment on attachment 66842 [details] Patch This is not actually ready for review yet. fast/frames/frame-limit.html still hits an ASSERT, but every other test passes. fast/frames/frame-limit.html hits an ASSERT: void Page::checkFrameCountConsistency() const { ASSERT(m_frameCount >= 0); int frameCount = 0; for (Frame* frame = mainFrame(); frame; frame = frame->tree()->traverseNext()) ++frameCount; ASSERT(m_frameCount + 1 == frameCount); } The frame count is off (too high) by one. Not sure what is causing this yet.
James Robinson
Comment 3 2010-09-07 23:05:51 PDT
Comment on attachment 66842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=66842&action=prettypatch > WebCore/dom/Node.cpp:752 > + for (Node* p = parentNode(); p && !p->childNeedsStyleRecalc(); p = p->parentNode()) > + p->setChildNeedsStyleRecalc(); > + if (document()->childNeedsStyleRecalc()) > + document()->scheduleStyleRecalc(); Why's this distinct from setNeedsStyleRecalc() now? > WebCore/html/HTMLFrameElementBase.cpp:199 > + // Loads may cause synchronous javascript execution (e.g. beforeload or > + // src=javascript), which could try to access the renderer before the normal > + // parser machinery would call lazyAttach() and set us as needing style > + // resolve. Calling lazyAttach() here marks this for a style resolve. > + setNeedsStyleRecalc(FullStyleChange); Comment seems a little out of sync with what the code is doing.
Eric Seidel (no email)
Comment 4 2010-09-08 03:27:36 PDT
Reduction of last test causing me trouble: <body> <script> var parent = document.createElement('iframe'); document.body.appendChild(parent); var frame = document.createElement("iframe"); frame.setAttribute("src", "data:text/plain,1"); parent.contentDocument.body.appendChild(frame); parent.parentNode.removeChild(parent); </script>
Eric Seidel (no email)
Comment 5 2010-09-09 00:36:01 PDT
Created attachment 67004 [details] Patch
Dave Hyatt
Comment 6 2010-09-09 09:42:52 PDT
Comment on attachment 67004 [details] Patch Ancessors should be Ancestors.
Dimitri Glazkov (Google)
Comment 7 2010-09-09 09:56:17 PDT
Comment on attachment 67004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=67004&action=prettypatch > WebCore/dom/Node.h:-314 > - virtual bool canLazyAttach() { return true; } What about HTMLPluginImageElement? It still returns false.
Dimitri Glazkov (Google)
Comment 8 2010-09-09 09:56:43 PDT
Comment on attachment 67004 [details] Patch Oops. sorry, transferring over dhyatt's rs and cqs
Eric Seidel (no email)
Comment 9 2010-09-09 10:32:22 PDT
Created attachment 67054 [details] Updated with Dave and Dimitri's comments
Dimitri Glazkov (Google)
Comment 10 2010-09-09 11:39:19 PDT
Comment on attachment 67054 [details] Updated with Dave and Dimitri's comments I am pretty sure you're doing everything right, but somehow I am certain this will not be the end of it :) Godspeed.
WebKit Commit Bot
Comment 11 2010-09-10 02:12:38 PDT
Comment on attachment 67054 [details] Updated with Dave and Dimitri's comments Clearing flags on attachment: 67054 Committed r67182: <http://trac.webkit.org/changeset/67182>
WebKit Commit Bot
Comment 12 2010-09-10 02:12:44 PDT
All reviewed patches have been landed. Closing bug.
Drew Yao
Comment 13 2011-03-30 17:57:01 PDT
*** Bug 57512 has been marked as a duplicate of this bug. ***
Drew Yao
Comment 14 2011-06-30 16:25:11 PDT
*** Bug 62104 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.