RESOLVED FIXED 76383
Web Inspector: do not merge iframes into a single DOM hierarchy. 
https://bugs.webkit.org/show_bug.cgi?id=76383
Summary Web Inspector: do not merge iframes into a single DOM hierarchy.
Pavel Feldman
Reported 2012-01-16 07:58:20 PST
<div> <iframe> <html> </html> </iframe> </div> DOM tree above has been returned in a single node hierarchy where <html>'s parent node was <iframe>. After this change, same tree will be rendered as <div> <iframe> #document <html> </html> </iframe> </div> with explicit #document entry that corresponds to the iframe's content document. It will not be returned as one of the iframe's children, but will rather be a contentDocument property of the iframe itself.
Attachments
Patch (18.04 KB, patch)
2012-01-16 08:00 PST, Pavel Feldman 		timothy: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Pavel Feldman
Comment 1 2012-01-16 08:00:02 PST
Created attachment 122638 [details] Patch
Timothy Hatcher
Comment 2 2012-01-16 08:24:54 PST
Comment on attachment 122638 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122638&action=review > Source/WebCore/ChangeLog:29 > + (): You should fix this. > Source/WebCore/inspector/front-end/DOMAgent.js:39 > + this._domAgent = domAgent; Why does domAgent need passed to everyone? WebInspector.domAgent is a singleton. > Source/WebCore/inspector/front-end/DOMAgent.js:67 > + this.children = [this._contentDocument]; Why does children contain itself? Shouldn't it stay empty so it can be populated with the root DOM nodes? > Source/WebCore/inspector/front-end/DOMAgent.js:508 > this.documentURL; Why is this here? Is it for the compiler? > Source/WebCore/inspector/front-end/ElementsTreeOutline.js:-1524 > - case Node.DOCUMENT_NODE: > - info.titleDOM.appendChild(document.createTextNode("Document")); > - break; > - Because it was never used?
Timothy Hatcher
Comment 3 2012-01-16 08:26:08 PST
(In reply to comment #0) > <div> > <iframe> > <html> > </html> > </iframe> > </div> > > DOM tree above has been returned in a single node hierarchy where <html>'s parent node was <iframe>. After this change, same tree will be rendered as > > <div> > <iframe> > #document > <html> > </html> > </iframe> > </div> > > with explicit #document entry that corresponds to the iframe's content document. It will not be returned as one of the iframe's children, but will rather be a contentDocument property of the iframe itself. So just walking the children/parentNode chain you will still get this: <div> <iframe> <html> </html> </iframe> </div> Right?
Pavel Feldman
Comment 4 2012-01-16 08:48:09 PST
> > + (): > > You should fix this. Done. > > > Source/WebCore/inspector/front-end/DOMAgent.js:39 > > + this._domAgent = domAgent; > > Why does domAgent need passed to everyone? WebInspector.domAgent is a singleton. I'm trying to get rid of the singletons where possible to ease the potential testing using mocks. It is also compiler non-friendly since singletons are usually assigned in the client code (inspector.js) that is outside the compiled component (DOMAgent in this case). > > > Source/WebCore/inspector/front-end/DOMAgent.js:67 > > + this.children = [this._contentDocument]; > > Why does children contain itself? Shouldn't it stay empty so it can be populated with the root DOM nodes? In this case, children of <iframe> is single element, contentDocument. It gets populated with this single root node instantly. I.e. you don't need to ask for frame's children to the the #document over the protocol. > > > Source/WebCore/inspector/front-end/DOMAgent.js:508 > > this.documentURL; > > Why is this here? Is it for the compiler? > Yes :( > > Source/WebCore/inspector/front-end/ElementsTreeOutline.js:-1524 > > - case Node.DOCUMENT_NODE: > > - info.titleDOM.appendChild(document.createTextNode("Document")); > > - break; > > - > > Because it was never used? Because there is a generic code that uses .nodeName below. Using #document as label makes it more clear to the user. (In reply to comment #3) > So just walking the children/parentNode chain you will still get this: > > <div> > <iframe> > <html> > </html> > </iframe> > </div> > > Right? Walking the children/parentNode chain on the protocol level will give <div> <iframe> </div>
Pavel Feldman
Comment 5 2012-01-16 08:56:41 PST
Committed r105067: <http://trac.webkit.org/changeset/105067>
Timothy Hatcher
Comment 6 2012-01-16 12:41:45 PST
Comment on attachment 122638 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122638&action=review > Source/WebCore/inspector/front-end/DOMAgent.js:509 > this._listeners = {}; This looks like dead code. No where is _listeners used. Looks like WebInspector.DOMDocument is a pointless subclass at this point.
Note You need to log in before you can comment on or make changes to this bug.