Closed Bug 591516 Opened 14 years ago Closed 14 years ago

IndexedDB: Add some UI to prompt for IndexedDB permissions

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

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

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 8 obsolete files)

Attached patch Patch, v1 (obsolete) — Splinter Review
First stab attached. Reusing existing offline strings.
Attachment #470080 - Flags: feedback?(jonas)
Attached patch Patch, v1 (obsolete) — Splinter Review
This is the whole thing. Adds prompts for initial use and then another prompt for exceeding the soft quota.
Attachment #470080 - Attachment is obsolete: true
Attachment #471629 - Flags: review?(jonas)
Attachment #470080 - Flags: feedback?(jonas)
This needs bug 593045 in order to work.
Depends on: 593045
Comment on attachment 471629 [details] [diff] [review]
Patch, v1

Gavin, can you review the browser.js/browser.properties changes?
Attachment #471629 - Flags: review?(tester)
Comment on attachment 471629 [details] [diff] [review]
Patch, v1

Bah, this gavin!
Attachment #471629 - Flags: review?(tester) → review?(gavin.sharp)
Comment on attachment 471629 [details] [diff] [review]
Patch, v1

We did this review over phone. Mostly looks good but found one problem when several windows in the same origin run into the quota limit at the same time.

FWIW, one solution might be to simply let them all prompt, and add JS code to make them all go away if one is clicked. But any solution is fine with me.
Attachment #471629 - Flags: review?(jonas) → review-
Attached patch Patch, v2 (obsolete) — Splinter Review
Ok, this fixes all the stuff we found in review.
Attachment #471629 - Attachment is obsolete: true
Attachment #472215 - Flags: review?(jonas)
Attachment #472215 - Flags: review?(gavin.sharp)
Attachment #471629 - Flags: review?(gavin.sharp)
Attached patch Use PopupNotifications, v1 (obsolete) — Splinter Review
This applies on top of the current patch to make the UI use the PopupNotifications stuff instead of NotificationBar. If we decide to go that way we'll need some icons to use, I'm currently just using a generic question mark.
Attachment #472290 - Flags: review?
Attachment #472290 - Flags: review? → review?(gavin.sharp)
Attachment #472215 - Flags: review?(gavin.sharp)
Attachment #472290 - Flags: review?(gavin.sharp)
Attached patch Patch, v3 (obsolete) — Splinter Review
This is a rolled up version of the above patches. Includes PopupNotifications. Gavin, can you review just the frontend changes?
Attachment #472214 - Attachment is obsolete: true
Attachment #472215 - Attachment is obsolete: true
Attachment #472290 - Attachment is obsolete: true
Attachment #472689 - Flags: review?(gavin.sharp)
For context on these panels, we have a design document for the structure of a doorhanger notification available here: https://bugzilla.mozilla.org/attachment.cgi?id=466966
Setting this to block bug 588240 so that we can track work on all of these new panels from a central place.
Blocks: 588240
Alex: Looking at attachment 466966 [details], if the user dismisses the door hanger by clicking outside it, should we deny any further attempts of *that page* to create any databases? If the user reloads the page, or opens another tab/window with the page, we would then ask again using the door hanger.


Or should we just deny that one time, and if the page immediately attempts to create another database, we ask again?
This is likely going to annoy web developers, but similar to geolocation, these prompts aren't necessarily yes/no, like you are calling a the user function that returns a boolean, they are usually yes/[waiting for response].  So for instance, if the user clicks outside to close the dialog, they can still get it back from accessing the parent.  Here's a mockup of how that would work, with the (suddenly relevant :) use case of storing data:

https://bugzilla.mozilla.org/attachment.cgi?id=467307

