Create a minimal webkit_common static library for use in browser process

Create a minimal webkit_common static library for use in browser process

This defines a library with a minimal set of WebKit symbols that can be linked
into locations that need to use pieces of the WebKit API without needing the
whole thing.

BUG=237267

[jamesr, 2013-05-14 23:22:19]

I don't think this is quite ready to land, but here it is for discussion.  The
idea is that in a static build the webkit_common library could be linked into
the browser process to provide implementations of WebString/WebURL/etc.  The set
of things in this library is pretty arbitrary - it's just things that look like
they will be needed in the browser.  Until we're a little further along in the
splitting process it'll be unclear if this is truly sufficient or not.

[jamesr, 2013-05-14 23:26:50]

In particular, it looks like we might need to pull the image decoders in to this
lib as well which would pull in a SharedBuffer, the WebCore geometry classes,
etc.

[abarth-chromium, 2013-05-14 23:31:42]

IMHO, we should reflect this somehow in the directory layout.

I have a script that moves all the Platform API headers from
Source/Platform/chromium/public to public/platform.  Would it make sense to have
a public/foo directory that contains the stuff that can be used in this common
library?

[darin (slow to review), 2013-05-14 23:33:30]

It might help to identify what parts of this are long-term and what parts are
short-term hacks.  I don't think we need any of the scrollbar stuff in the
browser process.  If that is needed now, then it is surely just to make the
linker happy.

[jamesr, 2013-05-15 20:12:08]

I think the scrollbar ones are definitely a short-term hack.  The only thing
that really makes sense to me to use in the browser long-term is the origin
stuff, although there aren't all that many users for that so maybe it isn't
needed either.

To use WebSecurityOrigin requires having Platform, WebSecurityOrigin, WebRL and
the strings classes.  Internally this requires pulling in wtf/, weborigin/, and
the wtf<->webkit glue.  I'm not sure it's worth making a separate directory for
this, but we could.

[abarth-chromium, 2013-05-15 22:15:00]

It would be nice to have some sort of guidance in the structure of the code to
help us not blow up the size of the common library...  I guess a compile failure
will keep us in check?

[jamesr, 2013-06-06 21:44:54]

This'll have to be broken up a bit to land since it involves some minor API
changes, but I think this about the minimum viable set.  The set of headers it
exposes are:

WebAssertion (used by other headers)
WebCString / WebString (used by compositor and IDB code, at least)
WebFilterOperation(s) (used by compositor)

It's *possible* to break these dependencies eventually, but I don't think it's
feasible in the short term. For the filter operations the correct solution is
underway but a ways out - it depends on some skia changes that are still in the
rough design phase.  The WebString ones are hard to break without either
dropping to less safe types (i.e. expose char*/size_t pairs), duplicating types,
and/or adding extra copies.

That said, I think we should treat this set as a burn-down list of dependencies
to break.

Re "how do people keep this working" the answer is "if it links, it works". 
I'll set up dependencies for things like the compositor unit tests on the
chromium side to only link in blink_minimal, so if someone removes something in
use or adds a new dependency the test targets will stop linking.

On a more positive note, this'll have an immediate improvement for building
things like aura - it'll take ~3000 fewer build steps to build these from
scratch with this patch.

[jamesr, 2013-06-06 21:47:56]

https://codereview.chromium.org/15079005/diff/18001/Source/WebKit/chromium/We...
File Source/WebKit/chromium/WebKit.gyp (right):

https://codereview.chromium.org/15079005/diff/18001/Source/WebKit/chromium/We...
Source/WebKit/chromium/WebKit.gyp:67: 'INSIDE_WEBKIT',
this is needed in order to build the blink API as >1 DLL.  In the API headers,
we have to be able to distinguish between 'will link into webkit.dll' and 'is
inside the webkit repository and has access to WTF'

https://codereview.chromium.org/15079005/diff/18001/public/blink.gyp
File public/blink.gyp (right):

https://codereview.chromium.org/15079005/diff/18001/public/blink.gyp#newcode51
public/blink.gyp:51: '../Source/common/common.gyp:blink_common',
the naming here is tricky since all target names have to be globally unique.
'webkit_common' is taken by a psuedo-unrelated target in src/webkit/... in the
chromium tree

