Closed Bug 886472 Opened 10 years ago Closed 10 years ago

The DoorHanger divider should default to hidden, not shown


(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Not set


(Not tracked)

Firefox 25


(Reporter: Margaret, Assigned: ckitching)



(1 file, 5 obsolete files)

We only show the divider on doorhangers if there are multiple ones inside a doorhanger popup, and we do this by manually calling showDivider/hideDivider in DoorHangerPopup:

However, now that DoorHanger views can be used outside of DoorHangerPopup, it makes more sense for it to default to hidden, so that we don't need to manually hide it after creating a DoorHanger.

Also, the showDividers() logic in DoorHangerPopup isn't quite correct, since we only want to make sure we hide the divider for the last *visible* DoorHanger, not just the last one. You can see this bug if you trigger a notification, open a different tab that triggers a different notification, then switch back to the first tab.
Assignee: nobody → ckitching
Comment on attachment 768027 [details] [diff] [review]
Patch causing DoorHanger dividers to default to hidden, the logic for showdividers() to hide divider of final hanger, and removing redundant call to hideDivider.

Review of attachment 768027 [details] [diff] [review]:

Looking good! I'm just giving an r- because I would like to see setting the visibility of the divider moved to the XML. When you have a new version of the patch, you can upload it and flag me for review again :)

::: mobile/android/base/
@@ +93,5 @@
>          LayoutInflater.from(context).inflate(R.layout.doorhanger, this);
>          mTextView = (TextView) findViewById(;
>          mChoicesLayout = (LinearLayout) findViewById(;
>          mDivider = findViewById(;
> +        mDivider.setVisibility(View.GONE);

You should do this with an attribute in doorhanger.xml. In general, we prefer to set things in the XML if possible because it's faster.

::: mobile/android/base/
@@ +294,5 @@
>              setFocusable(true);
>          }
>      }
>      private void showDividers() {

If you like, you could add some documentation above this method to say what it's doing. One sentence would suffice :)

@@ +296,5 @@
>      }
>      private void showDividers() {
>          int count = mContent.getChildCount();
> +        DoorHanger lastVisibleHanger = null;

Nit: Call this lastVisibleDoorHanger.

@@ +301,5 @@
>          for (int i = 0; i < count; i++) {
>              DoorHanger dh = (DoorHanger) mContent.getChildAt(i);
>              dh.showDivider();
> +            if(dh.getVisibility() == View.VISIBLE) {

Nit: Put a space between the if and the open paren (we generally try not to be super picky about style, but this would be consistent with the rest of the file).

@@ +308,2 @@
>          }
> +        if(lastVisibleHanger != null) {

Same here.
Attachment #768027 - Flags: review?(margaret.leibovic) → review-
Attached patch Revised patch. (obsolete) — Splinter Review
Attachment #768027 - Attachment is obsolete: true
Attachment #768396 - Flags: review?(margaret.leibovic)
Ah. It occurs to me that this is a patch for the patch. What is the proper procedure here? Should I have created a patch that is the composition of the two I have submitted, or is what I just did correct?
Attached patch Composed, corrected patch (obsolete) — Splinter Review
In the sort of likely event that this is really what was wanted - here you go.
Attachment #768405 - Flags: review?(margaret.leibovic)
Comment on attachment 768405 [details] [diff] [review]
Composed, corrected patch

Attachment #768405 - Flags: review?(margaret.leibovic) → review+
Attachment #768396 - Attachment is obsolete: true
Attachment #768396 - Flags: review?(margaret.leibovic)
You should upload a new patch with a final commit message (with bug number and r=margaret), and then add the "checkin-needed" keyword for someone to come along and land it (it makes their job easier if the final commit message is already in the patch :).
Actually correcting the commit message as opposed to breaking the patch...
Attachment #769834 - Attachment is obsolete: true
Attachment #769834 - Flags: checkin?
Attachment #769835 - Flags: review?(margaret.leibovic)
Attachment #769835 - Flags: checkin?
Oops, I think I wasn't totally clear in my directions... you need to set the "checkin-needed" keyword on the bug, that's the thing that people who land patches will be searching for. The checkin? flag on the patch isn't necessary, I think it's usually just used when people have multiple patches in a bug and they want to be clear about the landing state of all of them.
Keywords: checkin-needed
Attachment #769837 - Flags: checkin?
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.