Closed Bug 594583 Opened 10 years ago Closed 10 years ago

IndexedDB: Add some UI to clear IndexedDB databases


(Core :: Storage: IndexedDB, defect)

Not set



Tracking Status
blocking2.0 --- beta7+


(Reporter: bent.mozilla, Assigned: bent.mozilla)




(2 files, 2 obsolete files)

Attached patch Frontend patch, v1 (obsolete) — Splinter Review
Patch attached. Since this is flying in late (bug 573176 is missing Firefox 4) we're just going to add a little UI to the Page Info dialog allowing the user to clear the databases for a site and revoke permissions. Frontend patch attached.
Attachment #473265 - Flags: review?(
Blocks: IndexedDB
Attached patch Backend patch, v1 (obsolete) — Splinter Review
Backend support.
Attachment #473323 - Flags: review?(jonas)
Comment on attachment 473323 [details] [diff] [review]
Backend patch, v1

>diff --git a/dom/indexedDB/AsyncConnectionHelper.cpp b/dom/indexedDB/AsyncConnectionHelper.cpp
>-  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "GetOrCreateConnection failed!");
>+  if (NS_FAILED(rv)) {
>+    if (rv == NS_ERROR_NOT_AVAILABLE) {
>+      SetError(nsIIDBDatabaseException::RECOVERABLE_ERR);
>+      return NS_DispatchToMainThread(this, NS_DISPATCH_NORMAL);
>+    }
>+    NS_WARNING("GetOrCreateConnection failed!");
>+  }

This will make us crash in a burning inferno of hell fire.

Or maybe it's not so bad but could be simpler to just check the error code further down.

All of this needs lots and lots of comments!
Attachment #473323 - Flags: review?(jonas) → review+
Comment on attachment 473265 [details] [diff] [review]
Frontend patch, v1

>diff --git a/browser/base/content/pageinfo/pageInfo.xul b/browser/base/content/pageinfo/pageInfo.xul

>+            <button id="indexedDBClear" label="&permClearStorage;" onclick="onIndexedDBClear();"/>

This should probably have an accesskey.

>diff --git a/browser/base/content/pageinfo/permissions.js b/browser/base/content/pageinfo/permissions.js


These appear to be unused.

>+  "chrome://browser/locale/preferences/";

You should probably just copy the one string used from here into, rather than trying to re-use it.

>+// Load DownloadUtils module for convertByteUnits

You can do this right next to where it's used, to avoid doing it when there is no usage to report.

>+function onIndexedDBCheck()

>+function onIndexedDBRadio(permissionManager)

Given that the only difference between these and onCheckboxClick/onRadioClick seems to be the "indexedDB-unlimited" thing, I'd just use them and add a special case do the "unlimited" removal (aPartId == "indexedDB").

>+function initIndexedDBRow()

Similarly, you can just keep calling initRow() for this, in addition to calling initIndexedDBRow() to update the button/statustext, rather than duplicating the relevant code.
Attachment #473265 - Flags: review?( → feedback+
Updated with gavin's review comments addressed. Thanks!
Attachment #473265 - Attachment is obsolete: true
Attachment #473852 - Flags: ui-review?(beltzner)
Attachment #473852 - Flags: review?(
Now with comments!
Attachment #473323 - Attachment is obsolete: true
Attachment #473859 - Flags: review+
Attachment #473852 - Flags: ui-review?(beltzner) → ui-review+
Comment on attachment 473852 [details] [diff] [review]
Frontend patch, v2

>+function initIndexedDBRow()

>+  if (usage) {
>+    if (!("DownloadUtils" in window)) {
>+      Components.utils.import("resource://gre/modules/DownloadUtils.jsm");
>+    }
>+    usage = DownloadUtils.convertByteUnits(usage);
>+    usage = gBundle.getFormattedString("indexedDBUsage", usage);
>+  }
>+  if (usage) {

You can combine these blocks, right? And assign the result of getFormattedString directly to status.value, and avoid assigning the return value of convertByteUnits to "usage" (the re-use of "usage" for various things is kind of confusing).
Attachment #473852 - Flags: review?( → review+
Comment on attachment 473852 [details] [diff] [review]
Frontend patch, v2

>+# LOCALIZATION NOTE: The next string is for the disk usage of the
>+# database
>+#   e.g. indexedDBUsage : "50.23 MB"
>+#   %1$S = size (in bytes or megabytes, ...)
>+#   %2$S = unit of measure (bytes, KB, MB, ...)
>+indexedDBUsage=This web site is using %1$S %2$S
Shouldn't we be using proper PluralForms here?
(In reply to comment #7)
> Shouldn't we be using proper PluralForms here?

We're not pluralizing anything, so no.
(In reply to comment #8)
> We're not pluralizing anything, so no.
Um, numbers?  1 megabyte, 2 megabytes, etc
We're using the DownloadUtils thing, it spits out "This site is using 200 KB".
(In reply to comment #10)
> We're using the DownloadUtils thing, it spits out "This site is using 200 KB".
Right, my bad.  That's what I get for skimming the patch.
Closed: 10 years ago
Resolution: --- → FIXED
Component: DOM → DOM: IndexedDB
You need to log in before you can comment on or make changes to this bug.