Last Comment Bug 700913 - Add persistence and timeout features to Doorhangers
: Add persistence and timeout features to Doorhangers
Status: RESOLVED FIXED
[QA-]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Linux
: P2 normal (vote)
: ---
Assigned To: :Margaret Leibovic
:
:
Mentors:
Depends on: 701305 702386
Blocks: 697659 699513
  Show dependency treegraph
 
Reported: 2011-11-08 19:47 PST by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2016-07-29 14:20 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
patch (8.85 KB, patch)
2011-11-10 18:46 PST, :Margaret Leibovic
mark.finkle: review+
Details | Diff | Splinter Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2011-11-08 19:47:45 PST
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.
Comment 1 :Margaret Leibovic 2011-11-10 18:46:36 PST
Created attachment 573720 [details] [diff] [review]
patch

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!
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-11 08:47:45 PST
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
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-11 08:53:24 PST
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.
Comment 4 :Margaret Leibovic 2011-11-11 09:23:22 PST
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! :)
Comment 5 :Margaret Leibovic 2011-11-11 14:00:37 PST
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?
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-14 12:25:38 PST
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.
Comment 7 :Margaret Leibovic 2011-11-14 12:58:29 PST
(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!

Note You need to log in before you can comment on or make changes to this bug.