Bug 40675 – Web Inspector: native serialization support in the bindings for WebInspector.

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

[Ilya Tikhonovsky]

On the way to Remote Debuging we want to support JSON serialization on both sides of WebInspector transport.
As far as InspectorFrontend class is a simple proxy to WebInspector it would be better to generate it from an IDL file.
We have generator infostructure for binding and will reuse it for new generator.

[Ilya Tikhonovsky]

Created attachment 58890 [details]
[patch] initial version.

[Yury Semikhatsky]

Comment on attachment 58890 [details]
[patch] initial version. 


WebCore/WebCore.gyp/WebCore.gyp:624
 +              'generator_include_dirs': [
why do you need this?

WebCore/bindings/scripts/CodeGeneratorWI.pm:108
 +   * 1. Redistributions of source code must retain the above copyright
Please fix the license header.

WebCore/bindings/scripts/IDLStructure.pm:83
 +  our $supportedTypes = "((?:unsigned )?(?:int|short|(?:long )?long)|(?:$idlIdNs*))";
revert?

WebCore/inspector/InspectorFrontend2.wii:72
 +  //        void pausedScript(SerializedScriptValue* callFrames);
remove all commented code
WebCore/inspector/TimelineRecordFactory.cpp:53
 +  //    if (ScriptCallStack::stackTrace(5, frontend->scriptState(), stackTrace))
Please uncomment this. Also I don't understand why ScriptState used here is that of the front end.

WebKit/gtk/ChangeLog:5
 +          WebInspector: bugfix.
Change log should be more descriptive.

WebKit/qt/ChangeLog:5
 +          WebInspector: bugfix.
Change log should be more descriptive.

WebCore/CMakeLists.txt:290
 +      inspector/InspectorFrontend2.wii
I really don't like the idea of using .wii extension for idl files just to associate them with another code generator. Is it possible to leave .idl extension for them?

[WebKit Review Bot]

Attachment 58890 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3314227

[Pavel Feldman]

Comment on attachment 58890 [details]
[patch] initial version. 

Our IDL is not for bindings, so that the generation process should be separate. I don't think we can get away with current approach.

[Ilya Tikhonovsky]

Created attachment 58975 [details]
[patch] second iteration

.wii extension was changed to .inspector
CodeGeneratorWI.pm was moved to inspector folder as CodeGeneratorInspector.pm
StackTrace support was implemented.
Other minor changes.

[Yury Semikhatsky]

Comment on attachment 58975 [details]
[patch] second iteration

WebCore/WebCore.gyp/WebCore.gyp:613
 +              '../inspector/CodeGeneratorInspector.pm',
Can we put this to inspector/scripts/ the same way as with bindings?

WebCore/bindings/scripts/generate-bindings.pl:59
 +  #die('Must specify IDL search path.') unless @idlDirectories;
Remove this line?

WebCore/bindings/v8/ScriptCallStack.cpp:100
 +      v8::Context::Scope contextScope(v8::Context::GetCurrent());
There may be no current context. Please add a check.

WebCore/inspector/CodeGeneratorInspector.pm:1
 +  # Copyright (C) 2005, 2006 Nikolas Zimmermann <[e-mail]>
Please change the license header.

WebCore/CMakeLists.txt:290
 +      inspector/InspectorFrontend2.inspector
Maybe .inspectoridl or something like that, it would be too long though.

[Ilya Tikhonovsky]

Created attachment 58978 [details]
[patch] second iteration. rebaselined.

[WebKit Review Bot]

Attachment 58978 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/inspector/TimelineRecordFactory.cpp:55:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ilya Tikhonovsky]

Comment on attachment 58978 [details]
[patch] second iteration. rebaselined.

two files are missing

[Ilya Tikhonovsky]

Created attachment 58979 [details]
[patch] third iteration. with new files.

[Early Warning System Bot]

Attachment 58978 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3285276

[WebKit Review Bot]

Attachment 58979 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3319262

[Yury Semikhatsky]

Please address the comments at least those regarding v8::Context.

[Ilya Tikhonovsky]

Created attachment 58993 [details]
[patch] fourth iteration.

inspector -> inspectorIDL
ScriptCallStack was fixed.
License was fixed.

[Yury Semikhatsky]

Comment on attachment 58993 [details]
[patch] fourth iteration.

WebCore/bindings/v8/ScriptCallStack.cpp:102
 +      if (context.IsEmpty())
You don't need this check because of the one above. Please remove.

WebCore/bindings/v8/ScriptCallStack.cpp:104
 +      v8::HandleScope scope;
This line should go before v8::Context::GetCurrent() call which returns a local handle.

r- for that, otherwise looks good.

[Pavel Feldman]

Comment on attachment 58993 [details]
[patch] fourth iteration.

WebCore/GNUmakefile.am:324
 +  INSPECTOR_INTERFACES += WebCore/inspector/InspectorFrontend2.inspectorIDL
Could we please use standard .idl extension?

WebCore/WebCore.gypi:197
 +              'inspector/InspectorFrontend2.inspectorIDL',
We can create a separate folder for new idl files and call it 'remote' if this makes life of your code generator easier.


WebCore/WebCore.vcproj/WebCore.vcproj:20068
 +  				RelativePath="$(WebKitOutputDir)\obj\$(ProjectName)\DerivedSources\WebInspectorFrontend2.cpp"
Nit: I would use 'Remote' as a part of the name. Such as 'RemoteInspectorFrontend'.

WebCore/inspector/InspectorTimelineAgent.cpp: 
 +          ScriptObject record = TimelineRecordFactory::createGenericRecord(m_frontend, i->startTime);
