Bug 194725

<rdar://problem/48126604>

Created attachment 362169 [details] Patch

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

Comment on attachment 362169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362169&action=review r=me > Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:180 > + auto userGestureState = [&] () -> Optional<ProcessingUserGestureState> { > + if (!emulateUserGesture) > + return WTF::nullopt; > + return *emulateUserGesture ? Optional<ProcessingUserGestureState>(ProcessingUserGesture) : WTF::nullopt; > + }(); I think this would read easier as it avoids the lamba: auto userGestureState = (emulateUserGesture && *emulateUserGesture) ? Optional<ProcessingUserGestureState>(ProcessingUserGesture) : WTF::nullopt; And maybe some compilers could work with this: auto userGestureState = (emulateUserGesture && *emulateUserGesture) ? ProcessingUserGesture : WTF::nullopt; > Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:140 > // COMPATIBILITY (iOS 8): "saveResult" did not exist. We add compatibility comments about the boundary point when we add new things. So for this we would add: // COMPATIBILITY (iOS 12.2): "emulateUserGesture" did not exist. Since iOS 12.2 is the last Versioned protocol, and it won't have this! > Source/WebInspectorUI/UserInterface/Views/LogContentView.js:860 > + _executeInUserGestureSettingChanged() > + { > + this._executeInUserGestureNavigationItem.checked = WI.settings.executeInUserGesture.value; > + } There is an "execute" vs "emulate" typo: _executeInUserGestureSettingChanged Should probably be: _emulateInUserGestureSettingChanged In multiple places. > LayoutTests/inspector/runtime/evaluate-userGestureEmulation.html:14 > + RuntimeAgent.evaluate.invoke({expression: "document.getElementById('foo').click()", objectGroup: "test", emulateUserGesture: false}, (error) => { Nit: We tend to put code expressions in a template string to make them easier to identify: ... expression: `document.getElementById("foo").click()`, ... > LayoutTests/inspector/runtime/evaluate-userGestureEmulation.html:37 > + console.log(window.internals.isProcessingUserGesture() ? "In User Gesture" : "Not in User Gesture"); You can make this: TestPage.addResult(...) And it will show up better in the output (in the result test instead of at the top of the file).

Comment on attachment 362169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362169&action=review > Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:114 > +void InspectorRuntimeAgent::evaluate(ErrorString& errorString, const String& expression, const String* objectGroup, const bool* includeCommandLineAPI, const bool* doNotPauseOnExceptionsAndMuteConsole, const int* executionContextId, const bool* returnByValue, const bool* generatePreview, const bool* saveResult, const bool*, RefPtr<Protocol::Runtime::RemoteObject>& result, Optional<bool>& wasThrown, Optional<int>& savedResultIndex) NIT: just for clarity, can you add a `/* emulateUserGesture */` so it's clear what the `const bool*` refers to? We have a lot of them 😅 > Source/WebInspectorUI/UserInterface/Base/Setting.js:134 > enableControlFlowProfiler: new WI.Setting("enable-control-flow-profiler", false), > enableLineWrapping: new WI.Setting("enable-line-wrapping", false), > + emulateInUserGesture: new WI.Setting("emulate-in-user-gesture", false), NIT: should be alphabetically sorted. > Source/WebInspectorUI/UserInterface/Views/LogContentView.js:76 > + this._emulateInUserGestureNavigationItem.addEventListener(WI.CheckboxNavigationItem.Event.CheckedDidChange, () => { WI.settings.emulateInUserGesture.value = !WI.settings.emulateInUserGesture.value; }); Style: I personally prefer that arrow functions be split into multiple lines if the return value is not assumed (e.g. not having `{` or `}`). > Source/WebInspectorUI/UserInterface/Views/LogContentView.js:77 > + WI.settings.emulateInUserGesture.addEventListener(WI.Setting.Event.Changed, this._emulateInUserGestureSettingChanged, this); Style: please prefix event listener functions with "handle" this._handleEmulateInUserGestureSettingChanged > LayoutTests/inspector/runtime/evaluate-userGestureEmulation.html:13 > + test(resolve, reject) { We support `async`, so you could use that instead of a promise. async test() { await RuntimeAgent.evaluate.invoke({ expression: `document.getElementById('foo').click()`, objectGroup: "test", emulateUserGesture: false, }); } > LayoutTests/inspector/runtime/evaluate-userGestureEmulation.html:24 > + test(resolve, reject) { Ditto (>13). > LayoutTests/inspector/runtime/evaluate-userGestureEmulation.html:43 > +}, false); NIT: you can drop the `false`.

