android_webview: Remove dependency on webkit's ContextProviderInProcess.

android_webview: Remove dependency on webkit's ContextProviderInProcess.

Instead build our own simplified version, so we don't need to depend on
webkit/common/gpu.

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 android_webview_apk android_webview_test_apk
$ export CHROMIUM_OUT_DIR=out_android
$ 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/29c37c903e8f1d153fe2a77cebb972096880c386
Cr-Commit-Position: refs/heads/master@{#326968}

[tfarina, 2015-04-13 22:52:28]

Hi Bo, could you review this to me and help me get out all the details of
GrContext in CPIP correct?

Hopefully the diff does not look a mess and really reflects the changes I made
to get this compiling.

https://codereview.chromium.org/1083843002/diff/1/android_webview/browser/con...
File android_webview/browser/context_provider_in_process.cc (right):

https://codereview.chromium.org/1083843002/diff/1/android_webview/browser/con...
android_webview/browser/context_provider_in_process.cc:142: skia::RefPtr<class
SkGLContext> gl_context =
I copied this from cc/test/test_context_provide.cc

[boliu, 2015-04-14 00:29:52]

I spent half an hour trying to wrap my ahead around the dependencies (this stuff
is complicated!!)

So this duplicating the ContextProvider class is actually an *alternative* to
the original SetCreateContextCallback plan, ie if we decide to duplicate this
class, then we wouldn't need SetCreateContextCallback at all.

Here's the alternative plan:
* android_webview will have a separate implementation for cc::ContextProvider.
This implementation will only have features that's required by android_webview
(you are essentially starting this step in this CL). After this step,
android_webview should no longer depend on anything from webkit/
* Move what's currently in webkit/common/gpu to content/common/gpu (or
content/browser/android/in_process), like the cross-process ones.


Neither plan is perfect. I think both plans will involve duplicating some code,
but the original plan involved adding complex callbacks to content.

I'm not completely sold in either plan just yet. I want to talk to some other
people who are not at their desks at the moment. Can you hang on for a bit?

https://codereview.chromium.org/1083843002/diff/1/android_webview/browser/con...
File android_webview/browser/context_provider_in_process.cc (right):

https://codereview.chromium.org/1083843002/diff/1/android_webview/browser/con...
android_webview/browser/context_provider_in_process.cc:142: skia::RefPtr<class
SkGLContext> gl_context =
On 2015/04/13 22:52:28, tfarina wrote:
> I copied this from cc/test/test_context_provide.cc

You are creating a *new* context for GrContext, but it should be returning
*this* context, if that makes sense..

[boliu, 2015-04-14 21:45:46]

Ok, going with the "alternative plan", ie you can continue on this patch.

I'd recommend a clean implementation of cc::ContextProvider (*not*
cc_blink::ContextProviderWebContext), because a lot of functionality in the
existing ContextProviderInProcess is not needed. For GrContext, you can use
skia_bindings::CreateCommandBufferSkiaGLBinding. The new implementation should
not involve WebGraphicsContext3D.

[tfarina, 2015-04-17 20:44:04]

Bo, fyi, I'm sleeping on this.

Hopefully, I will be able to work on this on the weekend.

Regards,

[tfarina, 2015-04-19 02:44:25]

Patchset #3 (id:40001) has been deleted

[tfarina, 2015-04-19 02:46:34]

Bo, could you take a look at patch set 3 and answer the two questions below?

I don't want to proceed without knowing their answers.

Thanks in advance!

https://codereview.chromium.org/1083843002/diff/60001/android_webview/browser...
File android_webview/browser/context_provider_in_process.cc (left):

https://codereview.chromium.org/1083843002/diff/60001/android_webview/browser...
android_webview/browser/context_provider_in_process.cc:94: gfx::GpuPreference
gpu_preference = gfx::PreferDiscreteGpu;
What should goes in, in our implementation?

https://codereview.chromium.org/1083843002/diff/60001/android_webview/browser...
File android_webview/browser/context_provider_in_process.cc (right):

