Closed Bug 884069 Opened 6 years ago Closed 6 years ago

Create a Java-only DoorHanger API that doesn't depend on Gecko

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 24

People

(Reporter: Margaret, Assigned: Margaret)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch WIP (obsolete) — Splinter Review
This may need more work, but I made a new mixed content patch on top of this that's working well.
Attachment #763834 - Flags: feedback?(wjohnston)
Attachment #763834 - Flags: feedback?(sriram)
Comment on attachment 763834 [details] [diff] [review]
WIP

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

I don't really love doing things this way.... It feels like needlessly separating things. Maybe my Java-hate is just coming out.

If its readability we're worried about, a lot of the code in our current DoorHanger.java can be simplified quite a bit. For example, most of the try-catch blocks in setOptions can be replaced by using JSONObject.opt<Something>() methods. I think we could probably write the onClick handling for the JSON members in a clearer way as well. Something like:

button.setOnClickListener(new DoorHanger.JSONButtonHandler(buttonObject));

and then we can remove the tags entirely and just move our onClick handler into a small subclass... or maybe you can talk me into this.

::: mobile/android/base/DoorHangerPopup.java
@@ +22,5 @@
>      private static final String LOGTAG = "GeckoDoorHangerPopup";
>  
>      // Stores a set of all active DoorHanger notifications. A DoorHanger is
>      // uniquely identified by its tabId and value.
> +    private HashSet<GeckoDoorHanger> mDoorHangers;

It seems like the Popup shouldn't care at all if these are GeckoDoorhangers or just normal ones, should it? If it only cares about some, the comment above this is now wrong.

@@ +132,4 @@
>          if (oldDoorHanger != null)
>              removeDoorHanger(oldDoorHanger);
>  
> +        final GeckoDoorHanger newDoorHanger = new GeckoDoorHanger(mActivity, this, tabId, value, message, buttons, options);

So I'm not a huge fan of doing this. Why not just two constructors in DoorHanger. One that takes parameters and one that takes a single JSONObject. i.e:

final DoorHanger dh = new DoorHanger(context, popup, json);

(I don't really like that we have this cyclical reference between popup and the doorhanger either. Why should the doorhanger care where its shown? Maybe that's impossible to remove...)

That constructor should ideally pull apart the message and just call the native Java constructor.

::: mobile/android/base/GeckoDoorHanger.java
@@ +40,3 @@
>      private int mPersistence = 0;
>      private boolean mPersistWhileVisible = false;
>      private long mTimeout = 0;

Why are these persistence and timeout values only available to GeckoDoorHangers and not to Java ones?
Attachment #763834 - Flags: feedback?(wjohnston) → feedback-
Okay, you've convinced me that this approach is bad. I'm going to try something different. But I started writing out these responses, so I might as well post them for posterity...

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

> I don't really love doing things this way.... It feels like needlessly
> separating things. Maybe my Java-hate is just coming out.

We can probably find a compromise that doesn't involve creating a new file, but I do think we need to have the separate methods I made to avoid needing to pass around JSON as the only way to do things.

There are a few things that are only used in GeckoDoorHanger, like the persistence/timeout options, and tabId/value, since those are really only used by DoorHangerPopup, but I guess it doesn't hurt for a different consumer to use DoorHanger and just ignore those.

> If its readability we're worried about, a lot of the code in our current
> DoorHanger.java can be simplified quite a bit. For example, most of the
> try-catch blocks in setOptions can be replaced by using
> JSONObject.opt<Something>() methods.

It's not really readability so much as being able to create a doorhanger view without using JSON to tweak the parameters.

> I think we could probably write the
> onClick handling for the JSON members in a clearer way as well. Something
> like:
> 
> button.setOnClickListener(new DoorHanger.JSONButtonHandler(buttonObject));
> 
> and then we can remove the tags entirely and just move our onClick handler
> into a small subclass... or maybe you can talk me into this.

