Last Comment Bug 701305 - Refactor DoorHanger code to make it easier to support persistence/timeout options
: Refactor DoorHanger code to make it easier to support persistence/timeout opt...
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: P3 normal (vote)
: ---
Assigned To: :Margaret Leibovic
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on: 698598
Blocks: 700913
  Show dependency treegraph
 
Reported: 2011-11-10 00:30 PST by :Margaret Leibovic
Modified: 2012-01-09 11:57 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


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

Description :Margaret Leibovic 2011-11-10 00:30:49 PST
Created attachment 573441 [details] [diff] [review]
patch

The current remove/hide code for doorhanger notifications is spread around in a way that makes it hard to try to implement bug 700913. I based these changes off the way we handle doorhangers in PopupNotifications.jsm, and it should be easy to add persistence/timeout options on top of this patch.

Basically, I decided that we should just maintain a single set of doorhangers for each tab, and then we can let one method in DoorHangerPopup (updatePopupForTab) handle showing/hiding these doorhangers as appropriate.

I also moved the code that creates DoorHangers to DoorHangerPopup.addDoorHanger so that GeckoApp doesn't need to do as much.
Comment 1 :Margaret Leibovic 2011-11-10 00:34:32 PST
Also, I removed unnecessary calls to removeAllDoorhangers() in Tab, since handleLocationChange takes care of those cases for us (although I need to double check about reload).
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-10 05:34:44 PST
Comment on attachment 573441 [details] [diff] [review]
patch

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

>   doorhanger: {

>-    show: function(aMessage, aValue, aButtons, aTab) {
>-      // use the current tab if none is provided
>-      let tabID = aTab ? aTab : BrowserApp.selectedTab.id;
>+
>+    show: function(aMessage, aValue, aButtons, aTabID) {

One of the refactors I want to do is make the NativeWindow API use "Tab" objects and not the Tab.id". I think it feels more natural taht way. Internally, the API will send the ID over to Java, but we shouldn't need to force the JS side to deal with tabIDs.

We can do that in a NativeWindow API refactor though.


Nice cleanup
Comment 3 :Margaret Leibovic 2011-11-10 11:33:03 PST
This depends on the patches from bug 698598 that were recently backed out, so I'm gonna wait until those re-land to land this.
Comment 4 :Margaret Leibovic 2011-11-11 13:57:29 PST
https://hg.mozilla.org/projects/birch/rev/7c27d32e8c2f

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