[Android] Use UIResource for overscroll glow

[Android] Use UIResource for overscroll glow

Introduce a SystemUIResourceManager class for loading and providing access to
shared UIResourceId's.  Wire this class to the existing OverscrollGlow effect,
using the shared id's with a UIResourceLayer for both glow and edge layers.

This patch is a subset of powei@'s original SystemUIResource patch found at
https://codereview.chromium.org/377013002/.

BUG=326326
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288095

[jdduke (slow), 2014-07-31 16:27:44]

aelias@, powei@: I managed to re-use much of the original patch, replacing the
subscription model with a simply UIResourceId query and push model. PTAL.

[powei, 2014-07-31 17:24:28]

lgtm with one question.  Thanks for picking this up!

https://codereview.chromium.org/422013003/diff/1/ui/base/android/system_ui_re...
File ui/base/android/system_ui_resource_manager.h (right):

https://codereview.chromium.org/422013003/diff/1/ui/base/android/system_ui_re...
ui/base/android/system_ui_resource_manager.h:25: virtual void
EnsureResource(ResourceType resource) = 0;
So it looks like overscroll is not using this right now.  I'm guessing you'll
use this for the other resources?  Is it to avoid janking on the first frame?

[jdduke (slow), 2014-07-31 17:40:57]

https://codereview.chromium.org/422013003/diff/1/ui/base/android/system_ui_re...
File ui/base/android/system_ui_resource_manager.h (right):

https://codereview.chromium.org/422013003/diff/1/ui/base/android/system_ui_re...
ui/base/android/system_ui_resource_manager.h:25: virtual void
EnsureResource(ResourceType resource) = 0;
On 2014/07/31 17:24:28, powei wrote:
> So it looks like overscroll is not using this right now.  I'm guessing you'll
> use this for the other resources?  Is it to avoid janking on the first frame?

Yeah, it's unused, and I thought of just ripping it out.  The first time we
overscroll we'll just got a resource id of 0, so the effect itself will appear
to jank until we start getting non-zero id's, but we won't block the main
thread.  I tried it a few times on my Galaxy Nexus and didn't really notice any
jank (it seemed to behave better than the old approach of blocking resource
loading on the main thread).

Maybe I'll add calls to this when the OverscrollGlow is created, and probably
rename it to |PreloadResource|, what do you think?

[jdduke (slow), 2014-07-31 18:06:29]

On 2014/07/31 17:40:57, jdduke wrote:
> Maybe I'll add calls to this when the OverscrollGlow is created, and probably
> rename it to |PreloadResource|, what do you think?

I went ahead and did that, works pretty well.

[powei, 2014-07-31 19:04:18]

On 2014/07/31 18:06:29, jdduke wrote:
> On 2014/07/31 17:40:57, jdduke wrote:
> > Maybe I'll add calls to this when the OverscrollGlow is created, and
probably
> > rename it to |PreloadResource|, what do you think?
> 
> I went ahead and did that, works pretty well.

Yeah, looks good.

[jdduke (slow), 2014-08-06 23:06:18]

aelias@: Anything else you'd like to see from this patch?

[aelias_OOO_until_Jul13, 2014-08-06 23:18:55]

lgtm modulo minor comment

https://codereview.chromium.org/422013003/diff/60001/content/browser/android/...
File content/browser/android/edge_effect.cc (right):

https://codereview.chromium.org/422013003/diff/60001/content/browser/android/...
content/browser/android/edge_effect.cc:144:
ui_resource_layer_->SetTransform(gfx::Transform());
Are these two lines needed?  Could you remove them and just call
SetIsDrawable(false)?

[jdduke (slow), 2014-08-06 23:30:18]

yfriedman@: OWNER review for ui/base/android? Thanks.

https://codereview.chromium.org/422013003/diff/60001/content/browser/android/...
File content/browser/android/edge_effect.cc (right):

https://codereview.chromium.org/422013003/diff/60001/content/browser/android/...
content/browser/android/edge_effect.cc:144:
ui_resource_layer_->SetTransform(gfx::Transform());
On 2014/08/06 23:18:55, aelias wrote:
> Are these two lines needed?  Could you remove them and just call
> SetIsDrawable(false)?

