Remove title resources when they are no longer used

Remove title resources when they are no longer used

Previously, layer titles found their way into the resource manager
as ANDROID_RESOURCE_TYPE_DYNAMIC_BITMAP and were left there forever.
Over time, these recources accumulated (easily reproducable by
entering and exiting the tab switcher) and is more than likely the
cause of memory related crashes when entering the tab switcher.
The title layer itself was removed from the LayerTitleCache but the
resources that constitute the title were never correctly managed.

This change adds a function to the ResourceManager that allows the
LayerTitleCache to remove associated resources. Specifically, only
dynamic bitmaps can be manually removed.

BUG=669348
Review-Url: https://codereview.chromium.org/2578233002
Cr-Commit-Position: refs/heads/master@{#441773}
Committed: https://chromium.googlesource.com/chromium/src/+/e69847f70e6461ae430b28f8c8912513890648c5

[mdjones, 2016-12-15 19:30:39]

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

[mdjones, 2016-12-15 19:30:39]

An argument can probably be made to have the eviction logic somewhere else, but
I think this makes sense. ptal

[aelias_OOO_until_Jul13, 2016-12-15 21:08:22]

lgtm

[mdjones, 2016-12-16 22:36:09]

As per discussion with dtrainor, I updated the implementation so that
LayerTitleCache is responsible for creation and deletion of the title resources.

[mdjones, 2016-12-16 22:37:26]

Description was changed from

==========
Remove title resources when they are no longer used

Previously, layer titles found their way into the resource manager
as ANDROID_RESOURCE_TYPE_DYNAMIC_BITMAP and were left there forever.
Over time, these recources accumulated (easily reproducable by
entering and exiting the tab switcher) and is more than likely the
cause of memory related crashes when entering the tab switcher.
The title layer itself was removed from the LayerTitleCache but the
resources that constitute the title were never correctly managed.

This change adds a function "RemoveResource" to the resource manager
allowing the DecorationTitle to clean up the resources it knows to
be associated with itself.

BUG=669348
==========

to

==========
Remove title resources when they are no longer used

Previously, layer titles found their way into the resource manager
as ANDROID_RESOURCE_TYPE_DYNAMIC_BITMAP and were left there forever.
Over time, these recources accumulated (easily reproducable by
entering and exiting the tab switcher) and is more than likely the
cause of memory related crashes when entering the tab switcher.
The title layer itself was removed from the LayerTitleCache but the
resources that constitute the title were never correctly managed.

This change adds a function to the ResourceManager that allows the
LayerTitleCache to remove associated resources. Specifically, only
dynamic bitmaps can be manually removed.

BUG=669348
==========

[mdjones, 2016-12-16 22:38:36]

https://codereview.chromium.org/2578233002/diff/40001/ui/android/java/src/org...
File
ui/android/java/src/org/chromium/ui/resources/dynamics/DynamicResourceLoader.java
(right):

https://codereview.chromium.org/2578233002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/dynamics/DynamicResourceLoader.java:47:
mCallback.onResourceUnregistered(AndroidResourceType.DYNAMIC_BITMAP, resId);
Should I instead have this in ResourceManager?

[David Trainor- moved to gerrit, 2017-01-03 19:26:21]

https://codereview.chromium.org/2578233002/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/resources/ResourceLoader.java (right):

https://codereview.chromium.org/2578233002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/ResourceLoader.java:27: * Called
when a resource is unregistered (unneeded).
Since this is so specific to dynamic bitmaps, can we add some information to the
comment here?  Maybe make the native API also support all types but just limit
it to dynamic at the Java layer like you're already doing.  That way we don't
bleed the fact that it's restricted throughout the whole API chain.

https://codereview.chromium.org/2578233002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/ResourceLoader.java:35: protected
final ResourceLoaderCallback mCallback;
Leave this private and add a notifyResourceUnloaded?

https://codereview.chromium.org/2578233002/diff/40001/ui/android/java/src/org...
File
ui/android/java/src/org/chromium/ui/resources/dynamics/DynamicResourceLoader.java
(right):

https://codereview.chromium.org/2578233002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/dynamics/DynamicResourceLoader.java:47:
mCallback.onResourceUnregistered(AndroidResourceType.DYNAMIC_BITMAP, resId);
On 2016/12/16 22:38:36, mdjones wrote:
> Should I instead have this in ResourceManager?

What do you mean?

[mdjones, 2017-01-04 17:40:10]

https://codereview.chromium.org/2578233002/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/resources/ResourceLoader.java (right):

https://codereview.chromium.org/2578233002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/ResourceLoader.java:27: * Called
when a resource is unregistered (unneeded).
On 2017/01/03 19:26:20, David Trainor wrote:
> Since this is so specific to dynamic bitmaps, can we add some information to
the
> comment here?  Maybe make the native API also support all types but just limit
> it to dynamic at the Java layer like you're already doing.  That way we don't
> bleed the fact that it's restricted throughout the whole API chain.

Done.

https://codereview.chromium.org/2578233002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/ResourceLoader.java:35: protected
final ResourceLoaderCallback mCallback;
On 2017/01/03 19:26:20, David Trainor wrote:
> Leave this private and add a notifyResourceUnloaded?

Done.

https://codereview.chromium.org/2578233002/diff/40001/ui/android/java/src/org...
File
ui/android/java/src/org/chromium/ui/resources/dynamics/DynamicResourceLoader.java
(right):

https://codereview.chromium.org/2578233002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/dynamics/DynamicResourceLoader.java:47:
mCallback.onResourceUnregistered(AndroidResourceType.DYNAMIC_BITMAP, resId);
On 2017/01/03 19:26:20, David Trainor wrote:
> On 2016/12/16 22:38:36, mdjones wrote:
> > Should I instead have this in ResourceManager?
> 
> What do you mean?

I was more or less asking in a different way what you suggested above (adding
the helper to ResourceLoader), except I got ResourceLoader and Manager confused.

[David Trainor- moved to gerrit, 2017-01-05 21:25:31]

lgtm

https://codereview.chromium.org/2578233002/diff/40001/ui/android/java/src/org...
File
ui/android/java/src/org/chromium/ui/resources/dynamics/DynamicResourceLoader.java
(right):

https://codereview.chromium.org/2578233002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/resources/dynamics/DynamicResourceLoader.java:47:
mCallback.onResourceUnregistered(AndroidResourceType.DYNAMIC_BITMAP, resId);
On 2017/01/04 17:40:10, mdjones wrote:
> On 2017/01/03 19:26:20, David Trainor wrote:
> > On 2016/12/16 22:38:36, mdjones wrote:
> > > Should I instead have this in ResourceManager?
> > 
> > What do you mean?
> 
> I was more or less asking in a different way what you suggested above (adding
> the helper to ResourceLoader), except I got ResourceLoader and Manager
confused.

Acknowledged.

[mdjones, 2017-01-05 21:42:23]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2017-01-05 21:42:23]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2578233002/#ps60001
(title: "address comments")

