Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1268353005:
[DevTools] Support JQuery event listeners (Closed)
|
Created:
5 years, 4 months ago by kozy Modified:
5 years, 3 months ago CC:
apavlov+blink_chromium.org, blink-reviews, blink-reviews-bindings_chromium.org, caseq+blink_chromium.org, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, vivekg_samsung, vivekg, yurys+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Description[DevTools] Support JQuery event listeners
This CL adds base support of JQuery event listeners. Listeners which was installed by JQuery are shown in Event Listeners sidebar.
API for frameworks are provided. Developers can add own event listeners provider to global devtoolsFrameworkEventListeners array (it should be created if undefined). Provider functions should return two arrays:
1. Array with framework event listeners for current object in eventListeners field in format {type: string, useCapture: boolean, handler: function()}.
2. Array with internal framework event handlers for current object in internalHandlers field - one function per each handler. This array is used for filtering out native internal event handlers.
BUG=61643
R=yurys@chromium.org,pfeldman@chromium.org
Patch Set 1 : #
Total comments: 24
Patch Set 2 : Added UI #
Total comments: 7
Patch Set 3 : Fixed tests #Patch Set 4 : #
Total comments: 20
Patch Set 5 : #
Total comments: 12
Patch Set 6 : #Patch Set 7 : Added missing files #
Total comments: 6
Patch Set 8 : #Patch Set 9 : Rebased #
Total comments: 2
Patch Set 10 : Implemented in frontend code #
Total comments: 12
Patch Set 11 : #
Total comments: 13
Patch Set 12 : #
Total comments: 16
Patch Set 13 : #
Total comments: 30
Patch Set 14 : #
Total comments: 14
Patch Set 15 : #
Total comments: 15
Patch Set 16 : #Depends on Patchset: Messages
Total messages: 55 (16 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
ptal!
yurys@chromium.org changed reviewers: + paulirish@chromium.org
Did we decide to do it Firefox way rather than how Paul described in https://code.google.com/p/chromium/issues/detail?id=61643#c30 ?
https://codereview.chromium.org/1268353005/diff/60001/LayoutTests/inspector/d... File LayoutTests/inspector/domdebugger/domdebugger-getEventListeners-jquery2-expected.txt (right): https://codereview.chromium.org/1268353005/diff/60001/LayoutTests/inspector/d... LayoutTests/inspector/domdebugger/domdebugger-getEventListeners-jquery2-expected.txt:19: sourceName: domdebugger-getEventListeners.html The expectation is from another test. https://codereview.chromium.org/1268353005/diff/60001/LayoutTests/inspector/d... File LayoutTests/inspector/domdebugger/domdebugger-getEventListeners-jquery2.html (right): https://codereview.chromium.org/1268353005/diff/60001/LayoutTests/inspector/d... LayoutTests/inspector/domdebugger/domdebugger-getEventListeners-jquery2.html:13: InspectorTest.RuntimeAgent.evaluate("document.getElementById(\"jquery-handlers\")", "get-event-listeners-test", step1); Do we still need changes to the test above they look identical to this test? https://codereview.chromium.org/1268353005/diff/60001/Source/bindings/core/v8... File Source/bindings/core/v8/ScriptEventListener.cpp (right): https://codereview.chromium.org/1268353005/diff/60001/Source/bindings/core/v8... Source/bindings/core/v8/ScriptEventListener.cpp:118: void functionLocation(v8::Local<v8::Function> function, String& scriptId, int& lineNumber, int& columnNumber) style: getFunctionLocation since we use return parameters here. https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... File Source/core/inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InjectedScriptSource.js:1145: librariesEventListenersByObjectId: function(objectId) Can you move id -> object resolution to C++ ? https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InjectedScriptSource.js:1158: librariesEventListeners: function(node) frameworkEventListeners? https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InjectedScriptSource.js:1165: if (data) { && typeof data === "function" ? https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InjectedScriptSource.js:1176: listeners.push(listener); push(listeners, listener); to avoid calling push overriden by the page. https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InjectedScriptSource.js:1187: if (typeof events[key] === "function") { wrong alignment https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... File Source/core/inspector/InspectorDOMDebuggerAgent.cpp (right): https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:401: ScriptValue listeners = injectedScript.librariesEventListeners(objectId); Can we make injectedScript.librariesEventListeners return v8::Local<v8::Array>? https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:411: fprintf(stderr, "Added!\n"); remove debug print https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:442: fprintf(stderr, "handler is empty\n"); remove debug print https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:450: fprintf(stderr, "function is empty\n"); remove debug print
All done. Added UI: https://drive.google.com/file/d/0B7RLEApsOKBLX3JuMFl5Q0Z1ZHM/view?usp=sharing https://drive.google.com/file/d/0B7RLEApsOKBLdmtBUUIteWlUczA/view?usp=sharing https://codereview.chromium.org/1268353005/diff/60001/LayoutTests/inspector/d... File LayoutTests/inspector/domdebugger/domdebugger-getEventListeners-jquery2-expected.txt (right): https://codereview.chromium.org/1268353005/diff/60001/LayoutTests/inspector/d... LayoutTests/inspector/domdebugger/domdebugger-getEventListeners-jquery2-expected.txt:19: sourceName: domdebugger-getEventListeners.html On 2015/08/11 21:10:24, yurys wrote: > The expectation is from another test. Done. https://codereview.chromium.org/1268353005/diff/60001/LayoutTests/inspector/d... File LayoutTests/inspector/domdebugger/domdebugger-getEventListeners-jquery2.html (right): https://codereview.chromium.org/1268353005/diff/60001/LayoutTests/inspector/d... LayoutTests/inspector/domdebugger/domdebugger-getEventListeners-jquery2.html:13: InspectorTest.RuntimeAgent.evaluate("document.getElementById(\"jquery-handlers\")", "get-event-listeners-test", step1); On 2015/08/11 21:10:24, yurys wrote: > Do we still need changes to the test above they look identical to this test? It checks support for jquery-2.x, previous for jquery-1.x. https://codereview.chromium.org/1268353005/diff/60001/Source/bindings/core/v8... File Source/bindings/core/v8/ScriptEventListener.cpp (right): https://codereview.chromium.org/1268353005/diff/60001/Source/bindings/core/v8... Source/bindings/core/v8/ScriptEventListener.cpp:118: void functionLocation(v8::Local<v8::Function> function, String& scriptId, int& lineNumber, int& columnNumber) On 2015/08/11 21:10:24, yurys wrote: > style: getFunctionLocation since we use return parameters here. Done. https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... File Source/core/inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InjectedScriptSource.js:1145: librariesEventListenersByObjectId: function(objectId) On 2015/08/11 21:10:24, yurys wrote: > Can you move id -> object resolution to C++ ? Done. https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InjectedScriptSource.js:1158: librariesEventListeners: function(node) On 2015/08/11 21:10:24, yurys wrote: > frameworkEventListeners? Done. https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InjectedScriptSource.js:1165: if (data) { On 2015/08/11 21:10:24, yurys wrote: > && typeof data === "function" ? Done. https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InjectedScriptSource.js:1176: listeners.push(listener); On 2015/08/11 21:10:24, yurys wrote: > push(listeners, listener); to avoid calling push overriden by the page. Done. https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InjectedScriptSource.js:1187: if (typeof events[key] === "function") { On 2015/08/11 21:10:24, yurys wrote: > wrong alignment Done. https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... File Source/core/inspector/InspectorDOMDebuggerAgent.cpp (right): https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:401: ScriptValue listeners = injectedScript.librariesEventListeners(objectId); On 2015/08/11 21:10:24, yurys wrote: > Can we make injectedScript.librariesEventListeners return v8::Local<v8::Array>? Done. https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:411: fprintf(stderr, "Added!\n"); On 2015/08/11 21:10:24, yurys wrote: > remove debug print Done. https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:442: fprintf(stderr, "handler is empty\n"); On 2015/08/11 21:10:25, yurys wrote: > remove debug print Done. https://codereview.chromium.org/1268353005/diff/60001/Source/core/inspector/I... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:450: fprintf(stderr, "function is empty\n"); On 2015/08/11 21:10:25, yurys wrote: > remove debug print Done.
https://codereview.chromium.org/1268353005/diff/80001/Source/core/inspector/I... File Source/core/inspector/InjectedScript.cpp (right): https://codereview.chromium.org/1268353005/diff/80001/Source/core/inspector/I... Source/core/inspector/InjectedScript.cpp:38: #include "bindings/core/v8/V8Node.h" Please don't add dependencies on Blink concepts into InjectedScript as we are planning to extract it into v8 inspector. https://codereview.chromium.org/1268353005/diff/80001/Source/core/inspector/I... Source/core/inspector/InjectedScript.cpp:341: if (!object.IsEmpty() && V8Node::hasInstance(object, isolate)) { prefer early return https://codereview.chromium.org/1268353005/diff/80001/Source/core/inspector/I... Source/core/inspector/InjectedScript.cpp:353: v8::MaybeLocal<v8::Set> InjectedScript::frameworkEventHandlers(v8::Local<v8::Value> object) Please use Local<> instead as the exception is not passed on further. https://codereview.chromium.org/1268353005/diff/80001/Source/core/inspector/I... Source/core/inspector/InjectedScript.cpp:357: ScriptFunctionCall function(injectedScriptObject(), "frameworkEventHandlers"); Please extract common code into a method. https://codereview.chromium.org/1268353005/diff/80001/Source/core/inspector/I... File Source/core/inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/1268353005/diff/80001/Source/core/inspector/I... Source/core/inspector/InjectedScriptSource.js:1145: frameworkEventListeners: function(node) It'd be nice if we can have this code outside of InjectedScriptSource, e.g. front-end could evaluate it or have an API for the page to inject handler resolver. Maybe we can use the same approach as with custom formatters. https://codereview.chromium.org/1268353005/diff/80001/Source/core/inspector/I... File Source/core/inspector/InspectorDOMDebuggerAgent.cpp (right): https://codereview.chromium.org/1268353005/diff/80001/Source/core/inspector/I... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:422: v8::Local<v8::Value> listener = array->Get(array->CreationContext(), i).ToLocalChecked(); v8Context https://codereview.chromium.org/1268353005/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/elements/EventListenersWidget.js (right): https://codereview.chromium.org/1268353005/diff/80001/Source/devtools/front_e... Source/devtools/front_end/elements/EventListenersWidget.js:60: result.toolbar().appendToolbarItem(new WebInspector.ToolbarCheckbox(WebInspector.UIString("Resolve handlers"), WebInspector.UIString("Resolve framework's event handlers"), widget._resolveFrameworkHandlersSetting)); Framework handlers
Can we use the ↪ in front of the location. This will help indicate it is a resolved location. We use this same arrow for revealing the resolved location of distributed shadow dom content. lgtm on the UX otherwise.
Patchset #6 (id:160001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:180001) has been deleted
Framework's event listeners resolving was extracted to frameworks_support module. Developers of frameworks can add own functions to global devtoolsFrameworksEventListeners and devtoolsFrameworksEventHandlers arrays. First function return all abstract event listeners for passed object. Second function all internal framework's handlers. please take a look!
https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... File Source/core/inspector/InjectedScript.cpp (right): https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... Source/core/inspector/InjectedScript.cpp:429: ScriptFunctionCall function(injectedScriptObject(), "frameworksEventListeners"); Extract common code into a method. https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... File Source/core/inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... Source/core/inspector/InjectedScriptSource.js:1154: frameworksEventListeners: function(object) frameworkEventListeners https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... Source/core/inspector/InjectedScriptSource.js:1180: var getters = inspectedGlobalObject["devtoolsFrameworksEventHandlers"]; Please extract common part. https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... Source/core/inspector/InjectedScriptSource.js:1279: if (!formatters || !isArrayLike(formatters)) Drop !formatter check? https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... File Source/core/inspector/InspectorDOMDebuggerAgent.cpp (right): https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:386: v8::Maybe<bool> hasHandler = frameworkHandlers->Has(frameworkHandlers->CreationContext(), handler); You may need v8::TryCatch to protect against throwing Has. https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:405: v8::Local<v8::Value> listener = listeners->Get(v8Context, i).ToLocalChecked(); You should check if the field is present as the object is returned from the user's code. https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:422: LocalFrame* frame = toDocument(context)->frame(); Why do we need LocalFrame here? It should work in workers too. https://codereview.chromium.org/1268353005/diff/200001/Source/devtools/front_... File Source/devtools/front_end/frameworks_support/EventListeners.js (right): https://codereview.chromium.org/1268353005/diff/200001/Source/devtools/front_... Source/devtools/front_end/frameworks_support/EventListeners.js:13: { functionString = [...].join("\n"); https://codereview.chromium.org/1268353005/diff/200001/Source/devtools/front_... Source/devtools/front_end/frameworks_support/EventListeners.js:24: if (self.devtoolsFrameworksEventListeners._hasDevtoolsWrapper) Can we make front-end control that we evaluate this function once per context? https://codereview.chromium.org/1268353005/diff/200001/Source/devtools/protoc... File Source/devtools/protocol.json (right): https://codereview.chromium.org/1268353005/diff/200001/Source/devtools/protoc... Source/devtools/protocol.json:3854: { "name": "handlerType", "type": "string", "enum": ["general", "frameworksResolved", "frameworksInternal"], "optional": true, "description": "Type of event listener handler."} hidden: true Normal, FrameworkInternal, CalledByFramework?
Please take a look! https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... File Source/core/inspector/InjectedScript.cpp (right): https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... Source/core/inspector/InjectedScript.cpp:429: ScriptFunctionCall function(injectedScriptObject(), "frameworksEventListeners"); On 2015/08/13 23:49:06, yurys wrote: > Extract common code into a method. Done. https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... File Source/core/inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... Source/core/inspector/InjectedScriptSource.js:1154: frameworksEventListeners: function(object) On 2015/08/13 23:49:06, yurys wrote: > frameworkEventListeners frameworkUserEventListeners https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... Source/core/inspector/InjectedScriptSource.js:1180: var getters = inspectedGlobalObject["devtoolsFrameworksEventHandlers"]; On 2015/08/13 23:49:06, yurys wrote: > Please extract common part. Done. https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... Source/core/inspector/InjectedScriptSource.js:1279: if (!formatters || !isArrayLike(formatters)) On 2015/08/13 23:49:06, yurys wrote: > Drop !formatter check? typeof null === "object" in js https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... File Source/core/inspector/InspectorDOMDebuggerAgent.cpp (right): https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:386: v8::Maybe<bool> hasHandler = frameworkHandlers->Has(frameworkHandlers->CreationContext(), handler); On 2015/08/13 23:49:07, yurys wrote: > You may need v8::TryCatch to protect against throwing Has. Acknowledged. https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:405: v8::Local<v8::Value> listener = listeners->Get(v8Context, i).ToLocalChecked(); On 2015/08/13 23:49:07, yurys wrote: > You should check if the field is present as the object is returned from the > user's code. Done. https://codereview.chromium.org/1268353005/diff/200001/Source/core/inspector/... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:422: LocalFrame* frame = toDocument(context)->frame(); On 2015/08/13 23:49:06, yurys wrote: > Why do we need LocalFrame here? It should work in workers too. Done. https://codereview.chromium.org/1268353005/diff/200001/Source/devtools/front_... File Source/devtools/front_end/frameworks_support/EventListeners.js (right): https://codereview.chromium.org/1268353005/diff/200001/Source/devtools/front_... Source/devtools/front_end/frameworks_support/EventListeners.js:13: { On 2015/08/13 23:49:07, yurys wrote: > functionString = [...].join("\n"); Done. https://codereview.chromium.org/1268353005/diff/200001/Source/devtools/front_... Source/devtools/front_end/frameworks_support/EventListeners.js:24: if (self.devtoolsFrameworksEventListeners._hasDevtoolsWrapper) On 2015/08/13 23:49:07, yurys wrote: > Can we make front-end control that we evaluate this function once per context? Done. https://codereview.chromium.org/1268353005/diff/200001/Source/devtools/protoc... File Source/devtools/protocol.json (right): https://codereview.chromium.org/1268353005/diff/200001/Source/devtools/protoc... Source/devtools/protocol.json:3854: { "name": "handlerType", "type": "string", "enum": ["general", "frameworksResolved", "frameworksInternal"], "optional": true, "description": "Type of event listener handler."} On 2015/08/13 23:49:07, yurys wrote: > hidden: true > > Normal, FrameworkInternal, CalledByFramework? Done.
lgtm https://codereview.chromium.org/1268353005/diff/220001/Source/core/inspector/... File Source/core/inspector/InjectedScript.cpp (right): https://codereview.chromium.org/1268353005/diff/220001/Source/core/inspector/... Source/core/inspector/InjectedScript.cpp:559: if (!hadException && !result.isEmpty()) Can we simply return result.v8Value()? https://codereview.chromium.org/1268353005/diff/220001/Source/core/inspector/... File Source/core/inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/1268353005/diff/220001/Source/core/inspector/... Source/core/inspector/InjectedScriptSource.js:1174: _callGlobalObjectMethodAndReturnArray: function(object, methodName, errorPrefix) _callCustomGetters https://codereview.chromium.org/1268353005/diff/220001/Source/core/inspector/... File Source/core/inspector/InspectorDOMDebuggerAgent.cpp (right): https://codereview.chromium.org/1268353005/diff/220001/Source/core/inspector/... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:449: Consider inlining this function. https://codereview.chromium.org/1268353005/diff/220001/Source/devtools/front_... File Source/devtools/front_end/elements/EventListenersWidget.js (right): https://codereview.chromium.org/1268353005/diff/220001/Source/devtools/front_... Source/devtools/front_end/elements/EventListenersWidget.js:48: this._isFrameworkSupportSetup = Symbol("isFrameworkEventListenerSupportSetup"); _frameworkSupportInitializedSymbol https://codereview.chromium.org/1268353005/diff/220001/Source/devtools/front_... Source/devtools/front_end/elements/EventListenersWidget.js:62: result.toolbar().appendToolbarItem(new WebInspector.ToolbarCheckbox(WebInspector.UIString("Show framework user's listeners"), WebInspector.UIString("Show framework user event listeners"), widget._showFrameworkListenersSetting)); Framework user listeners https://codereview.chromium.org/1268353005/diff/220001/Source/devtools/front_... Source/devtools/front_end/elements/EventListenersWidget.js:190: fulfill(undefined); fulfil()
kozyatinskiy@chromium.org changed reviewers: + dgozman@chromium.org
All Done. Dmitry, could you check that I've added framework_support module correctly? Pavel, please take a look. https://codereview.chromium.org/1268353005/diff/220001/Source/core/inspector/... File Source/core/inspector/InjectedScript.cpp (right): https://codereview.chromium.org/1268353005/diff/220001/Source/core/inspector/... Source/core/inspector/InjectedScript.cpp:559: if (!hadException && !result.isEmpty()) On 2015/08/14 17:24:20, yurys wrote: > Can we simply return result.v8Value()? Done. https://codereview.chromium.org/1268353005/diff/220001/Source/core/inspector/... File Source/core/inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/1268353005/diff/220001/Source/core/inspector/... Source/core/inspector/InjectedScriptSource.js:1174: _callGlobalObjectMethodAndReturnArray: function(object, methodName, errorPrefix) On 2015/08/14 17:24:20, yurys wrote: > _callCustomGetters Done. https://codereview.chromium.org/1268353005/diff/220001/Source/core/inspector/... File Source/core/inspector/InspectorDOMDebuggerAgent.cpp (right): https://codereview.chromium.org/1268353005/diff/220001/Source/core/inspector/... Source/core/inspector/InspectorDOMDebuggerAgent.cpp:449: On 2015/08/14 17:24:20, yurys wrote: > Consider inlining this function. Done. https://codereview.chromium.org/1268353005/diff/220001/Source/devtools/front_... File Source/devtools/front_end/elements/EventListenersWidget.js (right): https://codereview.chromium.org/1268353005/diff/220001/Source/devtools/front_... Source/devtools/front_end/elements/EventListenersWidget.js:48: this._isFrameworkSupportSetup = Symbol("isFrameworkEventListenerSupportSetup"); On 2015/08/14 17:24:21, yurys wrote: > _frameworkSupportInitializedSymbol Done. https://codereview.chromium.org/1268353005/diff/220001/Source/devtools/front_... Source/devtools/front_end/elements/EventListenersWidget.js:62: result.toolbar().appendToolbarItem(new WebInspector.ToolbarCheckbox(WebInspector.UIString("Show framework user's listeners"), WebInspector.UIString("Show framework user event listeners"), widget._showFrameworkListenersSetting)); On 2015/08/14 17:24:21, yurys wrote: > Framework user listeners Done. https://codereview.chromium.org/1268353005/diff/220001/Source/devtools/front_... Source/devtools/front_end/elements/EventListenersWidget.js:190: fulfill(undefined); On 2015/08/14 17:24:21, yurys wrote: > fulfil() Acknowledged.
https://codereview.chromium.org/1268353005/diff/260001/Source/devtools/front_... File Source/devtools/front_end/framework_support/EventListeners.js (right): https://codereview.chromium.org/1268353005/diff/260001/Source/devtools/front_... Source/devtools/front_end/framework_support/EventListeners.js:7: WebInspector.FrameworksSupport = {} EventListenersFrameworkSupport https://codereview.chromium.org/1268353005/diff/260001/Source/devtools/front_... Source/devtools/front_end/framework_support/EventListeners.js:12: WebInspector.FrameworksSupport.eventListenersInitializeFunction = function() Let's make this |initializeOnExecutionContext(context)|. https://codereview.chromium.org/1268353005/diff/260001/Source/devtools/front_... File Source/devtools/front_end/framework_support/module.json (right): https://codereview.chromium.org/1268353005/diff/260001/Source/devtools/front_... Source/devtools/front_end/framework_support/module.json:2: "dependencies": ["common"], nit: strange indent
All done. Please take a look. https://codereview.chromium.org/1268353005/diff/260001/Source/devtools/front_... File Source/devtools/front_end/framework_support/EventListeners.js (right): https://codereview.chromium.org/1268353005/diff/260001/Source/devtools/front_... Source/devtools/front_end/framework_support/EventListeners.js:7: WebInspector.FrameworksSupport = {} On 2015/08/14 19:02:25, dgozman wrote: > EventListenersFrameworkSupport Done. https://codereview.chromium.org/1268353005/diff/260001/Source/devtools/front_... Source/devtools/front_end/framework_support/EventListeners.js:12: WebInspector.FrameworksSupport.eventListenersInitializeFunction = function() On 2015/08/14 19:02:25, dgozman wrote: > Let's make this |initializeOnExecutionContext(context)|. Done. https://codereview.chromium.org/1268353005/diff/260001/Source/devtools/front_... File Source/devtools/front_end/framework_support/module.json (right): https://codereview.chromium.org/1268353005/diff/260001/Source/devtools/front_... Source/devtools/front_end/framework_support/module.json:2: "dependencies": ["common"], On 2015/08/14 19:02:25, dgozman wrote: > nit: strange indent Done.
Patchset #9 (id:300001) has been deleted
https://codereview.chromium.org/1268353005/diff/320001/Source/devtools/front_... File Source/devtools/front_end/elements/EventListenersWidget.js (right): https://codereview.chromium.org/1268353005/diff/320001/Source/devtools/front_... Source/devtools/front_end/elements/EventListenersWidget.js:62: result.toolbar().appendToolbarItem(new WebInspector.ToolbarCheckbox(WebInspector.UIString("Framework user's listeners"), WebInspector.UIString("Show framework user event listeners"), widget._showFrameworkListenersSetting)); Checkbox: Framework listeners Tooltip: Resolve event listeners bound with frameworks https://codereview.chromium.org/1268353005/diff/320001/Source/devtools/front_... File Source/devtools/front_end/framework_support/EventListeners.js (right): https://codereview.chromium.org/1268353005/diff/320001/Source/devtools/front_... Source/devtools/front_end/framework_support/EventListeners.js:47: self.devtoolsFrameworkUserEventListeners = self.devtoolsFrameworkUserEventListeners || []; self.devtoolsFrameworkEventListeners I understand the distinction by putting "User" in there, but we can simplify by leaving it out. Plus, if the frameworks themselves are binding to elements, it will resolve to a different location than their addEventListener location anyhow.
Did you expect to land this before branch?
On 2015/08/24 19:00:28, paulirish wrote: > Did you expect to land this before branch? It's not ready yet. I'll upload new implementation today.
New implementation uploaded. Please take a look.
https://codereview.chromium.org/1268353005/diff/340001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersView.js (right): https://codereview.chromium.org/1268353005/diff/340001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:37: promises.push(objects[i].frameworkEventListeners(devtoolsFrameworkEventListeners)); Where is this global method defined? https://codereview.chromium.org/1268353005/diff/340001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:38: promises.push(objects[i].frameworkInternalEventHandlers(devtoolsFrameworkInternalHandlers)); ditto https://codereview.chromium.org/1268353005/diff/340001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:50: promises.push(this._addObjectPromise.call(this, objects[i], How do you encode the listeners? Please provide schema in the comment. https://codereview.chromium.org/1268353005/diff/340001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:88: promises.push(frameworkInternalHandlers.callFunctionJSONPromise(hasInSet, [WebInspector.RemoteObject.toCallArgument(listeners[i].handler())]).then(add.bind(this, "normal", listeners[i]))); This is hard to parse, could you refactor this? It does not need to fit one line. https://codereview.chromium.org/1268353005/diff/340001/Source/devtools/front_... File Source/devtools/front_end/sdk/EventListenerUtils.js (right): https://codereview.chromium.org/1268353005/diff/340001/Source/devtools/front_... Source/devtools/front_end/sdk/EventListenerUtils.js:19: functionObject.getOwnProperties(ownPropertiesCallback); You should promisify the getOwnProperties instead, place it in remote object. https://codereview.chromium.org/1268353005/diff/340001/Source/devtools/front_... Source/devtools/front_end/sdk/EventListenerUtils.js:53: functionObject.functionDetails(functionDetailsCallback); ditto, pure promisification required. https://codereview.chromium.org/1268353005/diff/340001/Source/devtools/front_... Source/devtools/front_end/sdk/EventListenerUtils.js:68: WebInspector.buildEventListener = function(frameworkEventListener) This should be a part of the event listeners view or widget, wherever it is called.. https://codereview.chromium.org/1268353005/diff/340001/Source/devtools/front_... Source/devtools/front_end/sdk/EventListenerUtils.js:70: var promises = [frameworkEventListener.callFunctionJSONPromise(function(){ return this; }, undefined)]; Also hard to parse what it does, it uses inline functions, etc. https://codereview.chromium.org/1268353005/diff/340001/Source/devtools/front_... Source/devtools/front_end/sdk/EventListenerUtils.js:111: WebInspector.castEventListeners = function(frameworkListenersResult) move into widget, also impossible to tell what layout it adopts to what. https://codereview.chromium.org/1268353005/diff/340001/Source/devtools/front_... Source/devtools/front_end/sdk/EventListenerUtils.js:136: WebInspector.castEventHandlers = function(frameworkHandlersResult) ditto https://codereview.chromium.org/1268353005/diff/340001/Source/devtools/front_... Source/devtools/front_end/sdk/EventListenerUtils.js:193: function devtoolsFrameworkEventListeners() ditto https://codereview.chromium.org/1268353005/diff/340001/Source/devtools/front_... Source/devtools/front_end/sdk/EventListenerUtils.js:283: function devtoolsFrameworkInternalHandlers() ditto
Reimplemented. Pavel, please take a look.
Lets extract utilities and land them first. This patch is too large otherwise. https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersView.js (right): https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:42: promises.push(objects[i].eventListeners().then(WebInspector.storeTo.bind(null, result, "eventListeners"))); What's storeTo? https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... File Source/devtools/front_end/sdk/RemoteObject.js (right): https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... Source/devtools/front_end/sdk/RemoteObject.js:595: frameworkEventListeners: function(getter) This does not belong to RemoteObject, I thought we agreed on moving it to the event listeners area. https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... File Source/devtools/front_end/sdk/RemoteObjectUtils.js (right): https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... Source/devtools/front_end/sdk/RemoteObjectUtils.js:12: WebInspector.storeTo = function(data, key, value) This should go to utilities.js as a separate patch. https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... Source/devtools/front_end/sdk/RemoteObjectUtils.js:22: WebInspector.assertCallFunctionResult = function(result) This seems too specific and needs to be next to the callFunction or even just wrap it. https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... Source/devtools/front_end/sdk/RemoteObjectUtils.js:35: WebInspector.identity = function() This is weird! https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... Source/devtools/front_end/sdk/RemoteObjectUtils.js:44: WebInspector.RemoteArray = function(object) These could go into RemoteObject.js, lets land it as a separate patch. https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... Source/devtools/front_end/sdk/RemoteObjectUtils.js:53: WebInspector.RemoteArray.createPromise = function(object) This does not need to be a promise, there is no asynchrony in it. https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... Source/devtools/front_end/sdk/RemoteObjectUtils.js:65: at: function(idx) no abbreviations in Blink. https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... Source/devtools/front_end/sdk/RemoteObjectUtils.js:107: return Promise.resolve(new WebInspector.RemoteFunction(object)); ditto https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... Source/devtools/front_end/sdk/RemoteObjectUtils.js:141: WebInspector.RemoteSet = function(object) Why do you need a writable wrapper like this?
https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersView.js (right): https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:35: addObjects: function(objects) I am trying to understand what it does and it is still to complex. Can we split things further apart? https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:43: promises.push(objects[i].frameworkEventListeners(WebInspector.EventListenersView._frameworkEventListenersGetter) In here we call remote objects' frameworkEventListeners, and pass the source of the function from here. But they don't exist without each other, so the source that you send over to callFunctionOn needs to be defined withing frameworkEventListeners function. It needs to have a clear contract with js-docked input and output so that front-end side users could just call it once and use it then. https://codereview.chromium.org/1268353005/diff/360001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:341: WebInspector.EventListenersView._frameworkInternalHandlersGetter = function() Can we do these two in one pass and return the structure like: { descriptors: [], handlers: [] }
This is getting close!
Pavel, ptal.
https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersUtils.js (right): https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:10: WebInspector.frameworkEventListeners = function(object, getter) Please don't put anything on the global WebInspector object. https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:10: WebInspector.frameworkEventListeners = function(object, getter) What is getter? https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:38: if (property.name === "eventListeners" && property.value) { Could you explain the structure of the objects you are expecting to receive in comments? https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:170: WebInspector._frameworkEventListenersGetter = function() Where is this used? https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersView.js (right): https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:42: promises.push(objects[i].eventListeners().then(storeResultTo.bind(null, result, "eventListeners"))); This is still hard to parse/understand. Can we be a little less generative here? https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:43: promises.push(WebInspector.frameworkEventListeners(objects[i], WebInspector._frameworkEventListenersGetter) WebInspector._frameworkEventListenersGetter should be properly encapsulated in WebInspector.frameworkEventListeners. Don't use private fields from elsewhere. https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... File Source/devtools/front_end/elements/EventListenersWidget.js (right): https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/elements/EventListenersWidget.js:70: doUpdate: function() Should we follow up with the throttler? https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... File Source/devtools/front_end/externs.js (right): https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/externs.js:668: var jQuery = {}; There is no jQuery in the front-end.
Patchset #13 (id:400001) has been deleted
All done. Please take a look! https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersUtils.js (right): https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:10: WebInspector.frameworkEventListeners = function(object, getter) On 2015/09/02 02:58:52, pfeldman wrote: > Please don't put anything on the global WebInspector object. Moved to WebInspector.EventListener https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:10: WebInspector.frameworkEventListeners = function(object, getter) On 2015/09/02 02:58:52, pfeldman wrote: > What is getter? Getter is moved into this function. https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:38: if (property.name === "eventListeners" && property.value) { On 2015/09/02 02:58:52, pfeldman wrote: > Could you explain the structure of the objects you are expecting to receive in > comments? Added before frameworkEventListenersGetter function. https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:170: WebInspector._frameworkEventListenersGetter = function() On 2015/09/02 02:58:51, pfeldman wrote: > Where is this used? Moved to WebInspector.EventListener.frameworkEventListeners. https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersView.js (right): https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:42: promises.push(objects[i].eventListeners().then(storeResultTo.bind(null, result, "eventListeners"))); On 2015/09/02 02:58:52, pfeldman wrote: > This is still hard to parse/understand. Can we be a little less generative here? I've added comment. https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:43: promises.push(WebInspector.frameworkEventListeners(objects[i], WebInspector._frameworkEventListenersGetter) On 2015/09/02 02:58:52, pfeldman wrote: > WebInspector._frameworkEventListenersGetter should be properly encapsulated in > WebInspector.frameworkEventListeners. Don't use private fields from elsewhere. Done. https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... File Source/devtools/front_end/elements/EventListenersWidget.js (right): https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/elements/EventListenersWidget.js:70: doUpdate: function() On 2015/09/02 02:58:52, pfeldman wrote: > Should we follow up with the throttler? This widget was moved to promise-based throttler support in https://codereview.chromium.org/1285183006. https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... File Source/devtools/front_end/externs.js (right): https://codereview.chromium.org/1268353005/diff/380001/Source/devtools/front_... Source/devtools/front_end/externs.js:668: var jQuery = {}; On 2015/09/02 02:58:52, pfeldman wrote: > There is no jQuery in the front-end. Removed
As currently implemented, the complexity is outside of my comfort zone. We either need to defragment this further or not proceed with the feature. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersUtils.js (right): https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:11: var listenersResult = {eventListeners: []}; use proper types, approach this as any other type in the front-end. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:16: .then(returnResult); catch() https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:29: * @return {!Promise<void>} annotate properly https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:34: return Promise.resolve(); resolve(undefined) https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:41: .then(storeResultTo.bind(null, listenersResult, "eventListeners"))); This is non-compiler-friendly. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:62: promises.push(listenerObject.callFunctionJSONPromise(identity, undefined).then(storeResultTo.bind(null, eventListenerData, "jsonData"))); remove the storeResulTo https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:74: function identity() rename this to something meaningful. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:76: return this; You should return what you need here: { type: this.type, useCapture: this.useCapture } https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:88: return typeof this.handler === "object" ? (this.handler.handlerEvent || this.handler.constructor) : null; What is handlerEvent? https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:98: return /** @type {!Promise<void>} */(functionObject.functionDetailsPromise().then(storeResultTo.bind(null, eventListenerData, "handlerDetails"))); Remove all storeTo https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:161: throw new Error("Exception in callFunction or empty result"); We don't throw on expected / user errors. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:171: "handler": function()|Object, Why handler is not a function? https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:190: function frameworkEventListenersGetter() Could you typedef what it returns? https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:246: var hasJQuery = (typeof jQuery !== 'undefined') && jQuery.fn; Should be window["jQuery"] https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:292: function jQueryInternalHandlers(node) Embed into the one above.
All done. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersUtils.js (right): https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:11: var listenersResult = {eventListeners: []}; On 2015/09/02 21:13:25, pfeldman wrote: > use proper types, approach this as any other type in the front-end. Done. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:16: .then(returnResult); On 2015/09/02 21:13:26, pfeldman wrote: > catch() Done. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:29: * @return {!Promise<void>} On 2015/09/02 21:13:26, pfeldman wrote: > annotate properly Done. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:34: return Promise.resolve(); On 2015/09/02 21:13:25, pfeldman wrote: > resolve(undefined) Done. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:41: .then(storeResultTo.bind(null, listenersResult, "eventListeners"))); On 2015/09/02 21:13:25, pfeldman wrote: > This is non-compiler-friendly. Replaced. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:62: promises.push(listenerObject.callFunctionJSONPromise(identity, undefined).then(storeResultTo.bind(null, eventListenerData, "jsonData"))); On 2015/09/02 21:13:25, pfeldman wrote: > remove the storeResulTo Done. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:74: function identity() On 2015/09/02 21:13:26, pfeldman wrote: > rename this to something meaningful. Done. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:76: return this; On 2015/09/02 21:13:26, pfeldman wrote: > You should return what you need here: > > { type: this.type, useCapture: this.useCapture } Done. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:88: return typeof this.handler === "object" ? (this.handler.handlerEvent || this.handler.constructor) : null; On 2015/09/02 21:13:26, pfeldman wrote: > What is handlerEvent? Removed. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:98: return /** @type {!Promise<void>} */(functionObject.functionDetailsPromise().then(storeResultTo.bind(null, eventListenerData, "handlerDetails"))); On 2015/09/02 21:13:25, pfeldman wrote: > Remove all storeTo Done. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:161: throw new Error("Exception in callFunction or empty result"); On 2015/09/02 21:13:25, pfeldman wrote: > We don't throw on expected / user errors. Errors are encapsulated in this file. Can I still use it? https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:171: "handler": function()|Object, On 2015/09/02 21:13:26, pfeldman wrote: > Why handler is not a function? function() https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:190: function frameworkEventListenersGetter() On 2015/09/02 21:13:26, pfeldman wrote: > Could you typedef what it returns? Done. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:246: var hasJQuery = (typeof jQuery !== 'undefined') && jQuery.fn; On 2015/09/02 21:13:26, pfeldman wrote: > Should be window["jQuery"] Done. https://codereview.chromium.org/1268353005/diff/420001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:292: function jQueryInternalHandlers(node) On 2015/09/02 21:13:26, pfeldman wrote: > Embed into the one above. Done.
https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersUtils.js (right): https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:8: WebInspector.FrameworkEventListener; What is the difference between all of these types? Lets distinguish between the types that exist on the frontend and on the backend. https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:42: function frameworkEventListenersCallback(object) You should call them based on what they do, not what they are processing result from. This one would be called getOwnProperties. https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:44: return object.getOwnPropertiesPromise().then(propertiesCallback); Why is propertiesCallback now here? Stack it above and call createEventListeners? https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:58: promises.push(WebInspector.RemoteArray.objectAsArray(property.value) Extract this into the method that returns eventListeners. https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:61: .then(eventListenersCallback)); Promise<eventListeners> https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:64: promises.push(WebInspector.RemoteArray.objectAsArray(property.value) Extract this into the method that returns Promise<internalHandlers>. https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:97: function truncateFrameworkEventListenerCallback(truncatedListener) strip the Callback suffixes in all over the place, those are promises, not callbacks.
All done in EventListenersUtils.. https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersUtils.js (right): https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:8: WebInspector.FrameworkEventListener; On 2015/09/02 22:55:34, pfeldman wrote: > What is the difference between all of these types? Lets distinguish between the > types that exist on the frontend and on the backend. Rename page types to Page* https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:42: function frameworkEventListenersCallback(object) On 2015/09/02 22:55:34, pfeldman wrote: > You should call them based on what they do, not what they are processing result > from. This one would be called getOwnProperties. Done. https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:44: return object.getOwnPropertiesPromise().then(propertiesCallback); On 2015/09/02 22:55:34, pfeldman wrote: > Why is propertiesCallback now here? Stack it above and call > createEventListeners? Done. https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:58: promises.push(WebInspector.RemoteArray.objectAsArray(property.value) On 2015/09/02 22:55:34, pfeldman wrote: > Extract this into the method that returns eventListeners. Done. https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:61: .then(eventListenersCallback)); On 2015/09/02 22:55:34, pfeldman wrote: > Promise<eventListeners> Done. https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:64: promises.push(WebInspector.RemoteArray.objectAsArray(property.value) On 2015/09/02 22:55:34, pfeldman wrote: > Extract this into the method that returns Promise<internalHandlers>. Done. https://codereview.chromium.org/1268353005/diff/440001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:97: function truncateFrameworkEventListenerCallback(truncatedListener) On 2015/09/02 22:55:34, pfeldman wrote: > strip the Callback suffixes in all over the place, those are promises, not > callbacks. Done.
https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersUtils.js (right): https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:8: WebInspector.PageEventListener; Still don't get which is what. https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:99: * @return {undefined} return {undefined}? https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:108: promises.push(listenerObject.callFunctionPromise(handlerFunction).then(assertCallFunctionResult) stack them? https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:149: return new WebInspector.EventListener(eventListenerData.handler._target, So you did not need this object, you could have used variables. https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersView.js (right): https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:40: var result = {object: objects[i]}; Why are you creating these one-time objects? Is this because you want to use promises? https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:43: promises.push(objects[i].eventListeners().then(storeResultTo.bind(null, result, "eventListeners"))); Please delete this utility function and do not use. https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:52: function listenersCallback(results) Please do not append "Callback" to the end of every function. What does this function do? Lets name it accordingly. https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:62: promises.push(frameworkInternalHandlers.object().callFunctionJSONPromise(internalFlags, listenersHandler) Why is there asynchrony even after you have event listeners array? I were saying UI does not have asynchrony. https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersView.js:73: return WebInspector.RemoteObject.toCallArgument(listener.handler()); Why do we have to do this again?
https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersUtils.js (right): https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:18: WebInspector.EventListenerRawResult; This should be entirely encapsulated into the functions below. Lets call it: WebInspector.EventListenersObjectBuildInInspectedPage
All done in EventListenersUtils.js. Rewrited EventListenersView.js. ptal. https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersUtils.js (right): https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:8: WebInspector.PageEventListener; On 2015/09/03 18:56:35, pfeldman wrote: > Still don't get which is what. Renamed to WebInspector.EventListenerObjectInInspectedPage https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:18: WebInspector.EventListenerRawResult; On 2015/09/03 20:22:45, pfeldman wrote: > This should be entirely encapsulated into the functions below. Lets call it: > > WebInspector.EventListenersObjectBuildInInspectedPage It looks like I can't encapsulate typedef: https://github.com/google/closure-compiler/issues/544 I've used this type only once, can I use it without typedef? https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:99: * @return {undefined} On 2015/09/03 18:56:35, pfeldman wrote: > return {undefined}? Removed. https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:108: promises.push(listenerObject.callFunctionPromise(handlerFunction).then(assertCallFunctionResult) On 2015/09/03 18:56:35, pfeldman wrote: > stack them? Done. https://codereview.chromium.org/1268353005/diff/460001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:149: return new WebInspector.EventListener(eventListenerData.handler._target, On 2015/09/03 18:56:35, pfeldman wrote: > So you did not need this object, you could have used variables. Done.
lgtm
The CQ bit was checked by kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from paulirish@chromium.org, yurys@chromium.org Link to the patchset: https://codereview.chromium.org/1268353005/#ps480001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268353005/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268353005/480001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Should we add a test for devtoolsFrameworkEventListeners?
Paul, you are absolutely right, I missed it.
phistuck@gmail.com changed reviewers: + phistuck@gmail.com
Just a drive-by. https://codereview.chromium.org/1268353005/diff/500001/Source/devtools/front_... File Source/devtools/front_end/components/EventListenersUtils.js (right): https://codereview.chromium.org/1268353005/diff/500001/Source/devtools/front_... Source/devtools/front_end/components/EventListenersUtils.js:91: function storeTrunkatedListener(truncatedListener) Nit - s/Trunkated/Truncated/
Message was sent while issue was closed.
Patchset #17 (id:500001) has been deleted
Message was sent while issue was closed.
Committed patchset #16 (id:480001) manually as 201767 (presubmit successful). It was landed yesterday evening before comments, I'll add test and fix nit in separated CL.. |
