Bug 27771 – [commit+] Web Inspector: Reimplement Elements Panel so that its interaction with DOM is serialized

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Feldman]

Today Elements panel is working synchronously with live DOM objects (or wrappers).
This is a proposal on how to make this DOM interaction serialized (and asynchronous).

These are the components that need to be introduced:

1) InspectorDOMAgent

InspectorDOMAgent lives in WebCore/inspector and tracks DOM modifications. It preserves
DOM Node to int bindings for all the nodes that it has sent to the frontend. Frontend can
issue particula requests to this agent such as 'getChildNodes'. Agent will respond with
stringified data structure that will 'describe' the nodes being requested. There overall
interface of this agent is not big (a handful of methods).

Agent like this lives and works in Chromium. The intention is to upstream it. I am 
attaching a draft of this class together with the InspectorFrontend/Backend interfaces
to the the bug.

2) WebInspector.DOMAgent

WebInspector.DOMAgent lives in the front-end and is receiving calls from the
InspectorDOMAgent. It will convert stringified node descriptions into the structures
that are very similar to JS Node wrappers. It will basically implement all the DOM
operations ElementsPanel needs. It is going to be fairly large (600 lines of code).

This piece lives in Chromium and only needs reformatting. See its code at:
http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/devtools/js/dom_agent.js

3) JavaScript on the agent side

It does not make much sense to process Search, CSS, Properties-related requests in 
the native code within InspectorDOMAgent. They look more elegant in JavaScript; they
do not need to be that fast; they won't lead to the case where all the DOM nodes we see
in the Elements Panel result in JS wrapper being created around the node. Hence there
will be a piece of JS code that will live in the page context and will process requests
such as 'getNodeStyles', 'applyStyleText', everything that is shorthandProperty-related.
InspectorBackend will have a generic 'executeUtilityFunction' method that will dispatch
parameters into this agent and get back to the frontend with the results.

It seems to be a relatively large refactoring. I was thinking of introducing
InspectorDOMAgent and WebInspector.DOMAgent first. They will get into code, but will not
be used by the Elements Panel by default. Then I was going to add Search / Styles / Properties
JS agent code, still with Elements Panel working as before. As a last step I would switch
to the new implementation.

Please tell me what you think about this plan!

[Pavel Feldman]

Created attachment 33654 [details]
InspectorDOMAgent draft

[Timothy Hatcher]

This approch seems good. It sounds like what I imagined and remember discussing with Ojan and others a while ago.

Sam, what do you think?

[Ojan Vafai]

At a high-level, this seems like the right approach to me. I think it will make the code more maintainable for supporting both out of process and in process inspectors.

As a followup task, we should also consider adding buffering of events we want to push from the inspected page to the inspector. For example, mutation events happen very frequently for some code/operations. We could likely get a significant performance boost by buffering the events up before pushing them.

[Pavel Feldman]

Created attachment 33935 [details]
InspectorDOMAgent and WebInspector.DOMAgent

This patch adds read and some write capabilities to the DOM tree. No styles, no properties, no search yet.

[Timothy Hatcher]

Comment on attachment 33935 [details]
InspectorDOMAgent and WebInspector.DOMAgent

> +WebInspector.DOMPayloadIndex = {
> +    ID : 0,
> +    TYPE : 1,
> +    NAME : 2,
> +    VALUE : 3,
> +    ATTRS : 4,
> +    HAS_CHILDREN : 5,
> +    CHILD_NODES : 6
> +};

I thought the payload was going to use objects with properties and not an array. It seems fragile to use an array like this. Maybe they are arrays to minimize the serialized size?

If we continue to use an array, this should have a comment about having the indexes stay in sync with the C++ file and visa versa. Also these names should not be all caps (should be TitleCase).

