IndexedDB: Add some UI to clear IndexedDB databases

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
7 years ago

People

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

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 473265 [details] [diff] [review]
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.
Attachment #473265 - Flags: review?(gavin.sharp)

Updated

8 years ago
Blocks: 553412
Created attachment 473323 [details] [diff] [review]
Backend patch, v1

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

>+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.
Attachment #473265 - Flags: review?(gavin.sharp) → feedback+
Created attachment 473852 [details] [diff] [review]
Frontend patch, v2

Updated with gavin's review comments addressed. Thanks!
Attachment #473265 - Attachment is obsolete: true
Attachment #473852 - Flags: ui-review?(beltzner)
Attachment #473852 - Flags: review?(gavin.sharp)
Created attachment 473859 [details] [diff] [review]
Backend patch, v1.1

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?(gavin.sharp) → 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?
https://developer.mozilla.org/En/Localization_and_Plurals#Developing_with_PluralForm
(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.
http://hg.mozilla.org/mozilla-central/rev/65c2aba970cd
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Component: DOM → DOM: IndexedDB
You need to log in before you can comment on or make changes to this bug.