Re: [webkit-dev] Tightening up our include discipline
I'd be interested in performance cost to that, especially on a test that creates a lot of JS wrapped Events. -Sam On Oct 18, 2011, at 6:42 PM, Adam Barth wrote: My thought on that is to use a single virtual function and return an atomic string, like we do for elements. Adam On Oct 18, 2011 6:10 PM, Sam Weinig wei...@apple.com wrote: For the specific case of Event, we do need some sort of poor man's RTTI here for the sake of the bindings, but we could consider alternatives. Since you need to identify the subclasses, you need to either use many virtual functions, as we do currently for Event, or have some tagging mechanism, which we do for classes like Element and EventListener. If we use a tagging technique, we need someway to ensure that the tags are unique. In EventListener, we list the types up front in an enum, though this has same downside as the many virtual functions in that the tendrils of WebAudio reach into the base class. For Element, we rely on qualified names, which doesn't require the icky reach, but are slightly more costly. (You still end up with the tendrils in the JS base class). Of course, it might make sense to solve the issue of #ifdefs in Event by simply removing the #ifdefs in Event, since they are just base class implementations. The only downside of that is if we later remove the feature, it is harder to find all the places it touched. -Sam On Oct 18, 2011, at 4:37 PM, Adam Barth wrote: The other day, Sam chided me on a bug for including a header from WebCore proper in a file in WebCore/platform. Even though I know I'm not supposed to do that, it's an easy mistake to make. There's some work going on in https://bugs.webkit.org/show_bug.cgi?id=49192 to improve the style checker to flag these sorts of bad dependencies. This check isn't live yet, but don't be surprised if some robotic user complains about a bad include in one of your patches. (As always, if the bot complains incorrectly, please let me know so we can eliminate false positives.) Relatedly, as part of the recent ENABLE cleanup effort, I noticed that some files (e.g., http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Event.cpp) attract lots of ifdefs because many unrelated features need to register themselves. I was thinking it might be an improvement to restructure things a bit so that, for example, WebAudio wouldn't need to reach its fingers into Event.cpp and instead could be better contained in the WebCore/webaudio directory. I welcome any thoughts you have on either of these topics. Thanks, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Tightening up our include discipline
There shouldn't be any performance costs. Instead of returning a boolean (as the virtual function call does today), it will return a constant AtomicString which can be compared to another AtomicString in one instruction. Adam On Wed, Oct 19, 2011 at 1:31 PM, Sam Weinig wei...@apple.com wrote: I'd be interested in performance cost to that, especially on a test that creates a lot of JS wrapped Events. -Sam On Oct 18, 2011, at 6:42 PM, Adam Barth wrote: My thought on that is to use a single virtual function and return an atomic string, like we do for elements. Adam On Oct 18, 2011 6:10 PM, Sam Weinig wei...@apple.com wrote: For the specific case of Event, we do need some sort of poor man's RTTI here for the sake of the bindings, but we could consider alternatives. Since you need to identify the subclasses, you need to either use many virtual functions, as we do currently for Event, or have some tagging mechanism, which we do for classes like Element and EventListener. If we use a tagging technique, we need someway to ensure that the tags are unique. In EventListener, we list the types up front in an enum, though this has same downside as the many virtual functions in that the tendrils of WebAudio reach into the base class. For Element, we rely on qualified names, which doesn't require the icky reach, but are slightly more costly. (You still end up with the tendrils in the JS base class). Of course, it might make sense to solve the issue of #ifdefs in Event by simply removing the #ifdefs in Event, since they are just base class implementations. The only downside of that is if we later remove the feature, it is harder to find all the places it touched. -Sam On Oct 18, 2011, at 4:37 PM, Adam Barth wrote: The other day, Sam chided me on a bug for including a header from WebCore proper in a file in WebCore/platform. Even though I know I'm not supposed to do that, it's an easy mistake to make. There's some work going on in https://bugs.webkit.org/show_bug.cgi?id=49192 to improve the style checker to flag these sorts of bad dependencies. This check isn't live yet, but don't be surprised if some robotic user complains about a bad include in one of your patches. (As always, if the bot complains incorrectly, please let me know so we can eliminate false positives.) Relatedly, as part of the recent ENABLE cleanup effort, I noticed that some files (e.g., http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Event.cpp) attract lots of ifdefs because many unrelated features need to register themselves. I was thinking it might be an improvement to restructure things a bit so that, for example, WebAudio wouldn't need to reach its fingers into Event.cpp and instead could be better contained in the WebCore/webaudio directory. I welcome any thoughts you have on either of these topics. Thanks, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Tightening up our include discipline
The other day, Sam chided me on a bug for including a header from WebCore proper in a file in WebCore/platform. Even though I know I'm not supposed to do that, it's an easy mistake to make. There's some work going on in https://bugs.webkit.org/show_bug.cgi?id=49192 to improve the style checker to flag these sorts of bad dependencies. This check isn't live yet, but don't be surprised if some robotic user complains about a bad include in one of your patches. (As always, if the bot complains incorrectly, please let me know so we can eliminate false positives.) Relatedly, as part of the recent ENABLE cleanup effort, I noticed that some files (e.g., http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Event.cpp) attract lots of ifdefs because many unrelated features need to register themselves. I was thinking it might be an improvement to restructure things a bit so that, for example, WebAudio wouldn't need to reach its fingers into Event.cpp and instead could be better contained in the WebCore/webaudio directory. I welcome any thoughts you have on either of these topics. Thanks, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Tightening up our include discipline
For the specific case of Event, we do need some sort of poor man's RTTI here for the sake of the bindings, but we could consider alternatives. Since you need to identify the subclasses, you need to either use many virtual functions, as we do currently for Event, or have some tagging mechanism, which we do for classes like Element and EventListener. If we use a tagging technique, we need someway to ensure that the tags are unique. In EventListener, we list the types up front in an enum, though this has same downside as the many virtual functions in that the tendrils of WebAudio reach into the base class. For Element, we rely on qualified names, which doesn't require the icky reach, but are slightly more costly. (You still end up with the tendrils in the JS base class). Of course, it might make sense to solve the issue of #ifdefs in Event by simply removing the #ifdefs in Event, since they are just base class implementations. The only downside of that is if we later remove the feature, it is harder to find all the places it touched. -Sam On Oct 18, 2011, at 4:37 PM, Adam Barth wrote: The other day, Sam chided me on a bug for including a header from WebCore proper in a file in WebCore/platform. Even though I know I'm not supposed to do that, it's an easy mistake to make. There's some work going on in https://bugs.webkit.org/show_bug.cgi?id=49192 to improve the style checker to flag these sorts of bad dependencies. This check isn't live yet, but don't be surprised if some robotic user complains about a bad include in one of your patches. (As always, if the bot complains incorrectly, please let me know so we can eliminate false positives.) Relatedly, as part of the recent ENABLE cleanup effort, I noticed that some files (e.g., http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Event.cpp) attract lots of ifdefs because many unrelated features need to register themselves. I was thinking it might be an improvement to restructure things a bit so that, for example, WebAudio wouldn't need to reach its fingers into Event.cpp and instead could be better contained in the WebCore/webaudio directory. I welcome any thoughts you have on either of these topics. Thanks, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Tightening up our include discipline
My thought on that is to use a single virtual function and return an atomic string, like we do for elements. Adam On Oct 18, 2011 6:10 PM, Sam Weinig wei...@apple.com wrote: For the specific case of Event, we do need some sort of poor man's RTTI here for the sake of the bindings, but we could consider alternatives. Since you need to identify the subclasses, you need to either use many virtual functions, as we do currently for Event, or have some tagging mechanism, which we do for classes like Element and EventListener. If we use a tagging technique, we need someway to ensure that the tags are unique. In EventListener, we list the types up front in an enum, though this has same downside as the many virtual functions in that the tendrils of WebAudio reach into the base class. For Element, we rely on qualified names, which doesn't require the icky reach, but are slightly more costly. (You still end up with the tendrils in the JS base class). Of course, it might make sense to solve the issue of #ifdefs in Event by simply removing the #ifdefs in Event, since they are just base class implementations. The only downside of that is if we later remove the feature, it is harder to find all the places it touched. -Sam On Oct 18, 2011, at 4:37 PM, Adam Barth wrote: The other day, Sam chided me on a bug for including a header from WebCore proper in a file in WebCore/platform. Even though I know I'm not supposed to do that, it's an easy mistake to make. There's some work going on in https://bugs.webkit.org/show_bug.cgi?id=49192 to improve the style checker to flag these sorts of bad dependencies. This check isn't live yet, but don't be surprised if some robotic user complains about a bad include in one of your patches. (As always, if the bot complains incorrectly, please let me know so we can eliminate false positives.) Relatedly, as part of the recent ENABLE cleanup effort, I noticed that some files (e.g., http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Event.cpp) attract lots of ifdefs because many unrelated features need to register themselves. I was thinking it might be an improvement to restructure things a bit so that, for example, WebAudio wouldn't need to reach its fingers into Event.cpp and instead could be better contained in the WebCore/webaudio directory. I welcome any thoughts you have on either of these topics. Thanks, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
