908364 - Doorhanger popup can appear on top of awesomescreen in editing mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[:Margaret Leibovic]

Attachment: Screenshot_2013-08-22-15-23-14.png

See bug 906041 comment 6 for STR.

If we avoid showing the notification while in editing mode, we'll want to make sure to show it after exiting editing mode. I'm worried we might end up with some tricky logic here :/

Comment 1

[:Margaret Leibovic]

Attachment: patch

I don't love that this patch sprinkles mDoorHangerPopup.enable() calls all over the place, but that's a by-product of the fact that there are multiple calls to mBrowserToolbar.cancelEdit/commitEdit. I suppose it would be nicer if BrowserToolbar could be handling these things, but it doesn't know about DoorHangerPopup.

I'm open to suggestions if anyone has ideas about how to make this better, but it does get the job done.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I seems like BrowserToolbar should have a callback interface for starting and ending editing. Then BrowserApp could implement the interface, and pass "this" to BrowserToolbar.

BrowsserToolbar would call the onStartEditing and onStopEditing(/*cancel or commit*/) giving BrowserApp know locations where we would enable and disable the doorhanger.

Comment 3

[Brian Nicholson (:bnicholson)]

I agree we should factor this out into some kind of interface. We already have hideBrowserSearch() at every call to cancelEdit()/commitEdit(), so an interface would be a nice way to get rid of this growing code duplication.

An alternative (but similar) approach you could use would be to pass the implemented callbacks directly to commitEdit()/cancelEdit().

Comment 4

[:Margaret Leibovic]

I just noticed we already have these listeners:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#451

However, we can't just stick our mDoorHangerPopup.enable/disable calls in there because there are other places we call enterEditingMode() and dismissEditingMode().

Maybe we need to reorganize some of this logic to put BrowserToolbar more in charge of the state of things, so that these listeners are always called.

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 800444 [details] [diff] [review]
patch

Removing r? for listener changes. Hopefully using interfaces will be more straightforward this time!

Comment 6

[:Margaret Leibovic]

Attachment: patch v2

To keep this patch well-scoped, I only put the doorhanger stuff in the listeners right now, but we could do some more refactoring on top of this.

One concern I have is that maybe I should have put the onStartEditing/onStopEditing calls lower down in startEditing/stopEditing, below the animation logic. However, to do that, I'd have to refactor those methods a bit to avoid the early returns.

Comment 7

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/33fbbf1340d9

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/33fbbf1340d9