I don't think they're needed, will remove.

[Yaron, 2014-08-06 23:39:12]

lgtm

[jdduke (slow), 2014-08-06 23:45:40]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-08-06 23:48:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/422013003/80001

[commit-bot: I haz the power, 2014-08-07 05:00:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-07 05:03:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-07 05:03:51]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jdduke (slow), 2014-08-07 13:44:43]

sky@: Owner review for ui/base/BUILD.gn and ui/base/ui_base.gyp? I must have
been confused by the per-file exceptions in ui/OWNERS...

[sky, 2014-08-07 16:22:49]

Said files LGTM

[jdduke (slow), 2014-08-07 17:29:52]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-08-07 17:35:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/422013003/80001

[commit-bot: I haz the power, 2014-08-07 17:51:30]

Change committed as 288095

[Miguel Garcia, 2014-08-08 10:57:13]

A revert of this CL has been created in
https://codereview.chromium.org/454863003/ by miguelg@chromium.org.

The reason for reverting is:  This is breaking chrome ToT downstream  and the
change quie complex so
    I don't feel qualified to try to fix it in place.
    
    The failure we hit is
     F/chromium( 2961): [FATAL:system_ui_resource_manager_impl.cc(35)] Check
failed: !bitmap.empty().
    
    See an example of a failed run in
   
https://uberchromegw.corp.google.com/i/clank.tot/builders/instrumentation-yak...
    
    (but just trying to boot clank with this patch will trigger it)
.

[jdduke (slow), 2014-08-08 16:47:48]

On 2014/08/08 10:57:13, Miguel Garcia wrote:
>     See an example of a failed run in
>    
>
https://uberchromegw.corp.google.com/i/clank.tot/builders/instrumentation-yak...
>     
>     (but just trying to boot clank with this patch will trigger it)
> .

Is there a log for the instrumentation-yakju-clankium-tot failure? That test
didn't start failing until a few revisions after my patch on ToT.  I'm also not
able to repro the DCHECK using a GN/N4/N7 on 4.1/4.4/L.

[Yaron, 2014-08-08 17:39:50]

On Fri, Aug 8, 2014 at 9:47 AM, <jdduke@chromium.org> wrote:

> On 2014/08/08 10:57:13, Miguel Garcia wrote:
>
>>      See an example of a failed run in
>>
>
>
> https://uberchromegw.corp.google.com/i/clank.tot/builders/instrumentation-
> yakju-clankium-tot/builds/32007
>
>       (but just trying to boot clank with this patch will trigger it)
>> .
>>
>
> Is there a log for the instrumentation-yakju-clankium-tot failure? That
> test
> didn't start failing until a few revisions after my patch on ToT.  I'm
> also not
> able to repro the DCHECK using a GN/N4/N7 on 4.1/4.4/L.
>

+zty. This seems like a bug with the build scripts. Even if the tests fail,
the scripts should keep going so we can see the logcat

>
> https://codereview.chromium.org/422013003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-08-08 18:34:18]

Ack. This behavior change was introduced in recipe 1.5. I'll fix it now.


On Fri, Aug 8, 2014 at 10:39 AM, Yaron Friedman <yfriedman@chromium.org>
wrote:

>
>
>
> On Fri, Aug 8, 2014 at 9:47 AM, <jdduke@chromium.org> wrote:
>
>> On 2014/08/08 10:57:13, Miguel Garcia wrote:
>>
>>>      See an example of a failed run in
>>>
>>
>>
>> https://uberchromegw.corp.google.com/i/clank.tot/
>> builders/instrumentation-yakju-clankium-tot/builds/32007
>>
>>       (but just trying to boot clank with this patch will trigger it)
>>> .
>>>
>>
>> Is there a log for the instrumentation-yakju-clankium-tot failure? That
>> test
>> didn't start failing until a few revisions after my patch on ToT.  I'm
>> also not
>> able to repro the DCHECK using a GN/N4/N7 on 4.1/4.4/L.
>>
>
> +zty. This seems like a bug with the build scripts. Even if the tests
> fail, the scripts should keep going so we can see the logcat
>
>>
>> https://codereview.chromium.org/422013003/
>>
>
>


-- 
Remember patience, discipline.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.