https://codereview.chromium.org/15079005/diff/18001/public/platform/WebCommon.h
File public/platform/WebCommon.h (right):

https://codereview.chromium.org/15079005/diff/18001/public/platform/WebCommon...
public/platform/WebCommon.h:34: #include "../common/WebAssertion.h"
i could also make all API headers that want to use WEBKIT_ASSERT pull this in
explicitly, if that seems cleaner.

[abarth-chromium, 2013-06-06 21:51:16]

https://codereview.chromium.org/15079005/diff/18001/Source/common/common.gyp
File Source/common/common.gyp (right):

https://codereview.chromium.org/15079005/diff/18001/Source/common/common.gyp#...
Source/common/common.gyp:41: '../weborigin/weborigin.gyp:weborigin',
Is WebOrigin still needed?

[jamesr, 2013-06-06 21:52:20]

On 2013/06/06 21:51:16, abarth wrote:
> https://codereview.chromium.org/15079005/diff/18001/Source/common/common.gyp
> File Source/common/common.gyp (right):
> 
>
https://codereview.chromium.org/15079005/diff/18001/Source/common/common.gyp#...
> Source/common/common.gyp:41: '../weborigin/weborigin.gyp:weborigin',
> Is WebOrigin still needed?

Good catch - it isn't. Hopefully it won't be.

[darin (slow to review), 2013-06-06 22:10:23]

It feels like this adds a layer of confusion to the API.  Given that it is
only temporary, are you sure it wouldn't be better to just keep the source
files where they are and let a new target exist for this temporary issue?

-Darin


On Thu, Jun 6, 2013 at 2:44 PM, <jamesr@chromium.org> wrote:

> This'll have to be broken up a bit to land since it involves some minor API
> changes, but I think this about the minimum viable set.  The set of
> headers it
> exposes are:
>
> WebAssertion (used by other headers)
> WebCString / WebString (used by compositor and IDB code, at least)
> WebFilterOperation(s) (used by compositor)
>
> It's *possible* to break these dependencies eventually, but I don't think
> it's
> feasible in the short term. For the filter operations the correct solution
> is
> underway but a ways out - it depends on some skia changes that are still
> in the
> rough design phase.  The WebString ones are hard to break without either
> dropping to less safe types (i.e. expose char*/size_t pairs), duplicating
> types,
> and/or adding extra copies.
>
> That said, I think we should treat this set as a burn-down list of
> dependencies
> to break.
>
> Re "how do people keep this working" the answer is "if it links, it works".
> I'll set up dependencies for things like the compositor unit tests on the
> chromium side to only link in blink_minimal, so if someone removes
> something in
> use or adds a new dependency the test targets will stop linking.
>
> On a more positive note, this'll have an immediate improvement for building
> things like aura - it'll take ~3000 fewer build steps to build these from
> scratch with this patch.
>
>
https://codereview.chromium.**org/15079005/<https://codereview.chromium.org/1...
>

[jamesr, 2013-06-06 22:12:27]

On 2013/06/06 22:10:23, darin wrote:
> It feels like this adds a layer of confusion to the API.  Given that it is
> only temporary, are you sure it wouldn't be better to just keep the source
> files where they are and let a new target exist for this temporary issue?
> 
> -Darin
> 

No, I'm definitely not sure this is the best way.  I can also accomplish this
almost entirely within the build system, with the only major exception being I
still have to use a different _EXPORT macro for things going into
blink_common.DLL and I'll still have to use the INSIDE_WEBKIT macro in a few
files.  I can sketch that up.

[darin (slow to review), 2013-06-06 22:13:27]

Oh, I see... that sucks.  Would it be possible to create a static lib for
the bits that need to be included in chrome.dll (or cc.dll)?

-Darin


On Thu, Jun 6, 2013 at 3:12 PM, <jamesr@chromium.org> wrote:

