Tiling priorities in Android Webview.

Tiling priorities in Android Webview.

Use the parent compositor's clip and transform for tile
priorities in child compositor.

When the transform matrix changes in parent compositor
(hardware_renderer.cc), it posts the matrix and the clip to
the child compositor. (The parent clip is in screen space
and the parent matrix transforms from webview space to
screen space) Child compositor will use them for tile
prioritization.

In child compositor during updating tile priority, the clip
from parent is transformed from screen space to view space, 
then from view space to content space. Then the result rect 
will intersect with content_bounds() and the intersection 
is used as tile priority input.


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

[hush (inactive), 2014-07-16 00:08:31]

boliu@ and aelias@ for the first pass

[aelias_OOO_until_Jul13, 2014-07-16 00:32:57]

https://codereview.chromium.org/394113002/diff/20001/android_webview/browser/...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/394113002/diff/20001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:58: if
(ui_loop_->BelongsToCurrentThread()) {
I don't think this is a good pattern, there should be a set contract about
whether things are run synchronously or not in advance, since some stuff may not
react correctly to reentrancy.  I suggest making this always async.

https://codereview.chromium.org/394113002/diff/20001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:97: base::AutoLock lock(lock_);
Chromium coding conventions are to avoid mutexes when possible.  Could we change
this to post tasks on the UI-thread message loop instead?  (It was unavoidable
in the other direction because we don't have a handle to the RenderThread
message-loop, but here we do.)

https://codereview.chromium.org/394113002/diff/20001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/394113002/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:403: screen_space_transform_for_tile_priority_ =
screen_space_transform();
Instead of the double-ProjectClippedRect below, wouldn't it work just as well to
multiply the TilingTransform into this matrix here?

https://codereview.chromium.org/394113002/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:465: if (visible_rect_in_content_space.IsEmpty()
||
|| !layer_tree_impl()->TilingTransform().IsIdentity()

https://codereview.chromium.org/394113002/diff/20001/cc/output/output_surface...
File cc/output/output_surface_client.h (right):

https://codereview.chromium.org/394113002/diff/20001/cc/output/output_surface...
cc/output/output_surface_client.h:45: const gfx::Rect& device_rect_for_tiling,
You're using a lot of different names for this rect.  Please standardize on one
across this CL: I suggest "ExternalTilingRect".

https://codereview.chromium.org/394113002/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/394113002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1365: external_transform_for_tiling_ =
transform;
I find it confusing to repurpose the "transform" argument for two completely
different things depending on mode.  Please add a new argument for tiling
transform to SetExternalDrawConstraints (and throughout the call stack from
android_webview) instead.

[boliu, 2014-07-16 17:25:10]

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
android_webview/browser/browser_view_renderer.cc:266: if
(shared_renderer_state_->IsParentDrawConstraintsDirty()) {
Don't bother with dirty. Copying 16 floats isn't that expensive

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:216: clip_ = clip;
child shouldn't need the clip, clip isn't suitable for tile management since it
can jump around wildly

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:70:
client_on_ui_->PostInvalidate();
I'd like all invalidates should go through EnsureContinuousInvalidate.

This is fine for testing and get other bits of code reviewed. We can think about
that later.

Actually I think we can do the PostInvalide bit in a separate patch. We should
be careful not to add more unnecessary post tasks

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
File android_webview/browser/shared_renderer_state.h (right):

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.h:26: class SynchronousCompositor;
Not needed?

https://codereview.chromium.org/394113002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/394113002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:471:
layer_tree_impl()->TilingTransform().GetInverse(&screen_to_view)) {
I assume this should only be read when
!layer_tree_impl()->resourceless_software_draw() ?

https://codereview.chromium.org/394113002/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/394113002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1362: if (!resourceless_software_draw_) {
Yeah I tend to agree with alex that repurposing is confusing.

What about add a SetExternalTilingConstraints the matrix a transform and rect?

[hush (inactive), 2014-07-16 20:45:33]

https://codereview.chromium.org/394113002/diff/20001/android_webview/browser/...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/394113002/diff/20001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:58: if
(ui_loop_->BelongsToCurrentThread()) {
This feels like refatoring. I will do that in a separate code review.
On 2014/07/16 00:32:56, aelias wrote:
> I don't think this is a good pattern, there should be a set contract about
> whether things are run synchronously or not in advance, since some stuff may
not
> react correctly to reentrancy.  I suggest making this always async.

https://codereview.chromium.org/394113002/diff/20001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:97: base::AutoLock lock(lock_);
I will do that in a separate code review
On 2014/07/16 00:32:56, aelias wrote:
> Chromium coding conventions are to avoid mutexes when possible.  Could we
change
> this to post tasks on the UI-thread message loop instead?  (It was unavoidable
> in the other direction because we don't have a handle to the RenderThread
> message-loop, but here we do.)

https://codereview.chromium.org/394113002/diff/20001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/394113002/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:403: screen_space_transform_for_tile_priority_ =
screen_space_transform();
That works.
Done.

On 2014/07/16 00:32:56, aelias wrote:
> Instead of the double-ProjectClippedRect below, wouldn't it work just as well
to
> multiply the TilingTransform into this matrix here?

https://codereview.chromium.org/394113002/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:465: if (visible_rect_in_content_space.IsEmpty()
||
On 2014/07/16 00:32:56, aelias wrote:
> || !layer_tree_impl()->TilingTransform().IsIdentity()

Done.

https://codereview.chromium.org/394113002/diff/20001/cc/output/output_surface...
File cc/output/output_surface_client.h (right):

https://codereview.chromium.org/394113002/diff/20001/cc/output/output_surface...
cc/output/output_surface_client.h:45: const gfx::Rect& device_rect_for_tiling,
I added external_tiling_rect_ and external_tiling_transform_

On 2014/07/16 00:32:56, aelias wrote:
> You're using a lot of different names for this rect.  Please standardize on
one
> across this CL: I suggest "ExternalTilingRect".

https://codereview.chromium.org/394113002/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/394113002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1365: external_transform_for_tiling_ =
transform;
I added external_tiling_transform as the parameter.
On 2014/07/16 00:32:56, aelias wrote:
> I find it confusing to repurpose the "transform" argument for two completely
> different things depending on mode.  Please add a new argument for tiling
> transform to SetExternalDrawConstraints (and throughout the call stack from
> android_webview) instead.

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
android_webview/browser/browser_view_renderer.cc:266: if
(shared_renderer_state_->IsParentDrawConstraintsDirty()) {
On 2014/07/16 17:25:10, boliu wrote:
> Don't bother with dirty. Copying 16 floats isn't that expensive

Done.

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:216: clip_ = clip;
okay.
I also change the function name to PostExternalMatrixToChildCompositor.

On 2014/07/16 17:25:10, boliu wrote:
> child shouldn't need the clip, clip isn't suitable for tile management since
it
> can jump around wildly

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:70:
client_on_ui_->PostInvalidate();
Yeah, I will address Alex's comments about the postInvalidate in a separate
patch as well.
On 2014/07/16 17:25:10, boliu wrote:
> I'd like all invalidates should go through EnsureContinuousInvalidate.
> 
> This is fine for testing and get other bits of code reviewed. We can think
about
> that later.
> 
> Actually I think we can do the PostInvalide bit in a separate patch. We should
> be careful not to add more unnecessary post tasks

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
File android_webview/browser/shared_renderer_state.h (right):

https://codereview.chromium.org/394113002/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.h:26: class SynchronousCompositor;
On 2014/07/16 17:25:10, boliu wrote:
> Not needed?

Done.

https://codereview.chromium.org/394113002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/394113002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:471:
layer_tree_impl()->TilingTransform().GetInverse(&screen_to_view)) {
In resourceless software draw, the transform is identity and DrawViewportSize()
is equal to TilingViewportSize(). So this if condition will fail.

On 2014/07/16 17:25:10, boliu wrote:
> I assume this should only be read when
> !layer_tree_impl()->resourceless_software_draw() ?

https://codereview.chromium.org/394113002/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/394113002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1362: if (!resourceless_software_draw_) {
On 2014/07/16 17:25:10, boliu wrote:
> Yeah I tend to agree with alex that repurposing is confusing.
> 
> What about add a SetExternalTilingConstraints the matrix a transform and rect?

Done.

[aelias_OOO_until_Jul13, 2014-07-16 21:23:32]

OK, over to danakj@ for initial CC review.  Note that hush@ said he will write
tests after the CC naming/etc gets an initial look-over.

https://codereview.chromium.org/394113002/diff/20001/android_webview/browser/...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/394113002/diff/20001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:58: if
(ui_loop_->BelongsToCurrentThread()) {
On 2014/07/16 20:45:31, hush wrote:
> This feels like refatoring. I will do that in a separate code review.

Sorry, that's not how we do things on Chrome-team unless there's high urgency
(which, although this needs to go in soonish, this doesn't reach).  Also, keep
in mind that if you refactor later, you will either have to cherry-pick the
refactoring to 37 or leave trunk different from 37, which will get painful.

https://codereview.chromium.org/394113002/diff/60001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/394113002/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:470:
layer_to_screen.ConcatTransform(external_tiling_transform);
Please bake it into screen_space_transform_for_tile_priority_ inside
UpdateTiles() instead.

https://codereview.chromium.org/394113002/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:472: layer_tree_impl()->DrawViewportSize() !=
external_tiling_rect.size() ||
Please cast the DrawViewportSize to a rect instead.  Right now, this is missing
the case where the size is the same but the rect position is nonzero.

[aelias_OOO_until_Jul13, 2014-07-16 21:26:34]

https://codereview.chromium.org/394113002/diff/20001/android_webview/browser/...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/394113002/diff/20001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:58: if
(ui_loop_->BelongsToCurrentThread()) {
On 2014/07/16 21:23:31, aelias wrote:
> On 2014/07/16 20:45:31, hush wrote:
> > This feels like refatoring. I will do that in a separate code review.
> 
> Sorry, that's not how we do things on Chrome-team unless there's high urgency
> (which, although this needs to go in soonish, this doesn't reach).  Also, keep
> in mind that if you refactor later, you will either have to cherry-pick the
> refactoring to 37 or leave trunk different from 37, which will get painful.

Sorry, I wrote this before seeing Bo's comment that he's planning to change it
later, which you're probably reacting to.  So never mind.  But keep in mind the
cherry-picking problem as you work on this.

https://codereview.chromium.org/394113002/diff/20001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:58: if
(ui_loop_->BelongsToCurrentThread()) {
On 2014/07/16 21:23:31, aelias wrote:
> On 2014/07/16 20:45:31, hush wrote:
> > This feels like refatoring. I will do that in a separate code review.
> 
> Sorry, that's not how we do things on Chrome-team unless there's high urgency
> (which, although this needs to go in soonish, this doesn't reach).  Also, keep
> in mind that if you refactor later, you will either have to cherry-pick the
> refactoring to 37 or leave trunk different from 37, which will get painful.

Sorry, I wrote this before seeing Bo's comment that he's planning to change this
area later, which you're probably reacting to.  In that case, it's fine.  Please
keep in mind the refactoring problem though.

[hush (inactive), 2014-07-17 00:05:53]

https://codereview.chromium.org/394113002/diff/20001/android_webview/browser/...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/394113002/diff/20001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:58: if
(ui_loop_->BelongsToCurrentThread()) {
Yes. I was reacting to Bo's comment about postInvalidate.
Thanks!

On 2014/07/16 21:26:33, aelias wrote:
> On 2014/07/16 21:23:31, aelias wrote:
> > On 2014/07/16 20:45:31, hush wrote:
> > > This feels like refatoring. I will do that in a separate code review.
> > 
> > Sorry, that's not how we do things on Chrome-team unless there's high
urgency
> > (which, although this needs to go in soonish, this doesn't reach).  Also,
keep
> > in mind that if you refactor later, you will either have to cherry-pick the
> > refactoring to 37 or leave trunk different from 37, which will get painful.
> 
> Sorry, I wrote this before seeing Bo's comment that he's planning to change it
> later, which you're probably reacting to.  So never mind.  But keep in mind
the
> cherry-picking problem as you work on this.

https://codereview.chromium.org/394113002/diff/60001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/394113002/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:470:
layer_to_screen.ConcatTransform(external_tiling_transform);
On 2014/07/16 21:23:31, aelias wrote:
> Please bake it into screen_space_transform_for_tile_priority_ inside
> UpdateTiles() instead.

Done.

https://codereview.chromium.org/394113002/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:472: layer_tree_impl()->DrawViewportSize() !=
external_tiling_rect.size() ||
Yes, indeed it is missing that case.
I will use DeviceViewport()
On 2014/07/16 21:23:31, aelias wrote:
> Please cast the DrawViewportSize to a rect instead.  Right now, this is
missing
> the case where the size is the same but the rect position is nonzero.

[boliu, 2014-07-17 01:51:58]

https://codereview.chromium.org/394113002/diff/20001/android_webview/browser/...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/394113002/diff/20001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:97: base::AutoLock lock(lock_);
On 2014/07/16 20:45:32, hush wrote:
> I will do that in a separate code review
> On 2014/07/16 00:32:56, aelias wrote:
> > Chromium coding conventions are to avoid mutexes when possible.  Could we
> change
> > this to post tasks on the UI-thread message loop instead?  (It was
unavoidable
> > in the other direction because we don't have a handle to the RenderThread
> > message-loop, but here we do.)
> 

Actually I prefer using lock here over post task. Android frameworks scheduling
of UI and RT really sucks. If we do a post task, we could miss multiple onDraws,
which is pretty bad. So I'd prefer DrawGL update this immediately, and onDraw
always pick up the latest transform. Only use post task to schedule invalidate
when needed.

[hush (inactive), 2014-07-19 03:48:21]

Patch set 7:
when the WebView is on a hardware layer (View.LAYER_TYPE_HARDWARE), the parent
transform matrix will just be an identity matrix. And the WebView does not know
what transformed are applied to the hardware layer by Android framework.

In this case, the "global visible rect" is still in screen space, but we don't
have the matrix to transform from screen space to view space. So WebView needs
to use the surface rect at (0, 0) for the tiling rect.

[aelias_OOO_until_Jul13, 2014-07-21 18:35:49]

+enne for CC review.  Note that hush@ said he'll add tests after initial
approval of the substantive changes.

[hush (inactive), 2014-07-24 06:21:06]

ping?

[enne (OOO), 2014-07-24 19:25:20]

(Can you also format your change description to wrap at 72 columns?)

https://codereview.chromium.org/394113002/diff/160001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/394113002/diff/160001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:201: SkColor color;
?

https://codereview.chromium.org/394113002/diff/160001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:404:
screen_space_transform_for_tile_priority_.ConcatTransform(
It seems a little bit like this viewport_rect_for_tile_priority_ and
screen_space_transform_for_tile_priority_ are a bit redundant.  It's not like
tile priorities are that smart about crazy perspective or rotation (they're just
treated as scales, but the scale comes from the content scale of the layer, not
the transform).  So it seems like you could just do this transform once and use
a transformed viewport rect here rather than a viewport + transform?

https://codereview.chromium.org/394113002/diff/160001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/394113002/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:226: gfx::Rect ExternalTilingRect() const;
bikeshed: "TilingRect" is a really ambiguous phrase in cc.  It's not always
external, either.  Maybe call this DeviceViewportRect(ForTiling) or
ViewportRectForTilePriority that you call it in PictureLayerImpl?

[hush (inactive), 2014-07-24 23:29:37]

https://codereview.chromium.org/394113002/diff/160001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/394113002/diff/160001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:201: SkColor color;
This is just a minor performance improvement. Don't create a temp variable for
each time in the iteration?

On 2014/07/24 19:25:19, enne wrote:
> ?

https://codereview.chromium.org/394113002/diff/160001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:404:
screen_space_transform_for_tile_priority_.ConcatTransform(
Yeah right. I will do this based on your other comment in LTHI:
Combine "ExternalTilingRect" and "ExternalTilingTransform" into one function
"ViewportRectForTilingPriority" in layer_tree_impl. It will be the external
tiling rect inversely transformed from screen space into view space. Then
picture layer impl will just use ViewportRectForTilingPriority().
On 2014/07/24 19:25:19, enne wrote:
> It seems a little bit like this viewport_rect_for_tile_priority_ and
> screen_space_transform_for_tile_priority_ are a bit redundant.  It's not like
> tile priorities are that smart about crazy perspective or rotation (they're
just
> treated as scales, but the scale comes from the content scale of the layer,
not
> the transform).  So it seems like you could just do this transform once and
use
> a transformed viewport rect here rather than a viewport + transform?

[enne (OOO), 2014-07-24 23:34:29]

https://codereview.chromium.org/394113002/diff/160001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/394113002/diff/160001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:201: SkColor color;
On 2014/07/24 23:29:37, hush wrote:
> This is just a minor performance improvement. Don't create a temp variable for
> each time in the iteration?

There's not performance improvement here.  Neither SkColor nor float has a
constructor to call, so it's just pushing the stack to make space.  By keeping
them in the inner loop you get better readability (in my opinion) by knowing
that they are scoped to a single loop iteration.

If you want to make unrelated changes, please put them in separate patches.

[hush (inactive), 2014-07-24 23:48:07]

https://codereview.chromium.org/394113002/diff/160001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/394113002/diff/160001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:201: SkColor color;
Okay. I will just revert this part.
On 2014/07/24 23:34:29, enne wrote:
> On 2014/07/24 23:29:37, hush wrote:
> > This is just a minor performance improvement. Don't create a temp variable
for
> > each time in the iteration?
> 
> There's not performance improvement here.  Neither SkColor nor float has a
> constructor to call, so it's just pushing the stack to make space.  By keeping
> them in the inner loop you get better readability (in my opinion) by knowing
> that they are scoped to a single loop iteration.
> 
> If you want to make unrelated changes, please put them in separate patches.

[enne (OOO), 2014-07-25 18:44:31]

https://codereview.chromium.org/394113002/diff/200001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/394113002/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1350: const gfx::Transform&
external_tiling_transform,
Does cc even need to know about external_tiling_transform? Can the caller do
this inverse since it's also handing in the external_tiling_rect?

[aelias_OOO_until_Jul13, 2014-07-25 18:53:35]

On 2014/07/25 at 18:44:31, enne wrote:
>
https://codereview.chromium.org/394113002/diff/200001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/394113002/diff/200001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl.cc:1350: const gfx::Transform&
external_tiling_transform,
> Does cc even need to know about external_tiling_transform? Can the caller do
this inverse since it's also handing in the external_tiling_rect?

After the transform is applied, it may be a quad, so CC does need to know about
it.

[enne (OOO), 2014-07-25 18:56:17]

On 2014/07/25 at 18:53:35, aelias wrote:
> On 2014/07/25 at 18:44:31, enne wrote:
> >
https://codereview.chromium.org/394113002/diff/200001/cc/trees/layer_tree_hos...
> > File cc/trees/layer_tree_host_impl.cc (right):
> > 
> >
https://codereview.chromium.org/394113002/diff/200001/cc/trees/layer_tree_hos...
> > cc/trees/layer_tree_host_impl.cc:1350: const gfx::Transform&
external_tiling_transform,
> > Does cc even need to know about external_tiling_transform? Can the caller do
this inverse since it's also handing in the external_tiling_rect?
> 
> After the transform is applied, it may be a quad, so CC does need to know
about it.

Tile prioritization behaves as if it's a rect, so even if it is a quad, that's
not going to get used.

[aelias_OOO_until_Jul13, 2014-07-25 19:04:31]

On 2014/07/25 at 18:56:17, enne wrote:
> On 2014/07/25 at 18:53:35, aelias wrote:
> > On 2014/07/25 at 18:44:31, enne wrote:
> > >
https://codereview.chromium.org/394113002/diff/200001/cc/trees/layer_tree_hos...
> > > File cc/trees/layer_tree_host_impl.cc (right):
> > > 
> > >
https://codereview.chromium.org/394113002/diff/200001/cc/trees/layer_tree_hos...
> > > cc/trees/layer_tree_host_impl.cc:1350: const gfx::Transform&
external_tiling_transform,
> > > Does cc even need to know about external_tiling_transform? Can the caller
do this inverse since it's also handing in the external_tiling_rect?
> > 
> > After the transform is applied, it may be a quad, so CC does need to know
about it.
> 
> Tile prioritization behaves as if it's a rect, so even if it is a quad, that's
not going to get used.

OK, I was thinking in terms of "theoretically correct API", but since this
prioritization behavior is realistically unlikely to change and the API surface
isn't across repos anyway, I guess I won't object if you want to push more logic
out of CC.

[enne (OOO), 2014-07-25 19:09:27]

On 2014/07/25 at 19:04:31, aelias wrote:
> On 2014/07/25 at 18:56:17, enne wrote:
> > On 2014/07/25 at 18:53:35, aelias wrote:
> > > On 2014/07/25 at 18:44:31, enne wrote:
> > > >
https://codereview.chromium.org/394113002/diff/200001/cc/trees/layer_tree_hos...
> > > > File cc/trees/layer_tree_host_impl.cc (right):
> > > > 
> > > >
https://codereview.chromium.org/394113002/diff/200001/cc/trees/layer_tree_hos...
> > > > cc/trees/layer_tree_host_impl.cc:1350: const gfx::Transform&
external_tiling_transform,
> > > > Does cc even need to know about external_tiling_transform? Can the
caller do this inverse since it's also handing in the external_tiling_rect?
> > > 
> > > After the transform is applied, it may be a quad, so CC does need to know
about it.
> > 
> > Tile prioritization behaves as if it's a rect, so even if it is a quad,
that's not going to get used.
> 
> OK, I was thinking in terms of "theoretically correct API", but since this
prioritization behavior is realistically unlikely to change and the API surface
isn't across repos anyway, I guess I won't object if you want to push more logic
out of CC.

Hmm, ok.  I see what you're saying; this is really an implementation detail of
cc.  I think this makes sense, then.

[aelias_OOO_until_Jul13, 2014-07-25 19:11:47]

OK, sounds good to leave as is then.  hush@, please write some new CC tests to
cover this behavior and then we can land this.

[enne (OOO), 2014-07-25 19:16:55]

https://codereview.chromium.org/394113002/diff/200001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/394113002/diff/200001/cc/output/output_surfac...
cc/output/output_surface.h:170: const gfx::Rect& external_tiling_rect,
In general this patch looks fine to me, but I still am bikeshedding a little on
external_tiling_rect and external_tiling_transform as names.  The name "tiling"
just doesn't really mean anything and is ambiguous with other uses of "tiling"
in cc.

Are there other names that could work here? One suggestion:
external_viewport_rect and external_viewport_transform make slightly more sense
to me.  "external" because they're not applied to quads in the same way that
transform/viewport in this function are?

Maybe a comment on this function about how these two parameters differ from
transform/viewport would help too?

[hush (inactive), 2014-07-29 00:55:57]

https://codereview.chromium.org/394113002/diff/200001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/394113002/diff/200001/cc/output/output_surfac...
cc/output/output_surface.h:170: const gfx::Rect& external_tiling_rect,
This function name actually suggests all parameters are "external" implicitly.
For example, transform is stored as "external_transform_" in
LTHI::SetExternalDrawConstraints.

So how about I make it verbose and consistent with what we use in
picture_layer_impl: viewport_rect_for_tile_priority and
transform_for_tile_priority

There are already comments in LTHI private members (external_transform_, 
external_viewport, etc.). Are those comments enough?

On 2014/07/25 19:16:55, enne wrote:
> In general this patch looks fine to me, but I still am bikeshedding a little
on
> external_tiling_rect and external_tiling_transform as names.  The name
"tiling"
> just doesn't really mean anything and is ambiguous with other uses of "tiling"
> in cc.
> 
> Are there other names that could work here? One suggestion:
> external_viewport_rect and external_viewport_transform make slightly more
sense
> to me.  "external" because they're not applied to quads in the same way that
> transform/viewport in this function are?
> 
> Maybe a comment on this function about how these two parameters differ from
> transform/viewport would help too?

[hush (inactive), 2014-07-29 18:28:51]

Hi Alex,
I wrote unit test for PictureLayerImpl. Can you take a look?

Hi Bo,
We talked in person yesterday about calling BVR::EnsureContinuousInvalidation in
shared renderer state so that we don't post invalidates on UI unnecessarily.
That part is also in this patch.

[enne (OOO), 2014-07-29 18:39:28]

https://codereview.chromium.org/394113002/diff/200001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/394113002/diff/200001/cc/output/output_surfac...
cc/output/output_surface.h:170: const gfx::Rect& external_tiling_rect,
On 2014/07/29 00:55:57, hush wrote:
> This function name actually suggests all parameters are "external" implicitly.
> For example, transform is stored as "external_transform_" in
> LTHI::SetExternalDrawConstraints.
> 
> So how about I make it verbose and consistent with what we use in
> picture_layer_impl: viewport_rect_for_tile_priority and
> transform_for_tile_priority
> 
> There are already comments in LTHI private members (external_transform_, 
> external_viewport, etc.). Are those comments enough?
> 
> On 2014/07/25 19:16:55, enne wrote:
> > In general this patch looks fine to me, but I still am bikeshedding a little
> on
> > external_tiling_rect and external_tiling_transform as names.  The name
> "tiling"
> > just doesn't really mean anything and is ambiguous with other uses of
"tiling"
> > in cc.
> > 
> > Are there other names that could work here? One suggestion:
> > external_viewport_rect and external_viewport_transform make slightly more
> sense
> > to me.  "external" because they're not applied to quads in the same way that
> > transform/viewport in this function are?
> > 
> > Maybe a comment on this function about how these two parameters differ from
> > transform/viewport would help too?
> 

"_for_tile_priority" is definitely better than "tiling", thanks.  And
consistency in variable naming all the way through also seems good.

Those comments are probably enough.

[boliu, 2014-07-29 19:02:42]

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:8: #include
"android_webview/browser/parent_compositor_draw_constraints.h"
This include should be in browser_view_renderer.h, so it's not needed here.

It's not in bvr.h, so how did this compile??

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:266: gfx::Rect
viewport_rect_for_tile_priority;
nit: move this down below to where it's first used.

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:267: parent_draw_constraints_ =
shared_renderer_state_->ParentDrawConstraints();
nit: move this to the first line of this block.

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:205: gfx::Rect parent_clip_;
Remove

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
File android_webview/browser/browser_view_renderer_client.h (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/browser_view_renderer_client.h:32: virtual void
EnsureContinuousInvalidation(bool force_invalidate) = 0;
Don't expose more than you need. Just call this "InvalidateOnce" and it doesn't
need to take a parameter.

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/hardware_renderer.cc:216: if
(!draw_constraints_.equals(draw_constraints)) {
Remember we are not looking for "equals" exactly. We don't care about the
surface size if we are not in a layer.

Let's move this block near the beginning of the method, let's say right after
checking context changed.

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
File android_webview/browser/parent_compositor_draw_constraints.cc (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.cc:10: :
is_layer(false), transform(gfx::Transform()), surface_rect(gfx::Rect()) {
Transform and Rect have default constructors

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.cc:19:
this->surface_rect = surface_rect;
Use initializer list. Otherwise why have this constructor in the first place?

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
File android_webview/browser/parent_compositor_draw_constraints.h (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.h:12: struct
ParentCompositorDrawConstraints {
nit: blank line

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.h:20: gfx::Rect
surface_rect);
gfx:: objects should be const & here.

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.h:21: bool
equals(ParentCompositorDrawConstraints other) const;
Parameter should be const &

Capitalize it "Equals"

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.h:25: #endif  //
ANDROID_WEBVIEW_BROWSER_PARENT_COMPOSITOR_DRAW_CONSTRAINTS_H_
nit: blank line

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/shared_renderer_state.cc:10: #include
"cc/output/compositor_frame.h"
remove both

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
File android_webview/browser/shared_renderer_state.h (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/shared_renderer_state.h:51: void
ClientPostInvalidateIfNeeded();
Why does this exist? And why is it public? Nothing calls this except in the
class, and it's only a single post task?

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/shared_renderer_state.h:60: const gfx::Rect&
ParentClip() const;
remove

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/shared_renderer_state.h:61: const
ParentCompositorDrawConstraints& ParentDrawConstraints() const;
Have to return a copy. Otherwise it's not thread safe.

https://codereview.chromium.org/394113002/diff/240001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/native/...
android_webview/native/aw_contents.cc:743:
browser_view_renderer_.EnsureContinuousInvalidation(force_invalidate);
DCHECK thread

https://codereview.chromium.org/394113002/diff/240001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.h (right):

https://codereview.chromium.org/394113002/diff/240001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.h:63:
gfx::Transform transform_for_tile_priority) OVERRIDE;
Hmm, we are really inconsistent with using const& vs value. I guess keep this
consistent and use const& for the transform.

https://codereview.chromium.org/394113002/diff/240001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/394113002/diff/240001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:205:
InvokeComposite(transform, clip, clip, empty, gfx::Transform(), false);
So cached_hw_viewport_rect_for_tile_priority_ and
cached_hw_transform_for_tile_priority_ would be the right correct tiling
information even in a software draw, so we don't need to special case
resourceless software draws in cc.

Can you file a crbug for this clean up?

[hush (inactive), 2014-07-29 21:14:22]

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:8: #include
"android_webview/browser/parent_compositor_draw_constraints.h"
I did compile this successfully, due to luck:
bvr.h include shared_renderer_state.h, which includes
parent_compositor_draw_constraints.h

But yeah, I should put this line to bvr.h
On 2014/07/29 19:02:41, boliu wrote:
> This include should be in browser_view_renderer.h, so it's not needed here.
> 
> It's not in bvr.h, so how did this compile??

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:266: gfx::Rect
viewport_rect_for_tile_priority;
On 2014/07/29 19:02:41, boliu wrote:
> nit: move this down below to where it's first used.

Done.

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:267: parent_draw_constraints_ =
shared_renderer_state_->ParentDrawConstraints();
On 2014/07/29 19:02:41, boliu wrote:
> nit: move this to the first line of this block.

Done.

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:205: gfx::Rect parent_clip_;
On 2014/07/29 19:02:41, boliu wrote:
> Remove

Done.

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
File android_webview/browser/browser_view_renderer_client.h (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/browser_view_renderer_client.h:32: virtual void
EnsureContinuousInvalidation(bool force_invalidate) = 0;
I will make it "InvalidateOnceIfNeede" since this function might just early out
if block_invalidates_ == true
On 2014/07/29 19:02:41, boliu wrote:
> Don't expose more than you need. Just call this "InvalidateOnce" and it
doesn't
> need to take a parameter.

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/hardware_renderer.cc:216: if
(!draw_constraints_.equals(draw_constraints)) {
Yeah.

Why do we want to move this to the beginning of the function? Just to reduce a
little bit of latency when you post it to UI?
Also for the record, this portion of the code will be in "DrawGL" after I rebase
past kModeSync CL: https://codereview.chromium.org/414163002/
On 2014/07/29 19:02:41, boliu wrote:
> Remember we are not looking for "equals" exactly. We don't care about the
> surface size if we are not in a layer.
> 
> Let's move this block near the beginning of the method, let's say right after
> checking context changed.

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
File android_webview/browser/parent_compositor_draw_constraints.cc (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.cc:10: :
is_layer(false), transform(gfx::Transform()), surface_rect(gfx::Rect()) {
okay. Removed.
On 2014/07/29 19:02:41, boliu wrote:
> Transform and Rect have default constructors

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.cc:19:
this->surface_rect = surface_rect;
used initializer list.
On 2014/07/29 19:02:41, boliu wrote:
> Use initializer list. Otherwise why have this constructor in the first place?

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
File android_webview/browser/parent_compositor_draw_constraints.h (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.h:12: struct
ParentCompositorDrawConstraints {
On 2014/07/29 19:02:41, boliu wrote:
> nit: blank line

Done.

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.h:20: gfx::Rect
surface_rect);
On 2014/07/29 19:02:42, boliu wrote:
> gfx:: objects should be const & here.

Done.

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.h:21: bool
equals(ParentCompositorDrawConstraints other) const;
On 2014/07/29 19:02:42, boliu wrote:
> Parameter should be const &
> 
> Capitalize it "Equals"

Done.

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.h:25: #endif  //
ANDROID_WEBVIEW_BROWSER_PARENT_COMPOSITOR_DRAW_CONSTRAINTS_H_
On 2014/07/29 19:02:41, boliu wrote:
> nit: blank line

Done.

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
File android_webview/browser/shared_renderer_state.h (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/shared_renderer_state.h:51: void
ClientPostInvalidateIfNeeded();
I removed this function.
On 2014/07/29 19:02:42, boliu wrote:
> Why does this exist? And why is it public? Nothing calls this except in the
> class, and it's only a single post task?

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/shared_renderer_state.h:60: const gfx::Rect&
ParentClip() const;
On 2014/07/29 19:02:42, boliu wrote:
> remove

Done.

https://codereview.chromium.org/394113002/diff/240001/android_webview/browser...
android_webview/browser/shared_renderer_state.h:61: const
ParentCompositorDrawConstraints& ParentDrawConstraints() const;
Yeah good point.
On 2014/07/29 19:02:42, boliu wrote:
> Have to return a copy. Otherwise it's not thread safe.

https://codereview.chromium.org/394113002/diff/240001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/394113002/diff/240001/android_webview/native/...
android_webview/native/aw_contents.cc:743:
browser_view_renderer_.EnsureContinuousInvalidation(force_invalidate);
On 2014/07/29 19:02:42, boliu wrote:
> DCHECK thread

Done.

https://codereview.chromium.org/394113002/diff/240001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.h (right):

https://codereview.chromium.org/394113002/diff/240001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.h:63:
gfx::Transform transform_for_tile_priority) OVERRIDE;
yeah... maybe because rect is considered "lighter weight" than transform.

On 2014/07/29 19:02:42, boliu wrote:
> Hmm, we are really inconsistent with using const& vs value. I guess keep this
> consistent and use const& for the transform.

https://codereview.chromium.org/394113002/diff/240001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/394113002/diff/240001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:205:
InvokeComposite(transform, clip, clip, empty, gfx::Transform(), false);
I created:
https://code.google.com/p/chromium/issues/detail?id=398587
On 2014/07/29 19:02:42, boliu wrote:
> So cached_hw_viewport_rect_for_tile_priority_ and
> cached_hw_transform_for_tile_priority_ would be the right correct tiling
> information even in a software draw, so we don't need to special case
> resourceless software draws in cc.
> 
> Can you file a crbug for this clean up?

[aelias_OOO_until_Jul13, 2014-07-29 22:59:45]

Thanks for the test, and it looks like enne@'s comments were all addressed. 
lgtm for cc/

[enne (OOO), 2014-07-29 23:12:05]

cc lgtm2, thanks.

[hush (inactive), 2014-07-29 23:15:56]

+Yaron for content/ reviews

[boliu, 2014-07-29 23:50:41]

https://codereview.chromium.org/394113002/diff/300001/android_webview/browser...
File android_webview/browser/browser_view_renderer_client.h (right):

https://codereview.chromium.org/394113002/diff/300001/android_webview/browser...
android_webview/browser/browser_view_renderer_client.h:34: virtual void
InvalidateOnceIfNeeded() = 0;
Call this something like "ParentConstraintsUpdated".

https://codereview.chromium.org/394113002/diff/300001/android_webview/browser...
File android_webview/browser/parent_compositor_draw_constraints.cc (right):

https://codereview.chromium.org/394113002/diff/300001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.cc:25: if (!is_layer)
 // Don't care about the surface size when neither is on a
nit: Move comment to above or into the block

Also anything that makes a block more than one line requires braces

https://codereview.chromium.org/394113002/diff/300001/android_webview/browser...
File android_webview/browser/shared_renderer_state.h (right):

https://codereview.chromium.org/394113002/diff/300001/android_webview/browser...
android_webview/browser/shared_renderer_state.h:16: #include
"ui/gfx/transform.h"
remove

[hush (inactive), 2014-07-30 00:11:33]

https://codereview.chromium.org/394113002/diff/300001/android_webview/browser...
File android_webview/browser/browser_view_renderer_client.h (right):

https://codereview.chromium.org/394113002/diff/300001/android_webview/browser...
android_webview/browser/browser_view_renderer_client.h:34: virtual void
InvalidateOnceIfNeeded() = 0;
Renamed it to UpdateParentDrawConstraints. And renamed the function in shared
renderer state as well.
On 2014/07/29 23:50:41, boliu wrote:
> Call this something like "ParentConstraintsUpdated".

https://codereview.chromium.org/394113002/diff/300001/android_webview/browser...
File android_webview/browser/parent_compositor_draw_constraints.cc (right):

https://codereview.chromium.org/394113002/diff/300001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.cc:25: if (!is_layer)
 // Don't care about the surface size when neither is on a
I moved the comment above the block
On 2014/07/29 23:50:41, boliu wrote:
> nit: Move comment to above or into the block
> 
> Also anything that makes a block more than one line requires braces

https://codereview.chromium.org/394113002/diff/300001/android_webview/browser...
File android_webview/browser/shared_renderer_state.h (right):

https://codereview.chromium.org/394113002/diff/300001/android_webview/browser...
android_webview/browser/shared_renderer_state.h:16: #include
"ui/gfx/transform.h"
On 2014/07/29 23:50:41, boliu wrote:
> remove

Done.

[boliu, 2014-07-30 00:16:37]

android webview lgtm % last two comments

sync compositor looks good too, although I'm not owner there

https://codereview.chromium.org/394113002/diff/340001/android_webview/browser...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/394113002/diff/340001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:151: void
SetTotalRootLayerScrollOffset(gfx::Vector2dF new_value_dip);
nit: move this back to immediately after private:

https://codereview.chromium.org/394113002/diff/340001/android_webview/browser...
File android_webview/browser/parent_compositor_draw_constraints.cc (right):

https://codereview.chromium.org/394113002/diff/340001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.cc:22: if (is_layer
!= other.is_layer)
You can do

if (is_layer != other.is_layer || transform != other.transform)
  return false;

// Don't care about the surface size when neither is on a layer.
return is_layer || surface_rect == other.surface_rect;

[hush (inactive), 2014-07-30 00:34:30]

https://codereview.chromium.org/394113002/diff/340001/android_webview/browser...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/394113002/diff/340001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:151: void
SetTotalRootLayerScrollOffset(gfx::Vector2dF new_value_dip);
Done
On 2014/07/30 00:16:37, boliu wrote:
> nit: move this back to immediately after private:

https://codereview.chromium.org/394113002/diff/340001/android_webview/browser...
File android_webview/browser/parent_compositor_draw_constraints.cc (right):

https://codereview.chromium.org/394113002/diff/340001/android_webview/browser...
android_webview/browser/parent_compositor_draw_constraints.cc:22: if (is_layer
!= other.is_layer)
Yeah this is cleaner.
The last line seems wrong though. It should be:
return !is_layer || surface_rect == other.surface_rect
On 2014/07/30 00:16:37, boliu wrote:
> You can do
> 
> if (is_layer != other.is_layer || transform != other.transform)
>   return false;
> 
> // Don't care about the surface size when neither is on a layer.
> return is_layer || surface_rect == other.surface_rect;

[Yaron, 2014-07-30 17:43:27]

lgtm

[hush (inactive), 2014-07-30 17:52:27]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2014-07-30 17:53:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hush@chromium.org/394113002/380001

[Ken Russell (switch to Gerrit), 2014-07-30 18:30:53]

On 2014/07/30 17:53:27, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/hush@chromium.org/394113002/380001

The GPU tryserver failures look real. Please contact me if you have any issue
reproducing them locally.

[commit-bot: I haz the power, 2014-07-30 19:24:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-07-30 19:45:40]

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

[commit-bot: I haz the power, 2014-07-30 19:45:41]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[hush (inactive), 2014-07-30 21:08:25]

On 2014/07/30 19:45:41, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
>   mac_gpu_triggered_tests on tryserver.chromium.gpu
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

This is indeed my CL's problem. I reverted locally and ran the gpu rasterization
test again and the test passed.
Investigating now.

[hush (inactive), 2014-07-30 21:55:18]

I just uploaded a patch for fixing gpu_rasterization test. The test passed
locally on my goobuntu box.

The issue is if LTHI::SetExternalDrawConstraints is never called, (which is what
happens in the gpu_rasterization test), LTHI::ViewportRectForTilePriority() will
return an empty rect. As a result, PictureLayerImpl see the viewport rect for
tile priority is different from the device viewport rect, and will use the empty
rect for tile priority. Then the test fails.

I added a boolean variable in LTHI to indicate whether it wants to default to
DeviceViewport() or not.

[aelias_OOO_until_Jul13, 2014-07-30 22:19:35]

Hmm, sorry, I don't like this new boolean.  The irreversible modal nature of it
feels wrong and it may cause problems if we ever try to use
SetExternalDrawConstraints for anything that's not WebView.

I suggest we just check if viewport_rect_for_tile_priority.IsEmpty() similarly
to what happens in ExternalViewport().  I don't think we need to support the
empty case: for resourceless software draws, the value should never be used (or,
even better, we can just set it to the previous hardware viewport and bypass
that old issue entirely); and for transforms moving the WebView off the screen,
the intersection that sets it to emptiness is applied at the last minute.

[aelias_OOO_until_Jul13, 2014-07-30 22:37:57]

Thanks for removing the boolean.  cc/ lgtm again

[hush (inactive), 2014-07-30 22:45:18]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2014-07-30 22:48:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hush@chromium.org/394113002/420001

[commit-bot: I haz the power, 2014-07-31 04:33:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)

[commit-bot: I haz the power, 2014-07-31 09:09:41]

Change committed as 286731