details are in bug 588613.  (If that bug doesn't make it for Firefox 4, but indexedDB does, then we'll just have to hope that user's don't accidentally click outside to close since we won't have a way to support undo).
(In reply to comment #13)
Yeah, clicking outside the notification does not answer the question, as Alex pointed out. We just wait for the response. Right now the icon persists in the URL bar so the user can pretty easily realize that the page is asking for something.
Comment on attachment 472689 [details] [diff] [review]
Patch, v3

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

>+  function IndexedDBPromptHelper__handlePermissionsPrompt(requestor) {

>+    var mainAction = {

>+      callback: function() {
>+        observer.observe(null, self._permissionsResponse,
>+                         Ci.nsIPermissionManager.ALLOW_ACTION);

Don't all of these need to pass back "contentWindow", so that the backend knows which request was allowed?

>+        label: gNavigatorBundle.getString("offlineApps.notNow"),
>+        accessKey: gNavigatorBundle.getString("offlineApps.notNowAccessKey"),
>+        callback: function() {
>+          observer.observe(null, self._permissionsResponse,
>+                           Ci.nsIPermissionManager.UNKNOWN_ACTION);
>+        }

These shouldn't have "not now" buttons - the equivalent for these is just clicking them away.

>+    var message = gNavigatorBundle.getFormattedString("offlineApps.available",
>+                                                      [ host ]);
>+    var options = {
>+      timeout: Date.now() + 30 * 60000 // 30 minutes

Why 30 minutes? That seems rather long... Note that page loads are the only thing that dismiss notifications - the timeout is a minimum time that the notification will be around regardles of any loads. If you load a new page, I think dismissing the notification makes sense, so I don't think this needs a timeout at all.

>+  _handleQuotaPrompt:

Most of the same comments apply to this too. Seems to me like these should actually share code, given that they mostly do the same thing.
Attached patch Patch, v3.1 (obsolete) — Splinter Review
With review comments addressed.

Oh, and no, I don't need to pass the content window back because the observer holds on to that itself.
Attachment #472689 - Attachment is obsolete: true
Attachment #473335 - Flags: review?(gavin.sharp)
Attachment #472689 - Flags: review?(gavin.sharp)
Attached patch TestsSplinter Review
Code changes have r=sicking.
Attachment #472690 - Attachment is obsolete: true
Attachment #473336 - Flags: review+
Attached patch Patch, v3.2Splinter Review
Oops, had a copy-paste bug in there. This passes all the tests again.
Attachment #473335 - Attachment is obsolete: true
Attachment #473341 - Flags: review?(gavin.sharp)
Attachment #473335 - Flags: review?(gavin.sharp)
(In reply to comment #17)
> Oh, and no, I don't need to pass the content window back because the observer
> holds on to that itself.

How does the observer know which request is being allowed/denied, when there are multiple pending?
Nevermind - I failed to notice that it's called directly (i.e. not through the observer service).
we are adding a content permission api which may make some of this easier.  See bug 594261.
(In reply to comment #22)
> we are adding a content permission api which may make some of this easier.  See
> bug 594261.

Once we get to fennec/electrolysis, yeah! This is for Firefox 4.
I am hoping to land this for Firefox 4.
Comment on attachment 473341 [details] [diff] [review]
Patch, v3.2

>diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties

>+indexedDB.usage=This website (%S) is attempting to store more than %SMB of data on your computer for offline use.

You should use positioned variables and include an l10n note explaining what they represent (see e.g. contextMenuSearchText earlier in this file).

>diff --git a/dom/indexedDB/test/helpers.js b/dom/indexedDB/test/helpers.js

>+  let uri = Components.classes["@mozilla.org/network/io-service;1"]
>+                      .getService(Components.interfaces.nsIIOService)
>+                      .newURI(window.location, null, null);

window.document.documentURIObject?

This really should use the better system being set up in bug 594261 if possible. I'm not opposed to doing that in a followup (maybe even post-4.0, depending), but we should definitely get a bug on file to make sure it happens, because this observer service stuff (and running code in all windows on each notification request) is rather gross :)
Attachment #473341 - Flags: review?(gavin.sharp) → review+
Blocks: 595024
(In reply to comment #24)
> I am hoping to land this for Firefox 4.

Great! I filed a followup bug 595041 to move to that code.
Flags: in-testsuite+
Target Milestone: --- → mozilla2.0b6
Version: unspecified → Trunk
Gavin and Ben: this bug should have gone through ui-review before landing.  Thanks for answering all of the follow up questions in email.
Filed follow up bug 614469 on the action button text.
No longer blocks: 621120
Component: DOM → DOM: IndexedDB
Target Milestone: mozilla2.0b7 → ---
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: