Created attachment 794203 [details] 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 :/
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → 26+
Created attachment 800444 [details] [diff] [review] 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.
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!
Created attachment 803857 [details] [diff] [review] 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.
Attachment #803857 - Flags: review?(bnicholson) → review+
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.