Bug 101619

Summary: Skip frame owner disconnect when there's no frames
Product: WebKit Reporter: Elliott Sprehn <esprehn>
Component: New BugsAssignee: Elliott Sprehn <esprehn>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, eric, haraken, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 102350    
Attachments:
Description Flags
Patch
none
Patch none

Elliott Sprehn
Reported 2012-11-08 09:51:04 PST
Skip frame owner disconnect when there's no frames
Attachments
Patch (1.93 KB, patch)
2012-11-08 09:56 PST, Elliott Sprehn 		no flags
DetailsFormatted DiffDiff
Patch (2.01 KB, patch)
2012-11-08 10:59 PST, Elliott Sprehn 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Elliott Sprehn
Comment 1 2012-11-08 09:56:58 PST
Created attachment 173059 [details] Patch
Ojan Vafai
Comment 2 2012-11-08 09:59:01 PST
Comment on attachment 173059 [details] Patch Looks great! I'm curious to see if we'll see benefit on any of the perf tests.
Elliott Sprehn
Comment 3 2012-11-08 10:01:04 PST
(In reply to comment #2) > (From update of attachment 173059 [details]) > Looks great! I'm curious to see if we'll see benefit on any of the perf tests. Yeah. Complicated pages seem to always have frames on them though, even the Review Patch page has an iframe for the comment form!
Adam Barth
Comment 4 2012-11-08 10:41:25 PST
Comment on attachment 173059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173059&action=review > Source/WebCore/dom/ContainerNodeAlgorithms.h:280 > + // If we know there's no frames to disconnect then don't bother traversing > + // the tree looking for them. > + Page* page = root->document()->page(); > + if (page && !page->subframeCount()) > + return; Why do we need to do this on a per-page basis? Isn't it enough to know that the current document has no subframes?
Ojan Vafai
Comment 5 2012-11-08 10:44:36 PST
Comment on attachment 173059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173059&action=review >> Source/WebCore/dom/ContainerNodeAlgorithms.h:280 >> + return; > > Why do we need to do this on a per-page basis? Isn't it enough to know that the current document has no subframes? Page is what has the data at the moment. Long-term plan is to add a count of frames in the subtree to NodeRareData and only do this traversal if the subtree has any frames.
Elliott Sprehn
Comment 6 2012-11-08 10:49:14 PST
(In reply to comment #4) > (From update of attachment 173059 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173059&action=review > > > Source/WebCore/dom/ContainerNodeAlgorithms.h:280 > > + // If we know there's no frames to disconnect then don't bother traversing > > + // the tree looking for them. > > + Page* page = root->document()->page(); > > + if (page && !page->subframeCount()) > > + return; > > Why do we need to do this on a per-page basis? Isn't it enough to know that the current document has no subframes? Yeah it is, but we don't track that currently. This is the first step in a longer road to optimization.
Elliott Sprehn
Comment 7 2012-11-08 10:50:47 PST
(In reply to comment #6) > (In reply to comment #4) > ... > > Why do we need to do this on a per-page basis? Isn't it enough to know that the current document has no subframes? > > Yeah it is, but we don't track that currently. This is the first step in a longer road to optimization. Actually, maybe I can just use document()->frame()->tree()->firstChild() to detect if there's subframes?
Elliott Sprehn
Comment 8 2012-11-08 10:59:34 PST
Created attachment 173073 [details] Patch Use the frame tree instead.
Adam Barth
Comment 9 2012-11-08 11:12:02 PST
Comment on attachment 173073 [details] Patch LGTM
Ojan Vafai
Comment 10 2012-11-08 11:21:09 PST
Clever! I like it.
Alexey Proskuryakov
Comment 11 2012-11-08 12:37:36 PST
How will this affect frames that are in shadow trees?
WebKit Review Bot
Comment 12 2012-11-08 12:37:53 PST
Comment on attachment 173073 [details] Patch Clearing flags on attachment: 173073 Committed r133933: <http://trac.webkit.org/changeset/133933>
WebKit Review Bot
Comment 13 2012-11-08 12:37:57 PST
All reviewed patches have been landed. Closing bug.
Adam Barth
Comment 14 2012-11-08 12:40:02 PST
> How will this affect frames that are in shadow trees? Are those not added to the FrameTree?
Adam Barth
Comment 15 2012-11-08 12:42:51 PST
It looks like they are added to the FrameTree, but they're skipped over in the "scoped" versions of these functions by testing Frame::inScope
Adam Barth
Comment 16 2012-11-08 12:43:56 PST
@esprehn: Can you add a test for this case?
Elliott Sprehn
Comment 17 2012-11-08 13:28:40 PST
(In reply to comment #16) > @esprehn: Can you add a test for this case? Sure.
Note You need to log in before you can comment on or make changes to this bug.