Refactor GpuDataManagerImpl to make it thread-safe.

Refactor GpuDataManagerImpl to make it thread-safe.

Try to reland r199530 after fixing an issue that causes Mac Debug to crash when switching GPUs.

The original review can be found here: https://codereview.chromium.org/14794006/

BUG=232556
TBR=joi@chromium.org, kbr@chromium.org, piman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200086

[Zhenyao Mo, 2013-05-14 01:26:46]

Tested on a dual-gpu mac debug.  patch set 1 caused browser crash, and patch set
2 doesn't.

You only need to review the diff between set 1 and set 2.

[Zhenyao Mo, 2013-05-14 02:08:10]

On 2013/05/14 01:26:46, Zhenyao Mo wrote:
> Tested on a dual-gpu mac debug.  patch set 1 caused browser crash, and patch
set
> 2 doesn't.
> 
> You only need to review the diff between set 1 and set 2.

Also tested on Release, no crash with gpu switching.

Ken: please review.

[piman, 2013-05-14 02:20:57]

lgtm

[Ken Russell (switch to Gerrit), 2013-05-14 02:53:05]

zmo@ and I talked about this offline and there's a problem with this patch --
the owning GpuDataManagerImpl will always be NULL when the display reconfig
callback is registered.

zmo's going to refactor things so the GpuDataManagerImpl is passed into the
constructor of the GpuDataManagerImplPrivate to make it impossible for it to be
NULL while the Private object is alive.

[Ken Russell (switch to Gerrit), 2013-05-14 20:09:13]

LGTM with a couple of comments.

https://codereview.chromium.org/14981007/diff/35001/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/14981007/diff/35001/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:83: if (manager)
This should be left as a DCHECK.

https://codereview.chromium.org/14981007/diff/35001/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private_unittest.cc (right):

https://codereview.chromium.org/14981007/diff/35001/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private_unittest.cc:310:
ScopedGpuDataManagerImpl manager;
It's confusing that some of these tests test the GpuDataManagerImpl and some
test the GpuDataManagerImplPrivate. I think it should be split into two test
files, gpu_data_manager_impl_unittest.cc and
gpu_data_manager_impl_private_unittest.cc.

[Jói, 2013-05-14 20:41:05]

LGTM for //content/public.

[Zhenyao Mo, 2013-05-14 21:56:31]

https://codereview.chromium.org/14981007/diff/35001/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/14981007/diff/35001/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private.cc:83: if (manager)
On 2013/05/14 20:09:13, kbr wrote:
> This should be left as a DCHECK.

Done.

https://codereview.chromium.org/14981007/diff/35001/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl_private_unittest.cc (right):

https://codereview.chromium.org/14981007/diff/35001/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl_private_unittest.cc:310:
ScopedGpuDataManagerImpl manager;
On 2013/05/14 20:09:13, kbr wrote:
> It's confusing that some of these tests test the GpuDataManagerImpl and some
> test the GpuDataManagerImplPrivate. I think it should be split into two test
> files, gpu_data_manager_impl_unittest.cc and
> gpu_data_manager_impl_private_unittest.cc.

Agree.  I'll do this as a separate CL because it needs some careful study that
which tests can be run using GpuDataManagerImpl and which ones require
GpuDataManagerImplPrivate.

[Zhenyao Mo, 2013-05-14 22:13:24]

Committed patchset #5 manually as r200086 (presubmit successful).