732336 - Consolidate doorhanger logic

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Attachment: 1/5) Move doohanger gecko event listeners from GeckoApp to DoorHangerPopup

When I was trying to debug bug 731963, I decided I wanted to make the doorhanger logic easier to follow. I don't want to land this until we have test coverage (bug 732212), but here are some WIP patches.

Doorhanger functionality remains in tact through patch 3, but it starts to have problems with patch 4.

Wes, I noticed that I'm not getting TabEvents.SELECTED notifications as expected - have you tested this?

Comment 1

[:Margaret Leibovic]

Attachment: (2/5) Store DoorHangers in a map in DoorHangerPopup instead of in Tab instances

Comment 2

[:Margaret Leibovic]

Attachment: (3/5) Make DoorHanger a private class inside DoorHangerPopup.java

Comment 3

[:Margaret Leibovic]

Attachment: (4/5) Add an onTabsChanged listener to update doorhanger popup on tab change events

Wes, can you help me figure out what's going wrong with this patch?

Comment 4

[:Margaret Leibovic]

Attachment: (5/5) Make needlessly public methods into private methods, and make logging more useful

Comment 5

[:Margaret Leibovic]

I don't think this should block. This is low-priority refactoring work.

Comment 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

related to bug 747023, bug 718705, bug 700437 	?

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 602272 [details] [diff] [review]
(4/5) Add an onTabsChanged listener to update doorhanger popup on tab change events

