Bug 188672

Sounds like a good idea. $event = "foo"; document.body.addEventListnener("click", () => { debugger; }); `$event` is defined, we should preserve it. E.g. show "foo" and not the click event object.

Created attachment 347302 [details] Patch

Comment on attachment 347302 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347302&action=review > Source/JavaScriptCore/inspector/InjectedScriptSource.js:1396 > + this.$event = injectedScript._eventValue; Given that events are a DOM concept, I don't think BasicCommandLineAPI will end up needing it. Since this is primarily for JSContext inspection (though it might make sense with ITMLKit inspection). > Source/WebCore/inspector/CommandLineAPIModuleSource.js:50 > + this.$event = injectedScript._eventValue; I wonder if we can just make this something like: this.__defineGetter__("$event", () => inspectedWindow.event); Is $event ever different from `window.event`? Yes it is possible for the page to have redefined the DOMWindow.prototype.event getter, so this isn't necessarily perfect but I don't think we need to be too concerned.

(In reply to Nikita Vasilyev from comment #1) > Sounds like a good idea. > > $event = "foo"; > document.body.addEventListnener("click", () => { > debugger; > }); > > `$event` is defined, we should preserve it. E.g. show "foo" and not the > click event object. Yes, this should be the normal behavior of the CommandLineAPI. The CommandLineAPI functions (keys, values, $0, $n, $exception, $event, etc) are in the background, so if the page has defined their own thing with an equivalent identifier, the page's thing should win out.

Comment on attachment 347302 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347302&action=review >> Source/WebCore/inspector/CommandLineAPIModuleSource.js:50 >> + this.$event = injectedScript._eventValue; > > I wonder if we can just make this something like: > > this.__defineGetter__("$event", () => inspectedWindow.event); > > Is $event ever different from `window.event`? Yes it is possible for the page to have redefined the DOMWindow.prototype.event getter, so this isn't necessarily perfect but I don't think we need to be too concerned. I just tried this, and since `window.event` is defined, I'm not even sure that we need this. Typing just `event` will give you the same result as `$event`. I think we should hold off on this until we have a better reason for differentiating between `$event` and `event`/`window.event`.

Comment on attachment 347302 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347302&action=review >>> Source/WebCore/inspector/CommandLineAPIModuleSource.js:50 >>> + this.$event = injectedScript._eventValue; >> >> I wonder if we can just make this something like: >> >> this.__defineGetter__("$event", () => inspectedWindow.event); >> >> Is $event ever different from `window.event`? Yes it is possible for the page to have redefined the DOMWindow.prototype.event getter, so this isn't necessarily perfect but I don't think we need to be too concerned. > > I just tried this, and since `window.event` is defined, I'm not even sure that we need this. Typing just `event` will give you the same result as `$event`. I think we should hold off on this until we have a better reason for differentiating between `$event` and `event`/`window.event`. Considering that `window.event` is non-standard, and is more likely to be overridden by the page's JavaScript than `$event`, I think we should consider adding this.

Created attachment 365110 [details] Patch

Comment on attachment 365110 [details] Patch Clearing flags on attachment: 365110 Committed r243161: <https://trac.webkit.org/changeset/243161>

All reviewed patches have been landed. Closing bug.

<rdar://problem/49030174>

The changes in https://trac.webkit.org/changeset/243161/webkit has broken inspector/timeline/line-column.html history: http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Ftimeline%2Fline-column.html I confirmed the test was working with the fix in 243155 and then broke in 243161 again.

(In reply to Truitt Savell from comment #11) > The changes in https://trac.webkit.org/changeset/243161/webkit > > has broken inspector/timeline/line-column.html > > history: > http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=inspector%2Ftimeline%2Fline-column.html > > I confirmed the test was working with the fix in 243155 and then broke in > 243161 again. Fixed test in followup <https://trac.webkit.org/changeset/243234/webkit>.