Bug 25063 - Refactor InspectorController to use ScriptState/Object
Summary: Refactor InspectorController to use ScriptState/Object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on: 24590 24623 24989
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-06 15:50 PDT by Dimitri Glazkov (Google)
Modified: 2009-04-07 15:57 PDT (History)
1 user (show)

See Also:


Attachments
Refactor InspectorController to use ScriptObject/State, v1 (38.87 KB, patch)
2009-04-06 15:58 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Refactor InspectorController to use ScriptObject/State, v2 (46.78 KB, patch)
2009-04-07 11:25 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Refactor InspectorController to use ScriptObject/State, v2.1 (46.54 KB, patch)
2009-04-07 11:35 PDT, Dimitri Glazkov (Google)
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2009-04-06 15:50:40 PDT
This is a continuation of the unfork-refactoring of Inspector effort, started in bug 24524.
Comment 1 Dimitri Glazkov (Google) 2009-04-06 15:58:12 PDT
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(-)
Comment 2 Timothy Hatcher 2009-04-06 16:23:59 PDT
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?
Comment 3 Dimitri Glazkov (Google) 2009-04-06 16:36:24 PDT
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.
Comment 4 Dimitri Glazkov (Google) 2009-04-07 11:25:57 PDT
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(-)
Comment 5 Dimitri Glazkov (Google) 2009-04-07 11:29:20 PDT
Comment on attachment 29314 [details]
Refactor InspectorController to use ScriptObject/State, v2

Oops. Never mind -- need to tweak this one.
Comment 6 Dimitri Glazkov (Google) 2009-04-07 11:35:07 PDT
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(-)
Comment 7 Dimitri Glazkov (Google) 2009-04-07 11:37:15 PDT
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.
Comment 8 Dimitri Glazkov (Google) 2009-04-07 12:26:43 PDT
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! :)
Comment 9 Dimitri Glazkov (Google) 2009-04-07 15:57:22 PDT
Landed as http://trac.webkit.org/changeset/42295 and http://trac.webkit.org/changeset/42296.