938205 - Factor out editing UI into a separate View

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

This is self-contained and should live in a separate view to unbloat BrowserToolbar a bit.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Remove unused imports from BrowserToolbar (r=sriram)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Factor out editing UI into ToolbarEditLayout (r=sriram)

Comment 3

[Lucas Rocha (:lucasr)]

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

More chopping coming soon.

Comment 4

[Sriram Ramasubramanian [: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;

Comment 5

[Sriram Ramasubramanian [:sriram]]

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.

Comment 6

[Lucas Rocha (:lucasr)]

(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 :-)

Comment 7

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/eb8ef0e80c9c
https://hg.mozilla.org/integration/fx-team/rev/ea5939461476

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out for suspicion of causing mass robocop failures.
https://hg.mozilla.org/integration/fx-team/rev/fa367f4785cb

https://tbpl.mozilla.org/php/getParsedLog.php?id=30965139&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=30963985&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=30964585&tree=Fx-Team

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d37c31a42f31
https://hg.mozilla.org/mozilla-central/rev/0b75c809f3f7