Android WebView: support building against system skia.

Android WebView: support building against system skia.

Hook up the use_system_skia gyp flag to make it possible to try building
against the system version of Skia in the Android WebView.

BUG=
R=djsollen@google.com, mkosiba@chromium.org, robertphillips@google.com

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

[Torne, 2013-08-06 15:46:14]

Hi Derek, Robert,

This is the basic gyp modification to build WebView against system skia (not
actually enabled by this CL; I've been setting GYP_DEFINES to test it). It
doesn't actually build successfully with this enabled, but it's close! I sent a
CL to fix one of the issues already (https://codereview.chromium.org/22407002/)
but there are some others that I'd like advice on (though these needn't block
landing this CL necessarily?):

1) third_party/WebKit/Source/core/platform/graphics/GraphicsContext3D.cpp uses
#include "GrGLInterface.h" which doesn't work with the system skia as the
exported include paths only have include/gpu, not include/gpu/gl. Should we
change the Android skia to export include/gpu/gl, or should we just change this
include to gl/GrGLInterface.h which seems to be what skia does internally? (in
which case the Chromium gyp files probably don't need to have include/gpu/gl in
include_dirs)

2) There's a mismatch in #defines between Chromium and the system skia: chromium
expects GR_GL_PER_GL_FUNC_CALLBACK to be defined but this doesn't get set by
anything in the Android build. I'm not sure if this is just that the Android
system headers need to define more flags, or whether there's actually a real
mismatch here? This is really a more general question: how do we ensure that the
defines used to parse the skia headers match between the skia build and the
chromium build? Chromium normally just sets a bunch directly in gyp, which
doesn't really work for use_system_skia. Ideally I'd expect the relevant defines
to all be set in the user config .h in the system skia such that no custom
defines need to be set in chromium?

[djsollen, 2013-08-07 14:41:56]

#1 update GraphicsContext3D to include gl/
#2 we need to define that in Android as well.

https://codereview.chromium.org/22435002/diff/1/skia/skia_system.gypi
File skia/skia_system.gypi (right):

https://codereview.chromium.org/22435002/diff/1/skia/skia_system.gypi#newcode20
skia/skia_system.gypi:20: 'animator/SkAnimatorView.h',
robert, how feasible is it to create/maintain gypi files in Skia that have all
our public includes?

torne, can we update the python behind shim_headers.gypi to take just the
directory and generate filenames for all .h files there?

https://codereview.chromium.org/22435002/diff/1/skia/skia_system.gypi#newcode280
skia/skia_system.gypi:280: 'views/SkSystemEventTypes.h',
chrome doesn't use the views or animator headers.

[Torne, 2013-08-07 14:49:44]

On 2013/08/07 14:41:56, djsollen wrote:
> #1 update GraphicsContext3D to include gl/

OK, I'll push a CL through to do that on the blink side.

> #2 we need to define that in Android as well.

What other defines might we need that are currently out of sync between Chromium
and Android? If you define it in Android, will it be defined in the WebView
build without me changing anything?

> https://codereview.chromium.org/22435002/diff/1/skia/skia_system.gypi
> File skia/skia_system.gypi (right):
> 
>
https://codereview.chromium.org/22435002/diff/1/skia/skia_system.gypi#newcode20
> skia/skia_system.gypi:20: 'animator/SkAnimatorView.h',
> robert, how feasible is it to create/maintain gypi files in Skia that have all
> our public includes?

This would be preferable if you can do this :)

> torne, can we update the python behind shim_headers.gypi to take just the
> directory and generate filenames for all .h files there?

Not really, because this is supposed to work in an environment where the sources
for skia are not actually present - linux distros like to delete the duplicated
sources for libraries that they are using the system versions of, and in the
WebView I'd prefer not to have to maintain/merge/check out the skia sources in
Android just to be able to list the headers. Not having the real headers
physically present also is a good sanity check to make sure they are not
included by accident..

>
https://codereview.chromium.org/22435002/diff/1/skia/skia_system.gypi#newcode280
> skia/skia_system.gypi:280: 'views/SkSystemEventTypes.h',
> chrome doesn't use the views or animator headers.

If chromium doesn't need the whole public API then feel free to leave the unused
parts out of the list if you move it to skia, but I just generated it with find
-name \*.h

[robertphillips, 2013-08-07 15:26:19]

https://codereview.chromium.org/22435002/diff/1/skia/skia_system.gypi
File skia/skia_system.gypi (right):

https://codereview.chromium.org/22435002/diff/1/skia/skia_system.gypi#newcode20
skia/skia_system.gypi:20: 'animator/SkAnimatorView.h',
Perhaps we can automatically generate the file but keep it on our side?

[Torne, 2013-08-07 15:27:57]