Heh, maybe a subclass would be good. But we do need to keep track of the callback id that we're currently putting in that tag, since that's what we're passing back to gecko, so that it knows what JS callback to use.

> ::: mobile/android/base/DoorHangerPopup.java
> @@ +22,5 @@
> >      private static final String LOGTAG = "GeckoDoorHangerPopup";
> >  
> >      // Stores a set of all active DoorHanger notifications. A DoorHanger is
> >      // uniquely identified by its tabId and value.
> > +    private HashSet<GeckoDoorHanger> mDoorHangers;
> 
> It seems like the Popup shouldn't care at all if these are GeckoDoorhangers
> or just normal ones, should it? If it only cares about some, the comment
> above this is now wrong.

I'll admit I failed to update the comment ;) Yes, it does expect them to be GeckoDoorHangers, because of the GeckoDoorHanger-only things I mentioned up above. But I wonder if we would ever want to let Java add notifications to DoorHangerPopup in addition to Java. If that's the case, we probably don't want DoorHangerPopup to be dependent on GeckoDoorHanger.

> @@ +132,4 @@
> >          if (oldDoorHanger != null)
> >              removeDoorHanger(oldDoorHanger);
> >  
> > +        final GeckoDoorHanger newDoorHanger = new GeckoDoorHanger(mActivity, this, tabId, value, message, buttons, options);
> 
> So I'm not a huge fan of doing this. Why not just two constructors in
> DoorHanger. One that takes parameters and one that takes a single
> JSONObject. i.e:
> 
> final DoorHanger dh = new DoorHanger(context, popup, json);

I'm just working with what's here right now. The only reason I changed this is because it seemed like the init() method is some historic artifact that's not necessary anymore, since we now call init() right after the doorhanger is constructed.

