Dumping $0, $1 etc in the console when inspecting Google Docs pages dumps some function from the page, rather than the selected element. Can we do something to keep console $ things separate from page $ things?
No matter what prefix (e.g. `$`) we pick, there's always a chance that the inspected page (or whatever the current context is) overrides it. A particularly malicious website could decide to blanket create $0...$99 as empty values on `window` to entirely prevent Web Inspector from being able to save values. One idea I had was to create a way for the developer to be able to customize the prefix from the Web Inspector frontend (e.g. an <input> in the Settings tab), so that if they notice that there are already existing values on the page, they could change `$` to be `$sadface` and avoid the collision/shadowing. This would 100% require backend support, but it would be a somewhat easy change to make. The discoverability of this isn't any worse than any other setting, but telling the "story" of this method would be much harder, specifically with the amount of space we traditionally have for any given setting in the Settings tab (e.g. one line with only a few words). I like this approach as it gives the most flexibility to the developer (some people would prefer a different prefix, like `_` or `temp` or `joe`). Alternatively (or in addition to), if we notice that a saved value (e.g. `$1`) would collide with an existing value on the page, we could surface a warning in the console suggesting that the developer change their prefix, or even try to do some smart "expansion" and add as many `_` as needed until there's no collision.
I suggested that if you right-click and "Log Element", inspector finds the first unused $ value that it can, and uses that.
Created attachment 368402 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Created attachment 368403 [details] Patch
Comment on attachment 368403 [details] Patch Attachment 368403 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12017008 New failing tests: http/tests/inspector/dom/cross-domain-inspected-node-access.html inspector/console/command-line-api.html
Created attachment 368407 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 368403 [details] Patch Attachment 368403 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12017653 New failing tests: http/tests/inspector/dom/cross-domain-inspected-node-access.html inspector/console/command-line-api.html
Created attachment 368409 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 368403 [details] Patch Attachment 368403 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12018022 New failing tests: http/tests/inspector/dom/cross-domain-inspected-node-access.html inspector/console/command-line-api.html
Created attachment 368413 [details] Archive of layout-test-results from ews114 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 368415 [details] Patch
Comment on attachment 368415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368415&action=review > Source/WebCore/inspector/CommandLineAPIModuleSource.js:58 > + this.__defineGetter__(prefix("0"), bind(commandLineAPIImpl._inspectedObject, commandLineAPIImpl)); We should probably also have `$0` in addition to the prefixed version for compatibility.
Created attachment 372824 [details] Patch After talking offline with @JoePeck, we decided that a better course of action is to have the setting be an alias instead, meaning that both the input value and "$" work as a prefix. This way, the existing functionality of "$" is preserved while still providing an alternate way of referencing the values that is less likely to collide with a value in the page.
Comment on attachment 372824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372824&action=review > Source/JavaScriptCore/inspector/InjectedScriptSource.js:-1462 > - delete this.$event; Since the command line API is recreated each time something is entered into the Console, there's no need to call `delete` as `this` is a brand new object.
Comment on attachment 372824 [details] Patch Attachment 372824 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12569288 New failing tests: inspector/debugger/command-line-api-exception.html inspector/debugger/command-line-api-exception-nested-catch.html
Created attachment 372827 [details] Archive of layout-test-results from ews100 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 372824 [details] Patch Attachment 372824 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12569431 New failing tests: inspector/debugger/command-line-api-exception.html inspector/debugger/command-line-api-exception-nested-catch.html
Created attachment 372830 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 372824 [details] Patch Attachment 372824 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12569448 New failing tests: inspector/debugger/command-line-api-exception.html inspector/debugger/command-line-api-exception-nested-catch.html
Created attachment 372832 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 372824 [details] Patch Attachment 372824 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12569809 New failing tests: storage/indexeddb/index-cursor.html css3/filters/blur-various-radii.html
Created attachment 372835 [details] Archive of layout-test-results from ews211 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews211 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 372839 [details] Patch
Comment on attachment 372839 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372839&action=review r=me > Source/JavaScriptCore/inspector/InjectedScriptSource.js:1459 > + this.$_ = injectedScript._lastResult; I wonder if these should be `__defineGetter__` as well. It doesn't really matter, because they get recreated every evaluation, but I suspect it would throw errors when/if folks try to assign to them. That should have its own test if we make such a change. > Source/JavaScriptCore/inspector/protocol/Runtime.json:322 > + "description": "Creates an additional reference to all saved values in the Console by replacing the default $ prefix with the given string.", This is not `replacing` but providing a parallel prefix. It seems more like a `setSavedResultPrefixAlias` but I'm fine with the existing naming. > Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:334 > + WI.settings.consoleSavedResultAlias.addEventListener(WI.Setting.Event.Changed, updateSavedVariableText); These event handlers need to get removed somewhere, or else they will exist forever on the global Setting. > Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:683 > + WI.settings.consoleSavedResultAlias.addEventListener(WI.Setting.Event.Changed, (event) => { These event handlers need to get removed somewhere, or else they will exist forever on the global Setting. > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:275 > + consoleSavedResultAliasInput.addEventListener("keydown", (event) => { > + if (!/[a-zA-Z0-9_$]/.test(event.key) || (consoleSavedResultAliasInput.selectionStart === 0 && /[0-9]/.test(event.key))) { > + event.preventDefault(); > + InspectorFrontendHost.beep(); > + } > + }); I guess this validation doesn't need to happen on the backend because this is actually becoming a property, not a variable name. Maybe special case if the final value is "$" it could clear it. > LayoutTests/inspector/runtime/setSavedResultAlias.html:46 > + InspectorTest.newline(); This would totally have made sense as 3 tests instead of just newlines.
Comment on attachment 372839 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372839&action=review >> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1459 >> + this.$_ = injectedScript._lastResult; > > I wonder if these should be `__defineGetter__` as well. It doesn't really matter, because they get recreated every evaluation, but I suspect it would throw errors when/if folks try to assign to them. That should have its own test if we make such a change. I like it! >> Source/JavaScriptCore/inspector/protocol/Runtime.json:322 >> + "description": "Creates an additional reference to all saved values in the Console by replacing the default $ prefix with the given string.", > > This is not `replacing` but providing a parallel prefix. It seems more like a `setSavedResultPrefixAlias` but I'm fine with the existing naming. Ah oops. This was from an earlier version. >> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:334 >> + WI.settings.consoleSavedResultAlias.addEventListener(WI.Setting.Event.Changed, updateSavedVariableText); > > These event handlers need to get removed somewhere, or else they will exist forever on the global Setting. Hmmmm... I'll have to think of a way around this. >> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:275 >> + }); > > I guess this validation doesn't need to happen on the backend because this is actually becoming a property, not a variable name. > > Maybe special case if the final value is "$" it could clear it. Good call! >> LayoutTests/inspector/runtime/setSavedResultAlias.html:46 >> + InspectorTest.newline(); > > This would totally have made sense as 3 tests instead of just newlines. I'd rather not, as that would mean that each test relies on the state set by the previous test. I prefer keeping test cases completely independent.
Created attachment 375579 [details] Patch
Created attachment 375582 [details] Patch The placeholder should be "$" to match the default prefix.
Comment on attachment 375582 [details] Patch Attachment 375582 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12866317 New failing tests: http/tests/inspector/dom/cross-domain-inspected-node-access.html inspector/console/command-line-api.html
Created attachment 375589 [details] Archive of layout-test-results from ews102 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 375594 [details] Patch
Comment on attachment 375594 [details] Patch Clearing flags on attachment: 375594 Committed r248287: <https://trac.webkit.org/changeset/248287>
All reviewed patches have been landed. Closing bug.
<rdar://problem/53970472>