Re: [webkit-dev] Moving WebCore/accessibility code into WebCore/platform/
Hi Maciej, [.] > I think the main problem with this change is that the > current WebCore/accessibility code has lots of knowledge > of the DOM and render tree. Making WebCore/platform > depend on these Web-specific concepts is a layering > violation - imagine how it would work if it was a > top-level directory Platform that was a peer of WebCore, > and built before it without access to its headers. > That is logically how it should operate. That's absolutely right. When I sent the email I was thinking more of the platform specific nature of the accessibility layer and its adaptation to each port (the wrappers), but it's true that the relationship with those other trees is so tight that moving it into platform (as it is now) would be a problem. > If the accessibility code could be redesigned to have > a good abstraction between the parts that are tightly > tied to Web-specific classes and concepts, and the part > that is platform-specific, then the second part could > be moved to WebCore/platform. A refactoring like that > could be a good improvement, but is likely hard to do. Agreed on this too. Thanks for the feedback, Mario ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Moving WebCore/accessibility code into WebCore/platform/
On May 21, 2013, at 7:34 AM, Mario Sanchez Prada wrote: > Hi all, > > Following the discussion in the contributors meeting about layering > violations I was thinking about moving all the accessibility stuff inside > WebCore/accessibility into a new WebCore/platform/accessibility directory. > > My reasoning behind this could be summarized as this: > > * Accessibility code is actually already platform dependant, as every port > supporting this exposes the > accessibility hierarchy in slightly different ways (ATK flattens more the > hierarchy than Mac, for instance). > > * Besides the AccessibilityObject wrappers and partial platform-specific > implementation files (e.g. AccessibilityObjectMac.mm) present in places like > WebCore/accessibility/[atk|mac], there are other bits in > WebCore/accessibility that are platform specific as well (e.g. > AccessibilityRenderObject). These bits are guarded with "#if PLATFORM" > macros, which would still be necessary to meet the different requirements of > each port. > > * The number of ports adding support for accessibility is increasing, some of > them sharing code already (e.g. EFL and GTK port, both use ATK), so I believe > that would be a nice move to make. > > Of course, we could always add an exception to the style checker, but I feel > like relocating things would be a better approach in this case, thinking of > the long term. > > What do you think? I think the main problem with this change is that the current WebCore/accessibility code has lots of knowledge of the DOM and render tree. Making WebCore/platform depend on these Web-specific concepts is a layering violation - imagine how it would work if it was a top-level directory Platform that was a peer of WebCore, and built before it without access to its headers. That is logically how it should operate. If the accessibility code could be redesigned to have a good abstraction between the parts that are tightly tied to Web-specific classes and concepts, and the part that is platform-specific, then the second part could be moved to WebCore/platform. A refactoring like that could be a good improvement, but is likely hard to do. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Moving WebCore/accessibility code into WebCore/platform/
On May 21, 2013, at 11:14 AM, Benjamin Poulain wrote: > For me that rule was more about asking people to think twice and add the > proper abstraction when possible. > We do not want style checker warnings, which becomes a motivation for > creating better abstractions. > > For example, recently a second implementation of some of DOMFileSystemBase > was added for blackberry (most if it is an exact copy of the common code). > With a style checker warning, the author would have had to justify this kind > of decision (and hopefully fix the code instead). Again, I think in theory this is good, if it drives people to create the abstractions we need to keep our code from being littered with too literal #ifdefs. But instead the style checker warning is pushing at least some folks to move classes into the platform directory that do not belong there, just to avoid the warning, likely creating layering violations and misusing the platform library. Further, I am seeing this warning often when I do refactoring patches. I think what we did in the style checker is not a good enough way to communicate the nuance of how we do this. We need a document with a statement of principles about the platform approach to refer people to. And perhaps an approach that gives a warning rather than a style error that points to that document. — Darin___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Moving WebCore/accessibility code into WebCore/platform/
I rolled it out in http://trac.webkit.org/changeset/150457 On Tue, May 21, 2013 at 10:58 AM, Sam Weinig wrote: > > On May 21, 2013, at 10:56 AM, Darin Adler wrote: > > On May 21, 2013, at 10:42 AM, Sam Weinig wrote: > > On May 21, 2013, at 8:15 AM, Darin Adler wrote: > > Of course, we could always add an exception to the style checker > > > The style checker rule is wrong and should be removed. It’s better to use > a platform abstraction, but a platform #ifdef is also OK. > > > This was something we discussed during the contributors meeting. The > premise was that we should aim for having no PLATFORM() #ifdefs in > non-Platform and and should instead use other macros that are more > descriptive. I like that rule, but if isn't really possible, we should > probably remove the check. > > > It’s an interesting principle, but I think too far from reality right now. > > And it leads to crazy things like the Clipboard in the dom directory with > per-platform subclasses in the platform directory. > > — Darin > > > Fair enough. I'll talk to Jessie about removing it, and maybe we can > revisit it in the future. > > - Sam > > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > https://lists.webkit.org/mailman/listinfo/webkit-dev > > ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Moving WebCore/accessibility code into WebCore/platform/
On Tue, May 21, 2013 at 10:23 AM, Darin Adler wrote: > On this regard, I think it would be specially interesting to hear > the opinion of the people involved in the bug that tracked this addition: > https://bugs.webkit.org/show_bug.cgi?id=115567 > > > Sure. Jessie and Ben, when you have a chance could one or both of you > comment? > For me that rule was more about asking people to think twice and add the proper abstraction when possible. We do not want style checker warnings, which becomes a motivation for creating better abstractions. For example, recently a second implementation of some of DOMFileSystemBase was added for blackberry (most if it is an exact copy of the common code). With a style checker warning, the author would have had to justify this kind of decision (and hopefully fix the code instead). Benjamin ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Moving WebCore/accessibility code into WebCore/platform/
On May 21, 2013, at 10:56 AM, Darin Adler wrote: > On May 21, 2013, at 10:42 AM, Sam Weinig wrote: > >> On May 21, 2013, at 8:15 AM, Darin Adler wrote: >> Of course, we could always add an exception to the style checker >>> >>> The style checker rule is wrong and should be removed. It’s better to use a >>> platform abstraction, but a platform #ifdef is also OK. >> >> This was something we discussed during the contributors meeting. The >> premise was that we should aim for having no PLATFORM() #ifdefs in >> non-Platform and and should instead use other macros that are more >> descriptive. I like that rule, but if isn't really possible, we should >> probably remove the check. > > It’s an interesting principle, but I think too far from reality right now. > > And it leads to crazy things like the Clipboard in the dom directory with > per-platform subclasses in the platform directory. > > — Darin Fair enough. I'll talk to Jessie about removing it, and maybe we can revisit it in the future. - Sam___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Moving WebCore/accessibility code into WebCore/platform/
On May 21, 2013, at 10:42 AM, Sam Weinig wrote: > On May 21, 2013, at 8:15 AM, Darin Adler wrote: > >>> Of course, we could always add an exception to the style checker >> >> The style checker rule is wrong and should be removed. It’s better to use a >> platform abstraction, but a platform #ifdef is also OK. > > This was something we discussed during the contributors meeting. The premise > was that we should aim for having no PLATFORM() #ifdefs in non-Platform and > and should instead use other macros that are more descriptive. I like that > rule, but if isn't really possible, we should probably remove the check. It’s an interesting principle, but I think too far from reality right now. And it leads to crazy things like the Clipboard in the dom directory with per-platform subclasses in the platform directory. — Darin___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Moving WebCore/accessibility code into WebCore/platform/
On May 21, 2013, at 8:15 AM, Darin Adler wrote: > >> Of course, we could always add an exception to the style checker > > The style checker rule is wrong and should be removed. It’s better to use a > platform abstraction, but a platform #ifdef is also OK. This was something we discussed during the contributors meeting. The premise was that we should aim for having no PLATFORM() #ifdefs in non-Platform and and should instead use other macros that are more descriptive. I like that rule, but if isn't really possible, we should probably remove the check. Accessibility is a tricky one. The way it is currently written requires both cross platform and platform specific knowledge to co-exist. Looking at the base class, AccessibilityObject, it seems like there is a WebCore object (AccessibilityObject) that wraps a platform specific piece (m_wrapper, which on mac is a WebAccessibilityObjectWrapper and on GTK is a AtkObject, etc.). The way I would recommend moving forward is seeing if there is some way to abstract the platform specific piece in a C++ object that could live in platform (much like we do in other areas), and make all the WebCore code work in terms of it. -Sam ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Moving WebCore/accessibility code into WebCore/platform/
On May 21, 2013, at 9:03 AM, Mario Sanchez Prada wrote: >>> Of course, we could always add an exception to the style checker >> >> The style checker rule is wrong and should be removed. It's better to >> use a platform abstraction, but a platform #ifdef is also OK. > > What about refining that rule instead of just removing it? I can’t think of a good refined version of the rule. > Maybe adding a list of directories to be excluded or something like that > might work fine too. Not sure that would be helpful. > On this regard, I think it would be specially interesting to hear the opinion > of the people involved in the bug that tracked this addition: > https://bugs.webkit.org/show_bug.cgi?id=115567 Sure. Jessie and Ben, when you have a chance could one or both of you comment? — Darin___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Moving WebCore/accessibility code into WebCore/platform/
Hi Darin, > -Original Message- > [...] > This is a bad idea. The platform directory has never been intended as > the single place for all platform-specific code. It's the place for > platform abstractions that let us cut down on platform-specific code > elsewhere. It's not a good idea to try to put all platform-specific > code into the platform directory. I see. It's true that the accessibility layer is definitely not the same thing like other ones, such as network or graphics. It's actually more similar to the render object layer, so I guess you are right. > > * Besides the AccessibilityObject wrappers and partial platform- > specific implementation files (e.g. AccessibilityObjectMac.mm) present > in places like WebCore/accessibility/[atk|mac], there are other bits in > WebCore/accessibility that are platform specific as well (e.g. > AccessibilityRenderObject). These bits are guarded with "#if PLATFORM" > macros, which would still be necessary to meet the different > requirements of each port. > > There's no problem with this. Ok. > > Of course, we could always add an exception to the style checker > > The style checker rule is wrong and should be removed. It's better to > use a platform abstraction, but a platform #ifdef is also OK. What about refining that rule instead of just removing it? Maybe adding a list of directories to be excluded or something like that might work fine too. On this regard, I think it would be specially interesting to hear the opinion of the people involved in the bug that tracked this addition: https://bugs.webkit.org/show_bug.cgi?id=115567 > > but I feel like relocating things would be a better approach in this > case > > As you can see above, I do not agree. That's fine. Either removing that rule from the style checker or just refining it to better match the situation in places such as WebCore/accessibility will work fine here. Thanks for the feedback, Mario ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Moving WebCore/accessibility code into WebCore/platform/
On May 21, 2013, at 7:34 AM, Mario Sanchez Prada wrote: > Following the discussion in the contributors meeting about layering > violations I was thinking about moving all the accessibility stuff inside > WebCore/accessibility into a new WebCore/platform/accessibility directory. This is a bad idea. The platform directory has never been intended as the single place for all platform-specific code. It’s the place for platform abstractions that let us cut down on platform-specific code elsewhere. It’s not a good idea to try to put all platform-specific code into the platform directory. > * Besides the AccessibilityObject wrappers and partial platform-specific > implementation files (e.g. AccessibilityObjectMac.mm) present in places like > WebCore/accessibility/[atk|mac], there are other bits in > WebCore/accessibility that are platform specific as well (e.g. > AccessibilityRenderObject). These bits are guarded with "#if PLATFORM" > macros, which would still be necessary to meet the different requirements of > each port. There’s no problem with this. > Of course, we could always add an exception to the style checker The style checker rule is wrong and should be removed. It’s better to use a platform abstraction, but a platform #ifdef is also OK. > but I feel like relocating things would be a better approach in this case As you can see above, I do not agree. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] Moving WebCore/accessibility code into WebCore/platform/
Hi all, Following the discussion in the contributors meeting about layering violations I was thinking about moving all the accessibility stuff inside WebCore/accessibility into a new WebCore/platform/accessibility directory. My reasoning behind this could be summarized as this: * Accessibility code is actually already platform dependant, as every port supporting this exposes the accessibility hierarchy in slightly different ways (ATK flattens more the hierarchy than Mac, for instance). * Besides the AccessibilityObject wrappers and partial platform-specific implementation files (e.g. AccessibilityObjectMac.mm) present in places like WebCore/accessibility/[atk|mac], there are other bits in WebCore/accessibility that are platform specific as well (e.g. AccessibilityRenderObject). These bits are guarded with "#if PLATFORM" macros, which would still be necessary to meet the different requirements of each port. * The number of ports adding support for accessibility is increasing, some of them sharing code already (e.g. EFL and GTK port, both use ATK), so I believe that would be a nice move to make. Of course, we could always add an exception to the style checker, but I feel like relocating things would be a better approach in this case, thinking of the long term. What do you think? Thanks, Mario ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
