cc: Remove Display::SetExternalViewport.

cc: Remove Display::SetExternalViewport.

Instead expand the Display's size to cover the entire entire external
viewport (aka the clip area).

Also removes SetSurfaceSize from the SoftwareOutputSurface used by
WebView to draw the renderer's frame into an SkCanvas. Since the
Display is now sized to cover the whole clip area, it will Reshape()
to that size and the OutputSurface can use that size.

R=boliu@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/e9e8961052f406885b2ebcebfb27e181f624f88c
Cr-Commit-Position: refs/heads/master@{#422960}

[danakj, 2016-09-27 00:49:03]

Description was changed from

==========
webview-expand-sw

R=boliu@chromium.org
BUG=
==========

to

==========
webview-expand-sw

R=boliu@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-09-27 00:49:41]

Description was changed from

==========
webview-expand-sw

R=boliu@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Remove Display::SetExternalViewport.

And stop using Display::SetExternalClip for software draws. Instead simply
expand the Display's size to cover the entire
extire external viewport.

R=boliu@chromium.org
BUG=606056
==========

[danakj, 2016-09-27 00:49:46]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[danakj, 2016-09-27 00:50:15]

Maybe this works then?

The parent compositor seems to use SetExternalClip still, and I'm not sure if we
need that one or not. Maybe we do, for software there's a clip on the SkCanvas.

[commit-bot: I haz the power, 2016-09-27 00:50:16]

Dry run: 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-27 03:52:45]

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

[commit-bot: I haz the power, 2016-09-27 03:52:46]

Dry run: This issue passed the CQ dry run.

[boliu, 2016-09-27 18:12:10]

yeah, this breaks drawing when webview is offset from the origin of the canvas

I can dig a little deeper in the afternoon

[boliu, 2016-09-27 23:20:48]

two changes required from testing the webview shell, which only contains a
translate on the canvas, have't tested more complex transforms yet..

https://codereview.chromium.org/2377533002/diff/40001/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/2377533002/diff/40001/cc/surfaces/display.h#n...
cc/surfaces/display.h:128: gfx::Rect external_viewport_;
delete this

https://codereview.chromium.org/2377533002/diff/40001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_frame_sink.cc (left):

https://codereview.chromium.org/2377533002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_frame_sink.cc:330:
display_->SetExternalClip(viewport);
keep external clip

apparently the assumption that display will respect the clip already in the
canvas isn't cornrect :p

https://codereview.chromium.org/2377533002/diff/40001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2377533002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_frame_sink.cc:345:
adjusted_transform.matrix().postTranslate(-viewport.x(), -viewport.y(), 0);
drop this "adjustment"

the matrix contains translate, so this undoes that translate, so apparently old
code always had renderer compositor think it's drawing at 0,0, but at a
different viewport rect, if that makes sense...

and hardware draw path doesn't need this at all, ie x,y are always both 0

[danakj, 2016-09-29 22:55:23]

Description was changed from

==========
cc: Remove Display::SetExternalViewport.

And stop using Display::SetExternalClip for software draws. Instead simply
expand the Display's size to cover the entire
extire external viewport.

R=boliu@chromium.org
BUG=606056
==========

to

==========
cc: Remove Display::SetExternalViewport.

And stop using Display::SetExternalClip for software draws. Instead simply
expand the Display's size to cover the entire
extire external viewport.

R=boliu@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-09-29 22:55:52]

PTAL

https://codereview.chromium.org/2377533002/diff/40001/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/2377533002/diff/40001/cc/surfaces/display.h#n...
cc/surfaces/display.h:128: gfx::Rect external_viewport_;
On 2016/09/27 23:20:48, boliu wrote:
> delete this

Done.

https://codereview.chromium.org/2377533002/diff/40001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_frame_sink.cc (left):

https://codereview.chromium.org/2377533002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_frame_sink.cc:330:
display_->SetExternalClip(viewport);
On 2016/09/27 23:20:48, boliu wrote:
> keep external clip
> 
> apparently the assumption that display will respect the clip already in the
> canvas isn't cornrect :p