On 2013/08/07 15:26:19, robertphillips wrote:
> https://codereview.chromium.org/22435002/diff/1/skia/skia_system.gypi
> File skia/skia_system.gypi (right):
> 
>
https://codereview.chromium.org/22435002/diff/1/skia/skia_system.gypi#newcode20
> skia/skia_system.gypi:20: 'animator/SkAnimatorView.h',
> Perhaps we can automatically generate the file but keep it on our side?

You can automatically generate it, just not *at gyp/build time* :) It's fine if
it's checked into third_party/skia/gyp - I'm keeping that repository because of
how gyp includes work. I'd just like to be able to drop third_party/skia/include
and third_party/skia/src.

[djsollen, 2013-08-07 17:01:27]

fwiw... the 3 repo system will be going away soon and will be replace with one
repo that is rooted at /third_party/skia so it will be much harder to drop src
and include.

[Torne, 2013-08-07 17:06:06]

On 2013/08/07 17:01:27, djsollen wrote:
> fwiw... the 3 repo system will be going away soon and will be replace with one
> repo that is rooted at /third_party/skia so it will be much harder to drop src
> and include.

We can still blow away the files in there during the merge as a precaution to
prevent accidental usage, though. I've done that before for ICU (though I had to
reinstate ICU's sources now that v8 depends on having a host ICU *sadface*).

Anyway, the requirement to work without the sources being present isn't
originally from WebView, it's from linux distros, who are just shipping a
tarball of the entire source tree minus the bits they don't want and thus don't
care about repo boundaries.

[djsollen, 2013-08-08 12:53:14]

The gypi file has been committed to Skia and will show up in tomorrows roll. 
You can rebase this CL on that and we should be good to go.

[Torne, 2013-08-09 15:33:18]

OK, I've rebased this to use the new .gypi file in the skia repo. Okay to go
ahead?

[djsollen, 2013-08-09 15:47:17]

lgtm

https://codereview.chromium.org/22435002/diff/12001/skia/skia_system.gypi
File skia/skia_system.gypi (right):

https://codereview.chromium.org/22435002/diff/12001/skia/skia_system.gypi#new...
skia/skia_system.gypi:21: '../third_party/skia/gyp/exported_api_headers.gypi',
robert, you will need to update this when you do the next DEPS roll.

[commit-bot: I haz the power, 2013-08-09 16:10:25]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Torne, 2013-08-09 16:12:27]

+mkosiba for full committer review :)

[robertphillips, 2013-08-09 16:12:30]

lgtm

[mkosiba (inactive), 2013-08-09 16:19:43]

LGTM provided the android_aosp tryjob goes green :P

[Torne, 2013-08-09 16:22:19]

On 2013/08/09 16:19:43, mkosiba wrote:
> LGTM provided the android_aosp tryjob goes green :P

It's not actually enabled even on android_aosp (nothing sets use_system_skia=1)
so if it doesn't break compiling skia entirely it's fine ;p

[Torne, 2013-08-09 16:23:11]

On 2013/08/09 16:22:19, Torne wrote:
> On 2013/08/09 16:19:43, mkosiba wrote:
> > LGTM provided the android_aosp tryjob goes green :P
> 
> It's not actually enabled even on android_aosp (nothing sets
use_system_skia=1)
> so if it doesn't break compiling skia entirely it's fine ;p

And immediately after saying that: it appears to break compiling skia entirely.
:)

It worked for me :(

[Torne, 2013-08-09 16:43:39]

On 2013/08/09 16:23:11, Torne wrote:
> On 2013/08/09 16:22:19, Torne wrote:
> > On 2013/08/09 16:19:43, mkosiba wrote:
> > > LGTM provided the android_aosp tryjob goes green :P
> > 
> > It's not actually enabled even on android_aosp (nothing sets
> use_system_skia=1)
> > so if it doesn't break compiling skia entirely it's fine ;p
> 
> And immediately after saying that: it appears to break compiling skia
entirely.
> :)
> 
> It worked for me :(

Ah no, this is ok; the tryjob is building against too old a revision of Chromium
(LKGR), from before skia rolled to have this new file. The CQ's tryjobs will be
against HEAD so it should be fine.

[commit-bot: I haz the power, 2013-08-09 16:43:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/torne@chromium.org/22435002/12001

[commit-bot: I haz the power, 2013-08-09 17:07:45]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, media_unittests, net_unittests,
sql_unittests, ui_unittests, url_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-08-12 10:09:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/torne@chromium.org/22435002/41001

[commit-bot: I haz the power, 2013-08-12 13:02:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/torne@chromium.org/22435002/62001

[Torne, 2013-08-12 13:56:54]

Committed patchset #4 manually as r216989 (presubmit successful).