Comment on attachment 362169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362169&action=review >> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:180 >> + }(); > > I think this would read easier as it avoids the lamba: > > auto userGestureState = (emulateUserGesture && *emulateUserGesture) ? Optional<ProcessingUserGestureState>(ProcessingUserGesture) : WTF::nullopt; > > And maybe some compilers could work with this: > > auto userGestureState = (emulateUserGesture && *emulateUserGesture) ? ProcessingUserGesture : WTF::nullopt; I kind of like the lambda :) Also, I don't think I'd be able to use auto in either of your examples. Not a big deal though. >> Source/WebInspectorUI/UserInterface/Base/Setting.js:134 >> + emulateInUserGesture: new WI.Setting("emulate-in-user-gesture", false), > > NIT: should be alphabetically sorted. Will fix. >> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:76 >> + this._emulateInUserGestureNavigationItem.addEventListener(WI.CheckboxNavigationItem.Event.CheckedDidChange, () => { WI.settings.emulateInUserGesture.value = !WI.settings.emulateInUserGesture.value; }); > > Style: I personally prefer that arrow functions be split into multiple lines if the return value is not assumed (e.g. not having `{` or `}`). This is following the convention in the file. Should I change just this or the other places too? >> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:77 >> + WI.settings.emulateInUserGesture.addEventListener(WI.Setting.Event.Changed, this._emulateInUserGestureSettingChanged, this); > > Style: please prefix event listener functions with "handle" > > this._handleEmulateInUserGestureSettingChanged Another case where I'm following the local conventions. Should I change? >> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:860 >> + } > > There is an "execute" vs "emulate" typo: > > _executeInUserGestureSettingChanged > > Should probably be: > > _emulateInUserGestureSettingChanged > > In multiple places. Oops. >> LayoutTests/inspector/runtime/evaluate-userGestureEmulation.html:13 >> + test(resolve, reject) { > > We support `async`, so you could use that instead of a promise. > > async test() { > await RuntimeAgent.evaluate.invoke({ > expression: `document.getElementById('foo').click()`, > objectGroup: "test", > emulateUserGesture: false, > }); > } OK. >> LayoutTests/inspector/runtime/evaluate-userGestureEmulation.html:14 >> + RuntimeAgent.evaluate.invoke({expression: "document.getElementById('foo').click()", objectGroup: "test", emulateUserGesture: false}, (error) => { > > Nit: We tend to put code expressions in a template string to make them easier to identify: > > ... expression: `document.getElementById("foo").click()`, ... OK. >> LayoutTests/inspector/runtime/evaluate-userGestureEmulation.html:37 >> + console.log(window.internals.isProcessingUserGesture() ? "In User Gesture" : "Not in User Gesture"); > > You can make this: > > TestPage.addResult(...) > > And it will show up better in the output (in the result test instead of at the top of the file). My problem was that InspectorTest and other things were not available in the event handler. I assume that's because the "test" function has bound to a scope where those things are present. I'll check if TestPage is. (I also couldn't put the handler inside the test function, because document wasn't available) >> LayoutTests/inspector/runtime/evaluate-userGestureEmulation.html:43 >> +}, false); > > NIT: you can drop the `false`. OK.

Comment on attachment 362169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362169&action=review >>> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:180 >>> + }(); >> >> I think this would read easier as it avoids the lamba: >> >> auto userGestureState = (emulateUserGesture && *emulateUserGesture) ? Optional<ProcessingUserGestureState>(ProcessingUserGesture) : WTF::nullopt; >> >> And maybe some compilers could work with this: >> >> auto userGestureState = (emulateUserGesture && *emulateUserGesture) ? ProcessingUserGesture : WTF::nullopt; > > I kind of like the lambda :) > > Also, I don't think I'd be able to use auto in either of your examples. Not a big deal though. `InspectorRuntimeAgent` has an `asBool` which you could copy into this file. static bool asBool(const bool* b) { return b && *b; } And then you could inline the logic so you wouldn't even need `auto`. UserGestureIndicator userGestureIndicator(asBool(emulateUserGesture) ? ProcessingUserGesture : WTF::nullopt); >>> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:76 >>> + this._emulateInUserGestureNavigationItem.addEventListener(WI.CheckboxNavigationItem.Event.CheckedDidChange, () => { WI.settings.emulateInUserGesture.value = !WI.settings.emulateInUserGesture.value; }); >> >> Style: I personally prefer that arrow functions be split into multiple lines if the return value is not assumed (e.g. not having `{` or `}`). > > This is following the convention in the file. Should I change just this or the other places too? It's fine to leave the rest of the file as is. We should just do this moving forward. They're just names after all, so it's not that huge of a deal :P >>> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:77 >>> + WI.settings.emulateInUserGesture.addEventListener(WI.Setting.Event.Changed, this._emulateInUserGestureSettingChanged, this); >> >> Style: please prefix event listener functions with "handle" >> >> this._handleEmulateInUserGestureSettingChanged > > Another case where I'm following the local conventions. Should I change? Ditto (>76). >>> LayoutTests/inspector/runtime/evaluate-userGestureEmulation.html:37 >>> + console.log(window.internals.isProcessingUserGesture() ? "In User Gesture" : "Not in User Gesture"); >> >> You can make this: >> >> TestPage.addResult(...) >> >> And it will show up better in the output (in the result test instead of at the top of the file). > > My problem was that InspectorTest and other things were not available in the event handler. I assume that's because the "test" function has bound to a scope where those things are present. I'll check if TestPage is. > > (I also couldn't put the handler inside the test function, because document wasn't available) `TestPage` should always (IIRC) be available. At the very least, it should be available after the test starts (which is before any "click" handler would fire).

Committed r241633: <https://trac.webkit.org/changeset/241633>

*** Bug 162883 has been marked as a duplicate of this bug. ***