Bug 25419 – InspectorController refactoring proposal

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

I started putting up a proposal for the InspectorController refactoring. I did not want to post it in here since I wanted some color-coding and some comment capabilities. Please tell me if this works for you, otherwise I will re-post it in here. Anyway, here is the link:

http://docs.google.com/Doc?id=dcf65mvz_0dp6xt4c8

[Pavel Feldman]

Hi Timothy,

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

Thanks
Pavel

[Sam Weinig]

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?  

[Pavel Feldman]

(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.

[Timothy Hatcher]

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.

[Pavel Feldman]

(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.

[Timothy Hatcher]

(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.

[Pavel Feldman]

(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?

[Timothy Hatcher]

That sounds good!

[Pavel Feldman]

Created attachment 30186 [details]
patch

Patch with the first step of the refactoring.

[Pavel Feldman]

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

[Timothy Hatcher]

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.

[Pavel Feldman]

Created attachment 30194 [details]
patch

Addressed Timothy's comments.

[Pavel Feldman]

Created attachment 30240 [details]
step_2_patch

This patch wraps ScriptObject and ScriptState* or further serialization in InspectorFrontend.

[Timothy Hatcher]

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?

[Pavel Feldman]

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.

[Pavel Feldman]

Created attachment 30274 [details]
step_2_patch

(fixed missed rename in WebCore.pro).

[Pavel Feldman]

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.

[Timothy Hatcher]

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?

[Pavel Feldman]

(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.

[Eric Seidel (no email)]

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

[Timothy Hatcher]

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.

[Pavel Feldman]

(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 :)

[Timothy Hatcher]

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?

[Pavel Feldman]

(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.

[Dimitri Glazkov (Google)]

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

[Pavel Feldman]

(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!

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

Comment on attachment 30274 [details]
step_2_patch

Second patch bit-rotted. Pavel uploading a new one.

[Pavel Feldman]

Created attachment 30475 [details]
step_2_patch

[Dimitri Glazkov (Google)]

Comment on attachment 30475 [details]
step_2_patch

transferring xenon's review flag.

[Pavel Feldman]

Created attachment 30477 [details]
step_2_patch

[Pavel Feldman]

Created attachment 30478 [details]
step_3_patch

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

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