Bug 24524 – Introduce ScriptFunctionCall and ScriptObject abstractions

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

[Dimitri Glazkov (Google)]

The reason for this change is two-fold:

1) Unfork Inspector controller by reducing and eventually removing JSC-specific code using lightweight abstractions.

2) Refactor InspectorController to reduce its complexity

I intentionally limited the scope of the change to InspectorDatabaseResource, because it seemed like a good starting point.

[Dimitri Glazkov (Google)]

Created attachment 28496 [details]
Introduce ScriptObject and ScriptFunctionCall abstractions, v1.

 WebCore/ChangeLog                               |   29 ++++++
 WebCore/GNUmakefile.am                          |    5 +
 WebCore/WebCore.pro                             |    2 +
 WebCore/WebCore.vcproj/WebCore.vcproj           |   20 ++++
 WebCore/WebCore.xcodeproj/project.pbxproj       |   20 ++++
 WebCore/WebCoreSources.bkl                      |    2 +
 WebCore/bindings/js/ScriptFunctionCall.cpp      |  122 +++++++++++++++++++++++
 WebCore/bindings/js/ScriptFunctionCall.h        |   63 ++++++++++++
 WebCore/bindings/js/ScriptObject.cpp            |   63 ++++++++++++
 WebCore/bindings/js/ScriptObject.h              |   57 +++++++++++
 WebCore/bindings/js/ScriptObjectQuarantine.h    |   62 ++++++++++++
 WebCore/inspector/InspectorController.cpp       |   87 +---------------
 WebCore/inspector/InspectorController.h         |    4 -
 WebCore/inspector/InspectorDatabaseResource.cpp |   53 +++++++---
 WebCore/inspector/InspectorDatabaseResource.h   |   18 ++--
 15 files changed, 496 insertions(+), 111 deletions(-)

[Sam Weinig]

Why does ScriptObject contain an ExecState?

[Dimitri Glazkov (Google)]

(In reply to comment #2)
> Why does ScriptObject contain an ExecState?
> 

I struggled with this and open to ideas. Current InspectorController holds on to both ContextRef and ObjectRef, and it seemed really appealing to keep them coupled in one object.

But I understand that if we view ScriptObject as a more general abstraction, it seems less logical.

I am very open to ideas here.

[Dimitri Glazkov (Google)]

Comment on attachment 28496 [details]
Introduce ScriptObject and ScriptFunctionCall abstractions, v1.

Taking this out of review queue and marking obsolete. weinig schooled me and I am now on the right path.

[Dimitri Glazkov (Google)]

Created attachment 28533 [details]
Introduce ScriptObject and ScriptFunctionCall abstractions, v2.

 WebCore/ChangeLog                               |   29 ++++++
 WebCore/GNUmakefile.am                          |    5 +
 WebCore/WebCore.pro                             |    2 +
 WebCore/WebCore.vcproj/WebCore.vcproj           |   20 ++++
 WebCore/WebCore.xcodeproj/project.pbxproj       |   20 ++++
 WebCore/WebCoreSources.bkl                      |    2 +
 WebCore/bindings/js/ScriptFunctionCall.cpp      |  121 +++++++++++++++++++++++
 WebCore/bindings/js/ScriptFunctionCall.h        |   64 ++++++++++++
 WebCore/bindings/js/ScriptObject.cpp            |   43 ++++++++
 WebCore/bindings/js/ScriptObject.h              |   52 ++++++++++
 WebCore/bindings/js/ScriptObjectQuarantine.h    |   62 ++++++++++++
 WebCore/inspector/InspectorController.cpp       |   87 +----------------
 WebCore/inspector/InspectorController.h         |    4 -
 WebCore/inspector/InspectorDatabaseResource.cpp |   54 +++++++---
 WebCore/inspector/InspectorDatabaseResource.h   |   19 ++--
 15 files changed, 473 insertions(+), 111 deletions(-)

[Dimitri Glazkov (Google)]

Sam, what do you think? Did I learn my ABCs? :)

[Dimitri Glazkov (Google)]

Created attachment 28584 [details]
Introduce ScriptObject and ScriptFunctionCall abstractions, v2.1.

 WebCore/ChangeLog                               |   29 ++++++
 WebCore/GNUmakefile.am                          |    5 +
 WebCore/WebCore.pro                             |    2 +
 WebCore/WebCore.vcproj/WebCore.vcproj           |   20 ++++
 WebCore/WebCore.xcodeproj/project.pbxproj       |   20 ++++
 WebCore/WebCoreSources.bkl                      |    2 +
 WebCore/bindings/js/ScriptFunctionCall.cpp      |  121 +++++++++++++++++++++++
 WebCore/bindings/js/ScriptFunctionCall.h        |   64 ++++++++++++
 WebCore/bindings/js/ScriptObject.cpp            |   43 ++++++++
 WebCore/bindings/js/ScriptObject.h              |   50 +++++++++
 WebCore/bindings/js/ScriptObjectQuarantine.h    |   62 ++++++++++++
 WebCore/bindings/js/ScriptValue.h               |    1 +
 WebCore/inspector/InspectorController.cpp       |   87 +----------------
 WebCore/inspector/InspectorController.h         |    4 -
 WebCore/inspector/InspectorDatabaseResource.cpp |   54 +++++++---
 WebCore/inspector/InspectorDatabaseResource.h   |   19 ++--
 16 files changed, 472 insertions(+), 111 deletions(-)

[Dimitri Glazkov (Google)]

New patch up, addressing some of the comments on IRC:

* ScriptObject now inherits from ScriptValue. Pretty.
* I thought about changing "bind/unbind" method names, but couldn't come up with anything that's not verbose. I am open to ideas here, but I think this is pretty intuitive already.
* Going away from JSC API. I understand the dogfoodiness aspect of it, and I appreciate the desire. In fact, my using bindings functions was merely an attempt to homogenize the code base, not a direct desire to get rid of it.

So, here's my idea: perhaps we could convert all Script* objects to use JSC API? This could be a nice little GSoC project? :)

[Timothy Hatcher]

Comment on attachment 28584 [details]
Introduce ScriptObject and ScriptFunctionCall abstractions, v2.1.

Looks good, only two comments.

Don't indent the functions in the .cpp files inside the namespace block.

And getQuarantinedScriptObject seems a little large to be an inline function, so that should likely have a .cpp file to contain the implementation.

[Dimitri Glazkov (Google)]

Comments addressed, landed as http://trac.webkit.org/changeset/41684.