Summary: | Web Inspector: Expose the console object in JSContexts to interact with Web Inspector | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||||||||||||||
Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | berto, buildbot, bunhere, cdumez, commit-queue, dbates, d-r, eric.carlson, esprehn+autocc, fmalita, ggaren, glenn, graouts, gyuyoung.kim, japhet, jer.noble, joepeck, kangil.han, kondapallykalyan, macpherson, menard, mkwst, oliver, pdr, philipj, rakuco, rniwa, sam, schenney, sergio, timothy, webkit-bug-importer, zan | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||
Attachments: |
|
Description
Joseph Pecoraro
2014-01-30 13:14:01 PST
Created attachment 225722 [details]
[PATCH] Proposed Fix
This is basically the patch. I have a couple FIXMEs still in the code that are questions I need to have answered before landing. But I want to get this up early so that I can sort out build issues in other bots and get review comments.
Attachment 225722 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:32: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:33: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:39: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
Total errors found: 3 in 54 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 225722 [details] [PATCH] Proposed Fix 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. (In reply to comment #4) > (From update of attachment 225722 [details]) > 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? We could eventually make the InspectorConsoleAgent the ConsoleClient. But that is something that I wanted to try to get to later. > > 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? Done, good suggestion. > > Source/JavaScriptCore/inspector/JSGlobalObjectConsole.h:35 > > +class JSGlobalObjectConsole final : public JSC::ConsoleClient { > > I would call this JSConsoleClient. Done. > > 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". Yep, that fixes all my issues. Thanks for finding this! > > 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. Okay. > > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:679 > > + visitor.append(&thisObject->m_consoleStructure); // FIXME: Needed? > > Yes, needed. I was hoping for a reason =). Does this makes sure to include the console object as reachable through the global object to prevent it from getting garbage collected? Or is visitChildren used for other purposes? I also addressed Release build issues (requires Inline includes that Debug builds do not require). Created attachment 225900 [details]
[PATCH] Proposed Fix
Attachment 225900 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/ConsoleClient.h:70: The parameter name "type" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/ConsoleClient.h:70: The parameter name "level" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:32: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:33: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:39: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
Total errors found: 5 in 55 files
If any of these errors are false positives, please file a bug against check-webkit-style.
> > > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:679
> > > + visitor.append(&thisObject->m_consoleStructure); // FIXME: Needed?
> >
> > Yes, needed.
>
> I was hoping for a reason =). Does this makes sure to include the console object as reachable through the global object to prevent it from getting garbage collected? Or is visitChildren used for other purposes?
Yup! Any time you have a JavaScript object as a data member, you must visit it during GC. (That's part of why I wanted to remove the other data member -- either we needed to remove it, or we needed to visit it, and removing seemed simpler.)
Comment on attachment 225900 [details]
[PATCH] Proposed Fix
r=me
Comment on attachment 225900 [details] [PATCH] Proposed Fix Attachment 225900 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6518076224831488 New failing tests: http/tests/security/cross-frame-access-getOwnPropertyDescriptor.html Created attachment 225912 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 225900 [details] [PATCH] Proposed Fix Attachment 225900 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5095686940917760 New failing tests: http/tests/security/cross-frame-access-getOwnPropertyDescriptor.html Created attachment 225920 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 225900 [details] [PATCH] Proposed Fix Attachment 225900 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4694286545190912 New failing tests: http/tests/security/cross-frame-access-getOwnPropertyDescriptor.html Created attachment 225926 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 225928 [details]
[PATCH] For Bots 1
Attachment 225928 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:32: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:33: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:39: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
Total errors found: 3 in 56 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 225937 [details]
[PATCH] For Bots 2
Created attachment 225957 [details]
[PATCH] For Bots 3
Attachment 225957 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:32: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:33: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:39: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
Total errors found: 3 in 58 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 225970 [details]
[PATCH] For Bots 4
This should fix windows. Hopefully the last.
Attachment 225970 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:32: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:33: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:39: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
Total errors found: 3 in 58 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Follow-up windows fix, which I had mistakenly forgot to include in the landed commit: <http://trac.webkit.org/changeset/165202> |