[webkit-reviews] review denied: [Bug 127944] Web Inspector: Expose the console object in JSContexts to interact with Web Inspector : [Attachment 225722] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 127944: Web Inspector: Expose the console object in JSContexts to interact
with Web Inspector
https://bugs.webkit.org/show_bug.cgi?id=127944

Attachment 225722: [PATCH] Proposed Fix
https://bugs.webkit.org/attachment.cgi?id=225722&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225722&action=review


> Source/JavaScriptCore/inspector/JSGlobalObjectConsole.cpp:42
> +JSGlobalObjectConsole::JSGlobalObjectConsole(InspectorConsoleAgent*
consoleAgent)
> +    : ConsoleClient()
> +    , m_consoleAgent(consoleAgent)

So, a console points to a console client, and a client points to a console
agent? Do we really need all this indirection?

> Source/JavaScriptCore/inspector/JSGlobalObjectConsole.cpp:48
> +    internalAddMessage(MessageType::Log, level, exec, arguments);

This is duplicated a lot with PageConsole. Can we make logWithLevel and related
functions non-virtual, and make internalAddMessage virtual instead?

> Source/JavaScriptCore/inspector/JSGlobalObjectConsole.h:35
> +class JSGlobalObjectConsole final : public JSC::ConsoleClient {

I would call this JSConsoleClient.

> Source/JavaScriptCore/runtime/ConsolePrototype.cpp:142
> +    ConsoleClient* client = exec->lexicalGlobalObject()->consoleClient();
> +    if (!client)
> +	   return JSValue::encode(jsUndefined());

castedThis->globalObject() is probably what you want, if you want to be able to
"window.top.console.log".

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:466
> +    m_consolePrototype.set(vm, this, ConsolePrototype::create(vm, this,
ConsolePrototype::createStructure(vm, this, m_objectPrototype.get())));

Let's not make this a data member. We only need it in this function.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:679
> +    visitor.append(&thisObject->m_consoleStructure); // FIXME: Needed?

Yes, needed.

> Source/WebCore/bindings/js/ScriptCachedFrameData.cpp:62
> +	   // FIXME: Why does clearing this early prevent sub-window
(window.top.console.log) from working?
> +	   // e.g. LayoutTests/fast/events/suspend-timers.html
> +	   // window->setConsoleClient(nullptr);

The reason window.top.console.log fails is that you've implemented JSConsole to
use exec->lexicalGlobalObject, which means that, even if you specify
"window.top", the global object whose client you end up trying to call through
to is the global object the caller function was declared in. It sounds like you
want some other behavior -- maybe to use the global object the console function
was initially attached to.