Bug 25063 – Refactor InspectorController to use ScriptState/Object

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

[Dimitri Glazkov (Google)]

This is a continuation of the unfork-refactoring of Inspector effort, started in bug 24524.

[Dimitri Glazkov (Google)]

Created attachment 29291 [details]
Refactor InspectorController to use ScriptObject/State, v1

 WebCore/ChangeLog                                  |   24 ++
 WebCore/GNUmakefile.am                             |    1 +
 WebCore/WebCore.pro                                |    1 +
 WebCore/WebCore.vcproj/WebCore.vcproj              |    4 +
 WebCore/WebCore.xcodeproj/project.pbxproj          |    4 +
 WebCore/WebCoreSources.bkl                         |    1 +
 .../bindings/js/JSInspectorControllerCustom.cpp    |    2 +-
 WebCore/bindings/js/ScriptObject.cpp               |   30 +++
 WebCore/bindings/js/ScriptObject.h                 |   10 +
 WebCore/bindings/js/ScriptState.cpp                |   44 ++++
 WebCore/bindings/js/ScriptState.h                  |    3 +
 WebCore/inspector/InspectorController.cpp          |  232 ++++++--------------
 WebCore/inspector/InspectorController.h            |   16 +-
 13 files changed, 197 insertions(+), 175 deletions(-)

[Timothy Hatcher]

Comment on attachment 29291 [details]
Refactor InspectorController to use ScriptObject/State, v1

It would be nice if !m_webInspector meant m_webInspector.hasNoValue(). It would make lines like the following more readable.

     if (m_scriptState && !m_webInspector.hasNoValue())

What are we losing with the removal of handleException? Are exceptions that happen in the Inspector JavaScript code still reported in it's Console?

[Dimitri Glazkov (Google)]

Comment on attachment 29291 [details]
Refactor InspectorController to use ScriptObject/State, v1

Good idea on hasNoValue -> !value. As for handleException -- you have reminded me I need to be careful and get this straight. The idea was that any set/get/call failures in Script* world use reportException, but this obviously has some re-entrancy implications for ConsoleMessage::addToConsole. Updated patch coming up.

[Dimitri Glazkov (Google)]

Created attachment 29314 [details]
Refactor InspectorController to use ScriptObject/State, v2

 WebCore/ChangeLog                                  |   35 +++
 WebCore/GNUmakefile.am                             |    1 +
 WebCore/WebCore.pro                                |    1 +
 WebCore/WebCore.vcproj/WebCore.vcproj              |    4 +
 WebCore/WebCore.xcodeproj/project.pbxproj          |    4 +
 WebCore/WebCoreSources.bkl                         |    1 +
 .../bindings/js/JSInspectorControllerCustom.cpp    |    2 +-
 WebCore/bindings/js/ScriptFunctionCall.cpp         |   20 ++-
 WebCore/bindings/js/ScriptFunctionCall.h           |    5 +-
 WebCore/bindings/js/ScriptObject.cpp               |   53 ++++-
 WebCore/bindings/js/ScriptObject.h                 |   10 +
 WebCore/bindings/js/ScriptState.cpp                |   44 ++++
 WebCore/bindings/js/ScriptState.h                  |    3 +
 WebCore/inspector/ConsoleMessage.cpp               |    4 +-
 WebCore/inspector/InspectorController.cpp          |  235 ++++++--------------
 WebCore/inspector/InspectorController.h            |   17 +-
 16 files changed, 244 insertions(+), 195 deletions(-)

[Dimitri Glazkov (Google)]

Comment on attachment 29314 [details]
Refactor InspectorController to use ScriptObject/State, v2

Oops. Never mind -- need to tweak this one.

[Dimitri Glazkov (Google)]

Created attachment 29315 [details]
Refactor InspectorController to use ScriptObject/State, v2.1

 WebCore/ChangeLog                                  |   35 +++
 WebCore/GNUmakefile.am                             |    1 +
 WebCore/WebCore.pro                                |    1 +
 WebCore/WebCore.vcproj/WebCore.vcproj              |    4 +
 WebCore/WebCore.xcodeproj/project.pbxproj          |    4 +
 WebCore/WebCoreSources.bkl                         |    1 +
 .../bindings/js/JSInspectorControllerCustom.cpp    |    2 +-
 WebCore/bindings/js/ScriptFunctionCall.cpp         |   20 ++-
 WebCore/bindings/js/ScriptFunctionCall.h           |    5 +-
 WebCore/bindings/js/ScriptObject.cpp               |   53 ++++-
 WebCore/bindings/js/ScriptObject.h                 |   10 +
 WebCore/bindings/js/ScriptState.cpp                |   44 ++++
 WebCore/bindings/js/ScriptState.h                  |    3 +
 WebCore/inspector/ConsoleMessage.cpp               |    4 +-
 WebCore/inspector/InspectorController.cpp          |  231 ++++++--------------
 WebCore/inspector/InspectorController.h            |   17 +-
 16 files changed, 242 insertions(+), 193 deletions(-)

[Dimitri Glazkov (Google)]

Updated and improved:

 * I opted to add hasWebInspector() to simplify condition logic. I promise it wasn't because I was lazy to add the operators to ScriptValue :). I just thought it would be a bit more descriptive as to what is actually going on.

 * I made sure that exceptions are reported and any re-entrancy issues are addressed.

[Dimitri Glazkov (Google)]

One thing to mention -- I consciously changed handling of exceptions a little bit. They are now routed to inspector of inspector. In other words, exceptions in the inspector JS will be seen in the console of its inspector -- to see them, you need to inspect the inspector! :)

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/42295 and http://trac.webkit.org/changeset/42296.