Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On Wed, Jul 11, 2012 at 4:53 PM, Ryosuke Niwa rn...@webkit.org wrote: On Jul 11, 2012 8:43 AM, John Mellor joh...@chromium.org wrote: On Wed, Jul 11, 2012 at 4:21 PM, Ryosuke Niwa rn...@webkit.org wrote: What's the point of adding this comment when the URL contains all the information? All we need is the URL. If anything, we should be describing the difference between the inline boxes in CSS2.1 and our implementation instead. That would be great! I agree that there's probably limited value in just copy/pasting from specs like I did. Linking to the spec something is based on and describing the differences would add a lot of value. The problem is that we'll then incur the maintenace cost of keeping comments up-to-date and the risk of them getting out-of-date as we have previously discussed. - Ryosuke Like many others on this thread, I don't buy this argument. It's well known that developers spend much more time reading understanding code than writing it. This is especially true in an inevitably complex codebase like WebKit, with many interdependencies. Well chosen comments that add value (like describing the deviations from a spec) are worth maintaining. Indeed, 12 months down the line, the original author will probably have forgotten enough detail that they themselves would have benefited by appropriately commenting their code. Even to check simple things like whether some method can return null you typically have to grep through all references to the thing being returned, and often in turn through all references to those. It's not feasible for developers to read the full transitive closure of code that interacts with every class they use, so they skim it and make mistakes. These mistakes often then slip through code review because they look reasonable. The bugs caused by these incorrect assumptions (that wouldn't have occurred with appropriate comments) seem much more concerning than the hypothetical bugs caused by stale comments. To take an arbitrary example, lets say that while iterating through a ListHashSet something causes entries to be deleted. Intuitively it seems this needn't invalidate the iterator, as long as the entry the iterator is currently pointing to isn't removed. But is that actually the case in this particular implementation? A well-documented library like Java's LinkedHashSethttp://docs.oracle.com/javase/6/docs/api/java/util/LinkedHashSet.html will warn you if the set is modified at any time after the iterator is created, in any way except through the iterator's own remove method, the iterator will throw a ConcurrentModificationException and that's that. I just tried to find this out in WebKit and had to read though ListHashSetIterator, ListHashSetConstIterator, ListHashSetNode, ListHashSet::remove, ListHashSet::unlinkAndDelete, HashTable::remove, HashTable::internalCheckTableConsistency, HashTable::removeWithoutEntryConsistencyCheck, HashTable::removeAndInvalidateWithoutEntryConsistencyCheck, HashTable::invalidateIterators, HashTable::shrink, HashTable::rehash, ListHashSet::add, HashTable::add, HashTable::makeKnownGoodIterator, HashTableIterator, HashTable::AddResult and ListHashSetTranslator::translate, and even learn about using the template keywordhttp://stackoverflow.com/questions/610245/where-and-why-do-i-have-to-put-the-template-and-typename-keywords for disambiguation. Eventually there was enough information to conclude that yes, it probably is safe since the ListHashSetNodes are allocated on the heap by ListHashSetTranslator::translate, so even though the HashTable invalidates its own iterators and HashTable::rehash may reallocate its storage, the actual ListHashSetNode pointed to by ListHashSetConstInterator should continue to exist. But constantly having to do such deep research makes coding highly inefficient, and there's a high risk of making errors. All the best, John ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] Web APIs and class name collisions
Hello Webkittens! While implementing the Region interface ( http://dev.w3.org/csswg/css3-regions/#the-region-interface ) I've noticed that the name Region is already taken by a class in platform/graphics. I'd like to know what's the best approach in these kind of situations: 1. Rename the existing WebCore class to something else and use the name Region for the Web API so there's parity between the implementation and the spec 2. Somehow prefix the Web API implementation class name? As the Web APIs expand I suppose this situation may occur again in the future and I suppose there should be a rule describing what's the best approach to take. Thanks! Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Web APIs and class name collisions
One common thing we do is prefix DOM to DOM-level concepts. For example, DOMWindow and DOMFileSystem. I'm not sure if we have an established convention for CSS-level concepts. Adam On Thu, Jul 12, 2012 at 9:18 AM, Andrei Bucur abu...@adobe.com wrote: Hello Webkittens! While implementing the Region interface ( http://dev.w3.org/csswg/css3-regions/#the-region-interface ) I've noticed that the name Region is already taken by a class in platform/graphics. I'd like to know what's the best approach in these kind of situations: 1. Rename the existing WebCore class to something else and use the name Region for the Web API so there's parity between the implementation and the spec 2. Somehow prefix the Web API implementation class name? As the Web APIs expand I suppose this situation may occur again in the future and I suppose there should be a rule describing what's the best approach to take. Thanks! Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Web APIs and class name collisions
The spec itself consistently and deliberately calls them CSS Regions, so a CSS prefix could be appropriate. Thanks, Alan From: Adam Barth aba...@webkit.orgmailto:aba...@webkit.org To: Andrei Bucur abu...@adobe.commailto:abu...@adobe.com Cc: webkit-dev@lists.webkit.orgmailto:webkit-dev@lists.webkit.org webkit-dev@lists.webkit.orgmailto:webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Web APIs and class name collisions One common thing we do is prefix DOM to DOM-level concepts. For example, DOMWindow and DOMFileSystem. I'm not sure if we have an established convention for CSS-level concepts. Adam On Thu, Jul 12, 2012 at 9:18 AM, Andrei Bucur abu...@adobe.commailto:abu...@adobe.com wrote: Hello Webkittens! While implementing the Region interface ( http://dev.w3.org/csswg/css3-regions/#the-region-interface ) I've noticed that the name Region is already taken by a class in platform/graphics. I'd like to know what's the best approach in these kind of situations: 1. Rename the existing WebCore class to something else and use the name Region for the Web API so there's parity between the implementation and the spec 2. Somehow prefix the Web API implementation class name? As the Web APIs expand I suppose this situation may occur again in the future and I suppose there should be a rule describing what's the best approach to take. Thanks! Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.orgmailto:webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On Jul 12, 2012, at 6:50 AM, John Mellor joh...@chromium.org wrote: To take an arbitrary example, lets say that while iterating through a ListHashSet something causes entries to be deleted. Intuitively it seems this needn't invalidate the iterator, as long as the entry the iterator is currently pointing to isn't removed. But is that actually the case in this particular implementation? A well-documented library like Java's LinkedHashSet will warn you if the set is modified at any time after the iterator is created, in any way except through the iterator's own remove method, the iterator will throw a ConcurrentModificationException and that's that. I just tried to find this out in WebKit and had to read though ListHashSetIterator, ListHashSetConstIterator, ListHashSetNode, ListHashSet::remove, ListHashSet::unlinkAndDelete, HashTable::remove, HashTable::internalCheckTableConsistency, HashTable::removeWithoutEntryConsistencyCheck, HashTable::removeAndInvalidateWithoutEntryConsistencyCheck, HashTable::invalidateIterators, HashTable::shrink, HashTable::rehash, ListHashSet::add, HashTable::add, HashTable::makeKnownGoodIterator, HashTableIterator, HashTable::AddResult and ListHashSetTranslator::translate, and even learn about using the template keyword for disambiguation. Eventually there was enough information to conclude that yes, it probably is safe since the ListHashSetNodes are allocated on the heap by ListHashSetTranslator::translate, so even though the HashTable invalidates its own iterators and HashTable::rehash may reallocate its storage, the actual ListHashSetNode pointed to by ListHashSetConstInterator should continue to exist. But constantly having to do such deep research makes coding highly inefficient, and there's a high risk of making errors. I think documenting the public interfaces of core data structures is probably a worthwhile tradeoff, since they change rarely but are used in many places. In fact, ListHashSet already has a class comment and documents some method behavior that is not obvious from the signature. Covering the issue you mention seems worthwhile as well. https://bugs.webkit.org/show_bug.cgi?id=91106 Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Web APIs and class name collisions
From my knowledge the CSS prefix is reserved for the CSS engine classes in WebKit. Prefixing the Region class with CSS could prove confusing. Regards, Andrei. From: Alan Stearns stea...@adobe.commailto:stea...@adobe.com Date: Thursday, July 12, 2012 7:39 PM To: Adam Barth aba...@webkit.orgmailto:aba...@webkit.org, Andrei Bucur abu...@adobe.commailto:abu...@adobe.com Cc: webkit-dev@lists.webkit.orgmailto:webkit-dev@lists.webkit.org webkit-dev@lists.webkit.orgmailto:webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Web APIs and class name collisions The spec itself consistently and deliberately calls them CSS Regions, so a CSS prefix could be appropriate. Thanks, Alan From: Adam Barth aba...@webkit.orgmailto:aba...@webkit.org To: Andrei Bucur abu...@adobe.commailto:abu...@adobe.com Cc: webkit-dev@lists.webkit.orgmailto:webkit-dev@lists.webkit.org webkit-dev@lists.webkit.orgmailto:webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Web APIs and class name collisions One common thing we do is prefix DOM to DOM-level concepts. For example, DOMWindow and DOMFileSystem. I'm not sure if we have an established convention for CSS-level concepts. Adam On Thu, Jul 12, 2012 at 9:18 AM, Andrei Bucur abu...@adobe.commailto:abu...@adobe.com wrote: Hello Webkittens! While implementing the Region interface ( http://dev.w3.org/csswg/css3-regions/#the-region-interface ) I've noticed that the name Region is already taken by a class in platform/graphics. I'd like to know what's the best approach in these kind of situations: 1. Rename the existing WebCore class to something else and use the name Region for the Web API so there's parity between the implementation and the spec 2. Somehow prefix the Web API implementation class name? As the Web APIs expand I suppose this situation may occur again in the future and I suppose there should be a rule describing what's the best approach to take. Thanks! Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.orgmailto:webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Web APIs and class name collisions
I'd vote for CSSRegion or CSSOMRegion for the class you're adding but I'll also suggest we rename the existing Region class now that the term region has a specific semantic in CSS. Maybe LayoutRegion or ScreenRegion? - Ryosuke On Jul 12, 2012 10:13 AM, Eric Seidel e...@webkit.org wrote: I would go with CSSRegion, and stick it in the css/ folder. Much of the CSS folder is our implementation of the CSS Object Model (CSSOM). At some point it might make sense to pull all the classes which implement the CSSOM out of css/ into a new cssom/ similar to dom/, but that's a later discussion. -eric On Thu, Jul 12, 2012 at 10:03 AM, Andrei Bucur abu...@adobe.com wrote: From my knowledge the CSS prefix is reserved for the CSS engine classes in WebKit. Prefixing the Region class with CSS could prove confusing. Regards, Andrei. From: Alan Stearns stea...@adobe.com Date: Thursday, July 12, 2012 7:39 PM To: Adam Barth aba...@webkit.org, Andrei Bucur abu...@adobe.com Cc: webkit-dev@lists.webkit.org webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Web APIs and class name collisions The spec itself consistently and deliberately calls them CSS Regions, so a CSS prefix could be appropriate. Thanks, Alan From: Adam Barth aba...@webkit.org To: Andrei Bucur abu...@adobe.com Cc: webkit-dev@lists.webkit.org webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Web APIs and class name collisions One common thing we do is prefix DOM to DOM-level concepts. For example, DOMWindow and DOMFileSystem. I'm not sure if we have an established convention for CSS-level concepts. Adam On Thu, Jul 12, 2012 at 9:18 AM, Andrei Bucur abu...@adobe.com wrote: Hello Webkittens! While implementing the Region interface ( http://dev.w3.org/csswg/css3-regions/#the-region-interface ) I've noticed that the name Region is already taken by a class in platform/graphics. I'd like to know what's the best approach in these kind of situations: Rename the existing WebCore class to something else and use the name Region for the Web API so there's parity between the implementation and the spec Somehow prefix the Web API implementation class name? As the Web APIs expand I suppose this situation may occur again in the future and I suppose there should be a rule describing what's the best approach to take. Thanks! Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Web APIs and class name collisions
On Thu, Jul 12, 2012 at 1:25 PM, Ryosuke Niwa rn...@webkit.org wrote: I'd vote for CSSRegion or CSSOMRegion for the class you're adding but I'll also suggest we rename the existing Region class now that the term region has a specific semantic in CSS. Maybe LayoutRegion or ScreenRegion? IntRegion? It seems closer to an IntRect than a LayoutRect. - Ryosuke On Jul 12, 2012 10:13 AM, Eric Seidel e...@webkit.org wrote: I would go with CSSRegion, and stick it in the css/ folder. Much of the CSS folder is our implementation of the CSS Object Model (CSSOM). At some point it might make sense to pull all the classes which implement the CSSOM out of css/ into a new cssom/ similar to dom/, but that's a later discussion. -eric On Thu, Jul 12, 2012 at 10:03 AM, Andrei Bucur abu...@adobe.com wrote: From my knowledge the CSS prefix is reserved for the CSS engine classes in WebKit. Prefixing the Region class with CSS could prove confusing. Regards, Andrei. From: Alan Stearns stea...@adobe.com Date: Thursday, July 12, 2012 7:39 PM To: Adam Barth aba...@webkit.org, Andrei Bucur abu...@adobe.com Cc: webkit-dev@lists.webkit.org webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Web APIs and class name collisions The spec itself consistently and deliberately calls them CSS Regions, so a CSS prefix could be appropriate. Thanks, Alan From: Adam Barth aba...@webkit.org To: Andrei Bucur abu...@adobe.com Cc: webkit-dev@lists.webkit.org webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Web APIs and class name collisions One common thing we do is prefix DOM to DOM-level concepts. For example, DOMWindow and DOMFileSystem. I'm not sure if we have an established convention for CSS-level concepts. Adam On Thu, Jul 12, 2012 at 9:18 AM, Andrei Bucur abu...@adobe.com wrote: Hello Webkittens! While implementing the Region interface ( http://dev.w3.org/csswg/css3-regions/#the-region-interface ) I've noticed that the name Region is already taken by a class in platform/graphics. I'd like to know what's the best approach in these kind of situations: Rename the existing WebCore class to something else and use the name Region for the Web API so there's parity between the implementation and the spec Somehow prefix the Web API implementation class name? As the Web APIs expand I suppose this situation may occur again in the future and I suppose there should be a rule describing what's the best approach to take. Thanks! Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On Wed, Jul 11, 2012 at 6:07 PM, Alec Flett alecfl...@chromium.org wrote: I absolutely do not buy that the cost of keeping comments up to date and the cost of out-of-date comments outweighs the benefits - that has NEVER been my experience and if anything the benefits of comments grow as the number of people on a project grows. 20 minutes of your time documenting/updating comments is worth 10 minutes of 10 (or 100) developers' time *not* groveling through code. I also do not buy that all code can be written in a self-documenting fashion. That's like saying that nobody needs to write books about anything spec'ed by the W3C. Code is not documentation no more than a spec is code. The alternatives to comments are themselves subject to the out-of-date problem to an even greater degree. Someone modifying or reviewing code with comments nearby is far more likely to notice a problem with the comments than they are to know that some blog post somewhere is now out of date due to the code change. Even community knowledge is subject to out-of-date problems, because most people don't look at most patches. If anything I'm a firm believer that taking the time to write a comment forces you to write code correctly the first time. i.e. if I have to write in a comment this is true except in this one esoteric case then it forces me to reexamine that esoteric case and ask myself if that exception is really necessary. In fact the way I generally write my code in WebKit is to comment heavily so I can keep track of my own intentions, and then just before sending off for review, strip all my code of comments, and that's truly a waste. This is an excellent and compelling reason to have comments, and documentation in general. I cannot count the number of times I have discovered problems with something at the time I was forced to write about it. While Changelogs and bug reports are intended to enforce this level of thinking, in most cases they are not as specific as I would expect comments to be, particularly for medium to large patches. Even if the comment is out of date, the time spent verifying that it is incorrect is no different to the time one should spend verifying that a change itself is correct. Finally, arguing that something is bad because it might get out-of-date is not particularly helpful. Much of the world's information goes out of date, yet society still chooses to provide it. Everything from transit schedules to online help pages. The anti-comment argument here is really that it is not worth the effort of keeping the comments up to date. As several people have shown, it is quite easy to come up with a formula that shows the cost of maintaining comments is much lower than the cost of living without. Cheers, Stephen. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Web APIs and class name collisions
I'd prefer we keep Region for the low-level graphics primitive Region (just like Path), and use something prefixed for the higher-level layout concept. Simon On Jul 12, 2012, at 10:26 AM, Dana Jansens wrote: On Thu, Jul 12, 2012 at 1:25 PM, Ryosuke Niwa rn...@webkit.org wrote: I'd vote for CSSRegion or CSSOMRegion for the class you're adding but I'll also suggest we rename the existing Region class now that the term region has a specific semantic in CSS. Maybe LayoutRegion or ScreenRegion? IntRegion? It seems closer to an IntRect than a LayoutRect. - Ryosuke On Jul 12, 2012 10:13 AM, Eric Seidel e...@webkit.org wrote: I would go with CSSRegion, and stick it in the css/ folder. Much of the CSS folder is our implementation of the CSS Object Model (CSSOM). At some point it might make sense to pull all the classes which implement the CSSOM out of css/ into a new cssom/ similar to dom/, but that's a later discussion. -eric On Thu, Jul 12, 2012 at 10:03 AM, Andrei Bucur abu...@adobe.com wrote: From my knowledge the CSS prefix is reserved for the CSS engine classes in WebKit. Prefixing the Region class with CSS could prove confusing. Regards, Andrei. From: Alan Stearns stea...@adobe.com Date: Thursday, July 12, 2012 7:39 PM To: Adam Barth aba...@webkit.org, Andrei Bucur abu...@adobe.com Cc: webkit-dev@lists.webkit.org webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Web APIs and class name collisions The spec itself consistently and deliberately calls them CSS Regions, so a CSS prefix could be appropriate. Thanks, Alan From: Adam Barth aba...@webkit.org To: Andrei Bucur abu...@adobe.com Cc: webkit-dev@lists.webkit.org webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Web APIs and class name collisions One common thing we do is prefix DOM to DOM-level concepts. For example, DOMWindow and DOMFileSystem. I'm not sure if we have an established convention for CSS-level concepts. Adam On Thu, Jul 12, 2012 at 9:18 AM, Andrei Bucur abu...@adobe.com wrote: Hello Webkittens! While implementing the Region interface ( http://dev.w3.org/csswg/css3-regions/#the-region-interface ) I've noticed that the name Region is already taken by a class in platform/graphics. I'd like to know what's the best approach in these kind of situations: Rename the existing WebCore class to something else and use the name Region for the Web API so there's parity between the implementation and the spec Somehow prefix the Web API implementation class name? As the Web APIs expand I suppose this situation may occur again in the future and I suppose there should be a rule describing what's the best approach to take. Thanks! Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Web APIs and class name collisions
I'd prefer we keep Region for the low-level graphics primitive Region (just like Path), and use something prefixed for the higher-level layout concept. Simon On Jul 12, 2012, at 10:26 AM, Dana Jansens wrote: On Thu, Jul 12, 2012 at 1:25 PM, Ryosuke Niwa rn...@webkit.org wrote: I'd vote for CSSRegion or CSSOMRegion for the class you're adding but I'll also suggest we rename the existing Region class now that the term region has a specific semantic in CSS. Maybe LayoutRegion or ScreenRegion? IntRegion? It seems closer to an IntRect than a LayoutRect. - Ryosuke On Jul 12, 2012 10:13 AM, Eric Seidel e...@webkit.org wrote: I would go with CSSRegion, and stick it in the css/ folder. Much of the CSS folder is our implementation of the CSS Object Model (CSSOM). At some point it might make sense to pull all the classes which implement the CSSOM out of css/ into a new cssom/ similar to dom/, but that's a later discussion. -eric On Thu, Jul 12, 2012 at 10:03 AM, Andrei Bucur abu...@adobe.com wrote: From my knowledge the CSS prefix is reserved for the CSS engine classes in WebKit. Prefixing the Region class with CSS could prove confusing. Regards, Andrei. From: Alan Stearns stea...@adobe.com Date: Thursday, July 12, 2012 7:39 PM To: Adam Barth aba...@webkit.org, Andrei Bucur abu...@adobe.com Cc: webkit-dev@lists.webkit.org webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Web APIs and class name collisions The spec itself consistently and deliberately calls them CSS Regions, so a CSS prefix could be appropriate. Thanks, Alan From: Adam Barth aba...@webkit.org To: Andrei Bucur abu...@adobe.com Cc: webkit-dev@lists.webkit.org webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Web APIs and class name collisions One common thing we do is prefix DOM to DOM-level concepts. For example, DOMWindow and DOMFileSystem. I'm not sure if we have an established convention for CSS-level concepts. Adam On Thu, Jul 12, 2012 at 9:18 AM, Andrei Bucur abu...@adobe.com wrote: Hello Webkittens! While implementing the Region interface ( http://dev.w3.org/csswg/css3-regions/#the-region-interface ) I've noticed that the name Region is already taken by a class in platform/graphics. I'd like to know what's the best approach in these kind of situations: Rename the existing WebCore class to something else and use the name Region for the Web API so there's parity between the implementation and the spec Somehow prefix the Web API implementation class name? As the Web APIs expand I suppose this situation may occur again in the future and I suppose there should be a rule describing what's the best approach to take. Thanks! Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On Thu, Jul 12, 2012 at 10:43 AM, Stephen Chenney schen...@chromium.orgwrote: As several people have shown, it is quite easy to come up with a formula that shows the cost of maintaining comments is much lower than the cost of living without. I object to that conclusion. I've never seen any scientifically sound data posted on this thread either for or against having comments. Furthermore, just because we can come up with a formula doesn't mean that the formula models the nature of the world well. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Removing BUILDING_ON / TARGETING macros in favor of system availability macros
We just discovered (via a rollout on bug 91103) that there’s a bug
with __MAC_OS_X_VERSION_MAX_ALLOWED in the version of the 10.5 SDK as
present in Xcode 3.2.6. This may be the last version of the 10.5 SDK ever
released (I haven’t checked all of the early Xcode 4 releases). Chromium
uses this SDK for its builds, although will likely switch to the 10.6 SDK
in the near future.
In short, __MAC_OS_X_VERSION_MAX_ALLOWED, which is supposed to track the
SDK version, is set to 1060 instead of the expected 1050 in this version of
the 10.5 SDK.
--
// clang t.c -isysroot /Xcode3.2/SDKs/MacOSX10.5.sdk
-mmacosx-version-min=10.4 -o t
#include Availability.h
#include AvailabilityMacros.h
#include stdio.h
int main(int argc, char* argv[]) {
#define PRINT_MACRO(m) printf(%s = %d\n, # m, m)
PRINT_MACRO(MAC_OS_X_VERSION_MIN_REQUIRED);
PRINT_MACRO(MAC_OS_X_VERSION_MAX_ALLOWED);
PRINT_MACRO(__MAC_OS_X_VERSION_MIN_REQUIRED);
PRINT_MACRO(__MAC_OS_X_VERSION_MAX_ALLOWED);
#undef PRINT_MACRO
return 0;
}
--
produces
MAC_OS_X_VERSION_MIN_REQUIRED = 1040
MAC_OS_X_VERSION_MAX_ALLOWED = 1050
__MAC_OS_X_VERSION_MIN_REQUIRED = 1040
__MAC_OS_X_VERSION_MAX_ALLOWED = 1060
Oops.
I traced this problem to AvailabilityInternal.h, which in this SDK, says
(slightly abbreviated):
#elif defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__)
#ifndef __MAC_OS_X_VERSION_MAX_ALLOWED
#define __MAC_OS_X_VERSION_MAX_ALLOWED __MAC_10_6
#endif
#endif
At least for the purposes of testing whether the 10.5 SDK is in use, we’ll
need to use AvailabilityMacros.h’s MAC_OS_X_VERSION_MAX_ALLOWED instead
of Availability.h’s __MAC_OS_X_VERSION_MAX_ALLOWED. I guess it’s a
question of style whether the rest of WebKit should follow.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Removing BUILDING_ON / TARGETING macros in favor of system availability macros
Mark Rowe wrote: It'd be complicated to do this more widely since the AvailabilityMacros.h version of the macros are defined even when building for iOS. We'd need to review all of the uses to ensure that they were handled correctly. Given that I think it'd be better to just use this as a localized workaround until we drop support for building for Leopard. The one place where we found it to be significant so far is in a Chromium-specific file that’s not relevant to iOS, and we’re just going to use MAC_OS_X_VERSION_MAX_ALLOWED in that one instance for the time being. If it happens anywhere else with this SDK, it’ll likely also be Chromium-specific, and we’ll just deal with the ugliness of having to use a different macro for this purpose until we get rid of the 10.5 SDK altogether. Shouldn’t be long. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Web APIs and class name collisions
So far in the css/ directory we tried to renamed slowly classes so that : CSS* prefixed classes are the implementation of CSSOM whatevername is for internal classes. For example we renamed CSSStyleApplyProperty class to StyleBuilder because it's internal. Hope that helps. On Thu, Jul 12, 2012 at 2:52 PM, Simon Fraser simon.fra...@apple.com wrote: I'd prefer we keep Region for the low-level graphics primitive Region (just like Path), and use something prefixed for the higher-level layout concept. Simon On Jul 12, 2012, at 10:26 AM, Dana Jansens wrote: On Thu, Jul 12, 2012 at 1:25 PM, Ryosuke Niwa rn...@webkit.org wrote: I'd vote for CSSRegion or CSSOMRegion for the class you're adding but I'll also suggest we rename the existing Region class now that the term region has a specific semantic in CSS. Maybe LayoutRegion or ScreenRegion? IntRegion? It seems closer to an IntRect than a LayoutRect. - Ryosuke On Jul 12, 2012 10:13 AM, Eric Seidel e...@webkit.org wrote: I would go with CSSRegion, and stick it in the css/ folder. Much of the CSS folder is our implementation of the CSS Object Model (CSSOM). At some point it might make sense to pull all the classes which implement the CSSOM out of css/ into a new cssom/ similar to dom/, but that's a later discussion. -eric On Thu, Jul 12, 2012 at 10:03 AM, Andrei Bucur abu...@adobe.com wrote: From my knowledge the CSS prefix is reserved for the CSS engine classes in WebKit. Prefixing the Region class with CSS could prove confusing. Regards, Andrei. From: Alan Stearns stea...@adobe.com Date: Thursday, July 12, 2012 7:39 PM To: Adam Barth aba...@webkit.org, Andrei Bucur abu...@adobe.com Cc: webkit-dev@lists.webkit.org webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Web APIs and class name collisions The spec itself consistently and deliberately calls them CSS Regions, so a CSS prefix could be appropriate. Thanks, Alan From: Adam Barth aba...@webkit.org To: Andrei Bucur abu...@adobe.com Cc: webkit-dev@lists.webkit.org webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Web APIs and class name collisions One common thing we do is prefix DOM to DOM-level concepts. For example, DOMWindow and DOMFileSystem. I'm not sure if we have an established convention for CSS-level concepts. Adam On Thu, Jul 12, 2012 at 9:18 AM, Andrei Bucur abu...@adobe.com wrote: Hello Webkittens! While implementing the Region interface ( http://dev.w3.org/csswg/css3-regions/#the-region-interface ) I've noticed that the name Region is already taken by a class in platform/graphics. I'd like to know what's the best approach in these kind of situations: Rename the existing WebCore class to something else and use the name Region for the Web API so there's parity between the implementation and the spec Somehow prefix the Web API implementation class name? As the Web APIs expand I suppose this situation may occur again in the future and I suppose there should be a rule describing what's the best approach to take. Thanks! Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev -- Alexis Menard (darktears) Software Engineer openBossa @ INdT - Instituto Nokia de Tecnologia ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On Thu, Jul 12, 2012 at 10:53 AM, Ryosuke Niwa rn...@webkit.org wrote: On Thu, Jul 12, 2012 at 10:43 AM, Stephen Chenney schen...@chromium.org wrote: As several people have shown, it is quite easy to come up with a formula that shows the cost of maintaining comments is much lower than the cost of living without. I object to that conclusion. I've never seen any scientifically sound data posted on this thread either for or against having comments. Furthermore, just because we can come up with a formula doesn't mean that the formula models the nature of the world well. This is certainly true. I doubt you will see such a study, because it's very culturally-specific (in the sense that every group working on a shared code base is a culture). All code reading is training; you have to learn the styles and idioms of the codebase. As an aside, in WebKit, I think the culture is actually actually anti-comment enough that it's trained some people to not read comments at all, and so the value of comments is even lower. I have often had people ask questions about code in reviews that were answered by comments right above the lines in question :). -- Dirk ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Web APIs and class name collisions
I for one, very much appreciate your cleanup/organizational efforts! On Thu, Jul 12, 2012 at 12:37 PM, Alexis Menard alexis.men...@openbossa.org wrote: So far in the css/ directory we tried to renamed slowly classes so that : CSS* prefixed classes are the implementation of CSSOM whatevername is for internal classes. For example we renamed CSSStyleApplyProperty class to StyleBuilder because it's internal. Hope that helps. On Thu, Jul 12, 2012 at 2:52 PM, Simon Fraser simon.fra...@apple.com wrote: I'd prefer we keep Region for the low-level graphics primitive Region (just like Path), and use something prefixed for the higher-level layout concept. Simon On Jul 12, 2012, at 10:26 AM, Dana Jansens wrote: On Thu, Jul 12, 2012 at 1:25 PM, Ryosuke Niwa rn...@webkit.org wrote: I'd vote for CSSRegion or CSSOMRegion for the class you're adding but I'll also suggest we rename the existing Region class now that the term region has a specific semantic in CSS. Maybe LayoutRegion or ScreenRegion? IntRegion? It seems closer to an IntRect than a LayoutRect. - Ryosuke On Jul 12, 2012 10:13 AM, Eric Seidel e...@webkit.org wrote: I would go with CSSRegion, and stick it in the css/ folder. Much of the CSS folder is our implementation of the CSS Object Model (CSSOM). At some point it might make sense to pull all the classes which implement the CSSOM out of css/ into a new cssom/ similar to dom/, but that's a later discussion. -eric On Thu, Jul 12, 2012 at 10:03 AM, Andrei Bucur abu...@adobe.com wrote: From my knowledge the CSS prefix is reserved for the CSS engine classes in WebKit. Prefixing the Region class with CSS could prove confusing. Regards, Andrei. From: Alan Stearns stea...@adobe.com Date: Thursday, July 12, 2012 7:39 PM To: Adam Barth aba...@webkit.org, Andrei Bucur abu...@adobe.com Cc: webkit-dev@lists.webkit.org webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Web APIs and class name collisions The spec itself consistently and deliberately calls them CSS Regions, so a CSS prefix could be appropriate. Thanks, Alan From: Adam Barth aba...@webkit.org To: Andrei Bucur abu...@adobe.com Cc: webkit-dev@lists.webkit.org webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Web APIs and class name collisions One common thing we do is prefix DOM to DOM-level concepts. For example, DOMWindow and DOMFileSystem. I'm not sure if we have an established convention for CSS-level concepts. Adam On Thu, Jul 12, 2012 at 9:18 AM, Andrei Bucur abu...@adobe.com wrote: Hello Webkittens! While implementing the Region interface ( http://dev.w3.org/csswg/css3-regions/#the-region-interface ) I've noticed that the name Region is already taken by a class in platform/graphics. I'd like to know what's the best approach in these kind of situations: Rename the existing WebCore class to something else and use the name Region for the Web API so there's parity between the implementation and the spec Somehow prefix the Web API implementation class name? As the Web APIs expand I suppose this situation may occur again in the future and I suppose there should be a rule describing what's the best approach to take. Thanks! Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev -- Alexis Menard (darktears) Software Engineer openBossa @ INdT - Instituto Nokia de Tecnologia ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On Thu, Jul 12, 2012 at 3:44 PM, Dirk Pranke dpra...@chromium.org wrote: On Thu, Jul 12, 2012 at 10:53 AM, Ryosuke Niwa rn...@webkit.org wrote: On Thu, Jul 12, 2012 at 10:43 AM, Stephen Chenney schen...@chromium.org wrote: As several people have shown, it is quite easy to come up with a formula that shows the cost of maintaining comments is much lower than the cost of living without. I object to that conclusion. I've never seen any scientifically sound data posted on this thread either for or against having comments. Furthermore, just because we can come up with a formula doesn't mean that the formula models the nature of the world well. This is certainly true. I doubt you will see such a study, because it's very culturally-specific (in the sense that every group working on a shared code base is a culture). I should have been clearer. In this email thread there have been guesstimates of the form: Cost per year with poor commenting: t_understandWithoutComment * n_engineersNeedingToUnderstand Cost per year with good comments: t_maintainComments * n_patches + t_understandWithComment * n_engineersNeedingToUnderstand All costs are per-code unit and will likely vary depending on the code. We are better off without comments if: t_understandWithoutComment t_maintainComments * n_patches / n_engineers + t_understandWithComment We can argue about the appropriate values for t_* and n_*. The primary observation is that the benefit of comments rises as more engineers need to understand the code and patch levels (behavior changes) stay reasonably constant. More behavioral changes argue for fewer comments. We can all have fun fiddling the numbers to make our cases. Go for it in the privacy of your own home. For anything that looks like core code where lots of engineers need to understand it, I think commenting wins most of the time. My current favorite is inDocument(). For peripheral code with frequent changes in behavior, it's not worth it. There's a big grey area in between (like Ryosuke's editing code) where the best approach is not obvious. As an aside, in WebKit, I think the culture is actually actually anti-comment enough that it's trained some people to not read comments at all, and so the value of comments is even lower. I have often had people ask questions about code in reviews that were answered by comments right above the lines in question :). -- Dirk Absolutely. At heart this is a cultural issue and we could let it all play out by pushing our individual opinions in patches and reviews. Over time we'll see what happens. Cheers, Stephen. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On Thu, Jul 12, 2012 at 1:47 PM, Stephen Chenney schen...@chromium.orgwrote: On Thu, Jul 12, 2012 at 3:44 PM, Dirk Pranke dpra...@chromium.org wrote: On Thu, Jul 12, 2012 at 10:53 AM, Ryosuke Niwa rn...@webkit.org wrote: On Thu, Jul 12, 2012 at 10:43 AM, Stephen Chenney schen...@chromium.org wrote: As several people have shown, it is quite easy to come up with a formula that shows the cost of maintaining comments is much lower than the cost of living without. I object to that conclusion. I've never seen any scientifically sound data posted on this thread either for or against having comments. Furthermore, just because we can come up with a formula doesn't mean that the formula models the nature of the world well. This is certainly true. I doubt you will see such a study, because it's very culturally-specific (in the sense that every group working on a shared code base is a culture). I should have been clearer. In this email thread there have been guesstimates of the form: Thanks for the clarification. That makes a lot more sense. Cost per year with poor commenting: t_understandWithoutComment * n_engineersNeedingToUnderstand Cost per year with good comments: t_maintainComments * n_patches + t_understandWithComment * n_engineersNeedingToUnderstand poor and good are not really useful distinction because we can all agree that good comments are good. Also, we can't have all poor comments or all good comments. Thus, I postulate that the following formula will more likely depict the world accurately: Cost(engineer, numberOfComments) = timeToReadCodeWithoutComments(engineer, numberOfComments) + timeToReadCodeWithComments(engineer, numberOfComments) + timeToMaintainComments(engineer, numberOfComments) + timeWastedOnWrongComments(engineer, numberOfComments) Cost(engineers, numberOfComments) = sum of cost(engineer, numberOfComments) over engineers. Even in this model, there is a gross simplification that only the number of comments, not quality or the length of each comment, matters. We need some experiments to see whether such a simplification can be acceptable or not. Now, let us further assume that all engineers are equal, or that we can someone determine the average engineer among us (pending another experiment here). So the above formula reduces to: Cost'(numberOfComments) = numberOfEngineers × (timeToReadCodeWithoutComments(engineer, numberOfComments) + timeToReadCodeWithComments(engineer, numberOfComments) + timeToMaintainComments(engineer, numberOfComments) + timeWastedOnWrongComments(engineer, numberOfComments)) And the optimization problem we're solving is the minimization of Cost'(numberOfComments) over numberOfComments. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On Thu, Jul 12, 2012 at 2:03 PM, Ryosuke Niwa rn...@webkit.org wrote: On Thu, Jul 12, 2012 at 1:47 PM, Stephen Chenney schen...@chromium.orgwrote: On Thu, Jul 12, 2012 at 3:44 PM, Dirk Pranke dpra...@chromium.orgwrote: On Thu, Jul 12, 2012 at 10:53 AM, Ryosuke Niwa rn...@webkit.org wrote: On Thu, Jul 12, 2012 at 10:43 AM, Stephen Chenney schen...@chromium.org wrote: As several people have shown, it is quite easy to come up with a formula that shows the cost of maintaining comments is much lower than the cost of living without. I object to that conclusion. I've never seen any scientifically sound data posted on this thread either for or against having comments. Furthermore, just because we can come up with a formula doesn't mean that the formula models the nature of the world well. This is certainly true. I doubt you will see such a study, because it's very culturally-specific (in the sense that every group working on a shared code base is a culture). I should have been clearer. In this email thread there have been guesstimates of the form: Thanks for the clarification. That makes a lot more sense. Cost per year with poor commenting: t_understandWithoutComment * n_engineersNeedingToUnderstand Cost per year with good comments: t_maintainComments * n_patches + t_understandWithComment * n_engineersNeedingToUnderstand poor and good are not really useful distinction because we can all agree that good comments are good. Also, we can't have all poor comments or all good comments. Thus, I postulate that the following formula will more likely depict the world accurately: Cost(engineer, numberOfComments) = timeToReadCodeWithoutComments(engineer, numberOfComments) + timeToReadCodeWithComments(engineer, numberOfComments) + timeToMaintainComments(engineer, numberOfComments) + timeWastedOnWrongComments(engineer, numberOfComments) Cost(engineers, numberOfComments) = sum of cost(engineer, numberOfComments) over engineers. Even in this model, there is a gross simplification that only the number of comments, not quality or the length of each comment, matters. We need some experiments to see whether such a simplification can be acceptable or not. Now, let us further assume that all engineers are equal, or that we can someone determine the average engineer among us (pending another experiment here). So the above formula reduces to: Cost'(numberOfComments) = numberOfEngineers × (timeToReadCodeWithoutComments(engineer, numberOfComments) + timeToReadCodeWithComments(engineer, numberOfComments) + timeToMaintainComments(engineer, numberOfComments) + timeWastedOnWrongComments(engineer, numberOfComments)) where engineer here is the average engineer. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On Jul 12, 2012, at 1:47 PM, Stephen Chenney schen...@chromium.org wrote: On Thu, Jul 12, 2012 at 3:44 PM, Dirk Pranke dpra...@chromium.org wrote: On Thu, Jul 12, 2012 at 10:53 AM, Ryosuke Niwa rn...@webkit.org wrote: On Thu, Jul 12, 2012 at 10:43 AM, Stephen Chenney schen...@chromium.org wrote: As several people have shown, it is quite easy to come up with a formula that shows the cost of maintaining comments is much lower than the cost of living without. I object to that conclusion. I've never seen any scientifically sound data posted on this thread either for or against having comments. Furthermore, just because we can come up with a formula doesn't mean that the formula models the nature of the world well. This is certainly true. I doubt you will see such a study, because it's very culturally-specific (in the sense that every group working on a shared code base is a culture). I should have been clearer. In this email thread there have been guesstimates of the form: Cost per year with poor commenting: t_understandWithoutComment * n_engineersNeedingToUnderstand Cost per year with good comments: t_maintainComments * n_patches + t_understandWithComment * n_engineersNeedingToUnderstand All costs are per-code unit and will likely vary depending on the code. We are better off without comments if: t_understandWithoutComment t_maintainComments * n_patches / n_engineers + t_understandWithComment We can argue about the appropriate values for t_* and n_*. The primary observation is that the benefit of comments rises as more engineers need to understand the code and patch levels (behavior changes) stay reasonably constant. More behavioral changes argue for fewer comments. Surely we would expect the project's n_patches to scale approximately linearly with n_engineers. Or if not, I'm not super concerned about helping the engineers who are not contributing patches. You've also failed to account for the cost of misleading comments, and the cost of comments that add no information value (like m_refCount++; // increase reference count by 1). These can significantly hurt understandability. You may think my example of a low-information comment is a parody, but I've noticed that there is a high correlation between people who want to follow a policy of lots of comments and tendency to add comments almost exactly like that. If anyone doubts me, I can privately point you to some examples of comments in WebKit code that literally restate what the adjecent line of actual code does. I hate reading code like that, because it turns 1-page functions into 3-page functions. Thus, I'm much more interested in comments that pass the filter of people who prefer fewer comments and thus would spend their limited comment budget on ones that truly have value, than comments from people who believe in adding lots of comments. My Bayesian inference is that comments from the latter group have much lower average value per comment. When adding a comment, you should really think about whether the value outweighs the cost, just as when adding a line of actual functional code. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] PSA: rebaseline tooling
https://trac.webkit.org/wiki/Rebaseline I've recently consolidated the various rebaseline commands and made the tool work for non-Chromium ports. I especially like webkit-patch rebaseline path/to/test/i/just/broke.html, which lets you easily rebaseline the test for all ports once the bots have cycled. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] PSA: rebaseline tooling
On Thu, Jul 12, 2012 at 3:17 PM, Ojan Vafai o...@chromium.org wrote: https://trac.webkit.org/wiki/Rebaseline I've recently consolidated the various rebaseline commands and made the tool work for non-Chromium ports. I especially like webkit-patch rebaseline path/to/test/i/just/broke.html, which lets you easily rebaseline the test for all ports once the bots have cycled. Thanks Ojan! On a random note since you mentioned bots cycling. Is there any way for this tool/garden-o-matic to detect whether the bots it's pulling results from have cycled since a particular change? I've been bitten (and I know others have too) by trying to pull new baselines too early and then having to do it again, and having the tool tell me, say, the most recent revision that it has complete results for would be awesome. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] PSA: rebaseline tooling
At the top of the garden-o-matic page there is a line like Latest revision processed by every bot: 122499 (trunk is at 122524). I think that does what you want? On Thu, Jul 12, 2012 at 3:56 PM, Peter Kasting pkast...@google.com wrote: On Thu, Jul 12, 2012 at 3:17 PM, Ojan Vafai o...@chromium.org wrote: https://trac.webkit.org/wiki/Rebaseline I've recently consolidated the various rebaseline commands and made the tool work for non-Chromium ports. I especially like webkit-patch rebaseline path/to/test/i/just/broke.html, which lets you easily rebaseline the test for all ports once the bots have cycled. Thanks Ojan! On a random note since you mentioned bots cycling. Is there any way for this tool/garden-o-matic to detect whether the bots it's pulling results from have cycled since a particular change? I've been bitten (and I know others have too) by trying to pull new baselines too early and then having to do it again, and having the tool tell me, say, the most recent revision that it has complete results for would be awesome. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] PSA: rebaseline tooling
On Thu, Jul 12, 2012 at 4:16 PM, Dirk Pranke dpra...@chromium.org wrote: At the top of the garden-o-matic page there is a line like Latest revision processed by every bot: 122499 (trunk is at 122524). I think that does what you want? Ah, I hadn't noticed that. That sounds promising! PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] PSA: rebaseline tooling
If someone feels like doing extra hacking to improve this, I'm happy to walk you through the code. It'd be awesome if you could expand out that section to show which revision has been run on each bot. https://bugs.webkit.org/show_bug.cgi?id=91163 On Thu, Jul 12, 2012 at 4:18 PM, Peter Kasting pkast...@google.com wrote: On Thu, Jul 12, 2012 at 4:16 PM, Dirk Pranke dpra...@chromium.org wrote: At the top of the garden-o-matic page there is a line like Latest revision processed by every bot: 122499 (trunk is at 122524). I think that does what you want? Ah, I hadn't noticed that. That sounds promising! PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
