Add tinted static UI resource cache

Add tinted static UI resource cache

This change adds a method to the ResourceManager that allows users
to create a tinted version of a static resource. These resources are
cached until the layout changes. In practice, this new functionality
is used to tint the border resource and close button in the tab
switcher.

BUG=641517
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/4adb368fd86135a5f3f888895640b776395356b8
Cr-Commit-Position: refs/heads/master@{#417482}

[mdjones, 2016-08-29 17:59:05]

Description was changed from

==========
Add tinted static UI resource cache

This change adds a method to the ResourceManager that allows users
to create a tinted version of a static resource. These resources are
cached until the layout changes. In practice, this new functionality
is used to tint the border resource and close button in the tab
switcher.

BUG=641517
==========

to

==========
Add tinted static UI resource cache

This change adds a method to the ResourceManager that allows users
to create a tinted version of a static resource. These resources are
cached until the layout changes. In practice, this new functionality
is used to tint the border resource and close button in the tab
switcher.

BUG=641517
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[mdjones, 2016-08-29 18:14:23]

mdjones@chromium.org changed reviewers:
+ dtrainor@chromium.org

[mdjones, 2016-08-29 18:14:24]

ptal

[David Trainor- moved to gerrit, 2016-08-30 19:27:32]

https://codereview.chromium.org/2293573002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):

https://codereview.chromium.org/2293573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:1182:
mResourceManager = resourceManager;
Would it be cleaner to add a method to the host to get the resource manager?

https://codereview.chromium.org/2293573002/diff/20001/ui/android/resources/re...
File ui/android/resources/resource_manager_impl.cc (right):

https://codereview.chromium.org/2293573002/diff/20001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.cc:34: const int
max_tint_resource_count = 30;
needs to be of form kMaxTintResourceCount

https://codereview.chromium.org/2293573002/diff/20001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.cc:102: if (tinted_resource) return
tinted_resource;
new line here:

if (tinted_resource)
  return tinted_resource;

https://codereview.chromium.org/2293573002/diff/20001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.cc:106: if (!base_image) return
nullptr;
new line here too

https://codereview.chromium.org/2293573002/diff/20001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.cc:129:
base_image->ui_resource->GetBitmap(res_id, false).Draw(canvas,
Is res_id the right id?  It just happens to work because ScopedUiResource
doesn't really care about the id.  Maybe use base_image->ui_resource->id()? 
Just check what the right thing to pass in is.

https://codereview.chromium.org/2293573002/diff/20001/ui/android/resources/re...
File ui/android/resources/resource_manager_impl.h (right):

https://codereview.chromium.org/2293573002/diff/20001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.h:31: void
ClearTintedResourceCache(JNIEnv* env,
Move this out of the interface block of methods and into the // called from java
section

[mdjones, 2016-08-30 21:52:16]

https://codereview.chromium.org/2293573002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):

https://codereview.chromium.org/2293573002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:1182:
mResourceManager = resourceManager;
On 2016/08/30 19:27:32, David Trainor wrote:
> Would it be cleaner to add a method to the host to get the resource manager?

Turns out mRenderHost has one.

https://codereview.chromium.org/2293573002/diff/20001/ui/android/resources/re...
File ui/android/resources/resource_manager_impl.cc (right):

https://codereview.chromium.org/2293573002/diff/20001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.cc:34: const int
max_tint_resource_count = 30;
On 2016/08/30 19:27:32, David Trainor wrote:
> needs to be of form kMaxTintResourceCount

Done.

https://codereview.chromium.org/2293573002/diff/20001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.cc:102: if (tinted_resource) return
tinted_resource;
On 2016/08/30 19:27:32, David Trainor wrote:
> new line here:
> 
> if (tinted_resource)
>   return tinted_resource;

Done.

https://codereview.chromium.org/2293573002/diff/20001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.cc:106: if (!base_image) return
nullptr;
On 2016/08/30 19:27:32, David Trainor wrote:
> new line here too

Done.

https://codereview.chromium.org/2293573002/diff/20001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.cc:129:
base_image->ui_resource->GetBitmap(res_id, false).Draw(canvas,
On 2016/08/30 19:27:32, David Trainor wrote:
> Is res_id the right id?  It just happens to work because ScopedUiResource
> doesn't really care about the id.  Maybe use base_image->ui_resource->id()? 
> Just check what the right thing to pass in is.

Ah yes, I read through that code and originally put 0. Unfortunately I replaced
it with the wrong one.

https://codereview.chromium.org/2293573002/diff/20001/ui/android/resources/re...
File ui/android/resources/resource_manager_impl.h (right):

https://codereview.chromium.org/2293573002/diff/20001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.h:31: void
ClearTintedResourceCache(JNIEnv* env,
On 2016/08/30 19:27:32, David Trainor wrote:
> Move this out of the interface block of methods and into the // called from
java
> section

Done.

[David Trainor- moved to gerrit, 2016-08-31 05:23:09]

lgtm!

[mdjones, 2016-08-31 15:51:39]

mdjones@chromium.org changed reviewers:
+ vmpstr@chromium.org

[mdjones, 2016-08-31 15:51:40]

+vmpstr@ for cc/resources. Let me know if you're not the correct reviewer for
this. Thanks!

[aelias_OOO_until_Jul13, 2016-08-31 17:12:16]

aelias@chromium.org changed reviewers:
+ aelias@chromium.org
- vmpstr@chromium.org

[aelias_OOO_until_Jul13, 2016-08-31 17:12:26]

https://codereview.chromium.org/2293573002/diff/40001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/tab_layer.cc (left):

https://codereview.chromium.org/2293573002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/tab_layer.cc:641:
filters->Append(cc::FilterOperation::CreateColorMatrixFilter(colorMatrix));
I happened to notice this on my queue, I can review CC changes.

I think this patch is taking the wrong approach because it's doing a
memory/performance tradeoff and also adding memory management complexity,
whereas I see no reason why the tint can't be made cheaper without any memory
cost at all.  The full-power ColorMatrix FilterOperation seems slow, but as this
is a very simple type of filter, this perf problem should be solvable with a
more targeted filter.

I think SkFilter::MakeModeFilter(color, SkXfermode::kModulate_Mode) should be
equivalent to the matrix above.  I try changing CC to check the matrix for this
near-identity pattern in RenderSurfaceFilters::BuildImageFilter and special-case
it to create such an XferMode filter.  If that doesn't seem to improve the
performance, me and ajuma@ can give look more closely and give more advice.

https://codereview.chromium.org/2293573002/diff/40001/ui/android/resources/re...
File ui/android/resources/resource_manager_impl.cc (right):

https://codereview.chromium.org/2293573002/diff/40001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.cc:32: // TODO(mdjones): Implement
real caching logic; something similar to priority
If this is worth doing, we should do it before landing instead of leaving it as
indefinite TODO.  But I'd rather we looked for non-memory-using solutions first.

[mdjones, 2016-08-31 17:41:00]

https://codereview.chromium.org/2293573002/diff/40001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/tab_layer.cc (left):

https://codereview.chromium.org/2293573002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/tab_layer.cc:641:
filters->Append(cc::FilterOperation::CreateColorMatrixFilter(colorMatrix));
On 2016/08/31 17:12:26, aelias wrote:
> I happened to notice this on my queue, I can review CC changes.
> 
> I think this patch is taking the wrong approach because it's doing a
> memory/performance tradeoff and also adding memory management complexity,
> whereas I see no reason why the tint can't be made cheaper without any memory
> cost at all.  The full-power ColorMatrix FilterOperation seems slow, but as
this
> is a very simple type of filter, this perf problem should be solvable with a
> more targeted filter.
> 
> I think SkFilter::MakeModeFilter(color, SkXfermode::kModulate_Mode) should be
> equivalent to the matrix above.  I try changing CC to check the matrix for
this
> near-identity pattern in RenderSurfaceFilters::BuildImageFilter and
special-case
> it to create such an XferMode filter.  If that doesn't seem to improve the
> performance, me and ajuma@ can give look more closely and give more advice.

I chatted with enne and ajuma before implementing this patch. Being the simplest
filter, the color filter already takes a handful shortcuts through the code.
From my understanding, the major expense is creating render surfaces for filter
to be drawn to.

https://codereview.chromium.org/2293573002/diff/40001/ui/android/resources/re...
File ui/android/resources/resource_manager_impl.cc (right):

https://codereview.chromium.org/2293573002/diff/40001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.cc:32: // TODO(mdjones): Implement
real caching logic; something similar to priority
On 2016/08/31 17:12:26, aelias wrote:
> If this is worth doing, we should do it before landing instead of leaving it
as
> indefinite TODO.  But I'd rather we looked for non-memory-using solutions
first.

I'll look at this if we determine this is an appropriate solution.

[mdjones, 2016-08-31 17:49:12]

https://codereview.chromium.org/2293573002/diff/40001/ui/android/resources/re...
File ui/android/resources/resource_manager_impl.cc (right):

https://codereview.chromium.org/2293573002/diff/40001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.cc:34: const int
kMaxTintResourceCount = 30;
I can also reduce the memory footprint to max of 2mb by reducing the cache size
to 8 resources (at xxxhdpi).

[aelias_OOO_until_Jul13, 2016-08-31 18:01:32]

https://codereview.chromium.org/2293573002/diff/40001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/tab_layer.cc (left):

https://codereview.chromium.org/2293573002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/tab_layer.cc:641:
filters->Append(cc::FilterOperation::CreateColorMatrixFilter(colorMatrix));
On 2016/08/31 at 17:41:00, mdjones wrote:
> I chatted with enne and ajuma before implementing this patch. Being the
simplest filter, the color filter already takes a handful shortcuts through the
code. From my understanding, the major expense is creating render surfaces for
filter to be drawn to.

OK, did you take a trace to prove that?  If so could you post it on the bug? 
Also, do you have an estimate for how much memory this tint cache winds up
taking in practice?  (I realize the close button and border seem pretty small
but this stuff sometimes adds up especially when you generate 30 copies.)

Secondly the buganizer says "But why is the frame rate of the animation
dependent on number of tabs open?"  Do you understand this part of the problem? 
My intuition is that this tint problem is a fixed cost that still leaves us
within 60fps, but there's a per-invisible-tab-cost (JNI? something else?) that
puts us over the top.  And because there's no limit to the number of invisible
tabs, we need to address that to really get reliable no-jank.  I've noticed
myself when I have ~200 tabs, tab switcher framerate is very low, and I was
meaning to look into it.

I saw the bug is marked P1 and I gather that's because of http://b/30862870 . 
So I appreciate you need to get this finished soon and am willing to help you
out.  Even still I'd like us to be careful before adding yet more memory
management to the tab switcher.  It's constantly a source of memory spikes and
bugginess and it's one of the things sievers@ and I would like to simplify
rather than make more complex.

[vmpstr, 2016-08-31 18:01:54]

vmpstr@chromium.org changed reviewers:
+ vmpstr@chromium.org

[vmpstr, 2016-08-31 18:01:56]

cc lgtm (didn't take a look at the rest)

https://codereview.chromium.org/2293573002/diff/40001/cc/resources/ui_resourc...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/2293573002/diff/40001/cc/resources/ui_resourc...
cc/resources/ui_resource_bitmap.h:41: void Draw(SkCanvas& canvas, SkPaint&
paint);
Can you add a comment explaining what this function does?

Also, as a nit, can you rename this to "DrawToCanvas"

[mdjones, 2016-08-31 21:17:01]

https://codereview.chromium.org/2293573002/diff/40001/cc/resources/ui_resourc...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/2293573002/diff/40001/cc/resources/ui_resourc...
cc/resources/ui_resource_bitmap.h:41: void Draw(SkCanvas& canvas, SkPaint&
paint);
On 2016/08/31 18:01:55, vmpstr wrote:
> Can you add a comment explaining what this function does?
> 
> Also, as a nit, can you rename this to "DrawToCanvas"

Done.

[aelias_OOO_until_Jul13, 2016-08-31 23:37:45]

https://codereview.chromium.org/2293573002/diff/60001/ui/android/resources/re...
File ui/android/resources/resource_manager_impl.cc (right):

https://codereview.chromium.org/2293573002/diff/60001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.cc:34: const int
kMaxTintResourceCount = 30;
We chatted offline and agreed to land this more or less as is because it's
desired for 53.  I'd just ask for now that this value is changed to 4 (number of
assets needed for normal and incognito colors).  With that numeric change, I'll
give this lgt'm.

Then for 54 and up I'd like to keep discussing how we can do something better
for theme colors (particularly for theme colors that smoothly animate across the
spectrum which raises the risk of bloating up this cache enormously).

[aelias_OOO_until_Jul13, 2016-09-01 01:45:05]

lgtm

[mdjones, 2016-09-01 17:49:46]

This patch needs a bit more work. I found a bug related to the cache being full
and the color of the text and close button being used.

[mdjones, 2016-09-02 20:48:54]

Updated patch to kill the text color bug. ptal, thanks!

[aelias_OOO_until_Jul13, 2016-09-03 08:01:46]

https://codereview.chromium.org/2293573002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/LayerTitleCache.java
(right):

https://codereview.chromium.org/2293573002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/LayerTitleCache.java:104:
getUpdatedTitleWithThemeColor(tab, "", themeColor);
It's weird to override the tab's idea of its own themeColor with this one. 
What's preventing you from changing the Tab object's theme color instead?  Would
it be possible to instead reorder the sequence of events so that the tab's
themeColor is in the right state at the right time?

[mdjones, 2016-09-06 16:21:25]

https://codereview.chromium.org/2293573002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/LayerTitleCache.java
(right):

https://codereview.chromium.org/2293573002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/LayerTitleCache.java:104:
getUpdatedTitleWithThemeColor(tab, "", themeColor);
On 2016/09/03 08:01:46, aelias wrote:
> It's weird to override the tab's idea of its own themeColor with this one. 
> What's preventing you from changing the Tab object's theme color instead? 
Would
> it be possible to instead reorder the sequence of events so that the tab's
> themeColor is in the right state at the right time?

The tab still needs to know its color for the toolbar layer when not in the tab
switcher so I don't think we want to change that value. The problem here is that
the caching logic for the assets is far removed from the rest of the theme color
logic. At the original time of creation the title cache doesn't know if the
asset cache will be full. I believe reordering these events will present the
same problem.

[mdjones, 2016-09-06 16:53:05]

On 2016/09/06 16:21:25, mdjones wrote:
>
https://codereview.chromium.org/2293573002/diff/120001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/LayerTitleCache.java
> (right):
> 
>
https://codereview.chromium.org/2293573002/diff/120001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/LayerTitleCache.java:104:
> getUpdatedTitleWithThemeColor(tab, "", themeColor);
> On 2016/09/03 08:01:46, aelias wrote:
> > It's weird to override the tab's idea of its own themeColor with this one. 
> > What's preventing you from changing the Tab object's theme color instead? 
> Would
> > it be possible to instead reorder the sequence of events so that the tab's
> > themeColor is in the right state at the right time?
> 
> The tab still needs to know its color for the toolbar layer when not in the
tab
> switcher so I don't think we want to change that value. The problem here is
that
> the caching logic for the assets is far removed from the rest of the theme
color
> logic. At the original time of creation the title cache doesn't know if the
> asset cache will be full. I believe reordering these events will present the
> same problem.

Also, exclusively using native to generate titles will prevent the unnecessary
creation of resources that are never seen. Ordinarily I would separate out that
change from the asset caching, but issues with this patch require the change.

[mdjones, 2016-09-07 15:34:49]

The resource cache is now based on the number of tabs that are showing. When a
tab is not showing, its resources are removed. This is done on a frame-by-frame
basis. PTAL

[aelias_OOO_until_Jul13, 2016-09-07 19:34:44]

Just for clarity, "not lgtm" yet, although generally-speaking caching approach
looks great and I just have a bunch of smaller comments.

https://codereview.chromium.org/2293573002/diff/140001/chrome/browser/android...
File chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc
(right):

https://codereview.chromium.org/2293573002/diff/140001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc:58: if
(resource_manager_) resource_manager_->RemoveUnusedTints(used_tints_);
Is there a scenario where "if (resource_manager_)" would do anything?  Can we
DCHECK instead?

https://codereview.chromium.org/2293573002/diff/140001/chrome/browser/android...
File chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.h
(right):

https://codereview.chromium.org/2293573002/diff/140001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.h:116:
std::unordered_set<int> used_tints_;
Please add "include <unordered_set>" above, also may as well replace "#include
<vector>" with "#include <map>" and "#include <set>" (actually, my own oversight
in http://crrev.com/2253423003 :).

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
File ui/android/resources/resource_manager.h (right):

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
ui/android/resources/resource_manager.h:9: #include <unordered_set>
Also <unordered_map>

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
File ui/android/resources/resource_manager_impl.cc (right):

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
ui/android/resources/resource_manager_impl.cc:85:
tinted_resources_.erase(a.first);
I think the for loop implicit iterator would still be pointing at erased item so
I'm not certain it's correct?  The usual pattern for this is to use the return
value of erase() to advance, see the similar thing in
TabListSceneLayer::FinishBuildingFrame.

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
ui/android/resources/resource_manager_impl.cc:93: auto item =
tinted_resources_.find(tint_color);
I think the iterator name wouldn't be ridiculously long in this case so I'd
prefer avoiding "auto" for clarity.

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
ui/android/resources/resource_manager_impl.cc:110: if (!base_image)
What is this scenario, exactly?  Can we DCHECK?

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
ui/android/resources/resource_manager_impl.cc:113: SkBitmap tinted_bitmap;
Could you add a TRACE_EVENT0(browser,
"ResourceManagerImpl::GetStaticResourceWithTint") above this line?  The tint
texture creation has a risk of introducing small janks, this will make it more
easily visible in traces.

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
ui/android/resources/resource_manager_impl.cc:131:
SkColorFilter::MakeMatrixFilterRowMajor255(color_matrix));
Could you switch to SkFilter::MakeModeFilter(color, SkXfermode::kModulate_Mode)
like I suggested earlier?  It should be faster.

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
File ui/android/resources/resource_manager_impl.h (right):

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
ui/android/resources/resource_manager_impl.h:90: typedef std::unordered_map<int,
ResourceMap*> TintedResourceMap;
We should never be holding lifetime-managing raw ptrs in STL containers.  Today,
compiler support has improved enough that we can put scoped_ptr<ResourceMap>
inside STL containers, so I'd encourage that.

It looks like IDMap<T, IDMapOwnPointer> is a 2008-era base/ class with
effectively the same behavior as std::unordered_map<int, scoped_ptr<T>> that
encourages the use of raw pointers and has weirdo non-STL-style methods.  Looks
to me like IDMap should be deprecated.  Could you drive-by change the two other
uses of IDMap in this class to std::unordered_map<int, scoped_ptr<T>>?

[mdjones, 2016-09-08 16:34:44]

https://codereview.chromium.org/2293573002/diff/140001/chrome/browser/android...
File chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc
(right):

https://codereview.chromium.org/2293573002/diff/140001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc:58: if
(resource_manager_) resource_manager_->RemoveUnusedTints(used_tints_);
On 2016/09/07 19:34:44, aelias wrote:
> Is there a scenario where "if (resource_manager_)" would do anything?  Can we
> DCHECK instead?

Done.

https://codereview.chromium.org/2293573002/diff/140001/chrome/browser/android...
File chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.h
(right):

https://codereview.chromium.org/2293573002/diff/140001/chrome/browser/android...
chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.h:116:
std::unordered_set<int> used_tints_;
On 2016/09/07 19:34:44, aelias wrote:
> Please add "include <unordered_set>" above, also may as well replace "#include
> <vector>" with "#include <map>" and "#include <set>" (actually, my own
oversight
> in http://crrev.com/2253423003 :).

Done.

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
File ui/android/resources/resource_manager.h (right):

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
ui/android/resources/resource_manager.h:9: #include <unordered_set>
On 2016/09/07 19:34:44, aelias wrote:
> Also <unordered_map>

Done.

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
File ui/android/resources/resource_manager_impl.cc (right):

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
ui/android/resources/resource_manager_impl.cc:85:
tinted_resources_.erase(a.first);
On 2016/09/07 19:34:44, aelias wrote:
> I think the for loop implicit iterator would still be pointing at erased item
so
> I'm not certain it's correct?  The usual pattern for this is to use the return
> value of erase() to advance, see the similar thing in
> TabListSceneLayer::FinishBuildingFrame.

Done.

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
ui/android/resources/resource_manager_impl.cc:93: auto item =
tinted_resources_.find(tint_color);
On 2016/09/07 19:34:44, aelias wrote:
> I think the iterator name wouldn't be ridiculously long in this case so I'd
> prefer avoiding "auto" for clarity.

Done.

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
ui/android/resources/resource_manager_impl.cc:110: if (!base_image)
On 2016/09/07 19:34:44, aelias wrote:
> What is this scenario, exactly?  Can we DCHECK?

Resource might not be ready. DCHECK should be fine.

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
ui/android/resources/resource_manager_impl.cc:113: SkBitmap tinted_bitmap;
On 2016/09/07 19:34:44, aelias wrote:
> Could you add a TRACE_EVENT0(browser,
> "ResourceManagerImpl::GetStaticResourceWithTint") above this line?  The tint
> texture creation has a risk of introducing small janks, this will make it more
> easily visible in traces.

Done.

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
ui/android/resources/resource_manager_impl.cc:131:
SkColorFilter::MakeMatrixFilterRowMajor255(color_matrix));
On 2016/09/07 19:34:44, aelias wrote:
> Could you switch to SkFilter::MakeModeFilter(color,
SkXfermode::kModulate_Mode)
> like I suggested earlier?  It should be faster.

Done.

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
File ui/android/resources/resource_manager_impl.h (right):

https://codereview.chromium.org/2293573002/diff/140001/ui/android/resources/r...
ui/android/resources/resource_manager_impl.h:90: typedef std::unordered_map<int,
ResourceMap*> TintedResourceMap;
On 2016/09/07 19:34:44, aelias wrote:
> We should never be holding lifetime-managing raw ptrs in STL containers. 
Today,
> compiler support has improved enough that we can put scoped_ptr<ResourceMap>
> inside STL containers, so I'd encourage that.
> 
> It looks like IDMap<T, IDMapOwnPointer> is a 2008-era base/ class with
> effectively the same behavior as std::unordered_map<int, scoped_ptr<T>> that
> encourages the use of raw pointers and has weirdo non-STL-style methods. 
Looks
> to me like IDMap should be deprecated.  Could you drive-by change the two
other
> uses of IDMap in this class to std::unordered_map<int, scoped_ptr<T>>?

I updated the TintedResourceMap to hold a unique_ptr but I'd really like to
remove IDMap in a separate CL (later today). If you think it really needs to be
in this change I can add it.

[aelias_OOO_until_Jul13, 2016-09-08 22:04:41]

lgtm

https://codereview.chromium.org/2293573002/diff/160001/ui/android/resources/r...
File ui/android/resources/resource_manager_impl.cc (right):

https://codereview.chromium.org/2293573002/diff/160001/ui/android/resources/r...
ui/android/resources/resource_manager_impl.cc:182: 
nit: unnecessary newline

[mdjones, 2016-09-09 01:19:15]

https://codereview.chromium.org/2293573002/diff/160001/ui/android/resources/r...
File ui/android/resources/resource_manager_impl.cc (right):

https://codereview.chromium.org/2293573002/diff/160001/ui/android/resources/r...
ui/android/resources/resource_manager_impl.cc:182: 
On 2016/09/08 22:04:41, aelias wrote:
> nit: unnecessary newline

Done.

[mdjones, 2016-09-09 01:19:22]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-09-09 01:19:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, vmpstr@chromium.org, aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2293573002/#ps180001
(title: "nit")

[commit-bot: I haz the power, 2016-09-09 01:19:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 02:36:18]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-09 02:37:49]

Description was changed from

==========
Add tinted static UI resource cache

This change adds a method to the ResourceManager that allows users
to create a tinted version of a static resource. These resources are
cached until the layout changes. In practice, this new functionality
is used to tint the border resource and close button in the tab
switcher.

BUG=641517
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Add tinted static UI resource cache

This change adds a method to the ResourceManager that allows users
to create a tinted version of a static resource. These resources are
cached until the layout changes. In practice, this new functionality
is used to tint the border resource and close button in the tab
switcher.

BUG=641517
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/4adb368fd86135a5f3f888895640b776395356b8
Cr-Commit-Position: refs/heads/master@{#417482}
==========

[commit-bot: I haz the power, 2016-09-09 02:37:51]

Patchset 10 (id:??) landed as
https://crrev.com/4adb368fd86135a5f3f888895640b776395356b8
Cr-Commit-Position: refs/heads/master@{#417482}

[mdjones, 2016-10-12 01:14:57]

Patchset #11 (id:200001) has been deleted