> +    _makeStyle: function(payload)
> +    {
> +        var style = new WebInspector.CSSStyleDeclaration(payload);
> +        style.nodeId_ = this._id;

The underscore should be a prefix.

> +    WebInspector.DOMNode.call(this, null,
> +        [
> +            0,   // id
> +            9,   // type = Node.DOCUMENT_NODE,
> +            "",  // nodeName
> +            "",  // nodeValue
> +            [],  // attributes
> +            0,   // childNodeCount
> +        ]);

This is another place using an array is fragile. It this was an object with properties you could name each item without the need for a comment and not have to worry about it breaking if somthing changes. Also why not just use Node.DOCUMENT_NODE instead of 9?

> +    // Install onpopulate handler.
> +    var domAgent = this;
> +    var originalUpdateChildren = WebInspector.ElementsTreeElement.prototype.updateChildren;
> +    WebInspector.ElementsTreeElement.prototype.updateChildren = function()
> +    {
> +        domAgent.getChildNodesAsync(this.representedObject, originalUpdateChildren.bind(this));
> +    };

Having this here seems wrong. I assume this is temporary? Maybe add a comment.

> +
> +    // Mute console handle to avoid crash on selection change.
> +    WebInspector.Console.prototype.addInspectedNode = function()
> +    {
> +    };

Temporary?

> +    // Whitespace is ignored in InspectorDOMAgent already -> no need to filter.
> +    Preferences.ignoreWhitespace = false;

I guess we can remove this and all the users once we make the switch.

> +    this._id = payload[0];
> +    this.width = payload[1];
> +    this.height = payload[2];
> +    this.__disabledProperties = payload[3];
> +    this.__disabledPropertyValues = payload[4];
> +    this.__disabledPropertyPriorities = payload[5];

These should just have single underscore prefixes. The arrays still make a me nervous.

[Pavel Feldman]

Created attachment 33936 [details]
InspectorDOMAgent and WebInspector.DOMAgent

(In reply to comment #5)
> (From update of attachment 33935 [details])
> 
> I thought the payload was going to use objects with properties and not an
> array. It seems fragile to use an array like this. Maybe they are arrays to
> minimize the serialized size?
> 
> If we continue to use an array, this should have a comment about having the
> indexes stay in sync with the C++ file and visa versa. Also these names should
> not be all caps (should be TitleCase).
> 


It was done to minimize serialized size. But let us try dictionaries first and see if it is actually slow. Updated with dictionaries.


> 
> The underscore should be a prefix.
> 

Done.

> 
> This is another place using an array is fragile. It this was an object with
> properties you could name each item without the need for a comment and not have
> to worry about it breaking if somthing changes. Also why not just use
> Node.DOCUMENT_NODE instead of 9?
>

Done. (using dictionary and Node.DOCUMENT_NODE).

> 
> Having this here seems wrong. I assume this is temporary? Maybe add a comment.
> 
>

Yes. Commented and added TODO
 
> Temporary?
> 

Yes. Added TODO.

> > +    // Whitespace is ignored in InspectorDOMAgent already -> no need to filter.
> > +    Preferences.ignoreWhitespace = false;
> 
> I guess we can remove this and all the users once we make the switch.
>

Yes. Added TODO
 
> > +    this.__disabledProperties = payload[3];
> > +    this.__disabledPropertyValues = payload[4];
> > +    this.__disabledPropertyPriorities = payload[5];
> 
> These should just have single underscore prefixes. The arrays still make a me
> nervous.

Not using arrays anymore, but am using __disabledProperties since we feed StylesSidebarPane with it and latter relies upon this name.

[Timothy Hatcher]

> > > +    this.__disabledProperties = payload[3];
> > > +    this.__disabledPropertyValues = payload[4];
> > > +    this.__disabledPropertyPriorities = payload[5];
> > 
> > These should just have single underscore prefixes. The arrays still make a me
> > nervous.
> 
> Not using arrays anymore, but am using __disabledProperties since we feed
> StylesSidebarPane with it and latter relies upon this name.

I didn't realize we used double underscores there.

I can wait to see this in action!

[Adam Barth]

Comment on attachment 33936 [details]
InspectorDOMAgent and WebInspector.DOMAgent

Clearing review flag on attachment: 33936

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.pro
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/WebCoreSources.bkl
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorBackend.h
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	A	WebCore/inspector/InspectorDOMAgent.cpp
	A	WebCore/inspector/InspectorDOMAgent.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	A	WebCore/inspector/front-end/Callback.js
	A	WebCore/inspector/front-end/DOMAgent.js
	M	WebCore/inspector/front-end/ElementsPanel.js
	M	WebCore/inspector/front-end/WebKit.qrc
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/inspector/front-end/inspector.js
Committed r46687
	M	WebCore/WebCore.pro
	M	WebCore/ChangeLog
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.gypi
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/inspector/front-end/ElementsPanel.js
	A	WebCore/inspector/front-end/Callback.js
	A	WebCore/inspector/front-end/DOMAgent.js
	M	WebCore/inspector/front-end/WebKit.qrc
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorBackend.h
	A	WebCore/inspector/InspectorDOMAgent.cpp
	A	WebCore/inspector/InspectorDOMAgent.h
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/WebCoreSources.bkl
r46687 = 1560d6b88c90e759ec0b475db93b8d24ae8044a5 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46687

[Adam Barth]

All reviewed patches have been landed.  Closing bug.