Review of attachment 602272 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/DoorHangerPopup.java
@@ +56,5 @@
>  import org.json.JSONObject;
>  import org.json.JSONException;
>  
> +public class DoorHangerPopup extends PopupWindow implements GeckoEventListener,
> +        Tabs.OnTabsChangedListener {

You'll need to call Tabs.registerOnTabsChangedListener somewhere I think?

::: mobile/android/base/GeckoApp.java
@@ +744,2 @@
>                      mBrowserToolbar.setShadowVisibility(!(tab.getURL().startsWith("about:")));
> +                    Tabs.getInstance().notifyListeners(tab, Tabs.TabEvents.LOCATION_CHANGE);

Hmm.. Can we fire this from Tab.updateURI() instead (In fact, there's a ton of Tab.updateFavicon, Tab.updateURL, etc. stuff in there that seems like it could move somewhere.... but shuffling code around is what I do best).

Comment 8

[:Margaret Leibovic]

Attachment: (Part 1) Move doorhanger-related listeners to DoorHangerPopup

A lot has changed since I first wrote these patches, so I'm going to make another pass at cleaning up the doorhanger logic, mainly so it will be easier to fix 777045.

This patch moves the doorhanger-related listeners to DoorHangerPopup. Some other changes that made their way in here:
-Kill hidePopup and use dismiss directly (if there was ever a reason for a separate method, it doesn't exist anymore)
-Change the way we pass around an anchor view for the popup to pass it into the constructor instead of updatePopup. This lets us get rid of some method overloading grossness, and I noticed that there was actually a bug in the showAtLocation case (we weren't calling fixBackgroundForFirst or checking if the popup is already showing).
-I also cleaned up the method signatures to only be public or package-scope as necessary.

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 645482 [details] [diff] [review]
(Part 1) Move doorhanger-related listeners to DoorHangerPopup

Review of attachment 645482 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/DoorHangerPopup.java
@@ +65,5 @@
> +                GeckoApp.mAppContext.mMainHandler.post(new Runnable() {
> +                    public void run() {
> +                        Tab tab = Tabs.getInstance().getTab(tabId);
> +                        if (tab != null)
> +                            addDoorHanger(message, value, buttons, tab, options);

Not your code, but strange that we have addDoorHanger here and tab.removeDoorHanger below....

::: mobile/android/base/GeckoApp.java
@@ +1757,5 @@
>  
> +    void initializeDoorhangerPopup() { 
> +        mDoorHangerPopup = new DoorHangerPopup(this, null);
> +    }
> +

We can just do this in initializeChrome and kill this function.

Comment 10

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #9)
> Comment on attachment 645482 [details] [diff] [review]
> (Part 1) Move doorhanger-related listeners to DoorHangerPopup
> 
> Review of attachment 645482 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/DoorHangerPopup.java
> @@ +65,5 @@
> > +                GeckoApp.mAppContext.mMainHandler.post(new Runnable() {
> > +                    public void run() {
> > +                        Tab tab = Tabs.getInstance().getTab(tabId);
> > +                        if (tab != null)
> > +                            addDoorHanger(message, value, buttons, tab, options);
> 
> Not your code, but strange that we have addDoorHanger here and
> tab.removeDoorHanger below....

Yeah, working on a patch now to try to clean some of that up... there are way too many identically names methods in various classes that all call each other.

Comment 11

[:Margaret Leibovic]

Attachment: (Part 2) Refactor DoorHanger/Tab to centralize logic in DoorHangerPopup

This patch got kinda bigger than I anticipated, but there were lots of inter-dependent changes going on, so it would be hard to break it up more.

I made DoorHangerPopup keep track of the DoorHanger objects, so now Tab knows nothing about doorhangers (yay!). I also cleaned up DoorHanger a bit to get rid of unnecessary methods, and consolidate things. And along the way I believe I fixed the issue Kats was seeing in bug 777045.

One issue I see with this patch is that we don't remove tabs from the mTabsToDoorHangers map when they are destroyed, but I can fix that up when I add an onTabsChanged listener (didn't want to continue to scope creep this patch!).

I also keep intending to write some robocop tests for this stuff, but we don't really have a framework for creating arbitrary doorhangers, so I would have to make that :/

Comment 12

[:Margaret Leibovic]

Attachment: (Part 3) Listen for tab events in DoorHangerPopup

I'm pretty excited about this because it lets us kill GeckoApp.updatePopups, and also clean up GeckoApp.onTabChanged.

Comment 13

[Wesley Johnston (:wesj)]

This guy:

private HashMap<Integer, HashMap<String, DoorHanger>> mTabsToDoorHangers;

makes me nervous for some reason. Probably just me being silly, but margaret said she'd look at just using a flat list.

Comment 14

[:Margaret Leibovic]

Attachment: (Part 1) Move doorhanger-related listeners to DoorHangerPopup (v2)

A few minor changes. I addressed the comment to add the mDoorHangerPopup assignment to initializeChrome, but in order to avoid ended up with two DoorHangerPopups, I had to get rid of the call to super.initializeChrome in BrowserApp.

Comment 15

[:Margaret Leibovic]

Attachment: (Part 2) Refactor DoorHanger/Tab to centralize logic in DoorHangerPopup (v2)

I got rid of the HashMap, and instead I'm using a HashSet of DoorHangers. This is much simpler, and it led me to find some other problems with our current implementation (like the fact that we never actually removed DoorHanger views from the mContent LinearLayout).

Even though we need to iterate through this entire set sometimes, it should always be pretty small, so I don't think it's a big deal.

Comment 16

[:Margaret Leibovic]

Attachment: (Part 3) Listen for tab events in DoorHangerPopup (v2)

Updated to deal with the changes to the second patch.

Comment 17

[:Margaret Leibovic]

These patches are now bit-rotted by bug 778247.

Sriram, do you have a suggestion for the best way to run things on the UI thread from inside DoorHangerPopup? Should I store the activity like you did in Tabs? It kinda sucks that PopupWindow isn't a view, so we can't just call post on it.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

PopupWindow has a getContentView() method which gets you a view on which you can post things.

Comment 19

[Wesley Johnston (:wesj)]

Comment on attachment 645924 [details] [diff] [review]
(Part 2) Refactor DoorHanger/Tab to centralize logic in DoorHangerPopup (v2)

Review of attachment 645924 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like good cleanup. I saw the runnable's bit. I can stare again when you fix that, but it sounds like a simple fix. A couple tiny nits below and some thinking out loud stuff.

The initialization bits here also annoy me. For some reason when I see a separate init method I assume we're doing lazy initialization when we're showing the popup, but turns out we're just putting it off until we get addDoorHanger (and doorhangers are initialized when they're added as well). Maybe its just me, but I think we can be lazier. heh.

::: mobile/android/base/DoorHanger.java
@@ +27,5 @@
>  
>      private Context mContext;
> +    private final int mTabId;
> +    // Value used to identify the notification
> +    private final String mValue;

No need to move this, is there? Probably another separate bug, but I get confused by the term "value" here. I think at first glance I would expect getValue to return the message in the doorhanger to me. I wonder if we can rename it something like "type"?

@@ +95,5 @@
>              // If the checkbox is being used, pass its value
>              if (mCheckBox != null)
>                  response.put("checked", mCheckBox.isChecked());
> +        } catch (JSONException e) {
> +            Log.e(LOGTAG, "Error creating onClick response", e);

I don't really think the 'x' is worth breaking blame here.

@@ +109,3 @@
>      }
>  
> +    private void addButton(String aText, int aCallback) {

And probably no need to move this

::: mobile/android/base/DoorHangerPopup.java
@@ +113,5 @@
> +            removeDoorHanger(oldDoorHanger);
> +
> +        final DoorHanger newDoorHanger = new DoorHanger(mContext, tabId, value);
> +        mDoorHangers.add(newDoorHanger);
> +        Log.i(LOGTAG, "Added " + newDoorHanger.toString());

Throughout, I don't really like logging if we don't need it. If we want to leave it in, I think we should add a final static boolean DEBUG flag for it.

@@ +163,5 @@
> +                doorHangersToRemove.add(dh);
> +        }
> +
> +        for (DoorHanger dh : doorHangersToRemove) {
> +            removeDoorHanger(dh);

This will wind up creating a different runnable for every doorhanger we remove. I wonder if we can batch them? Maybe a removeDoorHangers method. Not a big enough issue to worry about here I guess.

@@ +180,5 @@
>  
> +        // Show doorhangers for the selected tab
> +        int tabId = tab.getId();
> +        Log.i(LOGTAG, "Updating DoorHangerPopup for tab " + tabId);
> +        boolean doorHangersToShow = false;

I'd rather something like willShowPopup.

@@ +218,5 @@
>          showAsDropDown(mAnchor, offset, 0);
>      }
>  
>      private void fixBackgroundForFirst() {
> +        for (int i = 0; i < mContent.getChildCount(); i++) {

I wonder if we can combine this with the loop in updatePopup (to minimize the number of times we loop over this (likely small) array).

Comment 20

[Wesley Johnston (:wesj)]

Comment on attachment 645926 [details] [diff] [review]
(Part 3) Listen for tab events in DoorHangerPopup (v2)

Review of attachment 645926 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me. Tests?

::: mobile/android/base/GeckoApp.java
@@ +822,5 @@
>          tab.setCheckerboardColor(Color.WHITE);
>  
>          mMainHandler.post(new Runnable() {
>              public void run() {
> +                Tabs.getInstance().notifyListeners(tab, Tabs.TabEvents.LOCATION_CHANGE, uri);

I like that this was easy! although its weird this is being sent from GeckoApp and not Tab.updateURL. Different bug....

Comment 21

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #19)

> The initialization bits here also annoy me. For some reason when I see a
> separate init method I assume we're doing lazy initialization when we're
> showing the popup, but turns out we're just putting it off until we get
> addDoorHanger (and doorhangers are initialized when they're added as well).
> Maybe its just me, but I think we can be lazier. heh.

Well, as long as we're adding the doorhanger to the selected tab, we'll be showing it right afterwards. And if we were lazier, we'd have to wait to add the doorhanger view to the content of the popup, which seems not worth it.

> ::: mobile/android/base/DoorHanger.java
> @@ +27,5 @@
> >  
> >      private Context mContext;
> > +    private final int mTabId;
> > +    // Value used to identify the notification
> > +    private final String mValue;
> 
> No need to move this, is there? Probably another separate bug, but I get
> confused by the term "value" here. I think at first glance I would expect
> getValue to return the message in the doorhanger to me. I wonder if we can
> rename it something like "type"?

Yeah, this is really used as an identifier. PopupNotifications uses "id" for this term. Maybe we should do that.

> @@ +95,5 @@
> >              // If the checkbox is being used, pass its value
> >              if (mCheckBox != null)
> >                  response.put("checked", mCheckBox.isChecked());
> > +        } catch (JSONException e) {
> > +            Log.e(LOGTAG, "Error creating onClick response", e);
> 
> I don't really think the 'x' is worth breaking blame here.

I changed this Log.e call to print a stack trace for the exception. Kats landed some patches recently to fix this up in a bunch of places, so I was following suit. I just figured I'd make the e param more consistent while I was at it.

> @@ +218,5 @@
> >          showAsDropDown(mAnchor, offset, 0);
> >      }
> >  
> >      private void fixBackgroundForFirst() {
> > +        for (int i = 0; i < mContent.getChildCount(); i++) {
> 
> I wonder if we can combine this with the loop in updatePopup (to minimize
> the number of times we loop over this (likely small) array).

I thought about that, too, but mDoorHangers is an unordered set, so iterating over it is actually different than iterating over mContent. I feel like it would be nicer to change the way we do our styling to avoid this altogether, but that could be a separate bug.

Comment 22

[:Margaret Leibovic]

Attachment: (Part 1) Move doorhanger-related listeners to DoorHangerPopup (v3)

I'd appreciate it if you could look at these again, since I made more changes to avoid GeckoApp.mAppContext. Since we set the content for the popup inside one of these runnables, calling getContentView() doesn't work for us.

We're already casting mContext to a GeckoApp in one spot, and we already operate under the assumption that this is being made from a GeckoApp activity, so I decided to just pass the activity into the constructor.

Comment 23

[:Margaret Leibovic]

Attachment: (Part 2) Refactor DoorHanger/Tab to centralize logic in DoorHangerPopup (v3)

This is a nicer diff than the last one - sorry for all the noisy changes!

Same thing here, I decided to hold onto the activity in DoorHanger. Also, I decided that it made sense to pass a reference to the popup into DoorHanger, so that it doesn't have to make roundabout calls to get at it.

We use "value" all over the place, and I don't want to try to stuff that change into this patch, so I think that would be a good separate rename patch/bug.

Comment 24

[:Margaret Leibovic]

Attachment: (Part 3) Listen for tab events in DoorHangerPopup (v3)

Just un-bitrotting for this one.

Comment 25

[Wesley Johnston (:wesj)]

Comment on attachment 647333 [details] [diff] [review]
(Part 1) Move doorhanger-related listeners to DoorHangerPopup (v3)

Review of attachment 647333 [details] [diff] [review]:
-----------------------------------------------------------------

If you can use mActivity.isTablet(), I'm fine with this, but we should file a follow up to find out if we can do the tablet switch in styles instead.

::: mobile/android/base/BrowserApp.java
@@ -217,5 @@
>          super.finishProfileMigration();
>      }
>  
>      @Override void initializeChrome(String uri, Boolean isExternalURL) {
> -        super.initializeChrome(uri, isExternalURL);

Add a little comment reminding us why not do this. I'm a little bit comment happy right now. heh.

::: mobile/android/base/DoorHangerPopup.java
@@ +27,5 @@
>      private static final String LOGTAG = "GeckoDoorHangerPopup";
>  
> +    private GeckoApp mActivity;
> +    private View mAnchor;
> +    private boolean mIsTablet;

I don't really want mIsTablet here, and I don't think we need it. WORST case, we have a GeckoApp we can use. i.e. mActivity.isTablet() right? (I wonder if we can just move this layout switch to the xml anyway? the positioning bit shouldn't really care about tablets or not from what I can see...)

Comment 26

[Wesley Johnston (:wesj)]

Comment on attachment 647336 [details] [diff] [review]
(Part 3) Listen for tab events in DoorHangerPopup (v3)

Review of attachment 647336 [details] [diff] [review]:
-----------------------------------------------------------------

I think we want this too?

::: mobile/android/base/GeckoApp.java
@@ +649,5 @@
>          tab.setCheckerboardColor(Color.WHITE);
>  
>          mMainHandler.post(new Runnable() {
>              public void run() {
> +                Tabs.getInstance().notifyListeners(tab, Tabs.TabEvents.LOCATION_CHANGE, uri);

This should be in the Tab object itself I think. Future bug....

Comment 27

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2231c3083b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d60c5d6e882
https://hg.mozilla.org/integration/mozilla-inbound/rev/24ed03720db7

(In reply to Wesley Johnston (:wesj) from comment #26)
> ::: mobile/android/base/GeckoApp.java
> @@ +649,5 @@
> >          tab.setCheckerboardColor(Color.WHITE);
> >  
> >          mMainHandler.post(new Runnable() {
> >              public void run() {
> > +                Tabs.getInstance().notifyListeners(tab, Tabs.TabEvents.LOCATION_CHANGE, uri);
> 
> This should be in the Tab object itself I think. Future bug....

Would you also want to move the notifyListeners calls for START/STOP/LOADED/etc? It seems like this is part of a bigger problem that we're handling all these tab-related gecko events in GeckoApp. Looking at these handlers, it seems like we might want to move them to Tabs. Like, check out what we're doing in handleDocumentStart:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1256

I might end up changing around the way Tabs works because of issues we found in bug 730039. I think we should look into this tab event issue after dealing with that bug.

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d2231c3083b9
https://hg.mozilla.org/mozilla-central/rev/0d60c5d6e882
https://hg.mozilla.org/mozilla-central/rev/24ed03720db7