Bug 25419

Summary: InspectorController refactoring proposal
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Pavel Feldman <pfeldman>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, emacemac7, jamesr, michaeln, nickshanks, sam, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://docs.google.com/Doc?id=dcf65mvz_0dp6xt4c8
Bug Depends on: 20031    
Bug Blocks:    
Attachments:
Description Flags
patch
timothy: review-
patch
timothy: review+
step_2_patch
none
step_2_patch
none
step_2_patch
timothy: review+
step_3_patch
timothy: review+
step_2_patch
dglazkov: review+
step_2_patch
dglazkov: review+
step_3_patch dglazkov: review+

Description Pavel Feldman 2009-04-27 04:29:23 PDT
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
Comment 1 Pavel Feldman 2009-04-28 15:01:14 PDT
Hi Timothy,

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

Thanks
Pavel
Comment 2 Sam Weinig 2009-04-28 16:26:16 PDT
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?  
Comment 3 Pavel Feldman 2009-04-28 22:50:38 PDT
(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.
Comment 4 Timothy Hatcher 2009-04-30 08:55:49 PDT
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.
Comment 5 Pavel Feldman 2009-04-30 09:19:22 PDT
(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.
Comment 6 Timothy Hatcher 2009-04-30 10:26:03 PDT
(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.
Comment 7 Pavel Feldman 2009-04-30 12:05:30 PDT
(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?
Comment 8 Timothy Hatcher 2009-04-30 12:12:58 PDT
That sounds good!
Comment 9 Pavel Feldman 2009-05-11 05:49:29 PDT
Created attachment 30186 [details]
patch

Patch with the first step of the refactoring.
Comment 10 Pavel Feldman 2009-05-11 05:51:13 PDT
Sending you the first patch for review (2 more to follow).
Comment 11 Timothy Hatcher 2009-05-11 10:07:11 PDT
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.
Comment 12 Pavel Feldman 2009-05-11 11:07:44 PDT
Created attachment 30194 [details]
patch

Addressed Timothy's comments.
Comment 13 Pavel Feldman 2009-05-12 10:23:04 PDT
Created attachment 30240 [details]
step_2_patch

This patch wraps ScriptObject and ScriptState* or further serialization in InspectorFrontend.
Comment 14 Timothy Hatcher 2009-05-12 12:42:00 PDT
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?
Comment 15 Pavel Feldman 2009-05-13 06:42:54 PDT
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.
Comment 16 Pavel Feldman 2009-05-13 06:45:10 PDT
Created attachment 30274 [details]
step_2_patch

(fixed missed rename in WebCore.pro).
Comment 17 Pavel Feldman 2009-05-13 08:42:26 PDT
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 18 Timothy Hatcher 2009-05-13 09:54:37 PDT
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?
Comment 19 Pavel Feldman 2009-05-13 10:12:18 PDT
(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.
Comment 20 Eric Seidel (no email) 2009-05-15 00:47:31 PDT
It looks like these are something for Timothy to land.  Assigning to him.
Comment 21 Timothy Hatcher 2009-05-15 06:23:14 PDT
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.
Comment 22 Pavel Feldman 2009-05-15 06:30:14 PDT
(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 :)
Comment 23 Timothy Hatcher 2009-05-15 06:36:00 PDT
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?
Comment 24 Pavel Feldman 2009-05-15 06:53:22 PDT
(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.
Comment 25 Dimitri Glazkov (Google) 2009-05-15 09:16:17 PDT
I'll land. Can we do this Monday morning (PST), so that you (Pavel) could watch the tree?
Comment 26 Pavel Feldman 2009-05-15 11:17:06 PDT
(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!
Comment 27 Dimitri Glazkov (Google) 2009-05-19 09:08:36 PDT
Patch 1 landed as http://trac.webkit.org/changeset/43859.
Comment 28 Dimitri Glazkov (Google) 2009-05-19 09:15:08 PDT
Comment on attachment 30274 [details]
step_2_patch

Second patch bit-rotted. Pavel uploading a new one.
Comment 29 Pavel Feldman 2009-05-19 09:36:57 PDT
Created attachment 30475 [details]
step_2_patch
Comment 30 Dimitri Glazkov (Google) 2009-05-19 09:37:35 PDT
Comment on attachment 30475 [details]
step_2_patch

transferring xenon's review flag.
Comment 31 Pavel Feldman 2009-05-19 09:40:56 PDT
Created attachment 30477 [details]
step_2_patch
Comment 32 Pavel Feldman 2009-05-19 09:44:14 PDT
Created attachment 30478 [details]
step_3_patch
Comment 33 Dimitri Glazkov (Google) 2009-05-19 09:46:29 PDT
Step 2 landed as http://trac.webkit.org/changeset/43861.
Comment 34 Dimitri Glazkov (Google) 2009-05-19 09:59:08 PDT
Step 3 landed as http://trac.webkit.org/changeset/43864.