Closed Bug 938205 Opened 6 years ago Closed 6 years ago

Factor out editing UI into a separate View

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(2 files)

This is self-contained and should live in a separate view to unbloat BrowserToolbar a bit.
Attachment #831599 - Flags: review?(sriram)
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+
(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
Closed: 6 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.