Checking include paths in WebCore for bad dependencies
Within WebCore there are a number of directories that can be thought of as components with fairly well-defined dependencies. For example, WebCore/platform is intended to be a base component that the rest of WebCore can depend on but that should not have any outward dependencies. It's possible that some day WebCore/dom will be a dependency of WebCore/html but not the other way around. However, in practice these dependencies get out of hand pretty quickly. There are a large number of files - both new and old - in WebCore/platform that uses classes from WebCore/dom, WebCore/rendering, etc. I'd like to add a step to check-webkit-style that looks at the #includes in a patch and generate a warning if the patch is introducing a bad dependency. For example, files under WebCore/platform/ should not depend on files under WebCore/dom/ or WebCore/rendering/. Historically the only enforcement for these abstraction boundaries has been review, but this is a bit fragile since it's not obvious when looking at #include "Foo.h" where Foo.h lives. Making bad includes show up when running check-webkit-style and in the style-ews will make these bad includes more visible and hopefully help people fix them. There's an initial patch up at https://bugs.webkit.org/show_bug.cgi?id=49192. Additionally, I'd like to use this tool to try to create and enforce some more boundaries and one-way dependencies within WebCore. Currently nearly everything in WebCore is interdependent on everything else within WebCore which makes it harder to understand the code and harder to patch it correctly. I made an attempt to diagram the dependencies by had to give up pretty early on: https://docs.google.com/drawings/edit?id=11PDlM-Vu12TQy24tPpv2cvviBvuZzheKxh.... We've also recently had a number of bugs where code in WebCore/rendering/ has called into editing or loader code that were not aware of the requirements of rendering code. Does this sound like a good set of goals? - James
On Nov 8, 2010, at 11:47 AM, James Robinson wrote:
For example, WebCore/platform is intended to be a base component that the rest of WebCore can depend on but that should not have any outward dependencies.
In JavaScriptCore we have the same thing with JavaScriptCore/wtf and the rest of JavaScriptCore.
Making bad includes show up when running check-webkit-style and in the style-ews will make these bad includes more visible and hopefully help people fix them. There's an initial patch up at https://bugs.webkit.org/show_bug.cgi?id=49192.
Sounds like a good idea.
Additionally, I'd like to use this tool to try to create and enforce some more boundaries and one-way dependencies within WebCore.
We should discuss any specific ideas for this we have to see if they are promising. I would not assume there are great opportunities for this, but there may be some. I wouldn’t want to rearrange code a lot to accomplish this because there’s no guarantee that it would make things easier to understand or modify. At one time, the C++ bindings in WebCore inherited from khtml had this issue. They were supposed to be a binding layer on top of the DOM but they were used elsewhere. We resolved that and then later removed the C++ bindings. -- Darin
On Nov 8, 2010, at 11:47 AM, James Robinson wrote:
Within WebCore there are a number of directories that can be thought of as components with fairly well-defined dependencies. For example, WebCore/platform is intended to be a base component that the rest of WebCore can depend on but that should not have any outward dependencies. It's possible that some day WebCore/dom will be a dependency of WebCore/html but not the other way around. However, in practice these dependencies get out of hand pretty quickly. There are a large number of files - both new and old - in WebCore/platform that uses classes from WebCore/dom, WebCore/rendering, etc.
I'd like to add a step to check-webkit-style that looks at the #includes in a patch and generate a warning if the patch is introducing a bad dependency.
Sounds like an excellent idea.
For example, files under WebCore/platform/ should not depend on files under WebCore/dom/ or WebCore/rendering/. Historically the only enforcement for these abstraction boundaries has been review, but this is a bit fragile since it's not obvious when looking at #include "Foo.h" where Foo.h lives. Making bad includes show up when running check-webkit-style and in the style-ews will make these bad includes more visible and hopefully help people fix them. There's an initial patch up at https://bugs.webkit.org/show_bug.cgi?id=49192.
Additionally, I'd like to use this tool to try to create and enforce some more boundaries and one-way dependencies within WebCore. Currently nearly everything in WebCore is interdependent on everything else within WebCore which makes it harder to understand the code and harder to patch it correctly. I made an attempt to diagram the dependencies by had to give up pretty early on: https://docs.google.com/drawings/edit?id=11PDlM-Vu12TQy24tPpv2cvviBvuZzheKxh.... We've also recently had a number of bugs where code in WebCore/rendering/ has called into editing or loader code that were not aware of the requirements of rendering code.
Does this sound like a good set of goals?
I'm not sure how easy it will be to introduce strict one-way boundaries. Many components have a naturally two-way interaction. To impose a layering, you need to introduce some kind of callback mechanism, perhaps with an abstract base class or the like. I guess it would be easier to evaluate this with specific examples. I tentatively agree that rendering code calling editing code (instead of vice versa) sounds potentially suspicious. I guess a tool could help us see how many violations there are of any proposed rule in current code. Regards, Maciej
participants (3)
-
Darin Adler
-
James Robinson
-
Maciej Stachowiak