[webkit-reviews] review denied: [Bug 195777] Web Inspector: provide a way to reset all settings : [Attachment 364754] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 195777: Web Inspector: provide a way to reset all settings
https://bugs.webkit.org/show_bug.cgi?id=195777

Attachment 364754: Patch

https://bugs.webkit.org/attachment.cgi?id=364754&action=review




--- Comment #6 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 364754
  --> https://bugs.webkit.org/attachment.cgi?id=364754
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