[webkit-reviews] review denied: [Bug 60558] StorageTracker should report actual local storage usage on disk : [Attachment 92998] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Anton D'Auria <adauria at apple.com>'s
request for review:
Bug 60558: StorageTracker should report actual local storage usage on disk
https://bugs.webkit.org/show_bug.cgi?id=60558

Attachment 92998: Patch
https://bugs.webkit.org/attachment.cgi?id=92998&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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-cr
eate-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-us
age.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.