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?
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
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?
(In reply to comment #7) > I'm preparing a separate CL that removes clearLocalStorage and replaces it with localStorage.clear() wdyt? Sounds good.
Comment on attachment 120971 [details] Patch Clearing flags on attachment: 120971 Committed r103991: <http://trac.webkit.org/changeset/103991>
All reviewed patches have been landed. Closing bug.