Remove return bool in SurfaceOzoneEGL::OnSwapBuffersAsync

Remove return bool in SurfaceOzoneEGL::OnSwapBuffersAsync

OnSwapBuffersAsync() takes a callback which will be called with the
status of the swap buffers call. The callback can be used in all cases,
so there is no need to have a return value to signal swap status in
certain cases.

BUG=none

Committed: https://crrev.com/02681f457a255a434c8b91f890a8ca647ac57630
Cr-Commit-Position: refs/heads/master@{#359323}

[dnicoara, 2015-11-11 18:34:08]

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

[commit-bot: I haz the power, 2015-11-11 18:34:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1436993002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1436993002/1

[commit-bot: I haz the power, 2015-11-11 19:10:25]

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

[commit-bot: I haz the power, 2015-11-11 19:10:26]

Dry run: This issue passed the CQ dry run.

[dnicoara, 2015-11-11 19:24:43]

dnicoara@chromium.org changed reviewers:
+ spang@chromium.org

[dnicoara, 2015-11-11 19:24:43]

PTAL

[spang, 2015-11-11 20:58:40]

https://codereview.chromium.org/1436993002/diff/1/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/1436993002/diff/1/ui/gl/gl_surface_ozone.cc#n...
ui/gl/gl_surface_ozone.cc:391: last_swap_buffers_result_ =
frame->ScheduleOverlayPlanes(widget_);
Shouldn't this be

if (!frame->ScheduleOverlayPlanes(widget_)) {
  SwapCompleted(frame->callback, gfx::SwapResult::SWAP_FAILED);
  return;
}

https://codereview.chromium.org/1436993002/diff/1/ui/gl/gl_surface_ozone.cc#n...
ui/gl/gl_surface_ozone.cc:415: last_swap_buffers_result_ &= result !=
gfx::SwapResult::SWAP_FAILED;
There's no reason to &, just assign.

if (result == gfx::SwapResult::SWAP_FAILED)
  last_swap_buffers_result = false;

[dnicoara, 2015-11-11 21:07:08]

https://codereview.chromium.org/1436993002/diff/1/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/1436993002/diff/1/ui/gl/gl_surface_ozone.cc#n...
ui/gl/gl_surface_ozone.cc:391: last_swap_buffers_result_ =
frame->ScheduleOverlayPlanes(widget_);
On 2015/11/11 20:58:40, spang wrote:
> Shouldn't this be
> 
> if (!frame->ScheduleOverlayPlanes(widget_)) {
>   SwapCompleted(frame->callback, gfx::SwapResult::SWAP_FAILED);
>   return;
> }

I was thinking about this, but if some overlays have been scheduled there's no
way to clear them in the native surface. So the next swap buffers will add even
more buffers. And if that manages to succeed, now swap buffers fails because you
have too many overlays. That'd be worse to debug.

IMO ScheduleOverlayPlanes() shouldn't return a value. The native surface can't
allocate hardware resources until OnSwapBuffersAsync() anyways.

[dnicoara, 2015-11-11 21:33:57]

https://codereview.chromium.org/1436993002/diff/1/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/1436993002/diff/1/ui/gl/gl_surface_ozone.cc#n...
ui/gl/gl_surface_ozone.cc:391: last_swap_buffers_result_ =
frame->ScheduleOverlayPlanes(widget_);
On 2015/11/11 20:58:40, spang wrote:
> Shouldn't this be
> 
> if (!frame->ScheduleOverlayPlanes(widget_)) {
>   SwapCompleted(frame->callback, gfx::SwapResult::SWAP_FAILED);
>   return;
> }

Done.

https://codereview.chromium.org/1436993002/diff/1/ui/gl/gl_surface_ozone.cc#n...
ui/gl/gl_surface_ozone.cc:415: last_swap_buffers_result_ &= result !=
gfx::SwapResult::SWAP_FAILED;
On 2015/11/11 20:58:40, spang wrote:
> There's no reason to &, just assign.
> 
> if (result == gfx::SwapResult::SWAP_FAILED)
>   last_swap_buffers_result = false;

Done.

[spang, 2015-11-12 01:34:07]

https://codereview.chromium.org/1436993002/diff/20001/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/1436993002/diff/20001/ui/gl/gl_surface_ozone....
ui/gl/gl_surface_ozone.cc:396:
ozone_surface_->OnSwapBuffersAsync(frame->callback);
I think you should return in the error path instead of putting the success path
inside else {}.

https://codereview.chromium.org/1436993002/diff/20001/ui/gl/gl_surface_ozone....
ui/gl/gl_surface_ozone.cc:423: SubmitFrame();
same comment, avoid putting success path flow into else {} when you could just
return

[spang, 2015-11-12 02:21:53]

lgtm

[dnicoara, 2015-11-12 15:16:52]

https://codereview.chromium.org/1436993002/diff/20001/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/1436993002/diff/20001/ui/gl/gl_surface_ozone....
ui/gl/gl_surface_ozone.cc:396:
ozone_surface_->OnSwapBuffersAsync(frame->callback);
On 2015/11/12 01:34:07, spang wrote:
> I think you should return in the error path instead of putting the success
path
> inside else {}.

Done.

https://codereview.chromium.org/1436993002/diff/20001/ui/gl/gl_surface_ozone....
ui/gl/gl_surface_ozone.cc:423: SubmitFrame();
On 2015/11/12 01:34:07, spang wrote:
> same comment, avoid putting success path flow into else {} when you could just
> return

Done.

[dnicoara, 2015-11-12 15:16:56]

The CQ bit was checked by dnicoara@chromium.org

[dnicoara, 2015-11-12 15:16:56]

The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1436993002/#ps40001
(title: ".")

[commit-bot: I haz the power, 2015-11-12 15:17:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1436993002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1436993002/40001

[commit-bot: I haz the power, 2015-11-12 15:34:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-12 20:07:38]

Patchset 3 (id:??) landed as
https://crrev.com/02681f457a255a434c8b91f890a8ca647ac57630
Cr-Commit-Position: refs/heads/master@{#359323}