[webkit-reviews] review canceled: [Bug 28177] WebInspector: Migrate to DOMAgent (serialized access to DOM) : [Attachment 34565] Step1: Most of the work done. Call for your comments.

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has canceled Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 28177: WebInspector: Migrate to DOMAgent (serialized access to DOM)
https://bugs.webkit.org/show_bug.cgi?id=28177

Attachment 34565: Step1: Most of the work done. Call for your comments.
https://bugs.webkit.org/attachment.cgi?id=34565&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>

>  Node* InspectorDOMAgent::nodeForId(long id)
>  {
> +    if (!id)
> +	   return mainFrameDocument();

So 0 is a special id and it is always the main document?

>	   var reportCompletions = this._reportCompletions.bind(this,
bestMatchOnly, completionsReadyCallback, dotNotation, bracketNotation, prefix);

> -	   this._evalInInspectedWindow(expressionString, reportCompletions);
> +	   this._evalInInspectedWindow("(function() {" +
> +	       "var props =[]; " + 
> +	       "for (var prop in (" + expressionString + ")) props.push(prop);"
+
> +	       ((!dotNotation && !bracketNotation) ? "for (var prop in
window._inspectorCommandLineAPI) props.push(prop);" : "") +
> +	       "return props.join(\",\");" +
> +	   "})()", reportCompletions);

What is this change for? I would like to avoid using _evalInInspectedWindow
with a bunch of code that wasn't user typed. Can this code live in the
InjectedScript file?

> +	   var properties = result.split(",");

I don't understand this, where did result come from? Why does it need split?

> -	   WebInspector.updateFocusedNode(link.representedNode);
> +	   WebInspector.updateFocusedNode(link.representedNode._id);

Shouldn't updateFocusedNode be the one to access _id? Or some other lower level
transport layer?

> +	   var callback = function(success) {
> +	   };
> +	   InspectorController.addInspectedNode(node._id, callback);

Just pass the empty function inline to addInspectedNode or make it accept
"undefined" for the callback.


> +	   var evalCallback = function(result) {

We prefer the "function evalCallback(result)" style here, with the brace on the
second line.

> -		   InspectorController.highlightDOMNode(x);
> +		   InspectorController.highlightDOMNode(x._id);

Is this ._id stuff temporary? If not _id should drop the underscore.

> +    if (!InjectedScript._window()._inspectorCommandLineAPI) {
> +	   InjectedScript._window().eval("window._inspectorCommandLineAPI = { \

> +	       $: function() { return document.getElementById.apply(document,
arguments) }, \
> +	       $$: function() { return
document.querySelectorAll.apply(document, arguments) }, \
> +	       $x: function(xpath, context) { \
> +		   var nodes = []; \
> +		   try { \
> +		       var doc = context || document; \
> +		       var results = doc.evaluate(xpath, doc, null,
XPathResult.ANY_TYPE, null); \
> +		       var node; \
> +		       while (node = results.iterateNext()) nodes.push(node); \

> +		   } catch (e) {} \
> +		   return nodes; \
> +	       }, \
> +	       clear: function(o) {
_inspectorCommandLineAPI._clearConsoleRequested = true; }, \
> +	       dir: function() { return console.dir.apply(console, arguments)
}, \
> +	       dirxml: function() { return console.dirxml.apply(console,
arguments) }, \
> +	       inspect: function(o) { _inspectorCommandLineAPI._objectToInspect
= o; }, \
> +	       keys: function(o) { var a = []; for (var k in o) a.push(k);
return a; }, \
> +	       values: function(o) { var a = []; for (var k in o) a.push(o[k]);
return a; }, \
> +	       profile: function() { return console.profile.apply(console,
arguments) }, \
> +	       profileEnd: function() { return
console.profileEnd.apply(console, arguments) }, \
> +	       _clearConsoleRequested : false, \
> +	       _inspectedNodes: [], \
> +	       _objectToInspect: null, \
> +	       get $0() { return _inspectorCommandLineAPI._inspectedNodes[0] },
\
> +	       get $1() { return _inspectorCommandLineAPI._inspectedNodes[1] },
\
> +	       get $2() { return _inspectorCommandLineAPI._inspectedNodes[2] },
\
> +	       get $3() { return _inspectorCommandLineAPI._inspectedNodes[3] },
\
> +	       get $4() { return _inspectorCommandLineAPI._inspectedNodes[4] }
\
> +	   };");
> +    }
> +},

Does this still need to use InjectedScript._window().eval to install the API?
Or can that be done directly now if this script is in the inspected page? Or is
it?


> -    useDOMAgent: false
> +    useDOMAgent: true

So this is ready to flip? (Once you fix the remaining issues you mentioned.)