[commit-bot: I haz the power, 2017-01-05 21:43:01]

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, 2017-01-05 22:21:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483652543531140,
"parent_rev": "7bce08ea4e4dc637356476938df381f60987e930", "commit_rev":
"e69847f70e6461ae430b28f8c8912513890648c5"}

[commit-bot: I haz the power, 2017-01-05 22:22:28]

Description was changed from

==========
Remove title resources when they are no longer used

Previously, layer titles found their way into the resource manager
as ANDROID_RESOURCE_TYPE_DYNAMIC_BITMAP and were left there forever.
Over time, these recources accumulated (easily reproducable by
entering and exiting the tab switcher) and is more than likely the
cause of memory related crashes when entering the tab switcher.
The title layer itself was removed from the LayerTitleCache but the
resources that constitute the title were never correctly managed.

This change adds a function to the ResourceManager that allows the
LayerTitleCache to remove associated resources. Specifically, only
dynamic bitmaps can be manually removed.

BUG=669348
==========

to

==========
Remove title resources when they are no longer used

Previously, layer titles found their way into the resource manager
as ANDROID_RESOURCE_TYPE_DYNAMIC_BITMAP and were left there forever.
Over time, these recources accumulated (easily reproducable by
entering and exiting the tab switcher) and is more than likely the
cause of memory related crashes when entering the tab switcher.
The title layer itself was removed from the LayerTitleCache but the
resources that constitute the title were never correctly managed.

This change adds a function to the ResourceManager that allows the
LayerTitleCache to remove associated resources. Specifically, only
dynamic bitmaps can be manually removed.

BUG=669348

Review-Url: https://codereview.chromium.org/2578233002
Cr-Commit-Position: refs/heads/master@{#441773}
Committed:
https://chromium.googlesource.com/chromium/src/+/e69847f70e6461ae430b28f8c891...
==========

[commit-bot: I haz the power, 2017-01-05 22:22:29]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e69847f70e6461ae430b28f8c891...