Re: [webkit-dev] cr-linux-ews migration
The cr-linux-ews migration seems to have gone well. I'm now migrating the commit-queue. Please let me know if you notice any problems. Thanks! Adam On Mon, Jul 2, 2012 at 12:32 AM, Adam Barth aba...@webkit.org wrote: Hi webkit-dev, I've moved the cr-linux-ews from EC2 to Google Compute Engine. In the process, I've doubled the capacity. If you notice anything wonky, please let me know. If this transition goes well, I'll move the commit-queue (and expand its capacity) as well. Happy hacking! Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] Updates on Chromium's content_shell
Hey, I wanted to share some updates about content_shell (a multi-processed version of chromium's test shell): meanwhile, it is possible to generate pixel results. Out of 31431 tests that are executed on chromium-linux, 6234 did not match the expected results using content_shell, all others passed (80.17%)! The biggest issue is that almost all testController methods are not yet implemented. If you're interested in running content_shell tests yourself, I've put together a small description here: http://dev.chromium.org/developers/testing/webkit-layout-tests/content-shell The next step will be to put an API between the test controllers defined for chromium's DRT, so content_shell can reuse those. best -jochen ___ 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 5:31 PM, Maciej Stachowiak m...@apple.com wrote: 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. Not at all. Consider something like HashSet. Many many engineers need to use and understand it, yet very few engineers will ever submit patches to change it. The same holds, to a lesser extent, for something like RenderElement or GraphicsContext, to name just two. 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. I'd be happy to add a term to the cost function: Cost per year with good comments: t_maintainComments * n_patches + t_understandWithComment * n_engineersNeedingToUnderstand + t_discoverAndFixBadCommend * n_badComments I don't doubt there are poor comments, both outdated and useless. That's a reviewing failure. You have simply highlighted the fact that any standard for comments requires reviewer attention. Hence cost of maintaining comments. 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. Yes. I don't think you'll find much disagreement. I don't believe anyone is arguing for lots of comments. The primary focus of this discussion, as I recall reading, is: (1) class and member comments that describe system behavior, (2) comments on invariants in code and (3) references to sections of the spec that define behavior, and where we deviate. 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
CSSRegion is it then! I'll also make a patch to rename WebKitNamedFlow into CSSNamedFlow. Thx! Andrei. On 7/12/12 10: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] Updates on Chromium's content_shell
On Fri, Jul 13, 2012 at 4:16 AM, Jochen Eisinger joc...@chromium.orgwrote: I wanted to share some updates about content_shell (a multi-processed version of chromium's test shell): meanwhile, it is possible to generate pixel results. Out of 31431 tests that are executed on chromium-linux, 6234 did not match the expected results using content_shell, all others passed (80.17%)! This is a great news! Thanks a lot for working on this effort. Let us know if you needed a help in implementing testRunner methods. - 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 Fri, Jul 13, 2012 at 5:57 AM, Stephen Chenney schen...@chromium.orgwrote: I don't doubt there are poor comments, both outdated and useless. That's a reviewing failure. You have simply highlighted the fact that any standard for comments requires reviewer attention. Hence cost of maintaining comments. I don't know how to review a patch and make sure all relevant comments are updated. As I have illustrated before, you can be modifying a function X, then a completely random function A which calls B that in turn calls C that in turns D ... that in turn calls X may have a comment dependent on the previous behavior of X without ever mentioning X. How am I supposed to know that there is such a comment? - Ryosuke ___ 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
bikeshedding Just like we don't call the class DOMDocument, there is no need to add the CSS prefix where there aren't collisions (IMO). I do think we should drop the WebKit prefix from all classes, and use InterfaceName= the .idl to map from InternalName to WebKitExternalName. http://trac.webkit.org/wiki/WebKitIDL#InterfaceName /bikeshedding On Fri, Jul 13, 2012 at 7:17 AM, Andrei Bucur abu...@adobe.com wrote: CSSRegion is it then! I'll also make a patch to rename WebKitNamedFlow into CSSNamedFlow. Thx! Andrei. On 7/12/12 10: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
Re: [webkit-dev] Web APIs and class name collisions
On Fri, Jul 13, 2012 at 10:59 AM, Eric Seidel e...@webkit.org wrote: bikeshedding Just like we don't call the class DOMDocument, there is no need to add the CSS prefix where there aren't collisions (IMO). I do think we should drop the WebKit prefix from all classes, and use InterfaceName= the .idl to map from InternalName to WebKitExternalName. ^^^ yes, please :-) /me crawls back into his hole. http://trac.webkit.org/wiki/WebKitIDL#InterfaceName /bikeshedding On Fri, Jul 13, 2012 at 7:17 AM, Andrei Bucur abu...@adobe.com wrote: CSSRegion is it then! I'll also make a patch to rename WebKitNamedFlow into CSSNamedFlow. Thx! Andrei. On 7/12/12 10: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
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On Fri, Jul 13, 2012 at 1:56 PM, Ryosuke Niwa rn...@webkit.org wrote: On Fri, Jul 13, 2012 at 5:57 AM, Stephen Chenney schen...@chromium.orgwrote: I don't doubt there are poor comments, both outdated and useless. That's a reviewing failure. You have simply highlighted the fact that any standard for comments requires reviewer attention. Hence cost of maintaining comments. I don't know how to review a patch and make sure all relevant comments are updated. As I have illustrated before, you can be modifying a function X, then a completely random function A which calls B that in turn calls C that in turns D ... that in turn calls X may have a comment dependent on the previous behavior of X without ever mentioning X. How am I supposed to know that there is such a comment? How is that different than the same question but replace comment with behaviour? In both cases A is no longer doing what it expected. Something is going to break, and A will have to be fixed/updated, comment included. - Dana ___ 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 13, 2012, at 5:57 AM, Stephen Chenney schen...@chromium.org wrote: I'd be happy to add a term to the cost function: Cost per year with good comments: t_maintainComments * n_patches + t_understandWithComment * n_engineersNeedingToUnderstand + t_discoverAndFixBadCommend * n_badComments I don't doubt there are poor comments, both outdated and useless. That's a reviewing failure. You have simply highlighted the fact that any standard for comments requires reviewer attention. Hence cost of maintaining comments. I agree that reviewers should try to prevent inclusion of low-quality comments. I think the best way to avoid poor comments is for reviewers to be skeptical of every comment they see, and ask if the same information can be expressed as well in the code itself. There may be cases where it can't, typically why comments that explain the reason for doing something. But if there are more comment lines than code lines in a function, it probably needs rewriting. 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. Yes. I don't think you'll find much disagreement. I don't believe anyone is arguing for lots of comments. The primary focus of this discussion, as I recall reading, is: (1) class and member comments that describe system behavior, (2) comments on invariants in code and (3) references to sections of the spec that define behavior, and where we deviate. Really? I think there is an implicit assumption on the part of some that good comments means many comments, or at least, more than we typically have. For your specific examples, my opinions would be: (1a) Class comments can be useful if the class cannot be adequately and clearly described by its name. But ideally they should describe local properties of the class, not global properties of the system. They should especially avoid documenting facts that may change without the class holding the comment itself changing. (1b) Member comments are almost always bad. You will not see them at the point of use, and new uses will typically be produced by copying existing uses, not by reading the header. It is almost always superior to use a better name for the member. If you think you need a member comment, you almost surely gave the member a bad name. (2) Invariants in the code should be ASSERTs or COMPILE_ASSERTs, not comments. (3) Spec section references are almost always not worth it. People working on a particular aspect of Web technology should be aware of the relevant specs. And when the spec is updated, thus invalidating all the section numbers, you have a bunch of out-of-date comments. (3b) Documenting intentional noncompliance with a standard may be useful, but the key is to explain *why* it's necessary to deviate from the standard, not just how we're deviating, otherwise the next person to look at the code won't know whether it's ok to change the code to be compliant. Regards, Maciej ___ 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 13, 2012, at 11:03 AM, Dana Jansens dan...@chromium.org wrote: On Fri, Jul 13, 2012 at 1:56 PM, Ryosuke Niwa rn...@webkit.org wrote: On Fri, Jul 13, 2012 at 5:57 AM, Stephen Chenney schen...@chromium.org wrote: I don't doubt there are poor comments, both outdated and useless. That's a reviewing failure. You have simply highlighted the fact that any standard for comments requires reviewer attention. Hence cost of maintaining comments. I don't know how to review a patch and make sure all relevant comments are updated. As I have illustrated before, you can be modifying a function X, then a completely random function A which calls B that in turn calls C that in turns D ... that in turn calls X may have a comment dependent on the previous behavior of X without ever mentioning X. How am I supposed to know that there is such a comment? How is that different than the same question but replace comment with behaviour? In both cases A is no longer doing what it expected. Something is going to break, and A will have to be fixed/updated, comment included. Wrong behavior can often be observed through automated testing. Wrong comments cannot. - Maciej ___ 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 Fri, Jul 13, 2012 at 11:03 AM, Dana Jansens dan...@chromium.org wrote: On Fri, Jul 13, 2012 at 1:56 PM, Ryosuke Niwa rn...@webkit.org wrote: On Fri, Jul 13, 2012 at 5:57 AM, Stephen Chenney schen...@chromium.orgwrote: I don't doubt there are poor comments, both outdated and useless. That's a reviewing failure. You have simply highlighted the fact that any standard for comments requires reviewer attention. Hence cost of maintaining comments. I don't know how to review a patch and make sure all relevant comments are updated. As I have illustrated before, you can be modifying a function X, then a completely random function A which calls B that in turn calls C that in turns D ... that in turn calls X may have a comment dependent on the previous behavior of X without ever mentioning X. How am I supposed to know that there is such a comment? How is that different than the same question but replace comment with behaviour? In both cases A is no longer doing what it expected. Something is going to break, and A will have to be fixed/updated, comment included. Not necessarily. First off, the behavioral change may not have any user visible behavioral change, or that while X behaves differently, it doesn't affect the way A works due to some other changes in the same patch. Or it's possible that new behavior of A is expected and desirable but the change was made at much lower level and affected hundreds of other functions. - 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 13, 2012, at 11:13 AM, Alec Flett alecfl...@chromium.org wrote: And yes while incorrect behavior can be observed through automated testing, automated testing does not catch all incorrect behavior, especially unexpected never-before-seen behavior. Why do you think people write fuzzers? This is yet another way that folks are arguing comments can be wrong, code can't [thanks to compilers and automated testing], so don't write excess comments Do you think this is not a valid reason to express things in code instead of as comments whenever possible? Regards, Maciej ___ 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 Fri, Jul 13, 2012 at 11:13 AM, Alec Flett alecfl...@chromium.org wrote: On Fri, Jul 13, 2012 at 10:56 AM, Ryosuke Niwa rn...@webkit.org wrote: On Fri, Jul 13, 2012 at 5:57 AM, Stephen Chenney schen...@chromium.orgwrote: I don't doubt there are poor comments, both outdated and useless. That's a reviewing failure. You have simply highlighted the fact that any standard for comments requires reviewer attention. Hence cost of maintaining comments. I don't know how to review a patch and make sure all relevant comments are updated. As I have illustrated before, you can be modifying a function X, then a completely random function A which calls B that in turn calls C that in turns D ... that in turn calls X may have a comment dependent on the previous behavior of X without ever mentioning X. How am I supposed to know that there is such a comment? But the exact same thing can be said of code. Comments are not special in this regard. I'm not buying that argument because your example is not convincing. A simplistic example: You could be reviewing code that calls a method that takes an int, but in practice that method should never take a negative number. (it might even be documented correctly as such) I would have r-ed the original patch that added this function. We should either change the argument's type to unsigned or ASSERTed that it's non-negative. As such, I wouldn't consider this to be neither patch author's nor reviewer's fault. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Updates on Chromium's content_shell
On Fri, Jul 13, 2012 at 7:46 PM, Ryosuke Niwa rn...@webkit.org wrote: On Fri, Jul 13, 2012 at 4:16 AM, Jochen Eisinger joc...@chromium.orgwrote: I wanted to share some updates about content_shell (a multi-processed version of chromium's test shell): meanwhile, it is possible to generate pixel results. Out of 31431 tests that are executed on chromium-linux, 6234 did not match the expected results using content_shell, all others passed (80.17%)! This is a great news! Thanks a lot for working on this effort. Let us know if you needed a help in implementing testRunner methods. At this point, there are two things I could use help with: in general, moving methods to window.internals (and addressing the current shortcomings of that approach) helps a lot, as I can just instantiate this object in content_shell. The other thing is to work towards making layoutTestController (of chromium's DRT) a library. Adam mentioned a while ago that he'd be interested with helping with the latter as well -jochen - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Updates on Chromium's content_shell
What about keeping the discussion here, so others that might want to join the effort later can read it up? In general, I agree with your proposal. I guess starting with something small like EventSender might be a good first step. best -jochen On Fri, Jul 13, 2012 at 10:20 PM, Adam Barth aba...@webkit.org wrote: On Fri, Jul 13, 2012 at 1:10 PM, Jochen Eisinger joc...@chromium.orgwrote: On Fri, Jul 13, 2012 at 7:46 PM, Ryosuke Niwa rn...@webkit.org wrote: On Fri, Jul 13, 2012 at 4:16 AM, Jochen Eisinger joc...@chromium.orgwrote: I wanted to share some updates about content_shell (a multi-processed version of chromium's test shell): meanwhile, it is possible to generate pixel results. Out of 31431 tests that are executed on chromium-linux, 6234 did not match the expected results using content_shell, all others passed (80.17%)! This is a great news! Thanks a lot for working on this effort. Let us know if you needed a help in implementing testRunner methods. At this point, there are two things I could use help with: in general, moving methods to window.internals (and addressing the current shortcomings of that approach) helps a lot, as I can just instantiate this object in content_shell. The other thing is to work towards making layoutTestController (of chromium's DRT) a library. Adam mentioned a while ago that he'd be interested with helping with the latter as well Yes. The idea is to implement LayoutTestController in terms of the WebKit API and a (hopefully simple) delegate. Currently LayoutTestController knows too much about DumpRenderTree. That will let us share the implementation of LayoutTestController and avoid having to maintain yet another copy. Upstreaming the chromium-android port is at the top of my priority list, but I should be able to help out with this work as well. Should we talk off-list about how to approach this work? Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Updates on Chromium's content_shell
Yeah, EventSender is likely a good place to start. Here are some more details about what I had in mind: 1) We'll need to create a new target that builds a TestRunner.a (or LayoutTestController.a, whatever name is currently in fashion). This target is allowed to depend on WTF, WebKit (via the WebKit API), and probably webkit_support. 2) This target will contain LayoutTestController.cpp, EventSender.cpp, and all the other code that's needed to support the objects we inject when running LayoutTests. 3) To move code into this target, we'll need to abstract any dependencies on the rest of DumpRenderTree into a delegate interface. For example, EventSender has a #include TestShell.h, which we'll need to remove. Today, EventSender gets the WebView is by asking m_shell for it. Instead, it will need to ask the delegate. One of the trickier things in this project will be WebViewHost. TestRunner.a will need some object like that to subclass a bunch of WebKit API clients, but the design might need to change a bit. I haven't studied it carefully yet. Once this is done, but DumpRenderTree and ContentShell can link in TestRunner.a and implement the delegate. Hopefully the bulk of the interactions will be between TestRunner.a and the WebKit API so that the delegate will mostly be able providing the WebView and getting out of the way. Adam On Fri, Jul 13, 2012 at 1:56 PM, Jochen Eisinger joc...@chromium.orgwrote: What about keeping the discussion here, so others that might want to join the effort later can read it up? In general, I agree with your proposal. I guess starting with something small like EventSender might be a good first step. best -jochen On Fri, Jul 13, 2012 at 10:20 PM, Adam Barth aba...@webkit.org wrote: On Fri, Jul 13, 2012 at 1:10 PM, Jochen Eisinger joc...@chromium.orgwrote: On Fri, Jul 13, 2012 at 7:46 PM, Ryosuke Niwa rn...@webkit.org wrote: On Fri, Jul 13, 2012 at 4:16 AM, Jochen Eisinger joc...@chromium.orgwrote: I wanted to share some updates about content_shell (a multi-processed version of chromium's test shell): meanwhile, it is possible to generate pixel results. Out of 31431 tests that are executed on chromium-linux, 6234 did not match the expected results using content_shell, all others passed (80.17%)! This is a great news! Thanks a lot for working on this effort. Let us know if you needed a help in implementing testRunner methods. At this point, there are two things I could use help with: in general, moving methods to window.internals (and addressing the current shortcomings of that approach) helps a lot, as I can just instantiate this object in content_shell. The other thing is to work towards making layoutTestController (of chromium's DRT) a library. Adam mentioned a while ago that he'd be interested with helping with the latter as well Yes. The idea is to implement LayoutTestController in terms of the WebKit API and a (hopefully simple) delegate. Currently LayoutTestController knows too much about DumpRenderTree. That will let us share the implementation of LayoutTestController and avoid having to maintain yet another copy. Upstreaming the chromium-android port is at the top of my priority list, but I should be able to help out with this work as well. Should we talk off-list about how to approach this work? Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Updates on Chromium's content_shell
On Fri, Jul 13, 2012 at 2:15 PM, Darin Fisher da...@chromium.org wrote: On Fri, Jul 13, 2012 at 2:10 PM, Adam Barth aba...@webkit.org wrote: Yeah, EventSender is likely a good place to start. Here are some more details about what I had in mind: 1) We'll need to create a new target that builds a TestRunner.a (or LayoutTestController.a, whatever name is currently in fashion). This target is allowed to depend on WTF, WebKit (via the WebKit API), and probably webkit_support. 2) This target will contain LayoutTestController.cpp, EventSender.cpp, and all the other code that's needed to support the objects we inject when running LayoutTests. 3) To move code into this target, we'll need to abstract any dependencies on the rest of DumpRenderTree into a delegate interface. For example, EventSender has a #include TestShell.h, which we'll need to remove. Today, EventSender gets the WebView is by asking m_shell for it. Instead, it will need to ask the delegate. One of the trickier things in this project will be WebViewHost. TestRunner.a will need some object like that to subclass a bunch of WebKit API clients, but the design might need to change a bit. I haven't studied it carefully yet. TestRunner.a could just provide the WebViewClient and WebFrameClient implementations. The delegate you mention could just be a createWebView function. When Jochen and I discussed this topic before, I suggested just adding CreateWebView to webkit_support, but as a delegate function seems even better. We'd probably like to minimize dependencies on webkit_support since that is Chromium specific. In addition to functions that talk with the WebKit API, there are also functions that are more about controlling the test harness. For example, dumpAsText and waitUntilDone will need some pathway for communicating with the outside world besides WebKit API. Currently, this information is pulled by TestShell.cpp, so maybe we don't need the delegate and can just have both DumpRenderTree and ContentShell pull this information out of TestRunner.a via some interface. Adam Once this is done, but DumpRenderTree and ContentShell can link in TestRunner.a and implement the delegate. Hopefully the bulk of the interactions will be between TestRunner.a and the WebKit API so that the delegate will mostly be able providing the WebView and getting out of the way. Adam On Fri, Jul 13, 2012 at 1:56 PM, Jochen Eisinger joc...@chromium.orgwrote: What about keeping the discussion here, so others that might want to join the effort later can read it up? In general, I agree with your proposal. I guess starting with something small like EventSender might be a good first step. best -jochen On Fri, Jul 13, 2012 at 10:20 PM, Adam Barth aba...@webkit.org wrote: On Fri, Jul 13, 2012 at 1:10 PM, Jochen Eisinger joc...@chromium.orgwrote: On Fri, Jul 13, 2012 at 7:46 PM, Ryosuke Niwa rn...@webkit.orgwrote: On Fri, Jul 13, 2012 at 4:16 AM, Jochen Eisinger joc...@chromium.org wrote: I wanted to share some updates about content_shell (a multi-processed version of chromium's test shell): meanwhile, it is possible to generate pixel results. Out of 31431 tests that are executed on chromium-linux, 6234 did not match the expected results using content_shell, all others passed (80.17%)! This is a great news! Thanks a lot for working on this effort. Let us know if you needed a help in implementing testRunner methods. At this point, there are two things I could use help with: in general, moving methods to window.internals (and addressing the current shortcomings of that approach) helps a lot, as I can just instantiate this object in content_shell. The other thing is to work towards making layoutTestController (of chromium's DRT) a library. Adam mentioned a while ago that he'd be interested with helping with the latter as well Yes. The idea is to implement LayoutTestController in terms of the WebKit API and a (hopefully simple) delegate. Currently LayoutTestController knows too much about DumpRenderTree. That will let us share the implementation of LayoutTestController and avoid having to maintain yet another copy. Upstreaming the chromium-android port is at the top of my priority list, but I should be able to help out with this work as well. Should we talk off-list about how to approach this work? Adam ___ 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] webkit-dev Digest, Vol 86, Issue 8
oh hi: I didnot received your mail.i donot know why. Please resend me again. Thanks! Yanchao. Cui 在 2012-7-10 上午3:00， webkit-dev-requ...@lists.webkit.org写道： Send webkit-dev mailing list submissions to webkit-dev@lists.webkit.org To subscribe or unsubscribe via the World Wide Web, visit http://lists.webkit.org/mailman/listinfo/webkit-dev or, via email, send a message with subject or body 'help' to webkit-dev-requ...@lists.webkit.org You can reach the person managing the list at webkit-dev-ow...@lists.webkit.org When replying, please edit your Subject line so it is more specific than Re: Contents of webkit-dev digest... Today's Topics: 1. lists.webkit.org (William Siegrist) -- Message: 1 Date: Mon, 09 Jul 2012 10:20:01 -0700 From: William Siegrist wsiegr...@apple.com To: WebKit Development webkit-dev@lists.webkit.org Subject: [webkit-dev] lists.webkit.org Message-ID: 9e3eb5eb-04c4-4b65-9580-a19e65c36...@apple.com Content-Type: text/plain; CHARSET=US-ASCII lists.webkit.org is now on our new hardware. The migration did not go as smoothly as we had hoped, and you may have received some rejection emails in the past few hours. You may need to resend those messages. If you continue to have trouble sending mail (or receiving mail through a @ webkit.org alias), please let me know right away. Sorry for dropped mail, -Bill -- ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev End of webkit-dev Digest, Vol 86, Issue 8 * ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Updates on Chromium's content_shell
On Fri, Jul 13, 2012 at 2:24 PM, Adam Barth aba...@webkit.org wrote: On Fri, Jul 13, 2012 at 2:15 PM, Darin Fisher da...@chromium.org wrote: On Fri, Jul 13, 2012 at 2:10 PM, Adam Barth aba...@webkit.org wrote: Yeah, EventSender is likely a good place to start. Here are some more details about what I had in mind: 1) We'll need to create a new target that builds a TestRunner.a (or LayoutTestController.a, whatever name is currently in fashion). This target is allowed to depend on WTF, WebKit (via the WebKit API), and probably webkit_support. 2) This target will contain LayoutTestController.cpp, EventSender.cpp, and all the other code that's needed to support the objects we inject when running LayoutTests. 3) To move code into this target, we'll need to abstract any dependencies on the rest of DumpRenderTree into a delegate interface. For example, EventSender has a #include TestShell.h, which we'll need to remove. Today, EventSender gets the WebView is by asking m_shell for it. Instead, it will need to ask the delegate. One of the trickier things in this project will be WebViewHost. TestRunner.a will need some object like that to subclass a bunch of WebKit API clients, but the design might need to change a bit. I haven't studied it carefully yet. TestRunner.a could just provide the WebViewClient and WebFrameClient implementations. The delegate you mention could just be a createWebView function. When Jochen and I discussed this topic before, I suggested just adding CreateWebView to webkit_support, but as a delegate function seems even better. We'd probably like to minimize dependencies on webkit_support since that is Chromium specific. In addition to functions that talk with the WebKit API, there are also functions that are more about controlling the test harness. For example, dumpAsText and waitUntilDone will need some pathway for communicating with the outside world besides WebKit API. Currently, this information is pulled by TestShell.cpp, so maybe we don't need the delegate and can just have both DumpRenderTree and ContentShell pull this information out of TestRunner.a via some interface. I've created a meta bug to track this work: https://bugs.webkit.org/show_bug.cgi?id=91308 Adam Once this is done, but DumpRenderTree and ContentShell can link in TestRunner.a and implement the delegate. Hopefully the bulk of the interactions will be between TestRunner.a and the WebKit API so that the delegate will mostly be able providing the WebView and getting out of the way. Adam On Fri, Jul 13, 2012 at 1:56 PM, Jochen Eisinger joc...@chromium.orgwrote: What about keeping the discussion here, so others that might want to join the effort later can read it up? In general, I agree with your proposal. I guess starting with something small like EventSender might be a good first step. best -jochen On Fri, Jul 13, 2012 at 10:20 PM, Adam Barth aba...@webkit.org wrote: On Fri, Jul 13, 2012 at 1:10 PM, Jochen Eisinger joc...@chromium.orgwrote: On Fri, Jul 13, 2012 at 7:46 PM, Ryosuke Niwa rn...@webkit.orgwrote: On Fri, Jul 13, 2012 at 4:16 AM, Jochen Eisinger joc...@chromium.org wrote: I wanted to share some updates about content_shell (a multi-processed version of chromium's test shell): meanwhile, it is possible to generate pixel results. Out of 31431 tests that are executed on chromium-linux, 6234 did not match the expected results using content_shell, all others passed (80.17%)! This is a great news! Thanks a lot for working on this effort. Let us know if you needed a help in implementing testRunner methods. At this point, there are two things I could use help with: in general, moving methods to window.internals (and addressing the current shortcomings of that approach) helps a lot, as I can just instantiate this object in content_shell. The other thing is to work towards making layoutTestController (of chromium's DRT) a library. Adam mentioned a while ago that he'd be interested with helping with the latter as well Yes. The idea is to implement LayoutTestController in terms of the WebKit API and a (hopefully simple) delegate. Currently LayoutTestController knows too much about DumpRenderTree. That will let us share the implementation of LayoutTestController and avoid having to maintain yet another copy. Upstreaming the chromium-android port is at the top of my priority list, but I should be able to help out with this work as well. Should we talk off-list about how to approach this work? Adam ___ 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
