Bug 195777 – Web Inspector: provide a debug setting button to reset all settings for easier testing/presentation

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 364740 [details]
Patch

[Devin Rousso]

Created attachment 364741 [details]
[Image] After Patch is applied (Dark)

[Devin Rousso]

Created attachment 364742 [details]
[Image] After Patch is applied (Light)

[Devin Rousso]

Created attachment 364754 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/48917070>

[Joseph Pecoraro]

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

[Joseph Pecoraro]

The modal looks pretty good but does seem a bit out of place for macOS.

[Devin Rousso]

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!

[Devin Rousso]

Created attachment 364885 [details]
Patch

[Timothy Hatcher]

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.

[Devin Rousso]

Created attachment 370385 [details]
Patch

[Devin Rousso]

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

[Joseph Pecoraro]

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();
    });

[Devin Rousso]

Created attachment 370454 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 370454 [details]
Patch

Clearing flags on attachment: 370454

Committed r245657: <https://trac.webkit.org/changeset/245657>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.