Some minor cleanup in our popup code: * Take a generic Context instead of BrowserApp. * Require all ArrowPopups be anchored to some view before they're shown. This allows us to remove the ugly getView() from GeckoApp. https://tbpl.mozilla.org/?tree=Try&rev=fbe6eba50aee

Comment on attachment 8438708 [details] [diff] [review] Popup cleanup Review of attachment 8438708 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/DoorHangerPopup.java @@ -9,4 @@ > import org.mozilla.gecko.widget.ArrowPopup; > import org.mozilla.gecko.widget.DoorHanger; > import org.mozilla.gecko.prompts.PromptInput; > - Eclipse's fault. Added this line back (along with the one below) locally.

(In reply to Brian Nicholson (:bnicholson) from comment #1) > * Require all ArrowPopups be anchored to some view before they're shown. Hmm... We always wanted to make our main menu an ArrowPopup (and remove a lot of duplicated code we've got in it). I guess this will make that impossible.

(In reply to Wesley Johnston (:wesj) from comment #3) > (In reply to Brian Nicholson (:bnicholson) from comment #1) > > * Require all ArrowPopups be anchored to some view before they're shown. > > Hmm... We always wanted to make our main menu an ArrowPopup (and remove a > lot of duplicated code we've got in it). I guess this will make that > impossible. I haven't seen any discussion about that -- is there a bug? If there's no anchor, the arrow just gets placed at (0,0). I think the arrow should always point at *something*; otherwise, why use an ArrowPopup? But I'm also not sure why that would be impossible...couldn't we use the menu button as the anchor?

On devices without the menu button, we "anchor" to the bottom of the screen and should hide the arrow right? I think this is a significant refactor so I wouldn't worry. Bigger concern: we also have a "bug" with webapps right now where doorhangers point to nothing. There is nothing to anchor them too even. Will this break them?

(In reply to Wesley Johnston (:wesj) from comment #5) > On devices without the menu button, we "anchor" to the bottom of the screen > and should hide the arrow right? I think this is a significant refactor so I > wouldn't worry. Not sure what you mean -- we don't have an arrow on any menus right now, do we? I thought we removed the menu arrow awhile ago (which is why I'm confused why we'd move back to an ArrowPopup). > Bigger concern: we also have a "bug" with webapps right now where > doorhangers point to nothing. There is nothing to anchor them too even. Will > this break them? Yes, probably... :( I'll see if there's an easy way to fix this -- I'd really like to start chipping away at these arbitrary getters we've tacked onto GeckoApp.

Round 2. We can set the anchor view to the root view by default, so there's never a situation where mAnchor can be null.

Minor fixes.

So after testing, the last patch doesn't work either for anchorless doorhangers. The reason is that anchorLocation gets set to 0 for the root view, causing us to skip the "out of bounds" block, which shows the anchor below the anchor view as normal. Problem is, that's the bottom of the screen since the root view takes up the entire window. I just gave up and put the null checks for mAnchor back. We can still use the root view in the "out of bounds" block, so we can still get rid of GeckoApp#getView().

Comment on attachment 8438794 [details] [diff] [review] Popup cleanup, v4 Review of attachment 8438794 [details] [diff] [review]: ----------------------------------------------------------------- Kudos to you for the minimal cleanup, I feel like it would be tempting to do more :) ::: mobile/android/base/DoorHangerPopup.java @@ +10,2 @@ > import org.mozilla.gecko.widget.ArrowPopup; > import org.mozilla.gecko.widget.DoorHanger; Why is DoorHangerPopup in org.mozilla.gecko, but DoorHanger is in org.mozilla.gecko.widget (man, I really hate that name DoorHanger). Should we move this class into widget? I guess it's pretty full of a bunch of other dependencies, so maybe it's not widget material. SiteIdentityPopup is part of the toolbar package. If we generally expect doorhangers to be anchored to the toolbar, maybe this should move there as well. Although GeckoApp supports doorhangers that aren't anchored to anything, so maybe not then. Sigh, this is a dependency mess. ::: mobile/android/base/widget/ArrowPopup.java @@ +48,3 @@ > protected void init() { > + // Hide the default window background. Passing null prevents the below setOutTouchable() > + // call from working, so use an empty BitmapDrawable instead. Was this a bug that existed before? @@ +90,2 @@ > if (mAnchor == null || anchorLocation[1] < 0) { > + final View rootView = mContent.getRootView(); This assumes init() will always be called before show(). Are we sure that will happen? Should we add an mInflated check at the beginning of this method to be sure?

(In reply to :Margaret Leibovic from comment #10) > Kudos to you for the minimal cleanup, I feel like it would be tempting to do > more :) I tried, but this patch kept growing as I worked on these other doorhanger dependencies. Sorry! New changes: * ArrowPopup et al now take a Context rather than an Activity * Several menu_popup_X dimens were renamed to arrow_popup_X since these values were being used only in ArrowPopup. > ::: mobile/android/base/DoorHangerPopup.java > Why is DoorHangerPopup in org.mozilla.gecko, but DoorHanger is in > org.mozilla.gecko.widget (man, I really hate that name DoorHanger). Should > we move this class into widget? I guess it's pretty full of a bunch of other > dependencies, so maybe it's not widget material. Yeah, taking a survey in widget/, most of our widgets are pretty generic. DoorHangerPopup is very coupled to Fennec, so probably wouldn't qualify as a reusable component. Honestly, DoorHanger is also pretty heavy (and even has methods that accept JSON), so I'd argue that DoorHanger should be taken out of widget/. I can tack on this change if you agree. > SiteIdentityPopup is part of the toolbar package. If we generally expect > doorhangers to be anchored to the toolbar, maybe this should move there as > well. Although GeckoApp supports doorhangers that aren't anchored to > anything, so maybe not then. Sigh, this is a dependency mess. Yeah :-\ > ::: mobile/android/base/widget/ArrowPopup.java > @@ +48,3 @@ > > protected void init() { > > + // Hide the default window background. Passing null prevents the below setOutTouchable() > > + // call from working, so use an empty BitmapDrawable instead. > > Was this a bug that existed before? Yes, it just wasn't commented. I tried changing this to null and hit the bug described there. (I also added the extra 'mContext.getResources()' argument there because the no-args BitmapDrawable is deprecated.) > @@ +90,2 @@ > > if (mAnchor == null || anchorLocation[1] < 0) { > > + final View rootView = mContent.getRootView(); > > This assumes init() will always be called before show(). Are we sure that > will happen? Should we add an mInflated check at the beginning of this > method to be sure? It would probably crash anyway, but yeah, that's a good idea to make this mess slightly more explicit.

Comment on attachment 8442546 [details] [diff] [review] Popup cleanup, v5 Review of attachment 8442546 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/resources/values/dimens.xml @@ +108,5 @@ > <dimen name="toast_button_corner_radius">2dp</dimen> > + > + <!-- ArrowPopup dimensions. --> > + <dimen name="arrow_popup_arrow_width">40dip</dimen> > + <dimen name="arrow_popup_tablet_width">400dp</dimen> Instead of having a tablet-specific value in the general dimens.xml, could you just define this in the large-vll dimens.xml? ::: mobile/android/base/widget/ArrowPopup.java @@ +95,2 @@ > if (mAnchor == null || anchorLocation[1] < 0) { > + final View decorView = ((Activity) mContext).getWindow().getDecorView(); Why did you switch from using the root view to this decor view? ::: mobile/android/base/widget/DoorHanger.java @@ +43,5 @@ > private static int sSpinnerTextSize = -1; > > private static LayoutParams sButtonParams; > static { > + sButtonParams = new LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT, 1.0f); If you want you could file a mentor bug on fixing our use of fill_parent throughout our codebase.

(In reply to :Margaret Leibovic from comment #12) > Instead of having a tablet-specific value in the general dimens.xml, could > you just define this in the large-vll dimens.xml? Good point, I forgot that existed. > ::: mobile/android/base/widget/ArrowPopup.java > @@ +95,2 @@ > > if (mAnchor == null || anchorLocation[1] < 0) { > > + final View decorView = ((Activity) mContext).getWindow().getDecorView(); > > Why did you switch from using the root view to this decor view? The decor view *is* the root view -- I just changed how we were getting a handle to it. The last patch used mContent.getRootView(), but that's broken (I swear I tested it, but I'm not sure how it would have worked). mContent is the popup's content, which isn't even in a window yet. > ::: mobile/android/base/widget/DoorHanger.java > If you want you could file a mentor bug on fixing our use of fill_parent > throughout our codebase. Filed bug 1027831. There are a lot!