cc: Make OutputSurfaceFrame::sub_buffer_rect optional

cc: Make OutputSurfaceFrame::sub_buffer_rect optional

sub_buffer_rect is used to signal partial or empty swap, but previously
required a default value of full surface size to indicate regular
swap.  In preparation for adding swap with content bounds, clarify
the default case to not provide a value for sub_buffer_rect.

BUG=687690
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2699173002
Cr-Commit-Position: refs/heads/master@{#452234}
Committed: https://chromium.googlesource.com/chromium/src/+/55723a7098ece4916d203986b5f7d336a0c0a9f5

[halliwell, 2017-02-17 17:48:22]

Description was changed from

==========
cc: Make OutputSurfaceFrame::sub_buffer_rect optional

sub_buffer_rect is used to signal partial or empty swap, but previously
required a default value of full surface size to indicate regular
swap.  In preparation for adding swap with content bounds, clarify
the default case to not provide a value for sub_buffer_rect.

BUG=687690
==========

to

==========
cc: Make OutputSurfaceFrame::sub_buffer_rect optional

sub_buffer_rect is used to signal partial or empty swap, but previously
required a default value of full surface size to indicate regular
swap.  In preparation for adding swap with content bounds, clarify
the default case to not provide a value for sub_buffer_rect.

BUG=687690
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[halliwell, 2017-02-17 17:50:12]

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

[commit-bot: I haz the power, 2017-02-17 17:50:58]

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, 2017-02-17 19:37:09]

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

[commit-bot: I haz the power, 2017-02-17 19:37:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[halliwell, 2017-02-17 20:35:23]

Description was changed from

==========
cc: Make OutputSurfaceFrame::sub_buffer_rect optional

sub_buffer_rect is used to signal partial or empty swap, but previously
required a default value of full surface size to indicate regular
swap.  In preparation for adding swap with content bounds, clarify
the default case to not provide a value for sub_buffer_rect.

BUG=687690
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Make OutputSurfaceFrame::sub_buffer_rect optional

sub_buffer_rect is used to signal partial or empty swap, but previously
required a default value of full surface size to indicate regular
swap.  In preparation for adding swap with content bounds, clarify
the default case to not provide a value for sub_buffer_rect.

BUG=687690
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[halliwell, 2017-02-17 20:35:23]

halliwell@chromium.org changed reviewers:
+ danakj@chromium.org, rjkroege@chromium.org

[halliwell, 2017-02-17 20:36:41]

as discussed on https://codereview.chromium.org/2693023002/

[halliwell, 2017-02-21 15:49:59]

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

[commit-bot: I haz the power, 2017-02-21 15:50:50]

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, 2017-02-21 17:12:57]

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

[commit-bot: I haz the power, 2017-02-21 17:12:58]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-02-22 00:36:58]

Cool, couple comments but LGTM overall

https://codereview.chromium.org/2699173002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2699173002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2651: } else {
else if

https://codereview.chromium.org/2699173002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2656: } else {
i dont think we need this else, we clear the rect at the bottom of the method

https://codereview.chromium.org/2699173002/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2699173002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:110: if
(frame.sub_buffer_rect)
{}

https://codereview.chromium.org/2699173002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:113: else
{}

https://codereview.chromium.org/2699173002/diff/1/content/browser/compositor/...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2699173002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:77:
if (frame.sub_buffer_rect) {
no {} :)

https://codereview.chromium.org/2699173002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:80:
buffer_queue_->SwapBuffers(damage);
it might be more readable with a ternary

  buffer_queue_->SwapBuffers(frame.sub_buffer_rect ? frame.sub_buffer_rect :
swap_size_);

maybe not tho, wdyt?

https://codereview.chromium.org/2699173002/diff/1/services/ui/surfaces/displa...
File services/ui/surfaces/display_output_surface_ozone.cc (right):

