1150613 - Doorhanger should not reappear after being dismissed

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

STR:
1. Attempt to log into a site with a new login, doorhanger pops up
2. Dismiss by tapping outside the doorhanger
3. Switch tabs.
4. Switch back.

Expected: Tab is switched, no doorhanger shows
Actual: Doorhanger is shown again, despite being dismissed

We don't necessarily want to remove the doorhanger entirely, because we will want to summon it again in bug 700437. Just don't re-show the doorhanger popup.

Comment 1

[Chenxia Liu [:liuche]]

Ok, this is actually worse than I thought, I was able to repro a login save/update doorhanger following you around forever, across indefinite url changes, on Firefox 35, and 38.0.5.

1. Attempt to log into a site with a new login, doorhanger pops up
2. Tap on urlbar to go to awesome screen
3. Click on a link from top sites, or type in the url and hit enter

Expected: Doorhanger no longer shows
Actual: Login save/update doorhanger keeps showing up

Comment 2

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1150613 - Doorhanger should not reappear after being dismissed. r=margaret

Bug 1150613 - Doorhanger should not reappear after being dismissed. r=margaret

Comment 3

[:Margaret Leibovic]

Comment on attachment 8624510 [details]
MozReview Request: Bug 1150613 - Doorhanger should not reappear after being dismissed. r=margaret

https://reviewboard.mozilla.org/r/11643/#review10179

r+ with comments addressed.

::: mobile/android/base/DoorHangerPopup.java:330
(Diff revision 1)
> +    public void onDismiss() {

What happens if this gets called while the selected tab is switching? I just want to make sure that we can't run into an edge case where we remove doorhangers from the wrong tab.

::: mobile/android/base/DoorHangerPopup.java:247
(Diff revision 1)
> +                && (!isTransient || (isTransient && dh.shouldRemove(isShowing())))) {

This `isTransient` parameter name is a bit confusing... what we're really doing here is maybe keeping a doorhanger if the persistence/timeout parameters say so, so maybe this should be something more like `checkShouldRemove`. Or alternately, flip the true/false values to say `forceRemove` or something like that.

Also, you should update the method documentation to explain this parameter.

Comment 4

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/11643/#review10185

> What happens if this gets called while the selected tab is switching? I just want to make sure that we can't run into an edge case where we remove doorhangers from the wrong tab.

I think the case where the user dismisses a doorhanger and then is able to switch a tab fast enough to do something in between the dismiss() call and the onDismiss callback is really, really low. That being said, if onDismiss is a really delayed callback or something, then we'll have some concurrency problems. This might look like the case where for some location pages, the doorhanger displays and then gets hidden again. However, I just filed bug 1176138 for that, and include this edge case in the consideration.

This is blocking bug 1173887, and this seems like a really small edge case, so I'd rather land it, and then deal with this case if problems arise.

Comment 5

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/8c6f31ca0635

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c6f31ca0635