I think apu agent is sniffing those on the chromium side. So the change is going to break SpeedTracer and alike. r- for that.

[Joseph Pecoraro]

> +$typeTransform{"Object"} = {
> +    "param" => "const RefPtr<InspectorObject>&",
> +    "return" => "PassRefPtr<InspectorObject>",
> +    "forward" => "InspectorObject",
> +    "header" => "InspectorValues.h",
> +    "push" => "push"
> +};

This uses "return" and all the others use "retVal".
Easy to overlook because, as expected, the entire
interface returns "void" =)

---------------------------------

I see some missing functions in InspectorFrontend2.inspectorIDL that are
in InspectorFrontend.h. I take it, based on their non-simple implementations
that these would need to be manually implemented. Some of them have simple
implementations. But would it be worth adding these to the inspectorIDL
file, but commenting them out so they are remembered?

  ------
    void addConsoleMessage(const ScriptObject& messageObj, const Vector<ScriptString>& frames, const Vector<RefPtr<SerializedScriptValue> >& arguments, const String& message);
    void pausedScript(SerializedScriptValue* callFrames);

  #if ENABLE(WORKERS)
    void didCreateWorker(const InspectorWorkerResource&);
    void didDestroyWorker(const InspectorWorkerResource&);
  #endif // ENABLE(WORKER)
  
    void didEditScriptSource(long callId, bool success, const String& result, SerializedScriptValue* newCallFrames);
    void didGetScriptSource(long callId, const String& result);
    void didDispatchOnInjectedScript(long callId, SerializedScriptValue* result, bool isException);
  ------


Also, in InspectorFrontend these returns a bool, but its void in
the new version:

  ------ 
    bool updateResource(unsigned long identifier, const ScriptObject& resourceObj);
    bool addDatabase(const ScriptObject& dbObj);
    bool addDOMStorage(const ScriptObject& domStorageObj);
  ------
    void updateResource(in unsigned long identifier, in Object resourceObj);
    void addDatabase(in Object dbObj);
    void addDOMStorage(in Object domStorageObj);
  ------

It looks like the addDOMStorage's return value is never used, so that
could certainly be changed to void in both files for now. I'm guessing
the others as well. I think it would be worth changing the original
file to void (or the new version to bool) to keep them in sync again.


Many of the build file changes look like pure magic to me, so I'm
trusting all of your judgement =)

[Ilya Tikhonovsky]

(In reply to comment #17)
> > +$typeTransform{"Object"} = {
> > +    "param" => "const RefPtr<InspectorObject>&",
> > +    "return" => "PassRefPtr<InspectorObject>",
> > +    "forward" => "InspectorObject",
> > +    "header" => "InspectorValues.h",
> > +    "push" => "push"
> > +};
> 
> This uses "return" and all the others use "retVal".
> Easy to overlook because, as expected, the entire
> interface returns "void" =)

fixed.

> I see some missing functions in InspectorFrontend2.inspectorIDL that are
> in InspectorFrontend.h. I take it, based on their non-simple implementations
> that these would need to be manually implemented. Some of them have simple
> implementations. But would it be worth adding these to the inspectorIDL
> file, but commenting them out so they are remembered?

I'll keep only one currently used function. All the others will be added later in order of changing corresponding *inspector*Agents.

[Ilya Tikhonovsky]

Created attachment 59186 [details]
[patch] next iteration.

idl extension is used.

[WebKit Review Bot]

Attachment 59186 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3328380

[Ilya Tikhonovsky]

Created attachment 60517 [details]
[patch] rebaselined. for check at trybots.

[WebKit Review Bot]

Attachment 60517 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3409251

[Yury Semikhatsky]

Comment on attachment 60517 [details]
[patch] rebaselined. for check at trybots.

WebCore/bindings/js/ScriptCallStack.h:60
 +          static bool stackTrace(int, const RefPtr<InspectorArray>&);
Changes related to ScriptCallStack may well go in its own patch. Could you file a separate bug and create a separate patch for that? This would reduce the size of this huge patch.

[Ilya Tikhonovsky]

Created attachment 60538 [details]
[patch] Minimized version without generator.

[Yury Semikhatsky]

Comment on attachment 60538 [details]
[patch] Minimized version without generator.

WebCore/ChangeLog:9
 +          infostructure for binding and will reuse it for new generator.
typo: infostructure -> infrastructure

[Ilya Tikhonovsky]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/ScriptCallStack.cpp
	M	WebCore/bindings/js/ScriptCallStack.h
	M	WebCore/bindings/v8/ScriptCallStack.cpp
	M	WebCore/bindings/v8/ScriptCallStack.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/InspectorTimelineAgent.cpp
	M	WebCore/inspector/InspectorTimelineAgent.h
	M	WebCore/inspector/TimelineRecordFactory.cpp
	M	WebCore/inspector/TimelineRecordFactory.h
	M	WebKit/chromium/src/WebDevToolsAgentImpl.cpp
	M	WebKit/gtk/ChangeLog
	M	WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp
	M	WebKit/qt/ChangeLog
	M	WebKit/qt/WebCoreSupport/InspectorClientQt.cpp
Committed r62542

[Joseph Pecoraro]

I think all those functions returning a RefPtr should instead return a PassRefPtr,
by doing "return data.release()". See the examples on:
http://webkit.org/coding/RefPtr.html

> If a function’s result is a new object or ownership is being
> transferred for any other reason, the result should be a PassRefPtr.
> Since local variables are typically RefPtr, it’s common to call
> release in the return statement to transfer the RefPtr to the
> PassRefPtr.

I think the current approach does extra reference count arithmetic.