Bug 76383 - Web Inspector: do not merge iframes into a single DOM hierarchy.
Summary: Web Inspector: do not merge iframes into a single DOM hierarchy.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-16 07:58 PST by Pavel Feldman
Modified: 2012-01-16 12:41 PST (History)
10 users (show)

See Also:


Attachments
Patch (18.04 KB, patch)
2012-01-16 08:00 PST, Pavel Feldman
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 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.
Comment 1 Pavel Feldman 2012-01-16 08:00:02 PST
Created attachment 122638 [details]
Patch
Comment 2 Timothy Hatcher 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?
Comment 3 Timothy Hatcher 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?
Comment 4 Pavel Feldman 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>
Comment 5 Pavel Feldman 2012-01-16 08:56:41 PST
Committed r105067: <http://trac.webkit.org/changeset/105067>
Comment 6 Timothy Hatcher 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.