> On 2013/06/06 22:10:23, darin wrote:
>
>> It feels like this adds a layer of confusion to the API.  Given that it is
>> only temporary, are you sure it wouldn't be better to just keep the source
>> files where they are and let a new target exist for this temporary issue?
>>
>
>  -Darin
>>
>
>
> No, I'm definitely not sure this is the best way.  I can also accomplish
> this
> almost entirely within the build system, with the only major exception
> being I
> still have to use a different _EXPORT macro for things going into
> blink_common.DLL and I'll still have to use the INSIDE_WEBKIT macro in a
> few
> files.  I can sketch that up.
>
>
https://codereview.chromium.**org/15079005/<https://codereview.chromium.org/1...
>

[jamesr1, 2013-06-06 22:21:40]

Hmm, that would mean in the component build every target that wanted to use
the Blink API would have to statically link in a copy of WebString.cpp,
etc.  I think that's doable - there aren't any statics in here - but that's
also kind of icky.  I could give it a try.


On Thu, Jun 6, 2013 at 3:13 PM, Darin Fisher <darin@chromium.org> wrote:

> Oh, I see... that sucks.  Would it be possible to create a static lib for
> the bits that need to be included in chrome.dll (or cc.dll)?
>
> -Darin
>
>
> On Thu, Jun 6, 2013 at 3:12 PM, <jamesr@chromium.org> wrote:
>
>> On 2013/06/06 22:10:23, darin wrote:
>>
>>> It feels like this adds a layer of confusion to the API.  Given that it
>>> is
>>> only temporary, are you sure it wouldn't be better to just keep the
>>> source
>>> files where they are and let a new target exist for this temporary issue?
>>>
>>
>>  -Darin
>>>
>>
>>
>> No, I'm definitely not sure this is the best way.  I can also accomplish
>> this
>> almost entirely within the build system, with the only major exception
>> being I
>> still have to use a different _EXPORT macro for things going into
>> blink_common.DLL and I'll still have to use the INSIDE_WEBKIT macro in a
>> few
>> files.  I can sketch that up.
>>
>>
https://codereview.chromium.**org/15079005/<https://codereview.chromium.org/1...
>>
>
>

[darin (slow to review), 2013-06-06 22:39:25]

Whatever is easier.  It is just a temporary measure.  We'd have to compile
these modules twice though.  That might make the GYP a bit messy.


On Thu, Jun 6, 2013 at 3:21 PM, James Robinson <jamesr@google.com> wrote:

> Hmm, that would mean in the component build every target that wanted to
> use the Blink API would have to statically link in a copy of WebString.cpp,
> etc.  I think that's doable - there aren't any statics in here - but that's
> also kind of icky.  I could give it a try.
>
>
> On Thu, Jun 6, 2013 at 3:13 PM, Darin Fisher <darin@chromium.org> wrote:
>
>> Oh, I see... that sucks.  Would it be possible to create a static lib for
>> the bits that need to be included in chrome.dll (or cc.dll)?
>>
>> -Darin
>>
>>
>> On Thu, Jun 6, 2013 at 3:12 PM, <jamesr@chromium.org> wrote:
>>
>>> On 2013/06/06 22:10:23, darin wrote:
>>>
>>>> It feels like this adds a layer of confusion to the API.  Given that it
>>>> is
>>>> only temporary, are you sure it wouldn't be better to just keep the
>>>> source
>>>> files where they are and let a new target exist for this temporary
>>>> issue?
>>>>
>>>
>>>  -Darin
>>>>
>>>
>>>
>>> No, I'm definitely not sure this is the best way.  I can also accomplish
>>> this
>>> almost entirely within the build system, with the only major exception
>>> being I
>>> still have to use a different _EXPORT macro for things going into
>>> blink_common.DLL and I'll still have to use the INSIDE_WEBKIT macro in a
>>> few
>>> files.  I can sketch that up.
>>>
>>>
https://codereview.chromium.**org/15079005/<https://codereview.chromium.org/1...
>>>
>>
>>
>

[jamesr, 2013-06-07 01:23:05]

On 2013/06/06 22:10:23, darin wrote:
> It feels like this adds a layer of confusion to the API.  Given that it is
> only temporary, are you sure it wouldn't be better to just keep the source
> files where they are and let a new target exist for this temporary issue?

https://codereview.chromium.org/16024011/