Done.

https://codereview.chromium.org/2377533002/diff/40001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2377533002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_frame_sink.cc:337:
gfx::SkISizeToSize(canvas->getBaseLayerSize()));
I also removed this call, and made Reshape() store the size given to it, which
will be the size of the rect computed above since that is what we give to
Display::Resize

https://codereview.chromium.org/2377533002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_frame_sink.cc:345:
adjusted_transform.matrix().postTranslate(-viewport.x(), -viewport.y(), 0);
On 2016/09/27 23:20:48, boliu wrote:
> drop this "adjustment"
> 
> the matrix contains translate, so this undoes that translate, so apparently
old
> code always had renderer compositor think it's drawing at 0,0, but at a
> different viewport rect, if that makes sense...
> 
> and hardware draw path doesn't need this at all, ie x,y are always both 0

Done.

[danakj, 2016-09-29 22:56:03]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 22:56:18]

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

[danakj, 2016-09-29 22:57:26]

Description was changed from

==========
cc: Remove Display::SetExternalViewport.

And stop using Display::SetExternalClip for software draws. Instead simply
expand the Display's size to cover the entire
extire external viewport.

R=boliu@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Remove Display::SetExternalViewport.

And stop using Display::SetExternalClip for software draws. Instead
simply expand the Display's size to cover the entire entire external
viewport.

Also removes SetSurfaceSize from the SoftwareOutputSurface used by
WebView to draw the renderer's frame into an SkCanvas. Since the
Display is now sized to cover the whole clip area, it will Reshape()
to that size and the OutputSurface can use that size.

R=boliu@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-09-29 22:57:57]

Description was changed from

==========
cc: Remove Display::SetExternalViewport.

And stop using Display::SetExternalClip for software draws. Instead
simply expand the Display's size to cover the entire entire external
viewport.

Also removes SetSurfaceSize from the SoftwareOutputSurface used by
WebView to draw the renderer's frame into an SkCanvas. Since the
Display is now sized to cover the whole clip area, it will Reshape()
to that size and the OutputSurface can use that size.

R=boliu@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Remove Display::SetExternalViewport.

Instead expand the Display's size to cover the entire entire external
viewport (aka the clip area).

Also removes SetSurfaceSize from the SoftwareOutputSurface used by
WebView to draw the renderer's frame into an SkCanvas. Since the
Display is now sized to cover the whole clip area, it will Reshape()
to that size and the OutputSurface can use that size.

R=boliu@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-09-29 23:43:55]

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

[commit-bot: I haz the power, 2016-09-29 23:43:56]

Dry run: This issue passed the CQ dry run.

[boliu, 2016-09-30 16:20:28]

hmm, now the right and bottom is cut off if by the translation of viewport from
0,0

[boliu, 2016-09-30 16:33:00]

wait, reverting the Reshape part of the change didn't fix this.. maybe this was
an issue last time as well, it's just google.com is all white, so I didn't
notice missing content at the bottom...

[danakj, 2016-09-30 18:52:00]

On 2016/09/30 16:33:00, boliu wrote:
> wait, reverting the Reshape part of the change didn't fix this.. maybe this
was
> an issue last time as well, it's just http://google.com is all white, so I
didn't
> notice missing content at the bottom...

Should I be using the canvas base size instead of the clip right/bottom?

[danakj, 2016-09-30 18:58:24]

https://codereview.chromium.org/2377533002/diff/60001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_frame_sink.cc (left):

https://codereview.chromium.org/2377533002/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_frame_sink.cc:342:
client_->OnDraw(adjusted_transform, viewport, in_software_draw_);
Or it's probably related to this change..

[danakj, 2016-09-30 19:10:23]

https://codereview.chromium.org/2377533002/diff/60001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_frame_sink.cc (left):

