Factor out editing UI into a separate View

RESOLVED FIXED in Firefox 28

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

unspecified
Firefox 28
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
This is self-contained and should live in a separate view to unbloat BrowserToolbar a bit.
(Assignee)

Comment 1

5 years ago
Created attachment 831599 [details] [diff] [review]
Remove unused imports from BrowserToolbar (r=sriram)
(Assignee)

Comment 2

5 years ago
Created attachment 831600 [details] [diff] [review]
Factor out editing UI into ToolbarEditLayout (r=sriram)
(Assignee)

Updated

5 years ago
Attachment #831599 - Flags: review?(sriram)
(Assignee)

Comment 3

5 years ago
Comment on attachment 831600 [details] [diff] [review]
Factor out editing UI into ToolbarEditLayout (r=sriram)

More chopping coming soon.
Attachment #831600 - Flags: review?(sriram)
Comment on attachment 831599 [details] [diff] [review]
Remove unused imports from BrowserToolbar (r=sriram)

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

import omg.that.is.better;
Attachment #831599 - Flags: review?(sriram) → review+
Comment on attachment 831600 [details] [diff] [review]
Factor out editing UI into ToolbarEditLayout (r=sriram)

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

Looks good to me. Please change the mContext to getContext().

::: mobile/android/base/toolbar/ToolbarEditLayout.java
@@ +26,5 @@
> +import android.widget.ImageButton;
> +
> +public class ToolbarEditLayout extends GeckoLinearLayout {
> +
> +    private final Context mContext;

This is not needed. Use getContext() when needed.

@@ +105,5 @@
> +        final String contentDescription;
> +
> +        if (textType == TextType.SEARCH_QUERY) {
> +            imageResource = R.drawable.ic_url_bar_search;
> +            contentDescription = mContext.getString(R.string.search);

getContext() is sufficient.
Attachment #831600 - Flags: review?(sriram) → review+
(Assignee)

Comment 6

5 years ago
(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> Comment on attachment 831600 [details] [diff] [review]
> Factor out editing UI into ToolbarEditLayout (r=sriram)
> 
> Review of attachment 831600 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. Please change the mContext to getContext().
> 
> ::: mobile/android/base/toolbar/ToolbarEditLayout.java
> @@ +26,5 @@
> > +import android.widget.ImageButton;
> > +
> > +public class ToolbarEditLayout extends GeckoLinearLayout {
> > +
> > +    private final Context mContext;
> 
> This is not needed. Use getContext() when needed.
> 
> @@ +105,5 @@
> > +        final String contentDescription;
> > +
> > +        if (textType == TextType.SEARCH_QUERY) {
> > +            imageResource = R.drawable.ic_url_bar_search;
> > +            contentDescription = mContext.getString(R.string.search);
> 
> getContext() is sufficient.

Fair enough, done. I do wonder why you didn't catch that on the ToolbarEditText patch :-)
Turned out to be some sort of needs-clobber situation. Re-landed and confirmed green.

https://hg.mozilla.org/integration/fx-team/rev/d37c31a42f31
https://hg.mozilla.org/integration/fx-team/rev/0b75c809f3f7
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d37c31a42f31
https://hg.mozilla.org/mozilla-central/rev/0b75c809f3f7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.