It creates new shader instance on every onDraw() call. This means that simply scrolling any home panel will generate tons of new LinearGradient instances.

Comment on attachment 8477475 [details] [diff] [review] Only re-create LinearGradient when necessary (r=bnicholson) I had a nice patch series with the incremental steps but they add up to a full rewrite anyway.

Comment on attachment 8477475 [details] [diff] [review] Only re-create LinearGradient when necessary (r=bnicholson) Review of attachment 8477475 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/FadedTextView.java @@ +6,1 @@ > package org.mozilla.gecko.home; Unrelated, but this seems like a pretty generic, reusable widget. We might want to move this out of home/ and into widget/. @@ +40,5 @@ > public FadedTextView(Context context, AttributeSet attrs, int defStyle) { > super(context, attrs, defStyle); > > + setSingleLine(true); > + setEllipsize(null); I assume you're setting this manually (vs relying on the style) because they need to be set for this view to work properly? Maybe we should drop these styles at [1] and [2] since they will be ignored. [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/styles.xml?rev=65eb82e40f0a#117 [2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/styles.xml?rev=65eb82e40f0a#192 @@ +104,5 @@ > + updateGradientShader(); > + } > + > + @Override > + public void setText(CharSequence text, BufferType type) { Rather than trying to pinpoint all of the places that could potentially affect the shader, it might be safer to just call updateGradientShader() in every onDraw() (or maybe onMeasure?) pass. I particularly dislike overriding this version of setText since it relies on the internal assumption that other setTexts will call it. Of course, it will be slightly more expensive to do these checks on every draw, but that could be worth the cleaner code/reduced fragility. FWIW, I ended up making a similar decision with FloatingHintEditText after trying to override a number of different methods like you do here. Maybe you're more confident than I am that these will cover all the cases :)

