Move InitializeOnCurrentThread down from WebGraphicsContext3DImpl to WebGraphicsContext3DCommandBuf…

Move InitializeOnCurrentThread down from WebGraphicsContext3DImpl to WebGraphicsContext3DCommandBufferImpl and WebGraphicsContext3DInProcessCommandBufferImpl

in preparation for dis-inheriting these classes

BUG=338338
TBR=darin@chromium.org

Committed: https://crrev.com/7db0709dc9a1622c95c42ff349561da528e905b2
Cr-Commit-Position: refs/heads/master@{#297362}

[pilgrim_google, 2014-09-26 19:55:03]

pilgrim@chromium.org changed reviewers:
+ darin@chromium.org, jamesr@chromium.org

[jamesr, 2014-09-26 20:05:40]

https://codereview.chromium.org/609973002/diff/1/content/common/gpu/client/we...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/609973002/diff/1/content/common/gpu/client/we...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:145: //
former WebGraphicsContext3DImpl methods
comment's not useful. history lives in the revision control system

https://codereview.chromium.org/609973002/diff/1/content/common/gpu/client/we...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:146:
virtual bool InitializeOnCurrentThread();
why virtual?

https://codereview.chromium.org/609973002/diff/1/webkit/common/gpu/webgraphic...
File webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h
(right):

https://codereview.chromium.org/609973002/diff/1/webkit/common/gpu/webgraphic...
webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h:64:
virtual bool InitializeOnCurrentThread();
why is this virtual?

[pilgrim_google, 2014-09-29 17:47:00]

all green now

[jamesr, 2014-09-29 18:00:57]

https://codereview.chromium.org/609973002/diff/120001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/609973002/diff/120001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:145:
CONTENT_EXPORT bool InitializeOnCurrentThread();
what's the export for? all the callers I can see are in content/

https://codereview.chromium.org/609973002/diff/120001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/609973002/diff/120001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:962: }
this is not correct.

in the previous code on OS_ANDROID if
SynchronousCompositorFactory::GetInstance() returns NULL then the code falls
through and initializes the context using WGC3DCBI::CreateOffscreenContext().

With your patch, on android the *only* way to get an actual context is via
SynchronousCompositorFactory, which is only set for android_webview, so this
patch will (in practice) disable webgl completely for Chrome on android.

[pilgrim_google, 2014-09-29 21:17:40]

On 2014/09/29 18:00:57, jamesr wrote:
>
https://codereview.chromium.org/609973002/diff/120001/content/common/gpu/clie...
> File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
> (right):
> 
>
https://codereview.chromium.org/609973002/diff/120001/content/common/gpu/clie...
> content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:145:
> CONTENT_EXPORT bool InitializeOnCurrentThread();
> what's the export for? all the callers I can see are in content/

If I remove it, I get this in android_clang_dbg trybot:

FAILED: if [ ! -e lib/libcontent_browsertests.cr.so -o ! -e
lib/libcontent_browsertests.cr.so.TOC ]; then
/mnt/scratch0/b_used/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
-Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,noexecstack -fPIC -fuse-ld=gold
--gcc-toolchain=/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/..
-nostdlib -Wl,--no-undefined -target arm-linux-androideabi
--sysroot=/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm
-Wl,--icf=safe
-L/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//sources/cxx-stl/stlport/libs/armeabi-v7a
-Wl,-shared,-Bsymbolic
/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/usr/lib/crtbegin_so.o
-Wl,--warn-shared-textrel -Wl,-O1 -Wl,--as-needed -o
lib/libcontent_browsertests.cr.so -Wl,-soname=libcontent_browsertests.cr.so
@lib/libcontent_browsertests.cr.so.rsp && { readelf -d
lib/libcontent_browsertests.cr.so | grep SONAME ; nm -gD -f p
lib/libcontent_browsertests.cr.so | cut -f1-2 -d' '; } >
lib/libcontent_browsertests.cr.so.TOC; else
/mnt/scratch0/b_used/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
-Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,noexecstack -fPIC -fuse-ld=gold
--gcc-toolchain=/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/..
-nostdlib -Wl,--no-undefined -target arm-linux-androideabi
--sysroot=/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm
-Wl,--icf=safe
-L/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//sources/cxx-stl/stlport/libs/armeabi-v7a
-Wl,-shared,-Bsymbolic
/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/usr/lib/crtbegin_so.o
-Wl,--warn-shared-textrel -Wl,-O1 -Wl,--as-needed -o
lib/libcontent_browsertests.cr.so -Wl,-soname=libcontent_browsertests.cr.so
@lib/libcontent_browsertests.cr.so.rsp && { readelf -d
lib/libcontent_browsertests.cr.so | grep SONAME ; nm -gD -f p
lib/libcontent_browsertests.cr.so | cut -f1-2 -d' '; } >
lib/libcontent_browsertests.cr.so.tmp && if ! cmp -s
lib/libcontent_browsertests.cr.so.tmp lib/libcontent_browsertests.cr.so.TOC;
then mv lib/libcontent_browsertests.cr.so.tmp
lib/libcontent_browsertests.cr.so.TOC ; fi; fi
obj/content/browser/gpu/content_browsertests.gpu_ipc_browsertests.o:../../content/browser/gpu/gpu_ipc_browsertests.cc:function
(anonymous namespace)::ContextTestBase::SetUpOnMainThread(): error: undefined
reference to
'content::WebGraphicsContext3DCommandBufferImpl::InitializeOnCurrentThread()'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.


> 
>
https://codereview.chromium.org/609973002/diff/120001/content/renderer/render...
> File content/renderer/renderer_webkitplatformsupport_impl.cc (right):
> 
>
https://codereview.chromium.org/609973002/diff/120001/content/renderer/render...
> content/renderer/renderer_webkitplatformsupport_impl.cc:962: }
> this is not correct.
> 
> in the previous code on OS_ANDROID if
> SynchronousCompositorFactory::GetInstance() returns NULL then the code falls
> through and initializes the context using WGC3DCBI::CreateOffscreenContext().
> 
> With your patch, on android the *only* way to get an actual context is via
> SynchronousCompositorFactory, which is only set for android_webview, so this
> patch will (in practice) disable webgl completely for Chrome on android.

