Bug 55482

Created attachment 84249 [details] [patch] initial version

Comment on attachment 84249 [details] [patch] initial version View in context: https://bugs.webkit.org/attachment.cgi?id=84249&action=review > LayoutTests/http/tests/inspector/inspector-test.js:226 > +InspectorTest.dumpStepResult = function(step, expression, result) Please use InspectorTest.runTestSuite instead (look up for the syntax in other tests). > LayoutTests/inspector/protocol/runtime-agent-expected.txt:5 > +was called: "RuntimeAgent.evaluate('true', 'test', false, InspectorTest.callback)" Given that this is a protocol test, it would be nice to dump both: protocol request and response: { request : { } response : { } } > LayoutTests/inspector/protocol/runtime-agent-expected.txt:8 > + objectId : null We should stop sending objectId and hasChildren for primitive values. > LayoutTests/inspector/protocol/runtime-agent-expected.txt:19 > + objectId : { Form valid JSON (with "," - need to patch addObject). > LayoutTests/inspector/protocol/runtime-agent-expected.txt:20 > + injectedScriptId : 1 This can become the source of flake. However, if it does not, lets leave it like this. Btw, addObject allows printing types of members with given names instead of their values. > LayoutTests/inspector/protocol/runtime-agent-expected.txt:21 > + id : 1 This can become the source of flake > LayoutTests/inspector/protocol/runtime-agent-expected.txt:22 > + groupName : "test" I think we can safely remove groupName from objectId. We would need an id -> groupName map in injected script for getProperties that would be cleared whenever id is cleared. > LayoutTests/inspector/protocol/runtime-agent-expected.txt:34 > + $ : true Two problems here: 1) it does not list getElementBy* 2) We should really send these in array of strings. That was we would be able to send "__proto__" suggestion. Should be a separate patch though. Also, it is worth sending getCompletions('windo') and passing 'false' in includeCommandLineAPI. > LayoutTests/inspector/protocol/runtime-agent-expected.txt:68 > +result = true result should be success: true, right? > LayoutTests/inspector/protocol/runtime-agent-expected.txt:124 > +was called: "RuntimeAgent.getProperties(testObjectId, false, false, InspectorTest.callback)" is this a dupe?

(In reply to comment #2) The idea of the protocol tests is to record the arguments which were used and the format of answer which was receive for the each function. The results produced by this test are showing us the sharp corners of current implementation and the changes you've requested should be the target for other patches. > (From update of attachment 84249 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84249&action=review > > > LayoutTests/http/tests/inspector/inspector-test.js:226 > > +InspectorTest.dumpStepResult = function(step, expression, result) > > Please use InspectorTest.runTestSuite instead (look up for the syntax in other tests). runTestSuite requires much more symbols for doing the same, very simple thing - just call each function and dump all the results. > > > LayoutTests/inspector/protocol/runtime-agent-expected.txt:5 > > +was called: "RuntimeAgent.evaluate('true', 'test', false, InspectorTest.callback)" > > Given that this is a protocol test, it would be nice to dump both: protocol request and response: > > { > request : { > } > > response : { > } > } Will do. > > > LayoutTests/inspector/protocol/runtime-agent-expected.txt:8 > > + objectId : null > > We should stop sending objectId and hasChildren for primitive values. It should be the target for another patch. > > > LayoutTests/inspector/protocol/runtime-agent-expected.txt:19 > > + objectId : { > > Form valid JSON (with "," - need to patch addObject). Will do. > > > LayoutTests/inspector/protocol/runtime-agent-expected.txt:20 > > + injectedScriptId : 1 > > This can become the source of flake. However, if it does not, lets leave it like this. > Btw, addObject allows printing types of members with given names instead of their values. > > > LayoutTests/inspector/protocol/runtime-agent-expected.txt:21 > > + id : 1 > > This can become the source of flake > > > LayoutTests/inspector/protocol/runtime-agent-expected.txt:22 > > + groupName : "test" > > I think we can safely remove groupName from objectId. We would need an id -> groupName map in injected script for getProperties that would be cleared whenever id is cleared. It should be the target for another patch. > > > LayoutTests/inspector/protocol/runtime-agent-expected.txt:34 > > + $ : true > > Two problems here: > 1) it does not list getElementBy* > 2) We should really send these in array of strings. That was we would be able to send "__proto__" suggestion. Should be a separate patch though. It should be the target for another patch. > > Also, it is worth sending getCompletions('windo') and passing 'false' in includeCommandLineAPI. Will do. > > > LayoutTests/inspector/protocol/runtime-agent-expected.txt:68 > > +result = true > > result should be success: true, right? It should be the target for another patch. > > > LayoutTests/inspector/protocol/runtime-agent-expected.txt:124 > > +was called: "RuntimeAgent.getProperties(testObjectId, false, false, InspectorTest.callback)" > > is this a dupe? Nope. I'm checking that object property was assigned by the first setPropertyValue and dumped by the first getProperty was removed by the second setPropertyCall call. Probably this is not a protocol related thing and should be moved to some functional test suite for RuntimeAgent.

Created attachment 84538 [details] [patch] second iteration. > Given that this is a protocol test, it would be nice to dump both: protocol > request and response: > { > request : { > } > response : { > } > } done. > > LayoutTests/inspector/protocol/runtime-agent-expected.txt:20 > > + injectedScriptId : 1 > > This can become the source of flake. However, if it does not, lets leave it > like this. > Btw, addObject allows printing types of members with given names instead of > their values. > > > LayoutTests/inspector/protocol/runtime-agent-expected.txt:21 > > + id : 1 done. > > LayoutTests/inspector/protocol/runtime-agent-expected.txt:34 > > + $ : true > > Two problems here: > 1) it does not list getElementBy* > 2) We should really send these in array of strings. That was we would be able > to send "__proto__" suggestion. Should be a separate patch though. > > Also, it is worth sending getCompletions('windo') and passing 'false' in > includeCommandLineAPI. doesn't work as expected. No idea what is the reason.

