Bug 28177

Created attachment 34565 [details] Step1: Most of the work done. Call for your comments. The general idea of this patch is that we stop pushing live objects into the frontend upon evaluation and console.dir requests. It also migrates to WebInspector.DOMAgent. Patch is not complete: - it does not implement DOM search - it breaks commandLineAPI's inspect(o). But it already has a lot of changes and I would like to get initial feedback on it. I am fixing the ones above in the meanwhile. What will happen after this change: - More bugfixes if I miss anything - Move search worker into the InjectedScript - Look at debugging, database and the rest of the places that still push live objects into the frontend - Move InjectedScript into the isolated context and get rid of all the "quarantine*" calls and support.

Comment on attachment 34565 [details] Step1: Most of the work done. Call for your comments. > 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.)

(In reply to comment #2) > (From update of attachment 34565 [details]) > > > Node* InspectorDOMAgent::nodeForId(long id) > > { > > + if (!id) > > + return mainFrameDocument(); > > So 0 is a special id and it is always the main document? > Yes, it is. WebInspector.DOMDocument also has corresponding id == 0. > > 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? > Now that evaluate returns object wrappers instead of objects, I would need to do extra lookups in order to get a list of object properties used in completion. Instead, I evaluate an expression that returns comma-separated property list (basic types are not wrapped). I am not sure I want to put this into InjectedScript since I want split to be next to join, but I can move it as you suggest and mention the comma separator in the comments. > > + var properties = result.split(","); > > I don't understand this, where did result come from? Why does it need split? > This split corresponds to join in the expression above. > > - 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? > This method is also called from the InspectorController via the frontend. And it can only use basic types, proxies, not the live objects. So I needed to change WebInspector.updateFocusedNode's contract a bit. > > + 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. > Will do. > > > + var evalCallback = function(result) { > > We prefer the "function evalCallback(result)" style here, with the brace on the > second line. > I was thinking that all named functions get into the global object and as a result we will get clashing names (if in another function i use local named function with the same name). Isn't it so? > > - InspectorController.highlightDOMNode(x); > > + InspectorController.highlightDOMNode(x._id); > > Is this ._id stuff temporary? If not _id should drop the underscore. > I think you are right. It is time to make node proxy id public... > > + 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? > Yes, the plan is to introduce InjectedScript.CommandLineAPI class inside InjectedScript. I actually started it like that, but then realized that it will only work once InjectedScript can interact with the inspected window without the need to be quarantined. I'll do it later when InjectedScript runs in page's context. > > > - useDOMAgent: false > > + useDOMAgent: true > > So this is ready to flip? (Once you fix the remaining issues you mentioned.) Yes, search and inspect(o) are the only things missing!

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 34565 [details] [details]) > > > > 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? > > Now that evaluate returns object wrappers instead of objects, I would need to > do extra lookups in order to get a list of object properties used in > completion. Instead, I evaluate an expression that returns comma-separated > property list (basic types are not wrapped). I am not sure I want to put this > into InjectedScript since I want split to be next to join, but I can move it as > you suggest and mention the comma separator in the comments. > > > > + var properties = result.split(","); > > > > I don't understand this, where did result come from? Why does it need split? > > > > This split corresponds to join in the expression above. Ok, I see now. I overlooked the join. The evaluated code was hard to read. It would be best if it was just a normal function and not a string, so it was readable. That was my main point. It seems like any code we want ran in the inspected page should live in InjectedScript.js and not need to be evaluated. But I guess that can't happen just yet. > > > + var evalCallback = function(result) { > > > > We prefer the "function evalCallback(result)" style here, with the brace on the > > second line. > > > > I was thinking that all named functions get into the global object and as a > result we will get clashing names (if in another function i use local named > function with the same name). Isn't it so? Nope. They are locally scoped in the function, just like it was a var. > Yes, the plan is to introduce InjectedScript.CommandLineAPI class inside > InjectedScript. I actually started it like that, but then realized that it will > only work once InjectedScript can interact with the inspected window without > the need to be quarantined. I'll do it later when InjectedScript runs in page's > context. Ok, makes sense.

Created attachment 34733 [details] The patch. Comments addressed, added search and command line API support. I think that's enough to move us to the agent and flip the flag.

Comment on attachment 34733 [details] The patch. > + case "Storage": > + InspectorController.selectDOMStorage(o); > + break; Extra indent here. > - useDOMAgent: false > + useDOMAgent: true Can this just be removed now? > - if (obj instanceof win.Node) > + if (obj instanceof win.Node && !(obj instanceof win.Document)) Does this still let console.dirxml(document) output the HTML tree?

Thanks for the review! For committers: I'd like to land this myself in coordination with Chromium. Thanks. (In reply to comment #6) > (From update of attachment 34733 [details]) > > + case "Storage": > > + InspectorController.selectDOMStorage(o); > > + break; > > Extra indent here. Done. > > - useDOMAgent: false > > + useDOMAgent: true > > Can this just be removed now? > Sure. > > - if (obj instanceof win.Node) > > + if (obj instanceof win.Node && !(obj instanceof win.Document)) > > Does this still let console.dirxml(document) output the HTML tree? I don't think so. Let me remove this check. I actually was thinking we did not allow dirxml for document, but now I see it was working fine.

Created attachment 34745 [details] Patch with comments addressed (no need to review).

Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/bindings/js/JSInspectorBackendCustom.cpp M WebCore/bindings/js/ScriptObjectQuarantine.cpp M WebCore/bindings/js/ScriptObjectQuarantine.h M WebCore/bindings/js/ScriptValue.cpp M WebCore/bindings/js/ScriptValue.h M WebCore/bindings/v8/ScriptObjectQuarantine.cpp M WebCore/bindings/v8/ScriptObjectQuarantine.h M WebCore/bindings/v8/ScriptValue.h M WebCore/bindings/v8/custom/V8CustomBinding.h M WebCore/bindings/v8/custom/V8InspectorBackendCustom.cpp M WebCore/inspector/ConsoleMessage.cpp M WebCore/inspector/ConsoleMessage.h 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 M WebCore/inspector/InspectorDOMAgent.cpp M WebCore/inspector/InspectorDOMAgent.h M WebCore/inspector/InspectorDOMStorageResource.cpp M WebCore/inspector/InspectorFrontend.cpp M WebCore/inspector/InspectorFrontend.h M WebCore/inspector/front-end/ConsoleView.js M WebCore/inspector/front-end/DOMAgent.js M WebCore/inspector/front-end/ElementsPanel.js M WebCore/inspector/front-end/ElementsTreeOutline.js M WebCore/inspector/front-end/InjectedScript.js M WebCore/inspector/front-end/ObjectPropertiesSection.js M WebCore/inspector/front-end/ObjectProxy.js M WebCore/inspector/front-end/PropertiesSidebarPane.js M WebCore/inspector/front-end/StylesSidebarPane.js M WebCore/inspector/front-end/inspector.js M WebCore/inspector/front-end/utilities.js M WebCore/storage/Storage.h Committed r47231