https://codereview.chromium.org/2377533002/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_frame_sink.cc:342:
client_->OnDraw(adjusted_transform, viewport, in_software_draw_);
So LTHI::UpdateDrawProperties would use this transform to figure out where to
draw things. You said it includes the viewport shift. So before what would
happen..

Say your viewport is 5,5,100,100. And you're drawing a dot at 10,10.

The transform would be +5,+5. We would adjust it by the viewport to make it
identity. Now LTHI draws with identity putting the dot at 10,10 and clipping
around 5,5,100,100. So its world origin is at 0,0 in the canvas.

After this change, the transform stays +5,+5. So we draw the dot at 15,15. But
we're clipping 5,5,100,100, so bad things occur and we clip the bottom 5px in
LTHI.


I'm trying to explain to myself the relationship of the Display tho now that
it's viewport origin is 0,0 instead of 5,5.

[boliu, 2016-10-01 00:13:10]

On 2016/09/30 19:10:23, danakj wrote:
>
https://codereview.chromium.org/2377533002/diff/60001/content/renderer/androi...
> File content/renderer/android/synchronous_compositor_frame_sink.cc (left):
> 
>
https://codereview.chromium.org/2377533002/diff/60001/content/renderer/androi...
> content/renderer/android/synchronous_compositor_frame_sink.cc:342:
> client_->OnDraw(adjusted_transform, viewport, in_software_draw_);
> So LTHI::UpdateDrawProperties would use this transform to figure out where to
> draw things. You said it includes the viewport shift. So before what would
> happen..
> 
> Say your viewport is 5,5,100,100. And you're drawing a dot at 10,10.
> 
> The transform would be +5,+5. We would adjust it by the viewport to make it
> identity. Now LTHI draws with identity putting the dot at 10,10 and clipping
> around 5,5,100,100. So its world origin is at 0,0 in the canvas.

Also in the "before" case, the Display viewport is at 5,5,100,100, so in the
canvas, the dot ends up being at 15,15.

> 
> After this change, the transform stays +5,+5. So we draw the dot at 15,15. But
> we're clipping 5,5,100,100, so bad things occur and we clip the bottom 5px in
> LTHI.

Hmm, that sounds possible, but which clipping? I thought the Display clipped
correctly? Is there another clip in LTHI..?

> 
> 
> I'm trying to explain to myself the relationship of the Display tho now that
> it's viewport origin is 0,0 instead of 5,5.

[danakj, 2016-10-01 00:28:39]

On Fri, Sep 30, 2016 at 5:13 PM, <boliu@chromium.org> wrote:

> On 2016/09/30 19:10:23, danakj wrote:
> >
> https://codereview.chromium.org/2377533002/diff/60001/conten
> t/renderer/android/synchronous_compositor_frame_sink.cc
> > File content/renderer/android/synchronous_compositor_frame_sink.cc
> (left):
> >
> >
> https://codereview.chromium.org/2377533002/diff/60001/conten
> t/renderer/android/synchronous_compositor_frame_sink.cc#oldcode342
> > content/renderer/android/synchronous_compositor_frame_sink.cc:342:
> > client_->OnDraw(adjusted_transform, viewport, in_software_draw_);
> > So LTHI::UpdateDrawProperties would use this transform to figure out
> where to
> > draw things. You said it includes the viewport shift. So before what
> would
> > happen..
> >
> > Say your viewport is 5,5,100,100. And you're drawing a dot at 10,10.
> >
> > The transform would be +5,+5. We would adjust it by the viewport to make
> it
> > identity. Now LTHI draws with identity putting the dot at 10,10 and
> clipping
> > around 5,5,100,100. So its world origin is at 0,0 in the canvas.
>
> Also in the "before" case, the Display viewport is at 5,5,100,100, so in
> the
> canvas, the dot ends up being at 15,15.
>

Ok, so now it is at 10, 10 if we don't drop the transform's viewport
offset. We need to transform what the display draws, so maybe we need the
child quad trick (rather than more special things). Then we can keep the
transform without the viewport so LTHI seems 0,0 as the top-left of the
viewport.


