[webkit-reviews] review requested: [Bug 27771] Web Inspector: Reimplement Elements Panel so that its interaction with DOM is serialized : [Attachment 33936] InspectorDOMAgent and WebInspector.DOMAgent

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Aug 1 11:03:24 PDT 2009


Pavel Feldman <pfeldman at chromium.org> has asked  for review:
Bug 27771: Web Inspector: Reimplement Elements Panel so that its interaction
with DOM is serialized
https://bugs.webkit.org/show_bug.cgi?id=27771

Attachment 33936: InspectorDOMAgent and WebInspector.DOMAgent
https://bugs.webkit.org/attachment.cgi?id=33936&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
(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.


More information about the webkit-reviews mailing list