RESOLVED FIXED 60558
StorageTracker should report actual local storage usage on disk
https://bugs.webkit.org/show_bug.cgi?id=60558
Summary StorageTracker should report actual local storage usage on disk
Anton D'Auria
Reported 2011-05-10 10:13:19 PDT
Local Storage currently checks quota against key value pairs in memory. We need to provide an API to get usage on disk even if the key/value pairs haven't been loaded for an origin.
Attachments
Patch (7.27 KB, patch)
2011-05-10 10:29 PDT, Anton D'Auria
no flags
Patch (23.08 KB, patch)
2011-05-10 13:12 PDT, Anton D'Auria
no flags
Patch (23.07 KB, patch)
2011-05-10 14:46 PDT, Anton D'Auria
no flags
Patch (23.45 KB, patch)
2011-05-10 16:28 PDT, Anton D'Auria
no flags
Anton D'Auria
Comment 1 2011-05-10 10:29:38 PDT
Created attachment 92974 [details] Patch Uploading patch for initial review. Tests forthcoming.
Anton D'Auria
Comment 2 2011-05-10 13:12:13 PDT
Created attachment 92998 [details] Patch Adding tests and stubs. No need to add tests to skip lists on other platforms because storagetracker directory is already skipped in its entirety on all platforms except Mac.
David Levin
Comment 3 2011-05-10 14:13:41 PDT
Comment on attachment 92998 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92998&action=review > Source/WebCore/storage/StorageTracker.cpp:573 > + return 0.0f; Do you need to do 0.0f here as opposed to 0? (especially because long long is an int type). > Source/WebCore/storage/StorageTracker.cpp:579 > + return 0.0f; Ditto. > LayoutTests/storage/domstorage/localstorage/storagetracker/storage-tracker-6-create-expected.txt:2 > +StorageTracker test - write local storage for this origin. Should be called after storage-tracker-5-delete-one.html. Interdependencies among tests are really bad. Each test should stand on its own for lots of reasons. The simplest reason is that you can't guarantee that they will be run in the same instance of DRT. Other reasons are that it makes repro's harder. (Also if we can make tests completely independent then we'll be able to parallelize the running of them better and make the test runs go faster.) > LayoutTests/storage/domstorage/localstorage/storagetracker/storage-tracker-7-usage.html:21 > + shouldBeEqualToString("layoutTestController.originsWithLocalStorage().toString()", "file__0"); Why is this "file__0"? I suspect this is port specific. If so, it would be better to have it as test output (instead of hard coded in the test) so the various ports could have their own results.
Anton D'Auria
Comment 4 2011-05-10 14:32:33 PDT
(In reply to comment #3) > (From update of attachment 92998 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92998&action=review > > > Source/WebCore/storage/StorageTracker.cpp:573 > > + return 0.0f; > > Do you need to do 0.0f here as opposed to 0? (especially because long long is an int type). This is a mistake, thanks. > > LayoutTests/storage/domstorage/localstorage/storagetracker/storage-tracker-6-create-expected.txt:2 > > +StorageTracker test - write local storage for this origin. Should be called after storage-tracker-5-delete-one.html. > > Interdependencies among tests are really bad. > > Each test should stand on its own for lots of reasons. The simplest reason is that you can't guarantee that they will be run in the same instance of DRT. > > Other reasons are that it makes repro's harder. (Also if we can make tests completely independent then we'll be able to parallelize the running of them better and make the test runs go faster.) Unfortunately, there is no test harness for async WebCore APIs in WebKit1. We previously agreed to these sequential tests for StorageTracker will suffice, and I followed that precedent. > > > LayoutTests/storage/domstorage/localstorage/storagetracker/storage-tracker-7-usage.html:21 > > + shouldBeEqualToString("layoutTestController.originsWithLocalStorage().toString()", "file__0"); > > Why is this "file__0"? I suspect this is port specific. If so, it would be better to have it as test output (instead of hard coded in the test) so the various ports could have their own results. This is the cross-platform origin identifier (from WebCore::SecurityOrigin) for "file://".
Anton D'Auria
Comment 5 2011-05-10 14:46:28 PDT
Created attachment 93010 [details] Patch Removing 0.0f. Not changing tests because LocalStorage is written to disk asynchronously and we need to guarantee the db has content before getting its size. Not changing "file__0" because that's the WebCore::SecurityOrigin identifier for "file://".
Anton D'Auria
Comment 6 2011-05-10 16:28:09 PDT
WebKit Commit Bot
Comment 7 2011-05-10 18:53:21 PDT
The commit-queue encountered the following flaky tests while processing attachment 93042 [details]: http/tests/xmlhttprequest/remember-bad-password.html bug 51733 (author: ap@webkit.org) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 8 2011-05-10 18:53:42 PDT
Comment on attachment 93042 [details] Patch Clearing flags on attachment: 93042 Committed r86205: <http://trac.webkit.org/changeset/86205>
WebKit Commit Bot
Comment 9 2011-05-10 18:53:47 PDT
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 10 2011-09-07 11:46:21 PDT
According to TestFailures: http://build.webkit.org/TestFailures/#/SnowLeopard Intel Release (Tests) storage/domstorage/localstorage/storagetracker/storage-tracker-6-create.html: pretty diff (flaky) storage/domstorage/localstorage/storagetracker/storage-tracker-7-usage.html: pretty diff are both flaky/failing on Snow Leopard Mac. Please remove the tests or fix them. :(
Note You need to log in before you can comment on or make changes to this bug.