Bug 34204 – Web Inspector: Migrate from ScriptObject to InspectorValue as the InspectorFrontend / backend interchange object.

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

[Pavel Feldman]

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.

[Pavel Feldman]

Created attachment 56066 [details]
[WIP Patch] Handing over to Ilya.

[Ilya Tikhonovsky]

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.

[Yury Semikhatsky]

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?

[Ilya Tikhonovsky]

Created attachment 57459 [details]
[patch] Classes for native serialization to JSON.

[Pavel Feldman]

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?

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[Ilya Tikhonovsky]

Created attachment 57461 [details]
[patch] second iteration

InspectorValue -> InspectorBasicValue
InspectorBaseValue -> InspectorValue

[Eric Seidel (no email)]

Attachment 57461 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2803008

[WebKit Review Bot]

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

[Ilya Tikhonovsky]

Created attachment 57463 [details]
[patch] third iteration. With fixes for linux build.

[Eric Seidel (no email)]

Attachment 57463 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2802012

[WebKit Review Bot]

Attachment 57461 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/2750013

[Ilya Tikhonovsky]

Created attachment 57466 [details]
[patch] fourth iteration. With fixed mac export list.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

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!