Bug 75469 – Clear localStorage before making assumption about its contents

• Component: New Bugs
• Status: RESOLVED FIXED

Clear localStorage before making assumption about its contents

Created attachment 120937 [details]
Patch

This test fails flakily on our bots when there are already other entries in localStorage

Adam, can you please have a look?

[Daniel Bates]

Comment on attachment 120937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120937&action=review

> LayoutTests/platform/chromium/permissionclient/storage-permission.html:21
> +    localStorage.clear();

Is this necessary? Can we reuse/simplify the logic in clearLocalStorage.js (*)? By line 3 of LayoutTests/platform/chromium/permissionclient/storage-permission.html, <http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/permissionclient/storage-permission.html?rev=87597#L3>, we reference this script using the relative URL: resources/clearLocalStorage.js. However this script isn't included because it doesn't exist in directory LayoutTests/platform/chromium/permissionclient/resources. Instead, it exists in directory LayoutTests/storage/domstorage/localstorage/resources.

(*) I'm unclear how the decision was made to clear localStorage using the approach taken in <http://trac.webkit.org/browser/trunk/LayoutTests/storage/domstorage/localstorage/resources/clearLocalStorage.js?rev=31908> as opposed to calling localStorage.clear().

(In reply to comment #3)
> (From update of attachment 120937 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120937&action=review
> 
> > LayoutTests/platform/chromium/permissionclient/storage-permission.html:21
> > +    localStorage.clear();
> 
> Is this necessary? Can we reuse/simplify the logic in clearLocalStorage.js (*)? By line 3 of LayoutTests/platform/chromium/permissionclient/storage-permission.html, <http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/permissionclient/storage-permission.html?rev=87597#L3>, we reference this script using the relative URL: resources/clearLocalStorage.js. However this script isn't included because it doesn't exist in directory LayoutTests/platform/chromium/permissionclient/resources. Instead, it exists in directory LayoutTests/storage/domstorage/localstorage/resources.

Ouch, seems I forgot to copy this over...



> 
> (*) I'm unclear how the decision was made to clear localStorage using the approach taken in <http://trac.webkit.org/browser/trunk/LayoutTests/storage/domstorage/localstorage/resources/clearLocalStorage.js?rev=31908> as opposed to calling localStorage.clear().


I have no idea either. I think localStorage.clear() is preferable.

Created attachment 120971 [details]
Patch

[Daniel Bates]

Comment on attachment 120971 [details]
Patch

Can you elaborate on why we can't use the script clearLocalStorage.js?

(In reply to comment #6)
> (From update of attachment 120971 [details])
> Can you elaborate on why we can't use the script clearLocalStorage.js?

We can, but I don't see the benefit. clearLocalStorage seems to stem from before localStorage.clear() was implemented: http://trac.webkit.org/browser/trunk/WebCore/storage/Storage.idl?rev=31908

I'm preparing a separate CL that removes clearLocalStorage and replaces it with localStorage.clear() wdyt?

[Daniel Bates]

(In reply to comment #7)
> I'm preparing a separate CL that removes clearLocalStorage and replaces it with localStorage.clear() wdyt?

Sounds good.

[WebKit Review Bot]

Comment on attachment 120971 [details]
Patch

Clearing flags on attachment: 120971

Committed r103991: <http://trac.webkit.org/changeset/103991>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.