LOG and clear SurfaceFactory if it isn't empty on destruction.

LOG and clear SurfaceFactory if it isn't empty on destruction.

Otherwise it will leave dangling pointers in the SurfaceManager. Also add a DestroyAll method to allow owners to ensure every surface in the factory is destroyed.

BUG=428127
Committed: https://crrev.com/fda1c1146ba1944abd2c032ef25cf2f4d35e49c0
Cr-Commit-Position: refs/heads/master@{#302339}

[jbauman, 2014-10-31 00:41:55]

jbauman@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2014-10-31 04:23:29]

https://codereview.chromium.org/693623004/diff/20001/cc/surfaces/surface_fact...
File cc/surfaces/surface_factory.cc (right):

https://codereview.chromium.org/693623004/diff/20001/cc/surfaces/surface_fact...
cc/surfaces/surface_factory.cc:21: DestroyAll();
DCHECK should enforce things that we know are true - if it fails then the code
isn't behaving as expected. it's weird to have paths for the dcheck-fail path.
if this is something we do expect to happen sometimes even when this code is
behaving correctly (but maybe some other piece of code is bogus) then we should
probably instead log an error and continue

https://codereview.chromium.org/693623004/diff/20001/cc/surfaces/surface_fact...
File cc/surfaces/surface_factory.h (right):

https://codereview.chromium.org/693623004/diff/20001/cc/surfaces/surface_fact...
cc/surfaces/surface_factory.h:8: #include <set>
leftover?

[jbauman, 2014-10-31 20:49:56]

On 2014/10/31 04:23:29, jamesr wrote:
>
https://codereview.chromium.org/693623004/diff/20001/cc/surfaces/surface_fact...
> File cc/surfaces/surface_factory.cc (right):
> 
>
https://codereview.chromium.org/693623004/diff/20001/cc/surfaces/surface_fact...
> cc/surfaces/surface_factory.cc:21: DestroyAll();
> DCHECK should enforce things that we know are true - if it fails then the code
> isn't behaving as expected. it's weird to have paths for the dcheck-fail path.
> if this is something we do expect to happen sometimes even when this code is
> behaving correctly (but maybe some other piece of code is bogus) then we
should
> probably instead log an error and continue

Ok, I was hoping that this would be useful in tests of other code, but changed
to a LOG instead.
> 
>
https://codereview.chromium.org/693623004/diff/20001/cc/surfaces/surface_fact...
> File cc/surfaces/surface_factory.h (right):
> 
>
https://codereview.chromium.org/693623004/diff/20001/cc/surfaces/surface_fact...
> cc/surfaces/surface_factory.h:8: #include <set>
> leftover?

Presubmit was sad unless I added this (IWYU) so figured might as well put it in.

[jamesr, 2014-10-31 20:54:09]

If you think that condition really should never be hit change it to a DCHECK,
but delete the code after it if so.  I think this way's correct.

lgtm

https://codereview.chromium.org/693623004/diff/40001/cc/surfaces/surface_fact...
File cc/surfaces/surface_factory.h (right):

https://codereview.chromium.org/693623004/diff/40001/cc/surfaces/surface_fact...
cc/surfaces/surface_factory.h:44: const std::set<SurfaceSequence>&
dependency_set);
oh i see, probably because of this line. i think it's right

[jbauman, 2014-10-31 21:55:39]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-10-31 21:56:32]

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

[commit-bot: I haz the power, 2014-10-31 22:46:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-31 22:47:17]

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