IndexedDB: Histogram available disk space on attempt to open database
Created attachment 194154 [details] Patch
Created attachment 194155 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Created attachment 194156 [details] Patch
As written, this patch requires https://code.google.com/p/chromium/issues/detail?id=222432 to land in chromium first.
Josh/Alec, could you give this a look?
Created attachment 194160 [details] Patch
lgtm
WebKit API reviewers - r?
Comment on attachment 194160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194160&action=review > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:146 > +static void HistogramFreeSpace(const char* type, String fileName) HistogramFreeSpace -> histogramFreeSpace > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:149 > + String name = String::format("WebCore.IndexedDB.LevelDB.Open%sFreeDiskSpace", type); You should just use operator+ on String. It's faster. > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:156 > + HistogramSupport::histogramCustomCounts(name.utf8().data(), clampedDiskSpaceKBytes, 1, 1e9, 11); Should we COMPILE_ASSERT a relationship between 1e9 and INT_MAX?
Comment on attachment 194160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194160&action=review > Source/Platform/chromium/public/Platform.h:363 > + virtual long long availableDiskSpaceInBytes(const WebString& fileName) { return 0; } Is this a blocking call? Is there a restriction on calling this function from the main thread?
Created attachment 194372 [details] Patch
Comment on attachment 194160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194160&action=review >> Source/Platform/chromium/public/Platform.h:363 >> + virtual long long availableDiskSpaceInBytes(const WebString& fileName) { return 0; } > > Is this a blocking call? Is there a restriction on calling this function from the main thread? It does block and probably shouldn't be called from the main thread. Chromium only calls it from the browser process's WebKit thread (aka IndexedDB thread). Is there a way to enforce non-main-thread usage in other ports? >> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:146 >> +static void HistogramFreeSpace(const char* type, String fileName) > > HistogramFreeSpace -> histogramFreeSpace Done. >> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:149 >> + String name = String::format("WebCore.IndexedDB.LevelDB.Open%sFreeDiskSpace", type); > > You should just use operator+ on String. It's faster. Done, but semi-awkwardly. >> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:156 >> + HistogramSupport::histogramCustomCounts(name.utf8().data(), clampedDiskSpaceKBytes, 1, 1e9, 11); > > Should we COMPILE_ASSERT a relationship between 1e9 and INT_MAX? Done, though again semi-awkwardly.
Comment on attachment 194372 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194372&action=review > Source/Platform/chromium/public/Platform.h:363 > + virtual long long availableDiskSpaceInBytes(const WebString& fileName) { return 0; } I said "other ports" forgetting that this platform layer is only available to chromium. Should I put ASSERT(!isMainThread()) somewhere or just leave as is?
(In reply to comment #14) > (From update of attachment 194372 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194372&action=review > > > Source/Platform/chromium/public/Platform.h:363 > > + virtual long long availableDiskSpaceInBytes(const WebString& fileName) { return 0; } > > I said "other ports" forgetting that this platform layer is only available to chromium. Should I put ASSERT(!isMainThread()) somewhere or just leave as is? We should probably leave it as-is. I suspect we set the "main thread" bit on the in-process WebKit thread.
Comment on attachment 194372 [details] Patch Thanks!
(Want to wait until the platformsupport implementation lands so that I don't get confused about unexpected zeros showing up in the histogram)
Comment on attachment 194372 [details] Patch Clearing flags on attachment: 194372 Committed r146560: <http://trac.webkit.org/changeset/146560>
All reviewed patches have been landed. Closing bug.
Reverted r146560 for reason: invalid parameter to histogram Committed r146561: <http://trac.webkit.org/changeset/146561>
Created attachment 194442 [details] Patch
Adam, could I get you to re-review? The only change is on Line 152. "1, 1" became "1, 2" because the boundary is supposed to be higher than the minimum, not equal to it[1]. Untested code == broken code, indeed. 1. https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histogram.h&sq=package:chromium&type=cs&l=25
Tony, could you review this? It looks like Adam just left for vacation. It has WebKit API in it, but abarth@ has already approved that part. History is: r+ from abarth committed rolled out fixed non webkit-api and reuploaded now
Comment on attachment 194442 [details] Patch OK
Comment on attachment 194442 [details] Patch Clearing flags on attachment: 194442 Committed r146628: <http://trac.webkit.org/changeset/146628>