Bug 113331 – Web Inspector: Encapsulate SetEmbedderData/GetEmbedderData

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

[johnjbarton]

We currently have two different sets of V8 embedder data.

1) V8PerContextData. This data is stored as a pointer on the context via SetAlignedPointerInEmbedderData.

2) A string like "page,232", stored on the context via SetEmbedderData, and used by the Inspector support binding code.

Confusingly both systems index into the *same array* on the Context. Thus if you search the code base for SetEmbedderData it will appear that only index 0 is used.  Using index 1 or 2 in SetEmbedder data leads to mysterious crashes, because in fact these indexes are used by the V8PerContextData and friends.

The overall system would be less confusing and fragile of all uses this underlying array where together. 

(Using the V8PerContextData to store the Inspector value could be an extra step, but mixing the two kinds of data would be more confusing in my opinion).

Patch ready.

[johnjbarton]

Created attachment 195137 [details]
Patch

[Pavel Feldman]

Comment on attachment 195137 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195137&action=review

> Source/WebCore/bindings/v8/V8PerContextData.cpp:175
> +            wanted = snprintf(buffer, sizeof(buffer), "%s", worldName);

So you formatted the format above, but you don't actually need it here? Why formatting the format at all then? I.e. 
if (debugId == -1)
    snprintf(buffer, sizeof(buffer), "%s", worldName);
else
    snprintf(buffer, sizeof(buffer), "%s,%d", worldName, debugId);

sounds simpler.

> Source/WebCore/bindings/v8/V8PerContextData.cpp:185
> +v8::Handle<v8::Value> V8PerContextDebugData::getDebugData(v8::Handle<v8::Context> context)

No get prefixes in WebKit, simply debugData()

[johnjbarton]

Created attachment 195992 [details]
Fix 2/2 issues from review in comment #2

[Adam Barth]

Comment on attachment 195992 [details]
Fix 2/2 issues from review in comment #2

View in context: https://bugs.webkit.org/attachment.cgi?id=195992&action=review

> Source/WebCore/bindings/v8/V8PerContextData.h:143
> +    static v8::Handle<v8::Value> createDebugData(const char* worldName, int debugId);
> +    static v8::Handle<v8::Value> debugData(v8::Handle<v8::Context>);
> +    static void setDebugData(v8::Handle<v8::Context>, v8::Handle<v8::Value>);

These can be file-level statics and don't need to be declared in the header file.

[Pavel Feldman]

Comment on attachment 195992 [details]
Fix 2/2 issues from review in comment #2

Looks good given Adam's comment is fixed. Clearing r?.

[johnjbarton]

Created attachment 196159 [details]
Fix 1/1 issue from review in comment #4

[WebKit Review Bot]

Comment on attachment 196159 [details]
Fix 1/1 issue from review in comment #4

Clearing flags on attachment: 196159

Committed r147475: <http://trac.webkit.org/changeset/147475>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.