> (I don't really like that we have this cyclical reference between popup and
> the doorhanger either. Why should the doorhanger care where its shown? Maybe
> that's impossible to remove...)

That's because it wants to notify the popup when a button is clicked, so that it can update itself. Maybe we can do this with an interface or a listener or something, but that's a different bug.

> That constructor should ideally pull apart the message and just call the
> native Java constructor.
> 
> ::: mobile/android/base/GeckoDoorHanger.java
> @@ +40,3 @@
> >      private int mPersistence = 0;
> >      private boolean mPersistWhileVisible = false;
> >      private long mTimeout = 0;
> 
> Why are these persistence and timeout values only available to
> GeckoDoorHangers and not to Java ones?

I thought I had a reason for this, but now I'm not quite sure. I think I really started to just make GeckoDoorHanger contain the things that only DoorHangerPopup requires, but that's a dumb way to separate things.
Comment on attachment 763834 [details] [diff] [review]
WIP

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

I'll wait for the new patch ;)
Attachment #763834 - Flags: feedback?(sriram)
Attached patch WIP v2 (obsolete) — Splinter Review
This patch just refactors DoorHanger instead of making a new GeckoDoorHanger class. We could potentially factor more out of setOptions, but for now I'm just factoring out the things I'll need for bug 860581.

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=212db42bac8c
Attachment #763834 - Attachment is obsolete: true
Attachment #765072 - Flags: review?(wjohnston)
Attachment #765072 - Flags: review?(sriram)
Comment on attachment 765072 [details] [diff] [review]
WIP v2

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

Some doubts:

Now that any ArrowPopup can hold a DoorHanger in it, is it the responsibility of the creator of DoorHanger to create a popup and give it to this Doorhanger?
This sounds scary. I would like to have some utility method constructor there.

If DoorHangerPopup deals with Gecko only, I would suggest renaming it to GeckoDoorHangerPopup may be. And the wrapper for the above comment can be a DoorHangerPopup.

r- as I would like to see the changes.

::: mobile/android/base/DoorHanger.java
@@ +40,5 @@
>      private static int sInputPadding = -1;
>      private static int sSpinnerTextColor = -1;
>      private static int sSpinnerTextSize = -1;
>  
> +    private TextView mTextView;

This could be marked as "final".

@@ +53,5 @@
>      private boolean mPersistWhileVisible = false;
>      private long mTimeout = 0;
>  
> +    // The popup that holds this doorhanger.
> +    private final ArrowPopup mPopup;

New line after this.

@@ +55,5 @@
>  
> +    // The popup that holds this doorhanger.
> +    private final ArrowPopup mPopup;
> +    // The tab associated with this notification.
> +    private final int mTabId;

New line after this.

@@ +92,5 @@
> +
> +    DoorHanger(Context context) {
> +        this(context, null, 0, null);
> +    }
> +

This should be the first constructor.

@@ +101,5 @@
>      String getValue() {
>          return mValue;
>      }
>  
> +    List<PromptInput> getInputs() {

I like the package access here.

@@ +113,1 @@
>      public void showDivider() {

Please make these methods to have package access too.

@@ +127,5 @@
> +        SpannableString titleWithLink = new SpannableString(title + " " + label);
> +        URLSpan linkSpan = new URLSpan(url) {
> +            @Override
> +            public void onClick(View view) {
> +                Tabs.getInstance().loadUrlInTab(this.getURL());

"this" might not be needed.

@@ +134,2 @@
>  
> +        // prevent text outside the link from flashing when clicked

Caps at the start. Period at the end.

@@ +138,5 @@
> +
> +        titleWithLink.setSpan(linkSpan, title.length() + 1, titleWithLink.length(), 0);
> +        mTextView.setText(titleWithLink);
> +        mTextView.setMovementMethod(LinkMovementMethod.getInstance());
> +    }

We can set a link color at theme level. I would like to use that approach if possible. I don't think we use that color anywhere now.

@@ +145,1 @@
>          Button button = (Button) LayoutInflater.from(getContext()).inflate(R.layout.doorhanger_button, null);

Make this "final".

@@ +148,2 @@
>  
> +        final DoorHanger dh = this;

Not needed.

@@ +149,5 @@
> +        final DoorHanger dh = this;
> +        button.setOnClickListener(new Button.OnClickListener() {
> +            @Override
> +            public void onClick(View v) {
> +                listener.onClick(v, dh);

listener.onClick(v, Doorhanger.this);

@@ +155,5 @@
> +        });
> +
> +        if (mChoicesLayout == null) {
> +            // If this is the first button we're adding, make the choices layout visible.
> +            mChoicesLayout = (LinearLayout) findViewById(R.id.doorhanger_choices);

This part is confusing. If R.id.doorhanger_choices is going to be in XML, better hold it in the mChoicesLayout, in the constructor itself. This check is unnecessary.

@@ +159,5 @@
> +            mChoicesLayout = (LinearLayout) findViewById(R.id.doorhanger_choices);
> +            mChoicesLayout.setVisibility(View.VISIBLE);
> +            findViewById(R.id.divider_choices).setVisibility(View.VISIBLE);
> +        } else {
> +            // Add a divider for addtional buttons.

I feel this is unnecessary. The old code seems to work right here.

@@ +166,5 @@
>              divider.setBackgroundColor(0xFFD1D5DA);
>              mChoicesLayout.addView(divider);
>          }
>  
> +        LayoutParams layoutParams = new LayoutParams(LayoutParams.FILL_PARENT, LayoutParams.FILL_PARENT, 1.0f);

This better be a static layoutparams at the start.

static {
    sButtonParams = ... ;
}

And set it here.

@@ +178,3 @@
>          }
>  
> +        final boolean persistWhileVisible = options.optBoolean("persistWhileVisible");

Nice use of optXYZ() :)

::: mobile/android/base/DoorHangerPopup.java
@@ +22,3 @@
>  
>  public class DoorHangerPopup extends ArrowPopup
> +                             implements GeckoEventListener, Tabs.OnTabsChangedListener, DoorHanger.OnButtonClickListener {

Break it into 3 separate lines like in BrowserApp.java.

@@ +27,5 @@
>      // Stores a set of all active DoorHanger notifications. A DoorHanger is
>      // uniquely identified by its tabId and value.
>      private HashSet<DoorHanger> mDoorHangers;
>  
>      DoorHangerPopup(GeckoApp aActivity, View aAnchor) {

Rename it to "activity".

@@ +165,5 @@
> +    @Override
> +    public void onClick(View v, DoorHanger dh) {
> +        JSONObject response = new JSONObject();
> +        try {
> +            response.put("callback", v.getTag().toString());

I somehow don't like this. Please add a new listener.

onDoorHangerButtonClickListener(String tag, DoorHanger dh);

Ok, the name is too long :P May be OnInputClickListner(String tag, DoorHanger dh);

I don't like getting a tag and converting it to string.
Attachment #765072 - Flags: review?(sriram) → review-
(In reply to Sriram Ramasubramanian [:sriram] from comment #6)

Thanks for the detailed comments! I'll get to work on a new patch now.

> Now that any ArrowPopup can hold a DoorHanger in it, is it the
> responsibility of the creator of DoorHanger to create a popup and give it to
> this Doorhanger?
> This sounds scary. I would like to have some utility method constructor
> there.

Ideally DoorHanger wouldn't know about ArrowPopup at all, but I had to keep this one reference because it's used in shouldRemove(). However, that's only used by DoorHangerPopup, so I could find a way to factor it out to there. Perhaps a good solution would be to just make the popup a parameter to that method.

Other than this one use case, it doesn't matter if mPopup is null, which is why I made a second constructor that doesn't take an ArrowPopup.

> If DoorHangerPopup deals with Gecko only, I would suggest renaming it to
> GeckoDoorHangerPopup may be. And the wrapper for the above comment can be a
> DoorHangerPopup.

I don't think we need to start naming things GeckoWhatever unless there's a non-Gecko version, since most of our code deals with Gecko in one way or another.

> @@ +127,5 @@
> > +        SpannableString titleWithLink = new SpannableString(title + " " + label);
> > +        URLSpan linkSpan = new URLSpan(url) {
> > +            @Override
> > +            public void onClick(View view) {
> > +                Tabs.getInstance().loadUrlInTab(this.getURL());
> 
> "this" might not be needed.

...

> 
> @@ +138,5 @@
> > +
> > +        titleWithLink.setSpan(linkSpan, title.length() + 1, titleWithLink.length(), 0);
> > +        mTextView.setText(titleWithLink);
> > +        mTextView.setMovementMethod(LinkMovementMethod.getInstance());
> > +    }
> 
> We can set a link color at theme level. I would like to use that approach if
> possible. I don't think we use that color anywhere now.

These are just copy/pasted over. I would rather address them in a separate bug.

> @@ +149,5 @@
> > +        final DoorHanger dh = this;
> > +        button.setOnClickListener(new Button.OnClickListener() {
> > +            @Override
> > +            public void onClick(View v) {
> > +                listener.onClick(v, dh);
> 
> listener.onClick(v, Doorhanger.this);

Cool, I didn't know that you could do this. I'm too used to JS and didn't know what to do without .bind() ;)

> @@ +155,5 @@
> > +        });
> > +
> > +        if (mChoicesLayout == null) {
> > +            // If this is the first button we're adding, make the choices layout visible.
> > +            mChoicesLayout = (LinearLayout) findViewById(R.id.doorhanger_choices);
> 
> This part is confusing. If R.id.doorhanger_choices is going to be in XML,
> better hold it in the mChoicesLayout, in the constructor itself. This check
> is unnecessary.
> 
> @@ +159,5 @@
> > +            mChoicesLayout = (LinearLayout) findViewById(R.id.doorhanger_choices);
> > +            mChoicesLayout.setVisibility(View.VISIBLE);
> > +            findViewById(R.id.divider_choices).setVisibility(View.VISIBLE);
> > +        } else {
> > +            // Add a divider for addtional buttons.
> 
> I feel this is unnecessary. The old code seems to work right here.

The issue with the old code is that we were setting mChoicesLayout and the divider_choices view to visible in init, based on the buttons parameter there. Now that I'm changing the API to let consumers add buttons directly, we need some more logic in here to make the layout/divider visible if it's the first button that's being added. I figured using a null check on mChoicesLayout was a simple way to tell if we've added a button or not, but I could also just do |mChoicesLayout.getChildCount() == 0|, similar to the existing code.
 
> @@ +165,5 @@
> > +    @Override
> > +    public void onClick(View v, DoorHanger dh) {
> > +        JSONObject response = new JSONObject();
> > +        try {
> > +            response.put("callback", v.getTag().toString());
> 
> I somehow don't like this. Please add a new listener.
> 
> onDoorHangerButtonClickListener(String tag, DoorHanger dh);
> 
> Ok, the name is too long :P May be OnInputClickListner(String tag,
> DoorHanger dh);
> 
> I don't like getting a tag and converting it to string.

Once again, just dealing with existing code here. But I can look into making it better.
Attached patch patchSplinter Review
This patch removes the ArrowPopup dependency from DoorHanger. I also clarified the syntax of the button click listener a bit more to just take a doorhanger and a tag as parameters.
Attachment #765072 - Attachment is obsolete: true
Attachment #765072 - Flags: review?(wjohnston)
Attachment #765637 - Flags: review?(wjohnston)
Attachment #765637 - Flags: review?(sriram)
Comment on attachment 765637 [details] [diff] [review]
patch

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

This is looking good.
r+ with couple of changes I've mentioned.

::: mobile/android/base/DoorHanger.java
@@ +45,5 @@
> +    private final TextView mTextView;
> +    private final LinearLayout mChoicesLayout;
> +
> +    // Divider between doorhangers.
> +    private final View mDivider;

finals! yaaay!

@@ +61,5 @@
>      private boolean mPersistWhileVisible = false;
>      private long mTimeout = 0;
>  
> +    public interface OnButtonClickListener {
> +        void onButtonClick(DoorHanger dh, String tag);

Nice :)
Please make it "public".

@@ +147,5 @@
> +
> +        button.setOnClickListener(new Button.OnClickListener() {
> +            @Override
> +            public void onClick(View v) {
> +                listener.onButtonClick(DoorHanger.this, v.getTag().toString());

You might get away with this. Since "tag" is final. It's closed over this Listener. You could directly use tag here. (Closures in java are copied from javascript. Just like bind(), "final"s are "var"s ;) ).

@@ +151,5 @@
> +                listener.onButtonClick(DoorHanger.this, v.getTag().toString());
> +            }
> +        });
> +
> +        if (mChoicesLayout.getChildCount() == 0) {

Thanks :)

@@ +164,5 @@
>              mChoicesLayout.addView(divider);
>          }
>  
> +        if (sButtonParams == null) {
> +            sButtonParams = new LayoutParams(LayoutParams.FILL_PARENT, LayoutParams.FILL_PARENT, 1.0f);

This should go all the way up above the class. inside a "static" block.

@@ +274,5 @@
>          }
>      }
>  
> +
> +    /* 

White space.

@@ +276,5 @@
>  
> +
> +    /* 
> +     * Checks with persistence and timeout options to see if it's okay to remove a doorhanger.
> +     * 

One more white space.

::: mobile/android/base/DoorHangerPopup.java
@@ +170,5 @@
> +    @Override
> +    public void onButtonClick(DoorHanger dh, String tag) {
> +        JSONObject response = new JSONObject();
> +        try {
> +            response.put("callback", tag);

This is soooooo cleaaaan! :D
Attachment #765637 - Flags: review?(sriram) → review+
Addressed remaining comments and pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=83200352e301
Comment on attachment 765637 [details] [diff] [review]
patch

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

::: mobile/android/base/DoorHanger.java
@@ +122,3 @@
>      }
>  
> +    void addLink(String label, String url, String delimiter) {

Yeah. I kinda hate this and want us to try using HTML class:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/PromptInput.java#301

You mind filing a follow up to fix this?

@@ +172,5 @@
>  
> +    void setOptions(final JSONObject options) {
> +        final int persistence = options.optInt("persistence");
> +        if (persistence > 0) {
> +            mPersistence = persistence;

We allow the persistence to be 0 or -1, right....

@@ +178,5 @@
>  
> +        final boolean persistWhileVisible = options.optBoolean("persistWhileVisible");
> +        if (persistWhileVisible) {
> +            mPersistWhileVisible = true;
> +        }

Is there some reason to not just do mPersistWhileVisible = options.optBoolean()?

@@ +183,3 @@
>  
> +        final long timeout = options.optLong("timeout");
> +        if (timeout > 0) {

Same here, but here I guess it protects against negative numbers?

@@ +191,5 @@
> +            try {
> +                final String linkLabel = link.getString("label");
> +                final String linkUrl = link.getString("url");
> +                addLink(linkLabel, linkUrl, " ");
> +            } catch (JSONException e) { }

Hopefully we can get rid of this awfulness at some point... Then again, backwards compat...

::: mobile/android/base/DoorHangerPopup.java
@@ +123,5 @@
>       *
>       * This method must be called on the UI thread.
>       */
>      void addDoorHanger(final int tabId, final String value, final String message,
>                         final JSONArray buttons, final JSONObject options) {

I know this is old, but this is such a strange method signature, taking a mixture of JSON and Strings....
Attachment #765637 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #11)

> ::: mobile/android/base/DoorHanger.java
> @@ +122,3 @@
> >      }
> >  
> > +    void addLink(String label, String url, String delimiter) {
> 
> Yeah. I kinda hate this and want us to try using HTML class:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> PromptInput.java#301
> 
> You mind filing a follow up to fix this?

Yeah, definitely, I'll do that. Adding support for that will make the mixed content UI much nicer to implement.

> @@ +172,5 @@
> >  
> > +    void setOptions(final JSONObject options) {
> > +        final int persistence = options.optInt("persistence");
> > +        if (persistence > 0) {
> > +            mPersistence = persistence;

mPersistence is initialized to 0, so we don't need to check this case. I mostly just did this check because optInt returns 0 if the property isn't found, but that conveniently works for us.

> 
> @@ +178,5 @@
> >  
> > +        final boolean persistWhileVisible = options.optBoolean("persistWhileVisible");
> > +        if (persistWhileVisible) {
> > +            mPersistWhileVisible = true;
> > +        }
> 
> Is there some reason to not just do mPersistWhileVisible =
> options.optBoolean()?

Because I didn't notice that I could do that :)

> @@ +183,3 @@
> >  
> > +        final long timeout = options.optLong("timeout");
> > +        if (timeout > 0) {
> 
> Same here, but here I guess it protects against negative numbers?

Hm, yeah, I was just following a pattern, but I think that negative numbers argument is a good reason to avoid directly setting it.

> @@ +191,5 @@
> > +            try {
> > +                final String linkLabel = link.getString("label");
> > +                final String linkUrl = link.getString("url");
> > +                addLink(linkLabel, linkUrl, " ");
> > +            } catch (JSONException e) { }
> 
> Hopefully we can get rid of this awfulness at some point... Then again,
> backwards compat...

Well, we could just leave this as some sugar around whatever new API we implement for futzing around with the message view.

> ::: mobile/android/base/DoorHangerPopup.java
> @@ +123,5 @@
> >       *
> >       * This method must be called on the UI thread.
> >       */
> >      void addDoorHanger(final int tabId, final String value, final String message,
> >                         final JSONArray buttons, final JSONObject options) {
> 
> I know this is old, but this is such a strange method signature, taking a
> mixture of JSON and Strings....

Yeah. It doesn't seem too terrible, but we could file a follow-up to clean things up if you want.
Depends on: 885867
https://hg.mozilla.org/mozilla-central/rev/4f98cdea7ea4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
You need to log in before you can comment on or make changes to this bug.