>
>
> >
> > After this change, the transform stays +5,+5. So we draw the dot at
> 15,15. But
> > we're clipping 5,5,100,100, so bad things occur and we clip the bottom
> 5px in
> > LTHI.
>
> Hmm, that sounds possible, but which clipping? I thought the Display
> clipped
> correctly? Is there another clip in LTHI..?
>

The clip isn't transformed I think, which is why if the transform is non-0
they conflict.


>
> >
> >
> > I'm trying to explain to myself the relationship of the Display tho now
> that
> > it's viewport origin is 0,0 instead of 5,5.
>



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

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-10-04 02:02:37]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[danakj, 2016-10-04 02:02:47]

PTAL. Now without external clip.

[commit-bot: I haz the power, 2016-10-04 02:03:15]

Dry run: 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-10-04 02:34:51]

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

[commit-bot: I haz the power, 2016-10-04 02:34:52]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[danakj, 2016-10-04 18:52:35]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[danakj, 2016-10-04 18:52:48]

Ah, also removed the SetExternalClip for SurfacesInstance. The clip is already
passed as the clip rect of the root RenderPass, which may be enough. I'm not
sure how to test this code path tho.

[commit-bot: I haz the power, 2016-10-04 18:52:53]

Dry run: 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-10-04 18:56:05]

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

[commit-bot: I haz the power, 2016-10-04 18:56:06]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[danakj, 2016-10-04 19:58:30]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 19:59:15]

Dry run: 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-10-04 21:50:40]

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

[commit-bot: I haz the power, 2016-10-04 21:50:41]

Dry run: This issue passed the CQ dry run.

[boliu, 2016-10-04 21:53:05]

lgtm

[danakj, 2016-10-04 22:09:13]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2016-10-04 22:10:06]

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-10-04 22:16:47]

Description was changed from

==========
cc: Remove Display::SetExternalViewport.

Instead expand the Display's size to cover the entire entire external
viewport (aka the clip area).

Also removes SetSurfaceSize from the SoftwareOutputSurface used by
WebView to draw the renderer's frame into an SkCanvas. Since the
Display is now sized to cover the whole clip area, it will Reshape()
to that size and the OutputSurface can use that size.

R=boliu@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Remove Display::SetExternalViewport.

Instead expand the Display's size to cover the entire entire external
viewport (aka the clip area).

Also removes SetSurfaceSize from the SoftwareOutputSurface used by
WebView to draw the renderer's frame into an SkCanvas. Since the
Display is now sized to cover the whole clip area, it will Reshape()
to that size and the OutputSurface can use that size.

R=boliu@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-10-04 22:16:48]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-10-04 22:19:31]

Description was changed from

==========
cc: Remove Display::SetExternalViewport.

Instead expand the Display's size to cover the entire entire external
viewport (aka the clip area).

Also removes SetSurfaceSize from the SoftwareOutputSurface used by
WebView to draw the renderer's frame into an SkCanvas. Since the
Display is now sized to cover the whole clip area, it will Reshape()
to that size and the OutputSurface can use that size.

R=boliu@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Remove Display::SetExternalViewport.

Instead expand the Display's size to cover the entire entire external
viewport (aka the clip area).

Also removes SetSurfaceSize from the SoftwareOutputSurface used by
WebView to draw the renderer's frame into an SkCanvas. Since the
Display is now sized to cover the whole clip area, it will Reshape()
to that size and the OutputSurface can use that size.

R=boliu@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/e9e8961052f406885b2ebcebfb27e181f624f88c
Cr-Commit-Position: refs/heads/master@{#422960}
==========

[commit-bot: I haz the power, 2016-10-04 22:19:32]

Patchset 8 (id:??) landed as
https://crrev.com/e9e8961052f406885b2ebcebfb27e181f624f88c
Cr-Commit-Position: refs/heads/master@{#422960}