Tighten the restrictions on including webkit code from chrome/

Tighten the restrictions on including webkit code from chrome/

This removes the chrome/DEPS allowances for including webkit/,
adds temporary allowances in the subdirectories, and makes it
clear that in most cases the allowance is temporary and should
be removed.

This is an incremental step toward disallowing use of webkit/ code
directly from most chrome/ code (excepting specific subdirectories,
most notably chrome/renderer/).

BUG=146251
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156521

[stuartmorgan, 2012-09-03 13:17:51]

Here's a first-pass of auditing the webkit DEPS exclusions, per our discussion
last week.

I tried to strike a balance between a minimal restriction and a sane rule set.
So, e.g., if I would have had to add an allowance to 4 or 5 chrome/browser/
subdirectories I just added it to chrome/browser/DEPS instead. I can tweak as
necessary if you want the balance shifted one way or the other.

[stuartmorgan, 2012-09-07 08:44:31]

Darin, John: ping

[jam, 2012-09-07 16:00:10]

it's not clear to me that adding this will achieve much, without having a
taskforce dedicated to removing them. i.e. if someone wants to include another
header, they will get a checkdeps error and will just add another line.

[stuartmorgan, 2012-09-07 17:06:57]

On 2012/09/07 16:00:10, John Abd-El-Malek wrote:
> it's not clear to me that adding this will achieve much, without having a
> taskforce dedicated to removing them. i.e. if someone wants to include another
> header, they will get a checkdeps error and will just add another line.

I thought the normal pattern was to start discouraging new uses, commenting
clearly that new exclusions shouldn't be added, and then start removing old
usage.

What do you suggest for moving forward here based on the discussion from last
week? Should we just move forms, and maybe UA, out of webkit, and ignore the
larger problem of not wanting more stuff to collect in webkit/?

[jam, 2012-09-10 05:41:43]

On 2012/09/07 17:06:57, stuartmorgan wrote:
> On 2012/09/07 16:00:10, John Abd-El-Malek wrote:
> > it's not clear to me that adding this will achieve much, without having a
> > taskforce dedicated to removing them. i.e. if someone wants to include
another
> > header, they will get a checkdeps error and will just add another line.
> 
> I thought the normal pattern was to start discouraging new uses, commenting
> clearly that new exclusions shouldn't be added, and then start removing old
> usage.

This works when there are people allocated to removing all the last usages. Do
we have this in this case? Otherwise we just remain with a bunch of entries that
people will keep adding to over time.

> 
> What do you suggest for moving forward here based on the discussion from last
> week? Should we just move forms, and maybe UA, out of webkit, and ignore the
> larger problem of not wanting more stuff to collect in webkit/?

That's what I'm worried about, that we add all these DEPS entries but no one is
dedicated to removing them. It will add to developer confusion in the meantime,
and it won't accomplish much because if people see a bunch of entries that
aren't being worked on, they will just add more.

[stuartmorgan, 2012-09-10 07:48:32]

On 2012/09/10 05:41:43, John Abd-El-Malek wrote:
> This works when there are people allocated to removing all the last usages. Do
> we have this in this case?

Someone will be starting in on forms this week. UA is gated on DRT/TST work as I
understand it, and I had the impression that people were working on that stuff
now. The rest, probably not.

> Otherwise we just remain with a bunch of entries that
> people will keep adding to over time.

I'm not sure why chrome/ OWNERS would approve a change that added new use of
webkit/ if there's a DEPS file saying "Do not add anything to this list" and an
explicit policy that nobody should be adding new dependencies from
chrome/browser into webkit/.

> That's what I'm worried about, that we add all these DEPS entries but no one
is
> dedicated to removing them. It will add to developer confusion in the
meantime,
> and it won't accomplish much because if people see a bunch of entries that
> aren't being worked on, they will just add more.

I could have sworn that Darin said in the meeting that he wanted simple rules
enforced by DEPS--don't use webkit/ code from chrome/browser was one that was
mentioned--specifically to *reduce* developer confusion. It seems like the
alternative is a rule that "if you add new webkit/ dependencies from code that
is built on iOS then you'll break the build and have to revert, so you need to
figure out if the files you are changing will affect iOS by looking at the gyp
file".

Darin, could you weigh in here? I thought we had agreement about how to proceed
with webkit/, but apparently not. If the idea was that the people working on
upstreaming iOS are responsible for completely purging all chrome/browser and
content/browser code of webkit/ references, even in all the files that we don't
use, that seems impractical. We're going to do forms, I've been chopping up
webkit/glue to make it easier to dismantle incrementally, and I did this audit
(and was planning to do one for content too) but we don't have the manpower to
do all of the rest of the work alone.

[jam, 2012-09-10 15:22:47]

to reply to a few of the points:

1) we all agree that deps is the best way to ensure that people don't break
dependencies.  what i'm against is adding a bunch of exceptions with no one
working on them. i feel this would just lead to busy work, because people will
add more to keep checkdeps happy.

2) an explicit policy to not add more is good when people are working on adding
ways to ensure this is possible. but if there are already a bunch of includes
and no one working on them, these will lead to other includes (either related
ones, or new ones).

