936470 - Correctly track page action state to set title padding accordingly

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

See: https://bugzilla.mozilla.org/show_bug.cgi?id=935543#c0

Our title padding update code has been broken since we landed page action support in the toolbar.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Add listener API in PageActionLayout to track count changes (r=wesj)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Fix right padding in the Toolbar's title view (r=wesj)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Update padding when the page action count changes (r=wesj)

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Only show PageActionsLayout if it has page actions (r=wesj)

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 829275 [details] [diff] [review]
Fix right padding in the Toolbar's title view (r=wesj)

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

::: mobile/android/base/resources/layout/browser_toolbar.xml
@@ +153,5 @@
>                                                  android:layout_width="fill_parent"
>                                                  android:layout_height="fill_parent"
>                                                  android:layout_weight="1.0"
>                                                  android:singleLine="true"
> +                                                android:paddingRight="4dp"

There are two browser_toolbar.xml files. Do we need to update both? Also, to keep them in sync we should use a dimen.xml value.

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 829276 [details] [diff] [review]
Update padding when the page action count changes (r=wesj)

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

::: mobile/android/base/BrowserToolbar.java
@@ +313,5 @@
>          mPageActionLayout = (PageActionLayout) findViewById(R.id.page_action_layout);
> +        mPageActionLayout.setOnCountChangedListener(new PageActionLayout.OnCountChangedListener() {
> +            @Override
> +            public void onCountChanged() {
> +                setPageActionVisibility(mStop.getVisibility() == View.VISIBLE);

I am confused by this. You're waiting for the count to change, but then ignoring the count and just basing the visibility on whether or not stop is showing? If that's right, we should add a comment explaining.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 829274 [details] [diff] [review]
Add listener API in PageActionLayout to track count changes (r=wesj)

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

::: mobile/android/base/PageActionLayout.java
@@ +50,5 @@
>  
> +    private OnCountChangedListener mOnCountChangedListener;
> +
> +    public interface OnCountChangedListener {
> +        public void onCountChanged();

I think you should probably pass the new count to this method (and maybe the PageActionLayout as well).

Comment 8

[:Margaret Leibovic]

We don't show the title in the urlbar anymore.