The default bug view has changed. See this FAQ.

Refactor DoorHanger code to make it easier to support persistence/timeout options

RESOLVED FIXED

Status

()

Firefox for Android
General
P3
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

unspecified
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
Attachment #573441 - Flags: review?(sriram)
Attachment #573441 - Flags: review?(mark.finkle)
(Assignee)

Updated

5 years ago
Attachment #573441 - Attachment is patch: true
(Assignee)

Comment 1

5 years ago
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 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
Attachment #573441 - Flags: review?(mark.finkle) → review+
(Assignee)

Updated

5 years ago
Assignee: nobody → margaret.leibovic
(Assignee)

Updated

5 years ago
Attachment #573441 - Flags: review?(sriram)

Updated

5 years ago
Priority: -- → P3
(Assignee)

Comment 3

5 years ago
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.
Depends on: 698598
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/projects/birch/rev/7c27d32e8c2f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.