75469 2012-01-03 05:32:56 -0800 Clear localStorage before making assumption about its contents 2012-01-03 17:11:16 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jochen jochen abarth dbates webkit.review.bot oldest_to_newest 528738 0 jochen 2012-01-03 05:32:56 -0800 Clear localStorage before making assumption about its contents 528739 1 120937 jochen 2012-01-03 05:34:12 -0800 Created attachment 120937 Patch 528740 2 jochen 2012-01-03 05:36:42 -0800 This test fails flakily on our bots when there are already other entries in localStorage Adam, can you please have a look? 528848 3 120937 dbates 2012-01-03 10:50:17 -0800 Comment on attachment 120937 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(). 528853 4 jochen 2012-01-03 10:56:36 -0800 (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. 528906 5 120971 jochen 2012-01-03 11:31:49 -0800 Created attachment 120971 Patch 528913 6 120971 dbates 2012-01-03 11:36:07 -0800 Comment on attachment 120971 Patch Can you elaborate on why we can't use the script clearLocalStorage.js? 528921 7 jochen 2012-01-03 11:47:19 -0800 (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? 528929 8 dbates 2012-01-03 11:52:46 -0800 (In reply to comment #7) > I'm preparing a separate CL that removes clearLocalStorage and replaces it with localStorage.clear() wdyt? Sounds good. 529181 9 120971 webkit.review.bot 2012-01-03 17:11:11 -0800 Comment on attachment 120971 Patch Clearing flags on attachment: 120971 Committed r103991: <http://trac.webkit.org/changeset/103991> 529182 10 webkit.review.bot 2012-01-03 17:11:16 -0800 All reviewed patches have been landed. Closing bug. 120937 2012-01-03 05:34:12 -0800 2012-01-03 11:31:44 -0800 Patch bug-75469-20120103143410.patch text/plain 1285 jochen U3VidmVyc2lvbiBSZXZpc2lvbjogMTAzOTA2CmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9DaGFu Z2VMb2cgYi9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cKaW5kZXggYjE1N2U5ODAzOWI1MDU2Nzc4NGU4 ZTVkYjY3Mzk4ZDZhN2M2ODVkNS4uMjM1MWQ3ZmM2ZWI0NjFkNDEyZmNjNjBmMTVlYzM5M2RkYjBj MzcyZCAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCisrKyBiL0xheW91dFRlc3Rz L0NoYW5nZUxvZwpAQCAtMSwzICsxLDEyIEBACisyMDEyLTAxLTAzICBKb2NoZW4gRWlzaW5nZXIg IDxqb2NoZW5AY2hyb21pdW0ub3JnPgorCisgICAgICAgIENsZWFyIGxvY2FsU3RvcmFnZSBiZWZv cmUgbWFraW5nIGFzc3VtcHRpb24gYWJvdXQgaXRzIGNvbnRlbnRzCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03NTQ2OQorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogcGxhdGZvcm0vY2hyb21pdW0vcGVybWlz c2lvbmNsaWVudC9zdG9yYWdlLXBlcm1pc3Npb24uaHRtbDoKKwogMjAxMi0wMS0wMiAgQmFsYXpz IEtlbGVtZW4gIDxrYmFsYXpzQHdlYmtpdC5vcmc+CiAKICAgICAgICAgVW5yZXZpZXdlZCBRdC1X SzIgZ2FyZGVuaW5nLgpkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvcGxhdGZvcm0vY2hyb21pdW0v cGVybWlzc2lvbmNsaWVudC9zdG9yYWdlLXBlcm1pc3Npb24uaHRtbCBiL0xheW91dFRlc3RzL3Bs YXRmb3JtL2Nocm9taXVtL3Blcm1pc3Npb25jbGllbnQvc3RvcmFnZS1wZXJtaXNzaW9uLmh0bWwK aW5kZXggMzAwODZjMTk0YTYzMzgzYmE2ZTExMWM2ZTc3NmEzMTU0OGIzZDgxNS4uNzAyMDBmYTMz MzA0MjdjM2Q2MTlmYTBkNjYyZjlmYzUxYTA5ZGU5YiAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMv cGxhdGZvcm0vY2hyb21pdW0vcGVybWlzc2lvbmNsaWVudC9zdG9yYWdlLXBlcm1pc3Npb24uaHRt bAorKysgYi9MYXlvdXRUZXN0cy9wbGF0Zm9ybS9jaHJvbWl1bS9wZXJtaXNzaW9uY2xpZW50L3N0 b3JhZ2UtcGVybWlzc2lvbi5odG1sCkBAIC0xOCw2ICsxOCw4IEBAIGZ1bmN0aW9uIHJ1blRlc3Qo KQogICAgICAgICByZXR1cm47CiAgICAgfQogCisgICAgbG9jYWxTdG9yYWdlLmNsZWFyKCk7CisK ICAgICBpZiAod2luZG93LmxheW91dFRlc3RDb250cm9sbGVyICYmIGxheW91dFRlc3RDb250cm9s bGVyLnNldFN0b3JhZ2VBbGxvd2VkKQogICAgICAgICBsYXlvdXRUZXN0Q29udHJvbGxlci5zZXRT dG9yYWdlQWxsb3dlZCh0cnVlKTsKICAgICBlbHNlCg== 120971 2012-01-03 11:31:49 -0800 2012-01-03 17:11:11 -0800 Patch bug-75469-20120103203147.patch text/plain 1419 jochen U3VidmVyc2lvbiBSZXZpc2lvbjogMTAzOTA2CmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9DaGFu Z2VMb2cgYi9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cKaW5kZXggYjE1N2U5ODAzOWI1MDU2Nzc4NGU4 ZTVkYjY3Mzk4ZDZhN2M2ODVkNS4uMjM1MWQ3ZmM2ZWI0NjFkNDEyZmNjNjBmMTVlYzM5M2RkYjBj MzcyZCAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCisrKyBiL0xheW91dFRlc3Rz L0NoYW5nZUxvZwpAQCAtMSwzICsxLDEyIEBACisyMDEyLTAxLTAzICBKb2NoZW4gRWlzaW5nZXIg IDxqb2NoZW5AY2hyb21pdW0ub3JnPgorCisgICAgICAgIENsZWFyIGxvY2FsU3RvcmFnZSBiZWZv cmUgbWFraW5nIGFzc3VtcHRpb24gYWJvdXQgaXRzIGNvbnRlbnRzCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03NTQ2OQorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogcGxhdGZvcm0vY2hyb21pdW0vcGVybWlz c2lvbmNsaWVudC9zdG9yYWdlLXBlcm1pc3Npb24uaHRtbDoKKwogMjAxMi0wMS0wMiAgQmFsYXpz IEtlbGVtZW4gIDxrYmFsYXpzQHdlYmtpdC5vcmc+CiAKICAgICAgICAgVW5yZXZpZXdlZCBRdC1X SzIgZ2FyZGVuaW5nLgpkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvcGxhdGZvcm0vY2hyb21pdW0v cGVybWlzc2lvbmNsaWVudC9zdG9yYWdlLXBlcm1pc3Npb24uaHRtbCBiL0xheW91dFRlc3RzL3Bs YXRmb3JtL2Nocm9taXVtL3Blcm1pc3Npb25jbGllbnQvc3RvcmFnZS1wZXJtaXNzaW9uLmh0bWwK aW5kZXggMzAwODZjMTk0YTYzMzgzYmE2ZTExMWM2ZTc3NmEzMTU0OGIzZDgxNS4uNjdmYzc3ZTdh NTUyNzBlZjc1MDY3OTI3ZWNhNDliZTQwZTk3OGY4NiAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMv cGxhdGZvcm0vY2hyb21pdW0vcGVybWlzc2lvbmNsaWVudC9zdG9yYWdlLXBlcm1pc3Npb24uaHRt bAorKysgYi9MYXlvdXRUZXN0cy9wbGF0Zm9ybS9jaHJvbWl1bS9wZXJtaXNzaW9uY2xpZW50L3N0 b3JhZ2UtcGVybWlzc2lvbi5odG1sCkBAIC0xLDYgKzEsNSBAQAogPGh0bWw+CiA8aGVhZD4KLTxz Y3JpcHQgc3JjPSJyZXNvdXJjZXMvY2xlYXJMb2NhbFN0b3JhZ2UuanMiPjwvc2NyaXB0PgogPHNj cmlwdD4KIAogaWYgKHdpbmRvdy5sYXlvdXRUZXN0Q29udHJvbGxlcikKQEAgLTE4LDYgKzE3LDgg QEAgZnVuY3Rpb24gcnVuVGVzdCgpCiAgICAgICAgIHJldHVybjsKICAgICB9CiAKKyAgICBsb2Nh bFN0b3JhZ2UuY2xlYXIoKTsKKwogICAgIGlmICh3aW5kb3cubGF5b3V0VGVzdENvbnRyb2xsZXIg JiYgbGF5b3V0VGVzdENvbnRyb2xsZXIuc2V0U3RvcmFnZUFsbG93ZWQpCiAgICAgICAgIGxheW91 dFRlc3RDb250cm9sbGVyLnNldFN0b3JhZ2VBbGxvd2VkKHRydWUpOwogICAgIGVsc2UK