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)
Core
Storage: IndexedDB
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)
31.66 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
49.92 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
First stab attached. Reusing existing offline strings.
Attachment #470080 -
Flags: feedback?(jonas)
blocking2.0: --- → beta6+
Assignee | ||
Comment 1•14 years ago
|
||
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)
Assignee | ||
Comment 3•14 years ago
|
||
Comment on attachment 471629 [details] [diff] [review] Patch, v1 Gavin, can you review the browser.js/browser.properties changes?
Attachment #471629 -
Flags: review?(tester)
Assignee | ||
Comment 4•14 years ago
|
||
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-
Assignee | ||
Comment 6•14 years ago
|
||
Assignee | ||
Comment 7•14 years ago
|
||
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)
Assignee | ||
Comment 8•14 years ago
|
||
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?
Assignee | ||
Updated•14 years ago
|
Attachment #472290 -
Flags: review? → review?(gavin.sharp)
Attachment #472215 -
Flags: review?(jonas) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #472215 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•14 years ago
|
Attachment #472290 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 9•14 years ago
|
||
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)
Assignee | ||
Comment 10•14 years ago
|
||
Comment 11•14 years ago
|
||
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
Comment 12•14 years ago
|
||
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?
Comment 14•14 years ago
|
||
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).
Assignee | ||
Comment 15•14 years ago
|
||
(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 16•14 years ago
|
||
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.
Assignee | ||
Comment 17•14 years ago
|
||
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)
Assignee | ||
Comment 18•14 years ago
|
||
Code changes have r=sicking.
Attachment #472690 -
Attachment is obsolete: true
Attachment #473336 -
Flags: review+
Assignee | ||
Comment 19•14 years ago
|
||
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)
Comment 20•14 years ago
|
||
(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?
Comment 21•14 years ago
|
||
Nevermind - I failed to notice that it's called directly (i.e. not through the observer service).
Comment 22•14 years ago
|
||
we are adding a content permission api which may make some of this easier. See bug 594261.
Assignee | ||
Comment 23•14 years ago
|
||
(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.
Comment 24•14 years ago
|
||
I am hoping to land this for Firefox 4.
Comment 25•14 years ago
|
||
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+
Assignee | ||
Comment 26•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/b031db65eb69 http://hg.mozilla.org/mozilla-central/rev/882aba67b5d4
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 27•14 years ago
|
||
(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.
Updated•14 years ago
|
Flags: in-testsuite+
Target Milestone: --- → mozilla2.0b6
Version: unspecified → Trunk
Comment 28•14 years ago
|
||
Gavin and Ben: this bug should have gone through ui-review before landing. Thanks for answering all of the follow up questions in email.
Comment 29•14 years ago
|
||
Filed follow up bug 614469 on the action button text.
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.
Description
•