Closed
Bug 700913
Opened 13 years ago
Closed 13 years ago
Add persistence and timeout features to Doorhangers
Categories
(Firefox for Android Graveyard :: General, defect, P2)
Tracking
(firefox11 fixed, fennec11+)
RESOLVED
FIXED
People
(Reporter: mfinkle, Assigned: Margaret)
References
Details
(Whiteboard: [QA-])
Attachments
(1 file)
(deleted),
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
Desktop notifications support options for persistence and timeout, defined as:
* persistence: (integer, defaults to 0) The number of "pageloads" that this doorhanger ignores before closing automatically. If -1, the doorhanger never closes automatically, but must be closed by the user.
* timeout: (integer, defaults to 0) The number of milliseconds will stay visible at a minimum, regardless of "pageloads". A user can manually close the doorhanger. Any pageloads that occur after the timeout is exceeded will close the doorhanger. The doorhanger does not close exactly on the timeout.
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → margaret.leibovic
Updated•13 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•13 years ago
|
||
I tested this by adding persistence and timeout options to the content permission prompt here: http://mxr.mozilla.org/projects-central/source/birch/mobile/components/ContentPermissionPrompt.js#163. It worked!
Assignee | ||
Updated•13 years ago
|
Attachment #573720 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 2•13 years ago
|
||
Comment on attachment 573720 [details] [diff] [review]
patch
>diff --git a/embedding/android/DoorHanger.java b/embedding/android/DoorHanger.java
>+ public void setOptions(JSONObject options) {
>+ mTimeout = options.getLong("timeout");
>+ public boolean shouldRemove() {
>+ if (new Date().getTime() <= mTimeout) {
>+ return false;
This makes me think that the value passed from JS to Java is an end time, not a true "timeout" - which is fine, that's how the Gecko version works too, right?
I was just unsure if the JS Date and Java Date would be based on the same "origin"/ epoch, causing the comparison in shouldRemove to not be exactly right. If you have tested that using a | timeout: Date.now() + 5000 | in JS results in a 5 second delay in Java too, then let's use what you have implemented.
>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js
>+ /**
>+ * @param aOptions
>+ * An options JavaScript object holding additional properties for the
>+ * notification. The following properties are currently supported:
>+ * persistence: An integer. The notification will not automatically
>+ * dismiss for this many page loads.
>+ * timeout: A time in milliseconds. The notification will not
>+ */
The "timeout" comment is not complete
r+ if you verify the Date question and fix comment
Attachment #573720 -
Flags: review?(mark.finkle) → review+
Reporter | ||
Comment 3•13 years ago
|
||
I just found some info on the Internet informing me that the Java and JavaScript date-as-milliseconds are interoperable. You are good to go.
Assignee | ||
Comment 4•13 years ago
|
||
Yeah, I copied the way PopupNotificaions.jsm does it, which I assume was just copied from the notification bars. I just messed up copy/pasting the comment - will fix! :)
Assignee | ||
Comment 5•13 years ago
|
||
https://hg.mozilla.org/projects/birch/rev/2fbc10092549
I just looked back at comment 0 and realized that my patch doesn't handle the case described for setting persistence to -1. Desktop doesn't do that, so I didn't realize we wanted it. Want me to file a follow-up about that?
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 6•13 years ago
|
||
I saw it used in a few places:
http://mxr.mozilla.org/mozilla-central/search?string=persistence+=+-1
The BrowserGlue code uses it for the telemetry notification, for example.
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #6)
> I saw it used in a few places:
>
> http://mxr.mozilla.org/mozilla-central/search?string=persistence+=+-1
>
> The BrowserGlue code uses it for the telemetry notification, for example.
I filed bug 702386 and I'll fix it there!
Updated•13 years ago
|
Whiteboard: [QA?]
Updated•13 years ago
|
Whiteboard: [QA?] → [QA-]
Updated•13 years ago
|
tracking-fennec: --- → 11+
Updated•13 years ago
|
status-firefox11:
--- → fixed
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
•