Bug 60558

Summary: StorageTracker should report actual local storage usage on disk
Product: WebKit Reporter: Anton D'Auria <adauria>
Component: WebKit Misc.Assignee: Anton D'Auria <adauria>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, commit-queue, eric, jorlow, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Anton D'Auria 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.
Comment 1 Anton D'Auria 2011-05-10 10:29:38 PDT
Created attachment 92974 [details]
Patch

Uploading patch for initial review. Tests forthcoming.
Comment 2 Anton D'Auria 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.
Comment 3 David Levin 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.
Comment 4 Anton D'Auria 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://".
Comment 5 Anton D'Auria 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://".
Comment 6 Anton D'Auria 2011-05-10 16:28:09 PDT
Created attachment 93042 [details]
Patch
Comment 7 WebKit Commit Bot 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.
Comment 8 WebKit Commit Bot 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>
Comment 9 WebKit Commit Bot 2011-05-10 18:53:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Eric Seidel (no email) 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. :(