Summary: | Web Inspector: do not merge iframes into a single DOM hierarchy. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pavel Feldman <pfeldman> | ||||
Component: | Web Inspector (Deprecated) | Assignee: | Pavel Feldman <pfeldman> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Pavel Feldman
2012-01-16 07:58:20 PST
Created attachment 122638 [details]
Patch
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? (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? > > + (): > > 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> Committed r105067: <http://trac.webkit.org/changeset/105067> 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. |