[Webkit-unassigned] [Bug 27771] Web Inspector: Reimplement Elements Panel so that its interaction with DOM is serialized

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27771


Timothy Hatcher <timothy at hatcher.name> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33935|review?                     |review-
               Flag|                            |




--- Comment #5 from Timothy Hatcher <timothy at hatcher.name>  2009-08-01 06:33:25 PDT ---
(From update of attachment 33935)
> +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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.