https://codereview.chromium.org/1083843002/diff/60001/android_webview/browser...
android_webview/browser/context_provider_in_process.cc:41:
scoped_refptr<ContextProviderInProcess> ContextProviderInProcess::Create(
Which parameters should I pass here?

[boliu, 2015-04-19 08:40:08]

https://codereview.chromium.org/1083843002/diff/60001/android_webview/browser...
File android_webview/browser/context_provider_in_process.cc (left):

https://codereview.chromium.org/1083843002/diff/60001/android_webview/browser...
android_webview/browser/context_provider_in_process.cc:94: gfx::GpuPreference
gpu_preference = gfx::PreferDiscreteGpu;
On 2015/04/19 02:46:33, tfarina wrote:
> What should goes in, in our implementation?

Create a GLInProcessContext and use that directly.

I didn't know InProcessContextProvider existed. That's indeed a better starting
point than the one under webkit/.

https://codereview.chromium.org/1083843002/diff/60001/android_webview/browser...
File android_webview/browser/context_provider_in_process.cc (right):

https://codereview.chromium.org/1083843002/diff/60001/android_webview/browser...
android_webview/browser/context_provider_in_process.cc:41:
scoped_refptr<ContextProviderInProcess> ContextProviderInProcess::Create(
On 2015/04/19 02:46:33, tfarina wrote:
> Which parameters should I pass here?

You can hardcode all of these values I think. There's only HardwareRenderer uses
the new ContextProvider, so no need for customization at all at this point.

[tfarina, 2015-04-19 13:29:54]

Bo, please, see if I got the creation of GLInProcess right. I have had to pass
two parameters to ContextProviderInProcess.

PTAL.

https://codereview.chromium.org/1083843002/diff/60001/android_webview/browser...
File android_webview/browser/context_provider_in_process.cc (left):

https://codereview.chromium.org/1083843002/diff/60001/android_webview/browser...
android_webview/browser/context_provider_in_process.cc:94: gfx::GpuPreference
gpu_preference = gfx::PreferDiscreteGpu;
On 2015/04/19 08:40:07, boliu wrote:
> On 2015/04/19 02:46:33, tfarina wrote:
> > What should goes in, in our implementation?
> 
> Create a GLInProcessContext and use that directly.
> 
> I didn't know InProcessContextProvider existed. That's indeed a better
starting
> point than the one under webkit/.

Done.

https://codereview.chromium.org/1083843002/diff/60001/android_webview/browser...
File android_webview/browser/context_provider_in_process.cc (right):

https://codereview.chromium.org/1083843002/diff/60001/android_webview/browser...
android_webview/browser/context_provider_in_process.cc:41:
scoped_refptr<ContextProviderInProcess> ContextProviderInProcess::Create(
On 2015/04/19 08:40:07, boliu wrote:
> On 2015/04/19 02:46:33, tfarina wrote:
> > Which parameters should I pass here?
> 
> You can hardcode all of these values I think. There's only HardwareRenderer
uses
> the new ContextProvider, so no need for customization at all at this point.

Done.

[boliu, 2015-04-20 10:06:05]

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
File android_webview/browser/context_provider_in_process.cc (right):

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
android_webview/browser/context_provider_in_process.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
android_webview/browser/context_provider_in_process.cc:107:
context_->GetImplementation()->TraceBeginCHROMIUM(
Don't enable tracing. That hurts performance and should only be used in tests,
probably. Then you can remove debug_name_

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
android_webview/browser/context_provider_in_process.cc:160: void
ContextProviderInProcess::SetupLock() {
context_->SetLock(&context_lock_);

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
android_webview/browser/context_provider_in_process.cc:175: }
Hmm, nothing in cc calls VerifyContexts. There's only one caller outside of cc,
which should be refactored, then this method can be removed from ContextProvider
altogether.

In the meanwhile, ok to leave this empty.

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
File android_webview/browser/context_provider_in_process.h (right):

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
android_webview/browser/context_provider_in_process.h:28: class
ContextProviderInProcess : public cc::ContextProvider {
Call this a different name to avoid confusion, maybe
AwRenderThreadContextProvider

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
android_webview/browser/context_provider_in_process.h:63: base::ThreadChecker
context_thread_checker_;
We can assume main_thread and context_thread is always the same. So only need
one thread_checker, and don't need destroyed_lock_.

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
android_webview/browser/context_provider_in_process.h:66:
scoped_refptr<gpu::InProcessCommandBuffer::Service> service_;
You can create GLInProcessContext in the constructor, so no need to hold these
variables.

[tfarina, 2015-04-22 02:09:05]

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
File android_webview/browser/context_provider_in_process.cc (right):

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
android_webview/browser/context_provider_in_process.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2015/04/20 10:06:05, boliu wrote:
> 2015

Done.

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
android_webview/browser/context_provider_in_process.cc:107:
context_->GetImplementation()->TraceBeginCHROMIUM(
On 2015/04/20 10:06:05, boliu wrote:
> Don't enable tracing. That hurts performance and should only be used in tests,
> probably. Then you can remove debug_name_

Done.

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
android_webview/browser/context_provider_in_process.cc:160: void
ContextProviderInProcess::SetupLock() {
On 2015/04/20 10:06:05, boliu wrote:
> context_->SetLock(&context_lock_);

Done.

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
android_webview/browser/context_provider_in_process.cc:175: }
On 2015/04/20 10:06:05, boliu wrote:
> Hmm, nothing in cc calls VerifyContexts. There's only one caller outside of
cc,
> which should be refactored, then this method can be removed from
ContextProvider
> altogether.
> 
> In the meanwhile, ok to leave this empty.

Done.

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
File android_webview/browser/context_provider_in_process.h (right):

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
android_webview/browser/context_provider_in_process.h:28: class
ContextProviderInProcess : public cc::ContextProvider {
On 2015/04/20 10:06:05, boliu wrote:
> Call this a different name to avoid confusion, maybe
> AwRenderThreadContextProvider

Done.

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
android_webview/browser/context_provider_in_process.h:63: base::ThreadChecker
context_thread_checker_;
On 2015/04/20 10:06:05, boliu wrote:
> We can assume main_thread and context_thread is always the same. So only need
> one thread_checker, and don't need destroyed_lock_.

Done.

https://codereview.chromium.org/1083843002/diff/80001/android_webview/browser...
android_webview/browser/context_provider_in_process.h:66:
scoped_refptr<gpu::InProcessCommandBuffer::Service> service_;
On 2015/04/20 10:06:05, boliu wrote:
> You can create GLInProcessContext in the constructor, so no need to hold these
> variables.

Done.

[boliu, 2015-04-22 07:59:18]

https://codereview.chromium.org/1083843002/diff/140001/android_webview/browse...
File android_webview/browser/aw_render_thread_context_provider.cc (right):

https://codereview.chromium.org/1083843002/diff/140001/android_webview/browse...
android_webview/browser/aw_render_thread_context_provider.cc:91:
capabilities_.gpu = context_->GetImplementation()->capabilities();
These two lines can move to construtor too. This method can just be a DCHECK and
no-op.

https://codereview.chromium.org/1083843002/diff/140001/android_webview/browse...
android_webview/browser/aw_render_thread_context_provider.cc:155: base::AutoLock
lock(context_lock_);
Should never have to directly lock context_lock_ in this class. Remove all of
them.

[tfarina, 2015-04-23 12:07:51]

https://codereview.chromium.org/1083843002/diff/140001/android_webview/browse...
File android_webview/browser/aw_render_thread_context_provider.cc (right):

https://codereview.chromium.org/1083843002/diff/140001/android_webview/browse...
android_webview/browser/aw_render_thread_context_provider.cc:91:
capabilities_.gpu = context_->GetImplementation()->capabilities();
On 2015/04/22 07:59:18, boliu wrote:
> These two lines can move to construtor too. This method can just be a DCHECK
and
> no-op.

Done.

https://codereview.chromium.org/1083843002/diff/140001/android_webview/browse...
android_webview/browser/aw_render_thread_context_provider.cc:155: base::AutoLock
lock(context_lock_);
On 2015/04/22 07:59:18, boliu wrote:
> Should never have to directly lock context_lock_ in this class. Remove all of
> them.

Done.

[boliu, 2015-04-23 12:23:42]

https://codereview.chromium.org/1083843002/diff/160001/android_webview/androi...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/1083843002/diff/160001/android_webview/androi...
android_webview/android_webview.gyp:205: 'browser/aw_content_browser_client.h',
were you intentionally sorting this, or was presubmit complaining? If it's not
presubmit, can you do the sorting in a separate CL?

https://codereview.chromium.org/1083843002/diff/160001/android_webview/browse...
File android_webview/browser/aw_render_thread_context_provider.cc (right):

https://codereview.chromium.org/1083843002/diff/160001/android_webview/browse...
android_webview/browser/aw_render_thread_context_provider.cc:190: {
Now the the lock is removed, this block is not serving any pupose anymore, just
remove it as well

[boliu, 2015-04-23 12:24:41]

https://codereview.chromium.org/1083843002/diff/160001/android_webview/browse...
File android_webview/browser/aw_render_thread_context_provider.cc (right):

https://codereview.chromium.org/1083843002/diff/160001/android_webview/browse...
android_webview/browser/aw_render_thread_context_provider.cc:190: {
On 2015/04/23 12:23:41, boliu wrote:
> Now the the lock is removed, this block is not serving any pupose anymore,
just
> remove it as well

Maybe 'scope' is a better word..

[tfarina, 2015-04-24 15:04:55]

https://codereview.chromium.org/1083843002/diff/160001/android_webview/androi...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/1083843002/diff/160001/android_webview/androi...
android_webview/android_webview.gyp:205: 'browser/aw_content_browser_client.h',
On 2015/04/23 12:23:41, boliu wrote:
> were you intentionally sorting this, or was presubmit complaining? If it's not
> presubmit, can you do the sorting in a separate CL?

Done.

https://codereview.chromium.org/1083843002/diff/160001/android_webview/browse...
File android_webview/browser/aw_render_thread_context_provider.cc (right):

https://codereview.chromium.org/1083843002/diff/160001/android_webview/browse...
android_webview/browser/aw_render_thread_context_provider.cc:190: {
On 2015/04/23 12:23:41, boliu wrote:
> Now the the lock is removed, this block is not serving any pupose anymore,
just
> remove it as well

Done.

[boliu, 2015-04-24 15:36:54]

https://codereview.chromium.org/1083843002/diff/160001/android_webview/browse...
File android_webview/browser/aw_render_thread_context_provider.cc (right):

https://codereview.chromium.org/1083843002/diff/160001/android_webview/browse...
android_webview/browser/aw_render_thread_context_provider.cc:190: {
On 2015/04/24 15:04:55, tfarina wrote:
> On 2015/04/23 12:23:41, boliu wrote:
> > Now the the lock is removed, this block is not serving any pupose anymore,
> just
> > remove it as well
> 
> Done.

Not what I meant. I mean just remove the braces, but keep |destroyed_|

So before:
{
  if (detroyed)
    return;
  destroyed_ = true;
}

After:
if (destroyed)
  return;
destroyed_ = true;

The scope was needed for the lock previously, but don't need it anymore since
the lock is removed.

[tfarina, 2015-04-24 17:25:43]

https://codereview.chromium.org/1083843002/diff/160001/android_webview/browse...
File android_webview/browser/aw_render_thread_context_provider.cc (right):

https://codereview.chromium.org/1083843002/diff/160001/android_webview/browse...
android_webview/browser/aw_render_thread_context_provider.cc:190: {
On 2015/04/24 15:36:54, boliu wrote:
> On 2015/04/24 15:04:55, tfarina wrote:
> > On 2015/04/23 12:23:41, boliu wrote:
> > > Now the the lock is removed, this block is not serving any pupose anymore,
> > just
> > > remove it as well
> > 
> > Done.
> 
> Not what I meant. I mean just remove the braces, but keep |destroyed_|
> 
> So before:
> {
>   if (detroyed)
>     return;
>   destroyed_ = true;
> }
> 
> After:
> if (destroyed)
>   return;
> destroyed_ = true;
> 
> The scope was needed for the lock previously, but don't need it anymore since
> the lock is removed.

Done.

[boliu, 2015-04-24 22:21:36]

lgtm

[tfarina, 2015-04-25 10:58:03]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2015-04-25 10:58:12]

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

[commit-bot: I haz the power, 2015-04-25 13:10:52]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2015-04-25 13:12:38]

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