3) I didn't say the iOS devs have to do the work to remove all the webkit
includes. just that it's premature to add tight DEPS rules against existing
usages until then. i.e. if third_party/WebKit/foo.h is in one of the DEPS
allowed lists, and someone wants to use it from another directory, then it'll
spread through DEPS. or other headers similar to existing ones are added, then
owners will approve it.

[stuartmorgan, 2012-09-10 15:34:44]

On 2012/09/10 15:22:47, John Abd-El-Malek wrote:
> what i'm against is adding a bunch of exceptions with no one
> working on them. [...] i.e. if third_party/WebKit/foo.h is in one of the DEPS
> allowed lists, and someone wants to use it from another directory, then it'll
> spread through DEPS. or other headers similar to existing ones are added, then
> owners will approve it.

What do you think of a more relaxed version of this patch, where I basically
just move existing exceptions from chrome/DEPS to chrome/<foo>/DEPS, and don't
change all the subdirectories? That would align better with the idea that it's
about moving the code out of webkit/, not using the webkit/ code from fewer
parts of chrome/browser.

[stuartmorgan, 2012-09-10 15:51:18]

To make my question more concrete: here's a version that doesn't propagate
individual rules below chrome/browser/DEPS. That makes it focus on the files
being included--any given code webkit/ code that's used from chrome/browser/ can
be used from the rest of chrome/browser/--rather than the inclusion point. I
agree that makes more sense, given that the fix is in many cases going to be to
move the code out of webkit/, not to remove the includes.

[darin (slow to review), 2012-09-10 16:17:30]

It occurs to me that there may be a flaw in the recommendation to use checkdeps
to limit webkit includes in chrome/browser/ code.  You see, I think we will
always want to use enumeration and other POD types defined by WebKit in
chrome/browser/.  To try to eliminate those would result in duplication of such
types, which would be unfortunate.

[darin (slow to review), 2012-09-10 16:18:35]

On 2012/09/10 16:17:30, darin wrote:
> It occurs to me that there may be a flaw in the recommendation to use
checkdeps
> to limit webkit includes in chrome/browser/ code.  You see, I think we will
> always want to use enumeration and other POD types defined by WebKit in
> chrome/browser/.  To try to eliminate those would result in duplication of
such
> types, which would be unfortunate.

I think the long term goal should be to make the browser process not need to
execute any WebKit code, and we should enforce that with the component build. 
The browser process simply shouldn't load webkit.dll.

[stuartmorgan, 2012-09-10 17:00:54]

On 2012/09/10 16:18:35, darin wrote:
> I think the long term goal should be to make the browser process not need to
> execute any WebKit code, and we should enforce that with the component build. 
> The browser process simply shouldn't load webkit.dll.

Okay, I'll close this CL then.

What does that mean for webkit/forms (which is mostly stand-alone code, but has
a constructor that requires WebKit linkage for a conversion constructor)? Split
the conversion routine into a separate target but leave it all in webkit/? Or
still move all the non-conversion forms code out of webkit/?

[darin (slow to review), 2012-09-10 17:32:06]

On Mon, Sep 10, 2012 at 10:00 AM, <stuartmorgan@chromium.org> wrote:

> On 2012/09/10 16:18:35, darin wrote:
>
>> I think the long term goal should be to make the browser process not need
>> to
>> execute any WebKit code, and we should enforce that with the component
>> build.
>> The browser process simply shouldn't load webkit.dll.
>>
>
> Okay, I'll close this CL then.
>
> What does that mean for webkit/forms (which is mostly stand-alone code,
> but has
> a constructor that requires WebKit linkage for a conversion constructor)?
> Split
> the conversion routine into a separate target but leave it all in webkit/?
> Or
> still move all the non-conversion forms code out of webkit/?
>
>
Hmm...

Maybe it is reasonable to forbid includes of code from src/webkit/ but
still allow includes from src/third_party/WebKit.  With minor exceptions
(WindowOpenDisposition, ResourceType), src/webkit/ shouldn't really be in
the game of re-defining WebKit API enumerations.

src/webkit/ does however re-define some data structures to avoid WebString
and other non-POD WebKit API types.  PasswordForm is an example of this.
 Since PasswordForm is not consumed by any other code in src/webkit/, I
think it is reasonable to move it to src/content/, but keep the conversion
function in src/webkit/ or src/content/renderer/.

There was a thread last week about the future demise of src/webkit/.
 Ultimately, it will all get folded into src/content/renderer/ once
DumpRenderTree is just built on top of content_shell.  So, it make sense to
take steps in that direction.

-Darin



>
https://codereview.chromium.**org/10905061/<https://codereview.chromium.org/1...
>

[stuartmorgan, 2012-09-12 09:26:02]

On 2012/09/10 17:32:06, darin wrote:
> Maybe it is reasonable to forbid includes of code from src/webkit/
[...]
> There was a thread last week about the future demise of src/webkit/.
>  Ultimately, it will all get folded into src/content/renderer/ once
> DumpRenderTree is just built on top of content_shell.  So, it make sense to
> take steps in that direction.

Okay, I've pared this down to just making the webkit/ inclusions more
restrictive. Do we want to land this, or should I pitch the change?

[darin (slow to review), 2012-09-12 16:53:33]

OK, LGTM

[commit-bot: I haz the power, 2012-09-13 07:40:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stuartmorgan@chromium.org/10905061/11001

[commit-bot: I haz the power, 2012-09-13 10:07:36]

Change committed as 156521