cc: Get rid of SurfaceDestroyed and use SurfaceDiscarded instead

cc: Get rid of SurfaceDestroyed and use SurfaceDiscarded instead

cc::Display was observing SurfaceDestroyed to pass along that information
to the display scheduler. SurfaceDestroyed gets called when a surface
is no longer used by the corresponding client, but not when it's
no longer reachable from the root.

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

[Fady Samuel, 2017-06-01 21:42:13]

Description was changed from

==========
cc: Get rid of SurfaceDestroyed and use SurfaceDiscarded instead

cc::Display was observing SurfaceDestroyed to pass along that information
to the display scheduler. SurfaceDestroyed gets called when a surface
is no longer used by the corresponding client, but not when it's
no longer reachable from the root.

BUG=none
==========

to

==========
cc: Get rid of SurfaceDestroyed and use SurfaceDiscarded instead

cc::Display was observing SurfaceDestroyed to pass along that information
to the display scheduler. SurfaceDestroyed gets called when a surface
is no longer used by the corresponding client, but not when it's
no longer reachable from the root.

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

[Fady Samuel, 2017-06-01 21:46:39]

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

[Fady Samuel, 2017-06-01 21:46:52]

fsamuel@chromium.org changed reviewers:
+ eseckler@chromium.org, sunnyps@chromium.org

[commit-bot: I haz the power, 2017-06-01 21:47:19]

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-06-01 22:43:05]

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

[commit-bot: I haz the power, 2017-06-01 22:43:06]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eric Seckler, 2017-06-02 11:55:03]

Should we add a call to SurfaceModified in
CompositorFrameSinkSupport::DestroyCurrentSurface(), so that the
DisplayScheduler doesn't continue to wait for an ack on a destroyed but not
garbage-collected surface? Such as:

void CompositorFrameSinkSupport::DestroyCurrentSurface() {
  if (last_begin_frame_args_.isValid()) {
    // Mark old surface inactive - damage will be submitted on new one.
    BeginFrameAck ack(last_begin_frame_args_.source_id(),
                      last_begin_frame_args_.sequence_number(),
                      BeginFrameArgs::kInvalidSequenceNumber, false);
    surface_manager_->SurfaceModified(current_surface_->surface_id(), ack);
  }
  surface_manager_->DestroySurface(std::move(current_surface_));
}

[Eric Seckler, 2017-06-02 12:01:17]

https://codereview.chromium.org/2918883002/diff/20001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/2918883002/diff/20001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler.cc:140: DCHECK(!base::ContainsKey(surface_states_,
surface_id));
Hm, and it looks like this is no longer necessarily true. Probably need to get
rid of the DCHECK (and maybe replace it with a comment).

The DCHECK is triggered here:
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2...

Log snippet:
[ RUN      ] ExtensionApiTabTest.Tabs2
[...]
[15112:771:0601/152703.002830:FATAL:display_scheduler.cc(140)] Check failed:
!base::ContainsKey(surface_states_, surface_id).
0   browser_tests                       0x000000010524bc2c
base::debug::StackTrace::StackTrace(unsigned long) + 28
1   browser_tests                       0x00000001052710b0
logging::LogMessage::~LogMessage() + 224
2   browser_tests                       0x00000001079c31a8
cc::DisplayScheduler::SurfaceCreated(cc::SurfaceInfo const&) + 152
3   browser_tests                       0x00000001079e0683
cc::SurfaceManager::SurfaceCreated(cc::SurfaceInfo const&) + 275
4   browser_tests                       0x00000001079be331
cc::CompositorFrameSinkSupport::OnSurfaceActivated(cc::Surface*) + 241

[sunnyps, 2017-06-02 17:28:41]

We need a way to tell display scheduler not to expect damage from the surface. I
don't have a strong opinion on if there should be a separate callback or if we
should use OnSurfaceModified with ack.has_damage = false.