[webkit-reviews] review granted: [Bug 103469] Need a method to close all idle localstorage databases immediately. : [Attachment 176529] skip the new test on chromium since the test requires Safari's cache model.

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Yongjun Zhang
<yongjun_zhang at apple.com>'s request for review:
Bug 103469: Need a method to close all idle localstorage databases immediately.
https://bugs.webkit.org/show_bug.cgi?id=103469

Attachment 176529: skip the new test on chromium since the test requires
Safari's cache model.
https://bugs.webkit.org/attachment.cgi?id=176529&action=review

------- Additional Comments from David Kilzer (:ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=176529&action=review


r=me with the non-nit issues addressed.

> Source/WebCore/page/PageGroup.cpp:152
> +    if (!pageGroups)

Nit: This variable should be named s_pageGroups and be a static class variable
in the PageGroup class instead of a local static variable.
Not necessary to fix this for this patch, though.

> Tools/DumpRenderTree/gtk/TestRunnerGtk.cpp:1003
> +void TestRunner::closeIdleLocalStorageDatabases()
> +{
> +}

Nit:  Could add a FIXME comment here.

>
LayoutTests/storage/domstorage/localstorage/close-idle-localstorage-databases-i
mmediately.html:13
> +function log(a)
> +{
> +    document.getElementById("logger").innerHTML += a + "<br>";
> +}

Argument to function should have a better name like "message".

I would prefer it if we used fast/js/resources/js-test-pre.js with the
description(), testPassed() and testFailed() functions instead of rolling our
own custom messages.  We should also include
fast/js/resources/js-test-style.css as a stylesheet for the test.

>
LayoutTests/storage/domstorage/localstorage/close-idle-localstorage-databases-i
mmediately.html:36
> +	   if (loadCount == 1 ) {

Extra space between "1" and ")".

>
LayoutTests/storage/domstorage/localstorage/close-idle-localstorage-databases-i
mmediately.html:57
> +<div id="logger"></div>

"logger" becomes "console" here if we use fast/js/resources/js-test-pre.js.