Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1064263002:
android: Pass cc::ContextProvider to VideoContextProvider. (Closed)
|
Created:
5 years, 8 months ago by tfarina Modified:
5 years, 8 months ago Reviewers:
boliu CC:
chromium-reviews, darin-cc_chromium.org, jam, Avi (use Gerrit), darin (slow to review), jamesr, jochen (gone - plz use gerrit), pilgrim_google, piman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionandroid: Pass cc::ContextProvider to VideoContextProvider.
Instead of creating it under VideoContextProvider, which makes
refactoring this code harder, create it outside and pass it through
parameter.
This should help ease the transition of
https://codereview.chromium.org/920443003.
Tested on Android (with Nexus 5 device) with the following command
lines:
$ ./build/gyp_chromium -DOS=android -Goutput_dir=out_android
$ ninja -C out_android/Debug content_unittests content_unittests_apk
$ export CHROMIUM_OUT_DIR=out_android
$ build/android/test_runner.py gtest -s content_unittests --debug
AndroidWebView tests:
$ ninja -C out_android/Debug android_webview_apk android_webview_test_apk
$ build/android/adb_install_apk.py --apk=AndroidWebView.apk --debug
$ build/android/test_runner.py instrumentation --test-apk=AndroidWebViewTest
--test_data webview:android_webview/test/data/device_files --debug
BUG=338338
TEST=see above
R=boliu@chromium.org
Committed: https://crrev.com/3bc54dcf2b0d9f5e2b698af0f9061bba19a4bee1
Cr-Commit-Position: refs/heads/master@{#324309}
Patch Set 1 #
Total comments: 3
Patch Set 2 : pointer #
Total comments: 5
Patch Set 3 : context_ptr #
Total comments: 4
Patch Set 4 : attributes #Messages
Total messages: 14 (2 generated)
PTAL https://codereview.chromium.org/1064263002/diff/1/content/browser/android/in_... File content/browser/android/in_process/synchronous_compositor_factory_impl.cc (right): https://codereview.chromium.org/1064263002/diff/1/content/browser/android/in_... content/browser/android/in_process/synchronous_compositor_factory_impl.cc:271: context.Pass()); This second Pass() isn't really right, is it? Is there a better way to do this?
https://codereview.chromium.org/1064263002/diff/1/content/browser/android/in_... File content/browser/android/in_process/synchronous_compositor_factory_impl.cc (right): https://codereview.chromium.org/1064263002/diff/1/content/browser/android/in_... content/browser/android/in_process/synchronous_compositor_factory_impl.cc:271: context.Pass()); On 2015/04/07 23:16:27, tfarina wrote: > This second Pass() isn't really right, is it? Is there a better way to do this? Right, scoped_ptr can't be passed twice. The second time will just pass a nullptr. You can use a raw pointer: scoped_ptr<gpu::GLInProcessContext> context = ...; gpu::GLInProcessContext* context_pointer = context.get(); video_context_provider_ = new VideoContextProvider(..., context_pointer);
https://codereview.chromium.org/1064263002/diff/1/content/browser/android/in_... File content/browser/android/in_process/synchronous_compositor_factory_impl.cc (right): https://codereview.chromium.org/1064263002/diff/1/content/browser/android/in_... content/browser/android/in_process/synchronous_compositor_factory_impl.cc:271: context.Pass()); On 2015/04/08 03:26:08, boliu wrote: > On 2015/04/07 23:16:27, tfarina wrote: > > This second Pass() isn't really right, is it? Is there a better way to do > this? > > Right, scoped_ptr can't be passed twice. The second time will just pass a > nullptr. > > You can use a raw pointer: > > scoped_ptr<gpu::GLInProcessContext> context = ...; > gpu::GLInProcessContext* context_pointer = context.get(); > video_context_provider_ = new VideoContextProvider(..., context_pointer); Done.
https://codereview.chromium.org/1064263002/diff/20001/content/browser/android... File content/browser/android/in_process/synchronous_compositor_factory_impl.cc (right): https://codereview.chromium.org/1064263002/diff/20001/content/browser/android... content/browser/android/in_process/synchronous_compositor_factory_impl.cc:119: gpu::GLInProcessContext* gl_in_process_context_; Why the reorder? In theory, this would lead to a moment during destruction where gl_in_process_context_ is a dangling pointer, since member variables are destroyed in the reverse order they are declared. https://codereview.chromium.org/1064263002/diff/20001/content/browser/android... content/browser/android/in_process/synchronous_compositor_factory_impl.cc:267: context.get(), webkit::gpu::ContextProviderInProcess::Create( Just save the raw pointer in a local variable. This is really brittle if someone reorders the arguments in the future, and don't look closely at what this is doing.
https://codereview.chromium.org/1064263002/diff/20001/content/browser/android... File content/browser/android/in_process/synchronous_compositor_factory_impl.cc (right): https://codereview.chromium.org/1064263002/diff/20001/content/browser/android... content/browser/android/in_process/synchronous_compositor_factory_impl.cc:267: context.get(), webkit::gpu::ContextProviderInProcess::Create( On 2015/04/08 20:24:26, boliu wrote: > Just save the raw pointer in a local variable. This is really brittle if someone > reorders the arguments in the future, and don't look closely at what this is > doing. Unfortunately that didn't work very well with WrapContext, which takes a scoped_ptr, not a raw pointer. Am I missing something?
https://codereview.chromium.org/1064263002/diff/20001/content/browser/android... File content/browser/android/in_process/synchronous_compositor_factory_impl.cc (right): https://codereview.chromium.org/1064263002/diff/20001/content/browser/android... content/browser/android/in_process/synchronous_compositor_factory_impl.cc:267: context.get(), webkit::gpu::ContextProviderInProcess::Create( On 2015/04/08 20:26:25, tfarina wrote: > On 2015/04/08 20:24:26, boliu wrote: > > Just save the raw pointer in a local variable. This is really brittle if > someone > > reorders the arguments in the future, and don't look closely at what this is > > doing. > > Unfortunately that didn't work very well with WrapContext, which takes a > scoped_ptr, not a raw pointer. Am I missing something? This should work (ignore poor formatting): scoped_ptr<gpu::GLInProcessContext> context = CreateContextHolder(attributes, service_, gpu::GLInProcessContextSharedMemoryLimits(), false); gpu::GLInProcessContext* context_ptr = context.get(); video_context_provider_ = new VideoContextProvider( webkit::gpu::ContextProviderInProcess::Create( WrapContext(context.Pass(), GetDefaultAttribs()), "Video-Offscreen-main-thread"), context_ptr);
ptal https://codereview.chromium.org/1064263002/diff/20001/content/browser/android... File content/browser/android/in_process/synchronous_compositor_factory_impl.cc (right): https://codereview.chromium.org/1064263002/diff/20001/content/browser/android... content/browser/android/in_process/synchronous_compositor_factory_impl.cc:267: context.get(), webkit::gpu::ContextProviderInProcess::Create( On 2015/04/08 20:29:58, boliu wrote: > On 2015/04/08 20:26:25, tfarina wrote: > > On 2015/04/08 20:24:26, boliu wrote: > > > Just save the raw pointer in a local variable. This is really brittle if > > someone > > > reorders the arguments in the future, and don't look closely at what this is > > > doing. > > > > Unfortunately that didn't work very well with WrapContext, which takes a > > scoped_ptr, not a raw pointer. Am I missing something? > > This should work (ignore poor formatting): > > scoped_ptr<gpu::GLInProcessContext> context = > CreateContextHolder(attributes, service_, > gpu::GLInProcessContextSharedMemoryLimits(), false); > gpu::GLInProcessContext* context_ptr = context.get(); > video_context_provider_ = new VideoContextProvider( > webkit::gpu::ContextProviderInProcess::Create( > WrapContext(context.Pass(), GetDefaultAttribs()), > "Video-Offscreen-main-thread"), context_ptr); Cool! thanks. Done. https://codereview.chromium.org/1064263002/diff/40001/content/browser/android... File content/browser/android/in_process/synchronous_compositor_factory_impl.cc (right): https://codereview.chromium.org/1064263002/diff/40001/content/browser/android... content/browser/android/in_process/synchronous_compositor_factory_impl.cc:87: gpu::GLInProcessContext* gl_in_process_context) Making this a raw pointer was necessary to fix: ../../content/browser/android/in_process/synchronous_compositor_factory_impl.cc:268:35: error: no matching constructor for initialization of 'content::SynchronousCompositorFactoryImpl::VideoContextProvider' video_context_provider_ = new VideoContextProvider( ^ ../../content/browser/android/in_process/synchronous_compositor_factory_impl.cc:85:3: note: candidate constructor not viable: no known conversion from 'gpu::GLInProcessContext *' to 'scoped_ptr<gpu::GLInProcessContext>' for 2nd argument VideoContextProvider( ^ ../../content/browser/android/in_process/synchronous_compositor_factory_impl.cc:124:28: note: candidate constructor not viable: requires 1 argument, but 2 were provided DISALLOW_COPY_AND_ASSIGN(VideoContextProvider); ^ ../../base/macros.h:27:3: note: expanded from macro 'DISALLOW_COPY_AND_ASSIGN' TypeName(const TypeName&); \ ^ 1 error generated.
lgtm % last change https://codereview.chromium.org/1064263002/diff/40001/content/browser/android... File content/browser/android/in_process/synchronous_compositor_factory_impl.cc (right): https://codereview.chromium.org/1064263002/diff/40001/content/browser/android... content/browser/android/in_process/synchronous_compositor_factory_impl.cc:87: gpu::GLInProcessContext* gl_in_process_context) On 2015/04/08 20:56:46, tfarina wrote: > Making this a raw pointer was necessary to fix: > > ../../content/browser/android/in_process/synchronous_compositor_factory_impl.cc:268:35: > error: no matching constructor for initialization of > 'content::SynchronousCompositorFactoryImpl::VideoContextProvider' > video_context_provider_ = new VideoContextProvider( > ^ > ../../content/browser/android/in_process/synchronous_compositor_factory_impl.cc:85:3: > note: candidate constructor not viable: no known conversion from > 'gpu::GLInProcessContext *' to 'scoped_ptr<gpu::GLInProcessContext>' for 2nd > argument > VideoContextProvider( > ^ > ../../content/browser/android/in_process/synchronous_compositor_factory_impl.cc:124:28: > note: candidate constructor not viable: requires 1 argument, but 2 were provided > DISALLOW_COPY_AND_ASSIGN(VideoContextProvider); > ^ > ../../base/macros.h:27:3: note: expanded from macro 'DISALLOW_COPY_AND_ASSIGN' > TypeName(const TypeName&); \ > ^ > 1 error generated. Yep that's fine. https://codereview.chromium.org/1064263002/diff/40001/content/browser/android... content/browser/android/in_process/synchronous_compositor_factory_impl.cc:270: WrapContext(context.Pass(), GetDefaultAttribs()), reuse |attributes| here
Thanks for helping find a solution for this. https://codereview.chromium.org/1064263002/diff/40001/content/browser/android... File content/browser/android/in_process/synchronous_compositor_factory_impl.cc (right): https://codereview.chromium.org/1064263002/diff/40001/content/browser/android... content/browser/android/in_process/synchronous_compositor_factory_impl.cc:270: WrapContext(context.Pass(), GetDefaultAttribs()), On 2015/04/08 20:59:33, boliu wrote: > reuse |attributes| here Done.
The CQ bit was checked by tfarina@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org Link to the patchset: https://codereview.chromium.org/1064263002/#ps20002 (title: "attributes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1064263002/20002
Message was sent while issue was closed.
Committed patchset #4 (id:20002)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3bc54dcf2b0d9f5e2b698af0f9061bba19a4bee1 Cr-Commit-Position: refs/heads/master@{#324309} |