Should be fixed now.

[jamesr, 2014-09-29 21:22:31]

lgtm w/ one cleanup suggested.

https://codereview.chromium.org/609973002/diff/160001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/609973002/diff/160001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:961:
in_process_context.reset(
no need to separate declaration of the variable from initializing. these two
lines can just be

scoped_ptr<.....> in_process_context(factory->Create...);

https://codereview.chromium.org/609973002/diff/160001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:977:
context.reset(WebGraphicsContext3DCommandBufferImpl::CreateOffscreenContext(
ditto, this can just be

scoped_ptr<...> context = ...::CreateOffscreen()

[pilgrim_google, 2014-09-29 21:57:11]

The CQ bit was checked by pilgrim@chromium.org

[pilgrim_google, 2014-09-29 21:58:04]

The CQ bit was unchecked by pilgrim@chromium.org

[pilgrim_google, 2014-09-29 21:59:31]

pilgrim@chromium.org changed reviewers:
+ sievers@chromium.org

[pilgrim_google, 2014-09-29 21:59:31]

+sievers refactoring some pretty low level graphics stuff that touches an
android-only code path. please review.

[no sievers, 2014-09-30 02:01:42]

On 2014/09/29 21:59:31, pilgrim_google wrote:
> +sievers refactoring some pretty low level graphics stuff that touches an
> android-only code path. please review.

android change lgtm. That always uses the in-process thingie.

[pilgrim_google, 2014-09-30 02:57:35]

The CQ bit was checked by pilgrim@chromium.org

[commit-bot: I haz the power, 2014-09-30 03:05:21]

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

[commit-bot: I haz the power, 2014-09-30 05:01:09]

Committed patchset #10 (id:180001) as 72498f86d715071c2f209d4e891d6e70fa4a256a

[commit-bot: I haz the power, 2014-09-30 05:02:10]

Patchset 10 (id:??) landed as
https://crrev.com/7db0709dc9a1622c95c42ff349561da528e905b2
Cr-Commit-Position: refs/heads/master@{#297362}