Bug 34204 - Web Inspector: Migrate from ScriptObject to InspectorValue as the InspectorFrontend / backend interchange object.
Summary: Web Inspector: Migrate from ScriptObject to InspectorValue as the InspectorFr...
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: Pavel Feldman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-27 00:36 PST by Pavel Feldman
Modified: 2010-06-03 13:29 PDT (History)
10 users (show)

See Also:


Attachments
[WIP Patch] Handing over to Ilya. (133.16 KB, patch)
2010-05-14 06:54 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
[patch] initial version. (32.22 KB, patch)
2010-05-24 05:19 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
[patch] Classes for native serialization to JSON. (20.15 KB, patch)
2010-05-31 06:56 PDT, Ilya Tikhonovsky
pfeldman: review+
Details | Formatted Diff | Diff
[patch] second iteration (20.09 KB, patch)
2010-05-31 07:30 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
[patch] third iteration. With fixes for linux build. (20.09 KB, patch)
2010-05-31 07:39 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
[patch] fourth iteration. With fixed mac export list. (19.69 KB, patch)
2010-05-31 07:53 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2010-01-27 00:36:13 PST
Currently InjectedScript operates ScriptValue/Object and is running within inspected page's context. It is safe to pass objects between injected script and the page.
Frontend is running in its own context, entirely isolated from the inspected page. We should make sure that JS objects do not cross the boundary and I suggest that we enforce it via introducing explicit FrontendValue/Object classes (derived from ScriptValue/Object). InspectorFrontend then will only accept objects of this type.
Comment 1 Pavel Feldman 2010-05-14 06:54:54 PDT
Created attachment 56066 [details]
[WIP Patch] Handing over to Ilya.
Comment 2 Ilya Tikhonovsky 2010-05-24 05:19:50 PDT
Created attachment 56874 [details]
[patch] initial version.

It is the first patch with implementation for InspectorValues classes.
The changes for TimelineAgent, DOMAgent etc. will be processed separately.
Comment 3 Yury Semikhatsky 2010-05-24 06:00:36 PDT
Comment on attachment 56874 [details]
[patch] initial version.

WebCore/inspector/InspectorFrontend.h:53
 +          virtual void dispatchMessageToInspector(const String& message) = 0;
We already have InspectorFrontendClient which serves as the frontend delegate, this method should go there.

WebCore/inspector/InspectorValues.cpp:43
 +      JSONBuffer(size_t preallocate) : m_position(0), m_buffer(preallocate) { }
Should be possible to use Vector instead of this class, it has capacity in addition to size. The only issue there may be fixed resize strategy.

WebCore/inspector/InspectorValues.h:117
 +      explicit InspectorValue(bool value) : InspectorBaseValue(TypeBoolean) { m_boolValue = value; }
should other constructors be explicit?

WebCore/inspector/InspectorValues.h:103
 +  class InspectorValue : public InspectorBaseValue {
I think this should be InspectorPromitiveValue or something, InspectorValue is too generic.

Can we have tests for the JSON stuff?
Comment 4 Ilya Tikhonovsky 2010-05-31 06:56:02 PDT
Created attachment 57459 [details]
[patch] Classes for native serialization to JSON.
Comment 5 Pavel Feldman 2010-05-31 07:10:24 PDT
Comment on attachment 57459 [details]
[patch] Classes for native serialization to JSON.

WebCore/inspector/InspectorValues.h:47
 +  class InspectorBaseValue : public RefCounted<InspectorBaseValue> {
InspectorValue


WebCore/inspector/InspectorValues.h:78
 +  class InspectorValue : public InspectorBaseValue {
InspectorBasicValue?
Comment 6 Early Warning System Bot 2010-05-31 07:11:38 PDT
Attachment 57459 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2797010
Comment 7 WebKit Review Bot 2010-05-31 07:29:31 PDT
Attachment 57459 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2822008
Comment 8 Ilya Tikhonovsky 2010-05-31 07:30:40 PDT
Created attachment 57461 [details]
[patch] second iteration

InspectorValue -> InspectorBasicValue
InspectorBaseValue -> InspectorValue
Comment 9 Eric Seidel (no email) 2010-05-31 07:36:09 PDT
Attachment 57461 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2803008
Comment 10 WebKit Review Bot 2010-05-31 07:39:47 PDT
Attachment 57461 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2750012
Comment 11 Ilya Tikhonovsky 2010-05-31 07:39:49 PDT
Created attachment 57463 [details]
[patch] third iteration. With fixes for linux build.
Comment 12 Eric Seidel (no email) 2010-05-31 07:42:54 PDT
Attachment 57463 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2802012
Comment 13 WebKit Review Bot 2010-05-31 07:51:30 PDT
Attachment 57461 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/2750013
Comment 14 Ilya Tikhonovsky 2010-05-31 07:53:08 PDT
Created attachment 57466 [details]
[patch] fourth iteration. With fixed mac export list.
Comment 15 WebKit Commit Bot 2010-05-31 15:28:41 PDT
Comment on attachment 57466 [details]
[patch] fourth iteration. With fixed mac export list.

Clearing flags on attachment: 57466

Committed r60452: <http://trac.webkit.org/changeset/60452>
Comment 16 WebKit Commit Bot 2010-05-31 15:28:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Joseph Pecoraro 2010-06-03 13:29:12 PDT
Comment on attachment 57466 [details]
[patch] fourth iteration. With fixed mac export list.

> +++ b/WebCore/inspector/InspectorValues.cpp
> +    result.reserveCapacity(512);

For my education, was there any particular surrounding the
decision for 512?


> +++ b/WebCore/inspector/InspectorValues.h
> +    unsigned length() { return m_data.size(); }

I think this can this be const:

  unsigned length() const { ... }


Otherwise really well done.This looks very clean!