Bug 25419

Hi Timothy, Did you have a chance to look at this? If it sounds good I will start sending patches your way. Thanks Pavel

I would encourage doing this a bit more piecemeal. My preference would be to see the most important part, the proxy layer, added first, using the current API. Is there a reason the re-factoring needs to come before the proxy layer?

(In reply to comment #2) > I would encourage doing this a bit more piecemeal. My preference would be to > see the most important part, the proxy layer, added first, using the current > API. Is there a reason the re-factoring needs to come before the proxy layer? > That is exactly the plan. The thing is that refactoring suggested in step (1) is really small, but allows to narrow the proxy API significantly (proxy will not need to support JS objects instantiation after it). Let me add a step 1.5 where explicit proxy interface is being defined that will encapsulate serialization taking place in further steps. Is that what you are suggesting? But in either case, proxy needs to operate serialized values and that is where step (2) becomes necessary. Then one will need to dispatch serialized messages and step (3) is a proposal on how it should be handled. I would like to agree on the overall plan before I start doing anything. Beside the high level agreement, I need things like a green light on adding 'value' and JSON serialization.

I think a good approach would be to do step 1 (and maybe 2) now using the existing direct JS call approach. That way we know how well it works early on. Basically I think we should do each separable step in stages and land them. It would also be best to have separate patches for JSON with standalone tests. But maybe bug 20031 will give us the right JSON code. Over all I think the proposal makes sense.

(In reply to comment #4) > I think a good approach would be to do step 1 (and maybe 2) now using the > existing direct JS call approach. That way we know how well it works early on. > > Basically I think we should do each separable step in stages and land them. > > It would also be best to have separate patches for JSON with standalone tests. > But maybe bug 20031 will give us the right JSON code. > > Over all I think the proposal makes sense. > Ok, sure, let us start with step 1 and see how it goes. Actually, step 2 without migrating to Values is pretty much a noop. It would be great if 20031 addressed it, but as I understand, it is going to be resolved in JSC terms. Hence we would still need to find a platform (JS core)-independent serialization engine. We could try doing it from within JS: InspectorController receives a network event (didReceiveResponse), it passes it into javascript for serialization by means of Scrip* objects, javascript code serializes message. Quite an indirection, but no code duplication in the JSON serialization area. Also could have some performance implications. Do you think it is better than pulling Value/JSONReader/JSONWriter in? I think I can run some experiments on it.

(In reply to comment #5) > Ok, sure, let us start with step 1 and see how it goes. Actually, step 2 > without migrating to Values is pretty much a noop. It would be great if 20031 > addressed it, but as I understand, it is going to be resolved in JSC terms. > Hence we would still need to find a platform (JS core)-independent > serialization engine. > > We could try doing it from within JS: InspectorController receives a network > event (didReceiveResponse), it passes it into javascript for serialization by > means of Scrip* objects, javascript code serializes message. Quite an > indirection, but no code duplication in the JSON serialization area. Also could > have some performance implications. Do you think it is better than pulling > Value/JSONReader/JSONWriter in? I think I can run some experiments on it. We don't want two separate JSON implementations in WebKit. Bug 20031 is adding one to JSC, where it belongs. We don't want a seperate one up in WebCore just for the Inspector. What we could do is have JSON bridging classes like the Script classes Dimitri has added. So the Inspector can work with V8 or JSC's JSON parser/writer without tons of #ifdefs.

(In reply to comment #6) > (In reply to comment #5) > > Ok, sure, let us start with step 1 and see how it goes. Actually, step 2 > > without migrating to Values is pretty much a noop. It would be great if 20031 > > addressed it, but as I understand, it is going to be resolved in JSC terms. > > Hence we would still need to find a platform (JS core)-independent > > serialization engine. > > > > We could try doing it from within JS: InspectorController receives a network > > event (didReceiveResponse), it passes it into javascript for serialization by > > means of Scrip* objects, javascript code serializes message. Quite an > > indirection, but no code duplication in the JSON serialization area. Also could > > have some performance implications. Do you think it is better than pulling > > Value/JSONReader/JSONWriter in? I think I can run some experiments on it. > > We don't want two separate JSON implementations in WebKit. Bug 20031 is adding > one to JSC, where it belongs. We don't want a seperate one up in WebCore just > for the Inspector. What we could do is have JSON bridging classes like the > Script classes Dimitri has added. So the Inspector can work with V8 or JSC's > JSON parser/writer without tons of #ifdefs. > Ok, I did not realize that ScriptObject is that mature already. Here is the new plan then: - I am substituting Value with ScriptObject in the design doc - ScriptObject gets "String ScriptObject::stringify" and a factory "parse" method. - Both WebKit and Chromium use native JSON serialization behind the scenes. Does this sound good to you?

That sounds good!

Created attachment 30186 [details] patch Patch with the first step of the refactoring.

Sending you the first patch for review (2 more to follow).

Comment on attachment 30186 [details] patch I like this patch and it is a nice and mechanical change. I have a few comments and nitpicks, but over all nothing major. There are a couple of tab characters sprinkled in this patch. Please remove them, otherwise it will fail to commit. > + , m_bound(false) I prefer the m_scriptObjectCreated variable name you used for InspectorResource over m_bound. > + initBody: function(args) I think setMessageBody would be a better name. r- for the tabs. But please consider the other comments.

Created attachment 30194 [details] patch Addressed Timothy's comments.

Created attachment 30240 [details] step_2_patch This patch wraps ScriptObject and ScriptState* or further serialization in InspectorFrontend.

Comment on attachment 30240 [details] step_2_patch This patch does not include the JsonObject files. Also JsonObject should be named JSONObject per our style guidelines. ("Capitalize the first letter, including all letters in an acronym, in a class, struct, protocol, or namespace name.") 80 addDOMStorage.appendArgument(jsonObject.scriptObject()); I assume that scriptObject returns a string with the serialized JSON. If so maybe including "serialized" in the function would make things clear. Wont this require changes on the ForntEnd side too?

Created attachment 30273 [details] step_2_patch >> Also JsonObject should be named JSONObject per our style guidelines. >> ("Capitalize the first letter, including all letters in an acronym, >> in a class, struct, protocol, or namespace name.") Done. >> 80 addDOMStorage.appendArgument(jsonObject.scriptObject()); >> I assume that scriptObject returns a string with the serialized JSON. >> If so maybe including "serialized" in the function would make things clear. Not yet - it is returning underlying script object so far. It will have 'stringify' and 'parse' later. >> Wont this require changes on the ForntEnd side too? Again, not yet. Sorry about missing JSON* files - fell off my git cycle.

Created attachment 30274 [details] step_2_patch (fixed missed rename in WebCore.pro).

Created attachment 30277 [details] step_3_patch Split InspectorController into InspectorController and InspectorFrontend. Latter encapsulates all frontend interaction and is the only entity allowed to make ScriptFunctionCalls. The further plan is to serialize these script function calls.

Comment on attachment 30277 [details] step_3_patch 66 m_webInspector = ScriptObject(); It seems odd to create a new object during destruction. Is there a better way to do this?

(In reply to comment #18) > (From update of attachment 30277 [details] [review]) > 66 m_webInspector = ScriptObject(); > > It seems odd to create a new object during destruction. Is there a better way > to do this? > ScriptObject is supposed to be just a handle. And it is no change from what it was (see InspectorController:637). But I agree that it looks weird, so will look for better way in the meanwhile.

It looks like these are something for Timothy to land. Assigning to him.

These are not ready to land, they are steps that all need landed together to prevent breaking the Inspector. There are more steps to come and I assume someone at Google will land them when they are all done and reviewed.

(In reply to comment #21) > These are not ready to land, they are steps that all need landed together to > prevent breaking the Inspector. There are more steps to come and I assume > someone at Google will land them when they are all done and reviewed. > Timothy, these three are self-contained and are not breaking. I am currently working on ScriptArray and JSONMessage for further serialization. I think it would be nice if these were landed so that we avoid potential merging. And I think that landing person should be either Dmitry or yourself so that you can see if anything goes wrong. Or make me a committer so that I could baby-sit it :)

OK, that is good. I guess deep down I knew that, but wasn't thinking. I wont be able to land them today, due to other priorities. But feel free to get someone else to. Dimitri?

(In reply to comment #23) > OK, that is good. I guess deep down I knew that, but wasn't thinking. > > I wont be able to land them today, due to other priorities. But feel free to > get someone else to. Dimitri? > No worries, this is not urgent at all. I'll ping Dmitry/yourself in some time again.

I'll land. Can we do this Monday morning (PST), so that you (Pavel) could watch the tree?

(In reply to comment #25) > I'll land. Can we do this Monday morning (PST), so that you (Pavel) could watch > the tree? > SGTM, thanks!

Patch 1 landed as http://trac.webkit.org/changeset/43859.

Comment on attachment 30274 [details] step_2_patch Second patch bit-rotted. Pavel uploading a new one.

Created attachment 30475 [details] step_2_patch

Comment on attachment 30475 [details] step_2_patch transferring xenon's review flag.

Created attachment 30477 [details] step_2_patch

Created attachment 30478 [details] step_3_patch

Step 2 landed as http://trac.webkit.org/changeset/43861.

Step 3 landed as http://trac.webkit.org/changeset/43864.