594583 - IndexedDB: Add some UI to clear IndexedDB databases

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Frontend patch, v1

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.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Backend patch, v1

Backend support.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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!

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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

>+const INDEXEDDB_LIMITED = 3;
>+const INDEXEDDB_UNLIMITED = 4;

These appear to be unused.

>+const PREFS_STRING_BUNDLE_URI =
>+  "chrome://browser/locale/preferences/preferences.properties";

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

>+// Load DownloadUtils module for convertByteUnits
>+Components.utils.import("resource://gre/modules/DownloadUtils.jsm");

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.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Frontend patch, v2

Updated with gavin's review comments addressed. Thanks!

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Backend patch, v1.1

Now with comments!

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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).

Comment 7

[Shawn Wilsher :sdwilsh]

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?
https://developer.mozilla.org/En/Localization_and_Plurals#Developing_with_PluralForm

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #7)
> Shouldn't we be using proper PluralForms here?

We're not pluralizing anything, so no.

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> We're not pluralizing anything, so no.
Um, numbers?  1 megabyte, 2 megabytes, etc

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We're using the DownloadUtils thing, it spits out "This site is using 200 KB".

Comment 11

[Shawn Wilsher :sdwilsh]

(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.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://hg.mozilla.org/mozilla-central/rev/65c2aba970cd