Closed
Bug 702796
Opened 13 years ago
Closed 12 years ago
IndexedDB prompt should auto-dismiss after a timeout [in native fennec]
Categories
(Firefox for Android Graveyard :: General, defect, P3)
Tracking
(fennec+)
RESOLVED
FIXED
Firefox 21
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: mbrubeck, Assigned: kshriram18)
References
Details
(Whiteboard: [mentor=mbrubeck@mozilla.com][good first bug][lang=js][lang=java])
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
IndexedDB prompts in desktop Firefox and XUL Fennec will automatically time out and disappear after a 5-minute timer (see bug 595253 for more explanation). We probably want to implement this in native Fennec too, for the same reason (so content doesn't wait indefinitely for a response from the prompt).
Updated•13 years ago
|
Priority: -- → P3
Updated•13 years ago
|
tracking-fennec: --- → 11+
Reporter | ||
Updated•13 years ago
|
Assignee: mbrubeck → nobody
Whiteboard: [mentor=mbrubeck@mozilla.com][good first bug]
Reporter | ||
Updated•13 years ago
|
Whiteboard: [mentor=mbrubeck@mozilla.com][good first bug] → [mentor=mbrubeck@mozilla.com][good first bug][lang=js]
Updated•13 years ago
|
Assignee: nobody → michaelkohler
Updated•13 years ago
|
Assignee: michaelkohler → nobody
Reporter | ||
Comment 1•13 years ago
|
||
You can look at the patches in bug 701527 to see where the code for this lives. In particular, the NativeWindow and IndexedDB objects in browser.js:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js
Whiteboard: [mentor=mbrubeck@mozilla.com][good first bug][lang=js] → [mentor=mbrubeck@mozilla.com][good first bug][lang=js][lang=java]
Updated•12 years ago
|
tracking-fennec: 11+ → +
I think this prompt is broken currently on Fennec? I'll try to get a better testcase for this, but currently my IndexedDB-using drawing app hangs after I create a certain number of drawings. By hangs I mean, the request=idb.open('<new database name>', version) call completes, but none of the callbacks (request.onsuccess, request.onerror, request.onblocked) are even invoked when I'm creating the 8th or so new database. All future database opens (even to already existing databases) also block in the same way (no callbacks are ever invoked).
I'm speculating that what it happening is that the idb permissions dialog is being invoked but failing to ever become visible and so all the rest of the idb operations are waiting for the user to click on the invisible dialog.
I've opened bug 805560 for the "prompt is broken" problem described in comment 2. This bug should also stay open, for the "prompt should time out once it is unbroken" part of the issue.
Assignee | ||
Updated•12 years ago
|
Attachment #696678 -
Flags: feedback? → feedback?(mbrubeck)
Reporter | ||
Comment 5•12 years ago
|
||
Comment on attachment 696678 [details] [diff] [review]
Patch for IndexedDB prompt to auto-dismiss after a timeout[in native fennec]
Review of attachment 696678 [details] [diff] [review]:
-----------------------------------------------------------------
Good work! This is definitely the correct approach to port this code from desktop Firefox to Fennec. It looks like there are a few places where the code is not fully ported to Fennec APIs, which I tried to note below. With some minor changes, you should be able to build and test this patch and upload a new version for final review.
::: mobile/android/chrome/content/browser.js
@@ +5570,5 @@
> +
> + function timeoutNotification() {
> + // Remove the notification.
> + if (notification) {
> + notification.remove();
Instead of "notification.remove" I think you'll need to call NativeWindow.doorhanger.hide.
@@ +5579,5 @@
> + clearTimeout(timeoutId);
> +
> + // And tell the page that the popup timed out.
> + observer.observe(null, responseTopic,
> + Ci.nsIPermissionManager.UNKNOWN_ACTION);
You can put this all on one line. (In Fennec code we tend not to wrap lines unless they are longer than around 120 characters.)
@@ +5586,2 @@
> if (topic == this._quotaCancel) {
> + notification = NativeWindow.doorhanger.hide(notificationID, tab.id);
I don't think you should assign anything to "notification" here. NativeWindow.doorhanger.hide() does not return a value.
@@ +5622,5 @@
> +
> + // If the popup is being dismissed start the short timeout.
> + if (state == "dismissed") {
> + clearTimeout(timeoutId);
> + timeoutId = setTimeout(timeoutNotification, hiddenTimeoutDuration);
I don't think this "eventCallback" code is needed in Fennec. Unlike desktop Firefox, Fennec has no way to hide and then restore a doorhanger notification, though we might add one in the future (bug 700437).
For now, I think you can just remove the whole "options" section of your patch.
@@ +5636,5 @@
> + //timeout =
> + };
> +
> + notification = NativeWindow.doorhanger.show(message, notificationID,
> + buttons, tab.id, options);
NativeWindow.doorhanger.show does not return an object either. Instead of saving a "notification" variable, you can just use the notificationID and tab.id to dismiss the doorhanger later.
Attachment #696678 -
Flags: feedback?(mbrubeck) → feedback+
Assignee | ||
Comment 6•12 years ago
|
||
Addresses points in last comment. will check if there's anything else to add
Attachment #696678 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #703747 -
Flags: feedback?(mbrubeck)
Reporter | ||
Comment 7•12 years ago
|
||
Comment on attachment 703747 [details] [diff] [review]
Patch for IndexedDB prompt to auto-dismiss after a timeout [in native fennec]
Looks good, thanks! I think this is ready to check in with just two minor changes:
>+ const hiddenTimeoutDuration = 30000; // 30 seonds
You can remove this line because it is unused.
>+ const firstTimeoutDuration = 360000; // 5 minutes
I think this should be 300000 instead. (I see that desktop Firefox has the same mistake; want to submit a separate patch to fix that?)
I can make these changes myself before checking in the patch.
Attachment #703747 -
Flags: review+
Attachment #703747 -
Flags: feedback?(mbrubeck)
Attachment #703747 -
Flags: checkin?(mbrubeck)
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #703747 -
Attachment is obsolete: true
Attachment #703747 -
Flags: checkin?(mbrubeck)
Assignee | ||
Comment 9•12 years ago
|
||
Addresses the timeout duration value in comment #7
Reporter | ||
Comment 10•12 years ago
|
||
Comment on attachment 709397 [details] [diff] [review]
Patch that fixes timeout duration value
Gavin, this contains a trivial fix for a constant in desktop Firefox.
Attachment #709397 -
Flags: review?(gavin.sharp)
Reporter | ||
Updated•12 years ago
|
Attachment #709395 -
Flags: review+
Updated•12 years ago
|
Attachment #709397 -
Flags: review?(gavin.sharp) → review+
Reporter | ||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8769476a0b0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce5c110ac606
Assignee: nobody → kshriram18
Status: NEW → ASSIGNED
Comment 12•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ce5c110ac606 (Had the wrong bug # on it)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•