Comment on attachment 8477475 [details] [diff] [review] Only re-create LinearGradient when necessary (r=bnicholson) Review of attachment 8477475 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/FadedTextView.java @@ +47,5 @@ > mFadeWidth = a.getDimensionPixelSize(R.styleable.FadedTextView_fadeWidth, 0); > a.recycle(); > + } > + > + private int getWidthWithCompoundPadding() { Also, this should be named getWidthWith*out*CompoundPadding.

(In reply to Brian Nicholson (:bnicholson) from comment #4) > Comment on attachment 8477475 [details] [diff] [review] > Only re-create LinearGradient when necessary (r=bnicholson) > > Review of attachment 8477475 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/home/FadedTextView.java > @@ +47,5 @@ > > mFadeWidth = a.getDimensionPixelSize(R.styleable.FadedTextView_fadeWidth, 0); > > a.recycle(); > > + } > > + > > + private int getWidthWithCompoundPadding() { > > Also, this should be named getWidthWith*out*CompoundPadding. Well, this is a matter of perspective :-) I see "with padding" more as "give me the width accounting for any padding set in this view". I read "without padding" pretty much as equivalent to "give me the height of this view".

(In reply to Lucas Rocha (:lucasr) from comment #5) > Well, this is a matter of perspective :-) I see "with padding" more as "give > me the width accounting for any padding set in this view". Heh, that's an interesting interpretation, but I'm pretty sure most people would see "with" to mean "including". I guess the problem is that what you mean by "accounting for" isn't clear in the method name; saying "with" when you're subtracting it out is counter-intuitive. Maybe it would make sense to call this getWidthWithCompoundPaddingSubtracted?

(In reply to Brian Nicholson (:bnicholson) from comment #6) > (In reply to Lucas Rocha (:lucasr) from comment #5) > > Well, this is a matter of perspective :-) I see "with padding" more as "give > > me the width accounting for any padding set in this view". > > Heh, that's an interesting interpretation, but I'm pretty sure most people > would see "with" to mean "including". I guess the problem is that what you > mean by "accounting for" isn't clear in the method name; saying "with" when > you're subtracting it out is counter-intuitive. Maybe it would make sense to > call this getWidthWithCompoundPaddingSubtracted? Well, padding is by definition a subtraction in the available space i.e. 'adding' padding to a view means 'reduce the available space for content within the view'. I can't really see how 'give me width with padding' could be interpreted as 'give me the sum of width and padding'. But maybe that's just me :-) I'll go with getAvailableWidth() instead. Seems more intuitive. Deal?

(In reply to Brian Nicholson (:bnicholson) from comment #3) > Comment on attachment 8477475 [details] [diff] [review] > Only re-create LinearGradient when necessary (r=bnicholson) > > Review of attachment 8477475 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/home/FadedTextView.java > @@ +6,1 @@ > > package org.mozilla.gecko.home; > > Unrelated, but this seems like a pretty generic, reusable widget. We might > want to move this out of home/ and into widget/. File bug 1060394 to track this. > @@ +40,5 @@ > > public FadedTextView(Context context, AttributeSet attrs, int defStyle) { > > super(context, attrs, defStyle); > > > > + setSingleLine(true); > > + setEllipsize(null); > > I assume you're setting this manually (vs relying on the style) because they > need to be set for this view to work properly? Maybe we should drop these > styles at [1] and [2] since they will be ignored. > > [1] > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/ > values/styles.xml?rev=65eb82e40f0a#117 > [2] > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/ > values/styles.xml?rev=65eb82e40f0a#192 Good point. Still need these attributes in some Widget.TwoLinePageRow.Title sub-styles. Moved them accordingly. > @@ +104,5 @@ > > + updateGradientShader(); > > + } > > + > > + @Override > > + public void setText(CharSequence text, BufferType type) { > > Rather than trying to pinpoint all of the places that could potentially > affect the shader, it might be safer to just call updateGradientShader() in > every onDraw() (or maybe onMeasure?) pass. I particularly dislike overriding > this version of setText since it relies on the internal assumption that > other setTexts will call it. setText(CharSequence,BufferType) is the only overrideable version of setText(). All other versions are final. It's been like since Gingerbread at least. > Of course, it will be slightly more expensive to do these checks on every > draw, but that could be worth the cleaner code/reduced fragility. FWIW, I > ended up making a similar decision with FloatingHintEditText after trying to > override a number of different methods like you do here. Maybe you're more > confident than I am that these will cover all the cases :) Fair enough, there is some fragility involved here. I'm confident this patch will work fine but Android might later change in ways that breaks some of my assumptions here. So, let's go with the safer onDraw() then.

(In reply to Lucas Rocha (:lucasr) from comment #7) > Well, padding is by definition a subtraction in the available space i.e. > 'adding' padding to a view means 'reduce the available space for content > within the view'. I can't really see how 'give me width with padding' could > be interpreted as 'give me the sum of width and padding'. But maybe that's > just me :-) Well, I wouldn't say "give me width with padding" == "give me the sum of width and padding". I would say "give me width with padding" == "give me width". In other words, "width with padding" doesn't really make sense to me since padding is already part of the view's width. IMO, your argument might make more sense if we were talking about margins. > I'll go with getAvailableWidth() instead. Seems more intuitive. Deal? WFM -- I think that's an improvement since it doesn't mean the opposite of what I thought it meant. :) It's not super clear that it means "width excluding padding", but this method is internal to FadedTextView, so I'm not really too concerned about the naming anyway.

Comment on attachment 8477475 [details] [diff] [review] Only re-create LinearGradient when necessary (r=bnicholson) Approval Request Comment [Feature/regressing bug #]: bug 880393 (when FadedTextView landed) [User impact if declined]: a new shader instance is created and thrown away every time an list/grid item is drawn in about:home. [Describe test coverage new/current, TBPL]: I've just landed this in m-c. Let's bake it for a week or so and then uplift to beta and aurora. [Risks and why]: Fairly low. The view code is fairly simple and should be easy to track down any regressions. [String/UUID change made/needed]: n/a

This reads as a perf fix. I agree with uplifting this to aurora once it has been on m-c a little longer. If this is significant enough to uplift to beta (according to bug 880393 we shipped this issue in 26), then I'd like to see it land on beta before Mon, Sep 8, if possible, in order to make beta2.

Comment on attachment 8477475 [details] [diff] [review] Only re-create LinearGradient when necessary (r=bnicholson) Good find and fix. Aurora+ As for Beta, without knowing the user benefit of the fix, I think that it's hard to justify the risk. If this is demonstrated to be a significant fix on m-c or aurora, let's reevaluate taking this on beta.