Comment on attachment 84538 [details] [patch] second iteration. View in context: https://bugs.webkit.org/attachment.cgi?id=84538&action=review > LayoutTests/http/tests/inspector/protocol-test.js:3 > +InspectorTest.filterProps = function(something, nondeterministicProps) 1. reuse addObject 2. do not use abbreviations (filterProperties) 3. rename something to a more informative name (object?) > LayoutTests/inspector/protocol/runtime-agent-expected.txt:6 > +sent: request: { > LayoutTests/inspector/protocol/runtime-agent-expected.txt:14 > + includeCommandLineAPI : false It'd be better to default this to false. > LayoutTests/inspector/protocol/runtime-agent-expected.txt:18 > +received: response: { > LayoutTests/inspector/protocol/runtime-agent-expected.txt:21 > + domain : "Runtime" don't return domain > LayoutTests/inspector/protocol/runtime-agent-expected.txt:25 > + objectId : null don't return object ids for primitive values. > LayoutTests/inspector/protocol/runtime-agent-expected.txt:28 > + hasChildren : false No need to send this property for primitive values. > LayoutTests/inspector/protocol/runtime-agent-expected.txt:34 > +was called: "RuntimeAgent.evaluate("window","test",false)" nuke "was called" > LayoutTests/inspector/protocol/runtime-agent-expected.txt:58 > + groupName : "test" No need to return groupName. > LayoutTests/inspector/protocol/runtime-agent-expected.txt:62 > + hasChildren : true We should return ownPropertiesCound instead of this boolean flag. Rest can be determined on the caller site. > LayoutTests/inspector/protocol/runtime-agent-expected.txt:68 > +was called: "RuntimeAgent.getCompletions("document.get",true)" We should introduce evaluateAsJSON and use it instead. > LayoutTests/inspector/protocol/runtime-agent-expected.txt:107 > +was called: "RuntimeAgent.getCompletions("window.testObje",true)" Ditto. > LayoutTests/inspector/protocol/runtime-agent-expected.txt:204 > + result : true There should be no output parameters. > LayoutTests/inspector/protocol/runtime-agent-expected.txt:209 > +was called: "RuntimeAgent.setPropertyValue({"injectedScriptId":<number>,"id":<number>,"groupName":"test"},"removed_by_setPropertyValue","")" There is a bug in implementation (weird catch) > LayoutTests/inspector/protocol/runtime-agent-expected.txt:249 > + groupName : "test" groupName: > LayoutTests/inspector/protocol/runtime-agent-expected.txt:332 > +was called: "RuntimeAgent.releaseWrapperObjectGroup(1,"test")" Rename to releaseObjectGroup > LayoutTests/inspector/protocol/runtime-agent.html:44 > + , [ ["evaluate", 'true', 'test', false] This all is too obfuscated and magical. Could we make the format more verbose so that others could understand / use it? > LayoutTests/inspector/protocol/runtime-agent.html:45 > + , ["evaluate", 'window', 'test', false] , should be after argument > LayoutTests/inspector/protocol/runtime-agent.html:49 > + , "InspectorTest.testObjectId = InspectorTest.callResult.objectId" Is this a bug? All other members are arrays. We have not defined InspectorTest.callResult in this test, have we? > LayoutTests/inspector/protocol/runtime-agent.html:56 > + , {'injectedScriptId':true,'id':true} Also a bug: all other members are arrays, we don't have "injectedScriptId" protocol method, do we?

Created attachment 84549 [details] [patch] third version protocol test harness was adjusted.

Created attachment 84558 [details] [patch] fourth version

Created attachment 84560 [details] [patch] fifth version

Comment on attachment 84560 [details] [patch] fifth version Clearing flags on attachment: 84560 Committed r80272: <http://trac.webkit.org/changeset/80272>

All reviewed patches have been landed. Closing bug.

landed as r80341