https://codereview.chromium.org/2699173002/diff/1/services/ui/surfaces/displa...
services/ui/surfaces/display_output_surface_ozone.cc:87: if
(frame.sub_buffer_rect) {
no {}

https://codereview.chromium.org/2699173002/diff/1/services/ui/surfaces/displa...
services/ui/surfaces/display_output_surface_ozone.cc:90:
buffer_queue_->SwapBuffers(damage);
?: maybe here also?

[halliwell, 2017-02-22 14:19:28]

rjkroege: PTAL services/ui/surfaces changes.

https://codereview.chromium.org/2699173002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2699173002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2651: } else {
On 2017/02/22 00:36:57, danakj wrote:
> else if

Done.

https://codereview.chromium.org/2699173002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2656: } else {
On 2017/02/22 00:36:57, danakj wrote:
> i dont think we need this else, we clear the rect at the bottom of the method

Done.

https://codereview.chromium.org/2699173002/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2699173002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:110: if
(frame.sub_buffer_rect)
On 2017/02/22 00:36:57, danakj wrote:
> {}

Done.

https://codereview.chromium.org/2699173002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:113: else
On 2017/02/22 00:36:57, danakj wrote:
> {}

Done.

https://codereview.chromium.org/2699173002/diff/1/content/browser/compositor/...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2699173002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:80:
buffer_queue_->SwapBuffers(damage);
On 2017/02/22 00:36:57, danakj wrote:
> it might be more readable with a ternary
> 
>   buffer_queue_->SwapBuffers(frame.sub_buffer_rect ? frame.sub_buffer_rect :
> swap_size_);
> 
> maybe not tho, wdyt?

I prefer the ternary form, yes.

https://codereview.chromium.org/2699173002/diff/1/services/ui/surfaces/displa...
File services/ui/surfaces/display_output_surface_ozone.cc (right):

https://codereview.chromium.org/2699173002/diff/1/services/ui/surfaces/displa...
services/ui/surfaces/display_output_surface_ozone.cc:90:
buffer_queue_->SwapBuffers(damage);
On 2017/02/22 00:36:57, danakj wrote:
> ?: maybe here also?

Done.

[danakj, 2017-02-22 14:45:06]

LGTM

https://codereview.chromium.org/2699173002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2699173002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2652: // Expand the swap rect to the full surface
unless it's empty, and empty
Oh, this comment isn't quite right anymore. We're not expanding the rect so much
as neglecting to send one.

[rjkroege, 2017-02-22 19:20:38]

lgtm

[halliwell, 2017-02-22 20:59:39]

https://codereview.chromium.org/2699173002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2699173002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2652: // Expand the swap rect to the full surface
unless it's empty, and empty
On 2017/02/22 14:45:06, danakj wrote:
> Oh, this comment isn't quite right anymore. We're not expanding the rect so
much
> as neglecting to send one.

Indeed, removed (not sure a comment is super necessary here any longer, but let
me know if you disagree).

[halliwell, 2017-02-22 20:59:44]

The CQ bit was checked by halliwell@chromium.org

[halliwell, 2017-02-22 20:59:45]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
rjkroege@chromium.org
Link to the patchset: https://codereview.chromium.org/2699173002/#ps40001
(title: "Removed obsolete comment")

[commit-bot: I haz the power, 2017-02-22 21:00:30]

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-02-22 22:24:07]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487797184956250,
"parent_rev": "2786003858a820e942180760173db7c5d2e61251", "commit_rev":
"55723a7098ece4916d203986b5f7d336a0c0a9f5"}

[commit-bot: I haz the power, 2017-02-22 22:24:42]

Description was changed from

==========
cc: Make OutputSurfaceFrame::sub_buffer_rect optional

sub_buffer_rect is used to signal partial or empty swap, but previously
required a default value of full surface size to indicate regular
swap.  In preparation for adding swap with content bounds, clarify
the default case to not provide a value for sub_buffer_rect.

BUG=687690
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Make OutputSurfaceFrame::sub_buffer_rect optional

sub_buffer_rect is used to signal partial or empty swap, but previously
required a default value of full surface size to indicate regular
swap.  In preparation for adding swap with content bounds, clarify
the default case to not provide a value for sub_buffer_rect.

BUG=687690
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2699173002
Cr-Commit-Position: refs/heads/master@{#452234}
Committed:
https://chromium.googlesource.com/chromium/src/+/55723a7098ece4916d203986b5f7...
==========

[commit-bot: I haz the power, 2017-02-22 22:24:43]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/55723a7098ece4916d203986b5f7...