Re: [webkit-dev] coding style and comments
On Sun, Jan 30, 2011 at 4:41 PM, Ryan Leavengood leaveng...@gmail.comwrote: Maybe the solution here is better documentation outside the source code. I hope some of the more experienced WebKit developers can agree that there are parts of the code that are harder for new developers to dig into. Can't agree more. Articles on http://webkit.org/coding/technical-articles.html are great, and I'd love to see more overviews of how various parts of WebKit work. Some high-level docs that are kept updated might be nice. Of course the key here is keeping them updated, and if it is hard to keep source code comments up to date I don't know how much hope there is for outside docs. I don't think it's realistic for us to keep it updated though because it'll be a huge burden to WebKit contributors. But as long as it's dated, I don't think it'll be an issue. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
31.01.2011, 03:41, Ryan Leavengood leaveng...@gmail.com: Maybe the solution here is better documentation outside the source code. I hope some of the more experienced WebKit developers can agree that there are parts of the code that are harder for new developers to dig into. Some high-level docs that are kept updated might be nice. Of course the key here is keeping them updated, and if it is hard to keep source code comments up to date I don't know how much hope there is for outside docs. Of course this applies to any project and is by no means just a flaw in this project. At least if the comments are in the code there is hope that a reviewer would catch when comments get out of date. Generally it's easier to keep up to date docs inside sources, rather than external ones -- Regards, Konstantin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Fwd: coding style and comments
On Fri, Jan 28, 2011 at 10:21 AM, Peter Kasting pkast...@google.com wrote: On Fri, Jan 28, 2011 at 10:14 AM, Alexey Proskuryakov a...@webkit.orgwrote: Do you have any specific mechanism in mind for keeping global comments accurate? No more than I have for keeping API usage or function implementations correct; that is, if we have comments, they must be as important to reviewers and patch authors as the code is. How can we ensure that all comments are up to do date? For example, suppose function A calls B, and B calls C. Then in the call site of A, I comment Because A does X, we do Y. Now suppose for the moment that the behavior X of A is implemented by C. We then come back and modify C, thereby modifying the behavior X of A to X'. We suddenly have a wrong comment in the call site of A and we need to fix it! But how do we know that if the patch only changed one line in C? - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Fwd: coding style and comments
31.01.2011, 11:47, Ryosuke Niwa rn...@webkit.org: How can we ensure that all comments are up to do date? For example, suppose function A calls B, and B calls C. Then in the call site of A, I comment Because A does X, we do Y. Now suppose for the moment that the behavior X of A is implemented by C. We then come back and modify C, thereby modifying the behavior X of A to X'. We suddenly have a wrong comment in the call site of A and we need to fix it! But how do we know that if the patch only changed one line in C? - Ryosuke You can document A as function calling B, B as function calling C, and keep documentation of C up to date when it's behavior changes But if A is API function, its behavior (what C does) should be described in place anyway -- Regards, Konstantin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Fwd: coding style and comments
2011/1/31 Konstantin Tokarev annu...@yandex.ru You can document A as function calling B, B as function calling C, and keep documentation of C up to date when it's behavior changes I don't see how that can substitute my comment that Because A does X, do Y. Saying do Y because we call A isn't useful at all here. But if A is API function, its behavior (what C does) should be described in place anyway Okay, let me give you a more concrete example. In this case: http://trac.webkit.org/browser/trunk/WebCore/editing/IndentOutdentCommand.cpp?rev=41034#L79 we're working around an issue in moveParagraph: http://trac.webkit.org/browser/trunk/WebCore/editing/CompositeEditCommand.cpp?rev=41034#L737 Now, moveParagraph is a very complicated function that does a million of things depending on the structure of DOM and 5 arguments, and it's mutually recursive with 2 other editing commands ReplaceSelectionCommand and DeleteSelectionCommand, both of which depend on many other editing commands and the rest of WebCore. As a result, it's virtually impossible to describe the complete behavior of moveParagraph. And if we changed any one line in WebCore/editing, there is a significant chance that we're changing the behavior of either ReplaceSelectionCommand or DeleteSelectionCommand and subsequently, moveParagraph. This is just a tip of an iceberg. I can give you hundreds of examples where you can't describe the behavior of a function completely, and yet you need to be concerned with it in call sites. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] about QtWebKit documentation
On Fri, Jan 28, 2011 at 5:29 PM, Joe Mason jma...@rim.com wrote: I’m not clear from your question what you’re trying to do. If you’re just trying to write an app using the Qt port of WebKit, you should ask on webkit-h...@lists.webkit.org. This list is only for developing WebKit itself. The interface layer between Qt and WebKit is in webkit/WebKit/qt/Api. Docs are at http://doc.qt.nokia.com/4.7/qtwebkit.html For questions about the use of QtWebKit, we also have the DevNet forum: http://developer.qt.nokia.com/forums/viewforum/21/ For discussing the development of QtWebKit itself, webkit...@lists.webkit.org is the place to go (and this mailing list of course :)). cheers, Benjamin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Use of 'inf' values in WebKit renderer classes
I'm trying to understand how float 'inf' values are used in WebKit rendering. I'm particularly interested in a case where a very large coordinate for a rect, which gets converted to 'inf' since its true value is too large to fit in a float, is retained. The value of 'inf' can't really be used for further computation (other than perhaps to detect its 'inf'-ness), and can cause problems when passed to lower lever graphics libraries (e.g. Skia, I'm not sure how CG handles 'inf'). If we really wanted to retain 'inf' values that are usable in computation (e.g. values that could be mapped from infinity back to a finite point, such as a vanishing point for a set of parallel 3-D lines), then don't we need to keep a homogenous/projective representation for them? I can appreciate how keeping an extra coordinate for all points might be undesirable since it requires additional overhead that is only rarely used. I guess I'm trying to understand how plain 'inf' values *are actually used* in WebKit, so I can understand why we retain them. Cheers, James ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Sun, Jan 30, 2011 at 4:47 PM, Maciej Stachowiak m...@apple.com wrote: Well, I didn't mean to pick on the authors of this file. This is the impression I get from a lot of code that some call well-commented, by which they mean lots of comments. I agree that the comments you pointed out are pretty unhelpful. I tried to emphasize already that silly comments that just restate the next line of code are not at all what I mean by well commented, and that what I am interested in are comments about subtle but crucial details (e.g. complex ownership rules) or comments that sum up a huge swath of source code (e.g. a class-level comment that covers the critical high-level points the class is responsible for). I honestly don't think there is much disagreement about what kinds of comments are unhelpful. I think the disagreement here comes from past experience, where some people have mostly experienced low-quality and out-of-date comments and are justifiably uninterested in repeating that, and others have been helped by high-quality comments in complex codebases and want to see more of that in WebKit. It seems like the best rule of thumb would be that reviewers should look at any added comments and judge whether the comments are really valuable. I don't think we need to (or should) globally frown on comments -- just on bad comments. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Using Visual Studio 2010 for Apple's Windows WebKit port
Hi all- We'd like to switch Apple's Windows WebKit port to build with Visual Studio 2010 sometime in the next 6-8 months, and to drop support for building with Visual Studio 2005 at the same time. The biggest consequence of this will be that anyone wishing to build Apple's Windows port will have to switch from VS2005/VC++ Express 2005 to VS2010 or VC++ Express 2010. This change should not affect applications that use WebKit, as we don't pass memory ownership across the WebKit API boundary (i.e., mallocing memory inside WebKit for the application to free later, or vice-versa). I wanted to send this email as a heads-up for Windows developers. People working on the Cairo/libcurl/CFLite port, which shares project files with Apple's port, need to be especially aware of this change, since it will require converting our .vcproj/.vsprops files to the VS2010 format. Please let me (and the list) know if this change will cause you trouble, and if there's something we can do to make the transition easier. You can also comment in http://webkit.org/b/53445, which tracks this effort. Thanks! -Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using Visual Studio 2010 for Apple's Windows WebKit port
On Mon, Jan 31, 2011 at 12:30 PM, Adam Roben aro...@apple.com wrote: Please let me (and the list) know if this change will cause you trouble, and if there's something we can do to make the transition easier. This may make life hard on Chromium as right now we don't support building with VS2010. We are working on adding support (I think http://crbug.com/25628 may be the closest bug). I'm not sure what time frame that will happen in. I don't think we have any plans to drop VS2005 support. If WebKit drops support it may force us to do so as well. In both cases above we're not so much impacted by vcproj/sln changes (since we use GYP to construct our vcproj and sln files) as we are by code changes, e.g. code that requires VS2010 to build correctly. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using Visual Studio 2010 for Apple's Windows WebKit port
On Jan 31, 2011, at 3:41 PM, Peter Kasting wrote: On Mon, Jan 31, 2011 at 12:30 PM, Adam Roben aro...@apple.com wrote: Please let me (and the list) know if this change will cause you trouble, and if there's something we can do to make the transition easier. This may make life hard on Chromium as right now we don't support building with VS2010. We are working on adding support (I think http://crbug.com/25628 may be the closest bug). I'm not sure what time frame that will happen in. I don't think we have any plans to drop VS2005 support. If WebKit drops support it may force us to do so as well. In both cases above we're not so much impacted by vcproj/sln changes (since we use GYP to construct our vcproj and sln files) as we are by code changes, e.g. code that requires VS2010 to build correctly. I doubt that maintaining compatibility with VS2005's compiler would be an issue. As long as there's an EWS and/or buildbot to catch problems, we should be able to work around any compiler differences. I didn't mean to imply that we'd intentionally break compilation with VS2005's compiler; all I meant was that using VS2005 to build Apple's port would no longer be supported. -Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Jan 31, 2011, at 11:01 AM, Peter Kasting wrote: On Sun, Jan 30, 2011 at 4:47 PM, Maciej Stachowiak m...@apple.com wrote: Well, I didn't mean to pick on the authors of this file. This is the impression I get from a lot of code that some call well-commented, by which they mean lots of comments. I agree that the comments you pointed out are pretty unhelpful. I tried to emphasize already that silly comments that just restate the next line of code are not at all what I mean by well commented, and that what I am interested in are comments about subtle but crucial details (e.g. complex ownership rules) or comments that sum up a huge swath of source code (e.g. a class-level comment that covers the critical high-level points the class is responsible for). I think people who favor comments tend to produce a lot of exactly this kind of comment. Except in some cases its verbose multiline comments that exceed the number of actual lines of code. Here's some more files with many comments that are better deleted or replaced with refactoring: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.h http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.cpp To pick just one specific example: // ICOs always begin with a 2-byte 0 followed by a 2-byte 1. // CURs begin with 2-byte 0 followed by 2-byte 2. if (!memcmp(contents, \x00\x00\x01\x00, 4) || !memcmp(contents, \x00\x00\x02\x00, 4)) This would be more readable if the comments were deleted and the memcmps were replaced with calls to inline functions named hasICOMagicNumber/hasCURMagicNumber or the like. I honestly don't think there is much disagreement about what kinds of comments are unhelpful. I think the disagreement here comes from past experience, where some people have mostly experienced low-quality and out-of-date comments and are justifiably uninterested in repeating that, and others have been helped by high-quality comments in complex codebases and want to see more of that in WebKit. Well, I think a comments are good attitude can result in lots of low-quality comments, instead of reserving comments only for cases where they are high in value. It seems like the best rule of thumb would be that reviewers should look at any added comments and judge whether the comments are really valuable. I don't think we need to (or should) globally frown on comments -- just on bad comments. I would go further than that. Even if a comment is valuable, the reviewer (and the patch author) should think about whether what it says could be expressed in source code instead, with some refactoring. Comments should be a last resort for making code clear. I do agree that they have their place, but I think that place is fairly rare. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Mon, Jan 31, 2011 at 12:47 PM, Maciej Stachowiak m...@apple.com wrote: On Jan 31, 2011, at 11:01 AM, Peter Kasting wrote: I think people who favor comments tend to produce a lot of exactly this kind of comment. Except in some cases its verbose multiline comments that exceed the number of actual lines of code. Here's some more files with many comments that are better deleted or replaced with refactoring: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.h http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.cpp Oh good, files I know something about :) To pick just one specific example: // ICOs always begin with a 2-byte 0 followed by a 2-byte 1. // CURs begin with 2-byte 0 followed by 2-byte 2. if (!memcmp(contents, \x00\x00\x01\x00, 4) || !memcmp(contents, \x00\x00\x02\x00, 4)) This would be more readable if the comments were deleted and the memcmps were replaced with calls to inline functions named hasICOMagicNumber/hasCURMagicNumber or the like. Or just omitted altogether. I don't think the comments here add much. (They predate my involvement; they actually come from hyatt in 2006, and in fairness to him, this code at that time was very much throw in something quick that works to get a Windows port up and running.) Your choice of files is interesting otherwise though. Aside from those 2006-era comments in create(), there are only three other comments in ImageDecoder.cpp, two of which seem worth having to me. In ImageDecoder.h, many function declarations are commented, most to explain ownership details, caution users of functionality that needs to match in multiple places, or give more context to why callers would use the function. There are definitely some comments lying around here that aren't terribly useful, but the majority of these have been helpful when tracing through various different image decoding bugs, especially memory errors triggered by our heuristic that throws away image data for large images sometimes, or when refreshing my memory on what this code does when I come back to it after a year of doing something else. So I don't agree that many comments here (if many means the majority) are unhelpful. I guess, then, we do disagree about what types of comments are useful. I would go further than that. Even if a comment is valuable, the reviewer (and the patch author) should think about whether what it says could be expressed in source code instead, with some refactoring. Comments should be a last resort for making code clear. I do agree that they have their place, but I think that place is fairly rare. I think we are probably going to remain in disagreement, then. In my opinion, this is fine if you already know the code in question in detail, but I agree with Simon that this level of strictness is a barrier to learning new code and contributes to a privileged few sort of view of code ownership. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Fwd: coding style and comments
On Mon, Jan 31, 2011 at 12:47 AM, Ryosuke Niwa rn...@webkit.org wrote: How can we ensure that all comments are up to do date? For example, suppose function A calls B, and B calls C. Then in the call site of A, I comment Because A does X, we do Y. Now suppose for the moment that the behavior X of A is implemented by C. We then come back and modify C, thereby modifying the behavior X of A to X'. We suddenly have a wrong comment in the call site of A and we need to fix it! But how do we know that if the patch only changed one line in C? It seems like the one line patch to C just broke A. It had a dependency on the behavior of C that was worth documenting. Now you have changed C and the behavior of A is probably wrong (or at least wasteful). If you had the comment, at least a grep of the source would have found the dependency and alerted you that it was worth looking at this call site. - a ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Fwd: coding style and comments
On Mon, Jan 31, 2011 at 4:54 PM, Aaron Boodman a...@chromium.org wrote: It seems like the one line patch to C just broke A. It had a dependency on the behavior of C that was worth documenting. Now you have changed C and the behavior of A is probably wrong (or at least wasteful). Not necessarily. X' might be better a behavior and Y might no longer be needed because of that. If you had the comment, at least a grep of the source would have found the dependency and alerted you that it was worth looking at this call site. I don't think so. How do I know that modifying C would have changed the behavior of A? This was a very simple example with only one indirection, namely, B. But in the example I posted earlier (moveParagraph), a function calls hundreds of thousands of functions and it's virtually impossible even to enumerate all functions depended by the function. Yet, we must worry about the side-effects caused by the function in a call site. And we have tons of functions like this in editing because of the nature of what it does. So I insist on my point that keeping comments up-to-date is really hard if not impossible. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Implementing the device element
Hi, We have recently noticed a patch to implement the device element in WebKit. Since this is an important new feature, I thought we should have a discussion about the best way to implement it. Here's the actual specification for it: http://www.whatwg.org/specs/web-apps/current-work/complete/commands.html#devices . The approach proposed in https://bugs.webkit.org/show_bug.cgi?id=47264 brings the device probing and selection to the WebCore level. It does so by first creating a list of available devices using an interface to a platform-dependent client and then it passes this list to a client dialog to perform the actual selection from it. With its current design both operations are performed synchronously, possibly blocking the device element event handler for a long time. I think this is not a suitable design. Given the drawbacks of the approach above, I would like to propose an alternative design that solves these issues. First of all, we think that is not necessary at all to bring the device probing, available device lists, device selection or connection to WebCore. The device element isn't really about actual devices, but about connecting to them. We think that it would be the best for all platforms to actually delegate the actual probing, selection and connecting to them in an asynchronous client-based model and hold only connection and handler information in WebCore. Here's an example of the call flow in our proposal: 0. The device element is created in a no connection state and with an empty device connection descriptor. 1. User clicks the device element (a button, for example). 2. The device element changes its internal state from no connection to connecting, and asks to the device controller to connect to a device of its type sending also the document security origin. 3. The device controller, which handles and maps all the device requests of the page, forwards the connection request to the device page client. This client is implemented by the corresponding WebKit platforms. 4. The device client receives the request and, in a non-blocking way, implements the way it likes the device probing, selection, connecting and all the required security operations (the latter also missing in the existing patch). This allows every UA vendor to make their own choices about the UI for device selection. 5. When the platform has finished connecting to a device, it sends back to the device controller a device connection update message. This message comes with a device connection descriptor that contains the actual connection status, error codes/messages, device ids/names if connected and so on. In case of user cancelation, connection or communication error the same message is sent, but with different device connection descriptor values. 6. The device controller forwards the message to the original device element. The original element sets its connection descriptor to the received object and changes its state accordingly. It will change to connected if the connection was successful or to not connected in case of error. It will also fire the appropriate JS events. 7. If the connection was successful, the element will then create valid stream objects from its data attribute. These will not contain any real streaming data but just a url string. All the actual data is internally managed by the platforms, leaving only handler-like objects in WebCore. The url can be provided for the moment with the connection descriptor object, being later replaced by some type-independent object when device types other than media are defined. Requesting to disconnect the device would work much like requesting a connection, but providing the device descriptor object as a parameter instead. With this approach, not only we avoid to block WebCore with potentially long operations but we provide a great flexibility to all WebKit platforms to implement the nasty device handling details as they want to. This should also potentially increase the amount of code that can be reused by the different platforms, while avoiding any list matching code that can be found in the existing patch. I have a patch that implements this design, but I wanted to get some feedback on this list before sending it out for review. Please don't hesitate to make any suggestions that could help to improve this. Thanks, Leandro ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Fwd: coding style and comments
On Mon, Jan 31, 2011 at 5:23 PM, Ryosuke Niwa rn...@webkit.org wrote:
On Mon, Jan 31, 2011 at 4:54 PM, Aaron Boodman a...@chromium.org wrote:
It seems like the one line patch to C just broke A. It had a
dependency on the behavior of C that was worth documenting. Now you
have changed C and the behavior of A is probably wrong (or at least
wasteful).
Not necessarily. X' might be better a behavior and Y might no longer be
needed because of that.
That is exactly my point. Either way the one line change leaves orphan
code. Having a comment just improves your chances of finding it
(assuming the comment references the depended-upon code).
If you had the comment, at least a grep of the source would have found the
dependency and alerted you that it was worth looking at this call site.
I don't think so. How do I know that modifying C would have changed the
behavior of A?
To be specific, I'm talking about this situation:
void doA() {
// We don't need to frobulate here because doC() already did that.
}
void doB() {
doC();
}
void doC() {
.. complicated stuff ...
}
Now if someone comes along and changes doC, they will probably grep
for the call sites to update them. That grep would find the comment in
doA() too, bringing attention to the tricky indirect relationship.
Without the comment, the relationship would be harder to find.
This was a very simple example with only one indirection,
namely, B. But in the example I posted earlier (moveParagraph), a function
calls hundreds of thousands of functions and it's virtually impossible even
to enumerate all functions depended by the function. Yet, we must worry
about the side-effects caused by the function in a call site.
And we have tons of functions like this in editing because of the nature of
what it does. So I insist on my point that keeping comments up-to-date is
really hard if not impossible.
I'm not suggesting there should be a rule that all side-effects are
commented, that is clearly ridiculous. It is a matter of judgement to
determine when something warrants explanation. Typically it would be
when something is working in a way that is surprising or unexpected.
For example, moveParagraph() ends up calling lots of functions that
have lots of side-effects. Yet those shouldn't be surprising to
someone familiar with how editing works at a high level (note: this is
where class-level comments are super useful), so they shouldn't
require a comment.
- a
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using Visual Studio 2010 for Apple's Windows WebKit port
How hard will the transition be? If it's going to take a lot of time and cause a lot of churn anyway, would this be a good time to try and make that port use GYP or CMake? (I assume the answer is probably no, but figured it was worth asking anyway. :-) J On Mon, Jan 31, 2011 at 12:46 PM, Adam Roben aro...@apple.com wrote: On Jan 31, 2011, at 3:41 PM, Peter Kasting wrote: On Mon, Jan 31, 2011 at 12:30 PM, Adam Roben aro...@apple.com wrote: Please let me (and the list) know if this change will cause you trouble, and if there's something we can do to make the transition easier. This may make life hard on Chromium as right now we don't support building with VS2010. We are working on adding support (I think http://crbug.com/25628 may be the closest bug). I'm not sure what time frame that will happen in. I don't think we have any plans to drop VS2005 support. If WebKit drops support it may force us to do so as well. In both cases above we're not so much impacted by vcproj/sln changes (since we use GYP to construct our vcproj and sln files) as we are by code changes, e.g. code that requires VS2010 to build correctly. I doubt that maintaining compatibility with VS2005's compiler would be an issue. As long as there's an EWS and/or buildbot to catch problems, we should be able to work around any compiler differences. I didn't mean to imply that we'd intentionally break compilation with VS2005's compiler; all I meant was that using VS2005 to build Apple's port would no longer be supported. -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] coding style and comments
On Jan 31, 2011, at 1:06 PM, Peter Kasting wrote:
On Mon, Jan 31, 2011 at 12:47 PM, Maciej Stachowiak m...@apple.com wrote:
On Jan 31, 2011, at 11:01 AM, Peter Kasting wrote:
I think people who favor comments tend to produce a lot of exactly this kind
of comment. Except in some cases its verbose multiline comments that exceed
the number of actual lines of code. Here's some more files with many comments
that are better deleted or replaced with refactoring:
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.h
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.cpp
Oh good, files I know something about :)
To pick just one specific example:
// ICOs always begin with a 2-byte 0 followed by a 2-byte 1.
// CURs begin with 2-byte 0 followed by 2-byte 2.
if (!memcmp(contents, \x00\x00\x01\x00, 4) || !memcmp(contents,
\x00\x00\x02\x00, 4))
This would be more readable if the comments were deleted and the memcmps were
replaced with calls to inline functions named
hasICOMagicNumber/hasCURMagicNumber or the like.
Or just omitted altogether. I don't think the comments here add much. (They
predate my involvement; they actually come from hyatt in 2006, and in
fairness to him, this code at that time was very much throw in something
quick that works to get a Windows port up and running.)
Ye another option would be to use named constants for the magic strings.
Your choice of files is interesting otherwise though. Aside from those
2006-era comments in create(), there are only three other comments in
ImageDecoder.cpp, two of which seem worth having to me. In ImageDecoder.h,
many function declarations are commented, most to explain ownership details,
caution users of functionality that needs to match in multiple places, or
give more context to why callers would use the function. There are
definitely some comments lying around here that aren't terribly useful, but
the majority of these have been helpful when tracing through various
different image decoding bugs, especially memory errors triggered by our
heuristic that throws away image data for large images sometimes, or when
refreshing my memory on what this code does when I come back to it after a
year of doing something else.
So I don't agree that many comments here (if many means the majority)
are unhelpful. I guess, then, we do disagree about what types of comments
are useful.
I do think the majority of comments in there are unhelpful. Here are some
examples from the header that I hope speak for themselves:
// Whether or not the size information has been decoded yet. This
// default implementation just returns true if the size has been set and
// we have not seen a failure. Decoders may want to override this to
// lazily decode enough of the image to get the size.
virtual bool isSizeAvailable()
{
return !m_failed m_sizeAvailable;
}
// Returns the size of the image.
virtual IntSize size() const
{
return m_size;
}
// The total number of frames for the image. Classes that support
// multiple frames will scan the image data for the answer if they need
// to (without necessarily decoding all of the individual frames).
virtual size_t frameCount() { return 1; }
// The number of repetitions to perform for an animation loop.
virtual int repetitionCount() const { return cAnimationNone; }
// Called to obtain the ImageFrame full of decoded data for rendering.
// The decoder plugin will decode as much of the frame as it can before
// handing back the buffer.
virtual ImageFrame* frameBufferAtIndex(size_t) = 0;
// Whether or not the underlying image format even supports alpha
// transparency.
virtual bool supportsAlpha() const { return true; }
It seems to me that none of these comments add info that is worth the code
bloat.
Regards,
Maciej
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Fwd: coding style and comments
On Jan 31, 2011, at 5:48 PM, Aaron Boodman wrote:
On Mon, Jan 31, 2011 at 5:23 PM, Ryosuke Niwa rn...@webkit.org wrote:
On Mon, Jan 31, 2011 at 4:54 PM, Aaron Boodman a...@chromium.org wrote:
It seems like the one line patch to C just broke A. It had a
dependency on the behavior of C that was worth documenting. Now you
have changed C and the behavior of A is probably wrong (or at least
wasteful).
Not necessarily. X' might be better a behavior and Y might no longer be
needed because of that.
That is exactly my point. Either way the one line change leaves orphan
code. Having a comment just improves your chances of finding it
(assuming the comment references the depended-upon code).
If you had the comment, at least a grep of the source would have found the
dependency and alerted you that it was worth looking at this call site.
I don't think so. How do I know that modifying C would have changed the
behavior of A?
To be specific, I'm talking about this situation:
void doA() {
// We don't need to frobulate here because doC() already did that.
}
If previous frobulation is a precondition of doA, that's better expressed as an
ASSERT than as a comment. That way, if someone breaks the precondition, they'll
find out when they test, whether or not they think to search for related
comments.
I 3 asserts as a way to document preconditions.
Regards,
Maciej
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
This thread has probably gone the way of all webkit-dev threads on comments or ChangeLog files -- people's opinions vary, it turns into a bikeshed, and nothing really changes about how we code. Repeat in a year. w.r.t. ImageDecoder specifically, as I mentioned before I do agree that there are some comments that are either worthless or partially so, and I'll try and post some cleanup for this header on https://bugs.webkit.org/show_bug.cgi?id=53455 . PK P.S. I agree with you about assertions being better than comments to document pre- (and post-) conditions (where possible). ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Mon, Jan 31, 2011 at 7:18 PM, Peter Kasting pkast...@chromium.org wrote: P.S. I agree with you about assertions being better than comments to document pre- (and post-) conditions (where possible). Me too (where possible). - a ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Jan 31, 2011, at 7:18 PM, Peter Kasting wrote: This thread has probably gone the way of all webkit-dev threads on comments or ChangeLog files -- people's opinions vary, it turns into a bikeshed, and nothing really changes about how we code. Repeat in a year. Well, even though we didn't come to consensus, I hope we benefitted in exposing the silent majority of the list to some strong opinions on code quality, which definitely *is* a key value for the WebKit project, even if we don't have 100% agreement on the means. I, for one, am happy that we have so many people who care passionately about keeping the code clean and readable. w.r.t. ImageDecoder specifically, as I mentioned before I do agree that there are some comments that are either worthless or partially so, and I'll try and post some cleanup for this header on https://bugs.webkit.org/show_bug.cgi?id=53455 . Would be glad to review any code cleanup if you need me to. P.S. I agree with you about assertions being better than comments to document pre- (and post-) conditions (where possible). I find it very often is, even when it initially seems unlikely, and it's awesome when you run the layout tests in a debug build and see your precondition assertion fail. It's like having regression tests for your comments! Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
