Closed Bug 908364 Opened 11 years ago Closed 11 years ago

Doorhanger popup can appear on top of awesomescreen in editing mode

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

ARM
Android
defect
Not set
normal

Tracking

(fennec26+)

VERIFIED FIXED
Firefox 26
Tracking Status
fennec 26+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(2 files, 1 obsolete file)

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 :/
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → 26+
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #800444 - Flags: review?(bnicholson)
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.
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().
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 on attachment 800444 [details] [diff] [review]
patch

Removing r? for listener changes. Hopefully using interfaces will be more straightforward this time!
Attachment #800444 - Flags: review?(bnicholson)
Attached patch patch v2Splinter Review
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.
Attachment #800444 - Attachment is obsolete: true
Attachment #803857 - Flags: review?(bnicholson)
Attachment #803857 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/33fbbf1340d9
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.