If I've set a ton of breakpoints, it can be really annoying to have to try to visit all the sites (or open inspector2) that have breakpoints and delete them.
Created attachment 364740 [details] Patch
Created attachment 364741 [details] [Image] After Patch is applied (Dark)
Created attachment 364742 [details] [Image] After Patch is applied (Light)
Created attachment 364754 [details] Patch
<rdar://problem/48917070>
Comment on attachment 364754 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364754&action=review > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:281 > + for (let i = 0; i < window.localStorage.length; ++i) { > + let key = window.localStorage.key(i); > + if (key.startsWith(prefix)) > + window.localStorage.removeItem(key); > + } Doesn't this miss every other item? localStorage.clear() localStorage.a = 1; localStorage.b = 2; localStorage.c = 3; for (let i = 0; i < window.localStorage.length; ++i) { let key = window.localStorage.key(i); window.localStorage.removeItem(key); } console.log(localStorage.length); // => 1 You are mutating the thing that you are iterating over. A safer way would be to get a list of keys to remove and then remove them: localStorage.clear() localStorage.a = 1; localStorage.b = 2; localStorage.c = 3; let keysToRemove = []; for (let i = 0; i < window.localStorage.length; ++i) { let key = window.localStorage.key(i); keysToRemove.push(key) } for (let key of keysToRemove) window.localStorage.removeItem(key); console.log(localStorage.length); // => 0
The modal looks pretty good but does seem a bit out of place for macOS.
Comment on attachment 364754 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364754&action=review >> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:281 >> + } > > Doesn't this miss every other item? > > localStorage.clear() > localStorage.a = 1; > localStorage.b = 2; > localStorage.c = 3; > > for (let i = 0; i < window.localStorage.length; ++i) { > let key = window.localStorage.key(i); > window.localStorage.removeItem(key); > } > > console.log(localStorage.length); // => 1 > > You are mutating the thing that you are iterating over. > > A safer way would be to get a list of keys to remove and then remove them: > > localStorage.clear() > localStorage.a = 1; > localStorage.b = 2; > localStorage.c = 3; > > let keysToRemove = []; > for (let i = 0; i < window.localStorage.length; ++i) { > let key = window.localStorage.key(i); > keysToRemove.push(key) > } > > for (let key of keysToRemove) > window.localStorage.removeItem(key); > > console.log(localStorage.length); // => 0 Crap! Good catch!
Created attachment 364885 [details] Patch
Comment on attachment 364885 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364885&action=review I think we need a dialog UI for Web Inspector — maybe WI.Modal is that but as WI.ModalDialog. Blurring the entire window is not ideal and not something found in other apps. This would be a dialog with Reset and Cancel buttons. Having one button makes it hard to reason how to exit without resetting. > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:261 > + resetModalButton.addEventListener("click", (event) => { This handler is big for being inline. I would expect this to be moved to a member function. It also is more Manager like than View like code. Main.js would be a better place for this logic to be maintained.
Created attachment 370385 [details] Patch
Comment on attachment 370385 [details] Patch Moved the reset button to be inside the Debug panel, as that's we don't want users to ever hit this (especially not accidentally). It's mainly for engineers to use for testing/presenting :P
Comment on attachment 370385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370385&action=review r=me > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:364 > + WI.ObjectStore.reset(); > + InspectorFrontendHost.reopen(); ObjectStore.reset() is async and we should probably wait for it. Should this instead be: resetInspectorButton.addEventListener("click", async (event) => { WI.Setting.reset(); await WI.ObjectStore.reset(); InspectorFrontendHost.reopen(); });
Created attachment 370454 [details] Patch
Comment on attachment 370454 [details] Patch Clearing flags on attachment: 370454 Committed r245657: <https://trac.webkit.org/changeset/245657>
All reviewed patches have been landed. Closing bug.