[webkit-reviews] review granted: [Bug 55057] Web Inspector: refactor inspect() workflow so that it did not push dom nodes. : [Attachment 83546] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has granted Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 55057: Web Inspector: refactor inspect() workflow so that it did not push
dom nodes.
https://bugs.webkit.org/show_bug.cgi?id=55057

Attachment 83546: Patch
https://bugs.webkit.org/attachment.cgi?id=83546&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=83546&action=review

> Source/WebCore/bindings/js/JSInjectedScriptHostCustom.cpp:174
> +#if ENABLE(DATABASE)

Should be 2 lines above?

> Source/WebCore/bindings/js/JSInjectedScriptHostCustom.cpp:186
> +#if ENABLE(DOM_STORAGE)

same here

> Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:218
>      if (args.Length() < 1)

Why is this not under the #if guard below?

> Source/WebCore/inspector/InjectedScript.cpp:176
> +void InjectedScript::inspectNode(Node* node)

There are no nodes in the worker context, we will need to find a way to get rid
of this method along with Frame.h dependency there.

> Source/WebCore/inspector/InjectedScript.cpp:182
> +	  
function.appendArgument(InjectedScriptHost::nodeAsScriptValue(mainWorldScriptSt
ate(frame), node));

You should give the state from the m_injectedScriptObject, shouldn't you?

> Source/WebCore/inspector/InjectedScriptSource.js:70
> +	   if (arguments.length === 0)

What is this check for? If you need to check whether the object is valid you
should check it explicitely.

> Source/WebCore/inspector/InjectedScriptSource.js:213
> +    releaseObject: function(objectId)

Probably we could switch from the object groups to this method?

> Source/WebCore/inspector/InspectorDatabaseResource.cpp:41
> +static long nextUnusedId = 1;

What's the reason for this change?