Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Fri, Dec 4, 2009 at 1:52 PM, Adam Treat wrote:
> On Friday 04 December 2009 04:22:57 pm Dirk Pranke wrote:
>> On Fri, Dec 4, 2009 at 9:39 AM, Adam Treat wrote:
>> > I don't think we should be changing the style guide for anything besides
>> > clarifications of currently unwritten rules. No matter how the fashion
>> > may change or how developers may change. Changing the rules throws
>> > consistency out the window which is, I believe, the greatest benefit of
>> > having a style guide.
>>
>> While I agree with your points re: subjectivity, and I agree that any
>> two competent programmers can disagree on any
>> points, it is also true that some practices can be shown to be more
>> reliable, maintainable, or readable, and those
>> practices do change over time, partially as technology changes and
>> partially because this is a social process.
>
> You can't show that any practice is more readable than another because it is
> subjective as you admit. People can argue over the readability of different
> style options and come to different conclusions much as has been done in this
> long thread.
Not necessarily so; it depends on your definitions. If you define
"more readable" as "more readable to me", then
you are correct. If you define it as "a majority of the population
finds it more readable", then you can show this,
by a simple poll. I was using the latter definition. In addition,
people change their minds over time; I used to prefer:
if (x)
{
foo():
}
over K&R, but I have gradually switched sides over the years ...
> As for technology changing, what do you think drives this particular style
> change other than the subjective opinions of a set of developers?
I don't believe this particular style change is being driven by a
technology change. I was
making a general counterargument to your general argument.
>> Dunno if this changes any of your thinking or not ...
>
> Sorry, not really.
No worries :)
-- Dirk
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] TARGET_OS_EMBEDDED
On December 4, 2009 at 9:29:11 AM, Maciej Stachowiak wrote: > On Dec 4, 2009, at 5:51 AM, Zoltan Herczeg wrote: > > > Hi, > > > > it would be a great to have a macro in WebKit, which would be enabled on > > embedded systems. We could replace macros like PLATFORM(SYMBIAN) in > > TextCodecQt.cpp to this new macro. However, TARGET_OS_EMBEDDED macro > > enables WTF_PLATFORM_IPHONE. Well, not only symbian and iPhone exist in > > embedded domain. What is your suggestion? > > I think we should probably phase out TARGET_OS_EMBEDDED, as it seems too > general > to be useful. TARGET_OS_EMBEDDED is only used to define PLATFORM(IPHONE) (which expands to WTF_PLATFORM_IPHONE). TARGET_OS_EMBEDDED was not intended and should not be used within any of the WebKit projects itself. If other platforms are defining the "TARGET_OS_EMBEDDED" macro separately which is causing PLATFORM(IPHONE) to be defined unintentionally, then we should qualify the definition of PLATFORM(IPHONE) to include PLATFORM(DARWIN) (or similar) so that this doesn't happen. I don't see a need for a generic PLATFORM(EMBEDDED) macro currently. I think it's best to define specific platforms with the PLATFORM() macro and enable features on a per-platform basis as we're currently doing. Dave ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] More on test flakiness
On Dec 2, 2009, at 2:43 PM, Julie Parent wrote: In a recent code review where I was minimizing some test flakiness, Darin said: "I wish there was a way to isolate timing-dependent tests separately from the vast majority of tests that can run at any speed. I'd prefer to not have tests that pass or fail based on the speed or load of the computer, but if we do knowingly have them it would be *so* much better if they were identified somehow." I agree, and would like to implement something to address this. Possible ways to mark a test as timing-dependent: Put tests in a specific directory Append a suffix to the test name Add a function call to layoutTestController that is called explicitly for timing dependent tests Out of these ways to mark timing-sensitive tests, I prefer a specific directory. This is a list we should try to drive to 0 over time, so having a dedicated directory would make it easiest to see how far off we are from that goal. From here, the issue becomes how to use the knowledge. Some ideas: If one of these tests fails, don't turn the bots red, turn some other color If one of these tests fails, re-run it. If it passes the second time, consider it a normal pass Turn bots red as normal, but with an indicator that the test is known timing dependent (if we used a suffix on the test name, I guess this would just be obvious) Thoughts? I prefer the following: - If one of the tests fails a first time, re-run it a second time. - If it fails the second time, mark the bot red (with an indication that the test is timing sensitive). - If the test failed the first time only, mark the bot some color other than red or green, so we are reminded that this specific test is flaking out without actually marking the build as broken. Some theoretically timing-dependent tests may turn out to fail exceedingly often, or may turn out not to really be timing-dependent in practice, or may become more flaky at some future point, so it would be good to have some indicator of that in the buildbot. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] More on test flakiness
Given the feedback I've seen here, how about we do the following: Specifically mark timing sensitive tests. If a marked tests fails, re-run it. If it passes the second time, consider it a normal pass and keep bots green. If it fails the second time, turn bots red. The easiest way to mark the tests seems to be to move them into a specific directory. Any opposition or better ideas? Thanks, Julie On Thu, Dec 3, 2009 at 6:49 AM, Gustavo Noronha Silva wrote: > On Wed, 2009-12-02 at 14:51 -0800, Julie Parent wrote: > > As Eric just said to me in person, another option is to just re-run > > *any* failing test twice, and only turn tree red if it fails twice. > > (Chromium just recently started doing this, and it has greatly > > improved our tree greenness). This obviously doesn't explicitly > > identify timing dependent tests, but it solves the bigger issues that > > flaky tests cause. > > But that would turn moot the point of the suggestion, I think. Having > only tests that are expected to fail under special conditions be tested > twice makes sense, but if a test that isn't expected to fail under > special conditions fails, we should see that as a failure. > > To give you a bit of insight into this, in GTK+ we used to have tests > that only failed when they were preceded by a specific test. This was > very important information that would be lost if it was run after > itself, and thus passed. The problems that caused this were missing > support in DRT, and a couple of times real bugs that caused crashes. > > This is to say I think we would be better served by only running > known-time-dependent tests twice. > > Thanks, > > -- > Gustavo Noronha Silva > GNOME Project > > ___ > 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] Issues with WebWorkers that may block Safari support
On Dec 4, 2009, at 2:09 PM, Drew Wilson wrote: On Fri, Dec 4, 2009 at 2:00 PM, Maciej Stachowiak wrote: On Dec 4, 2009, at 1:40 PM, Drew Wilson wrote: Hi all, There have been various side discussions regarding the stability/ viability of Workers and SharedWorkers, and I wanted to make sure we understand all of the concerns, since as far as I know there isn't any one person who has been privy to all of the conversations (or maybe it's just me that's been out of the loop :). So I'll enumerate the issues I'm aware of below, and people can chime in on any that I've missed that may impact the ability to ship these features, and we can figure out how to get them addressed ASAP. 1) worker-lifecycle.html fails intermittently (https://bugs.webkit.org/show_bug.cgi?id=29344 ) The original report doesn't seem to be a true worker failure, but rather an artifact of the way GC works in JSC. Since JSC uses conservative GC, it's quite possible for the VM to think that there's a dangling reference to a Worker even though there isn't actually one, and that seems to be what's happening here (workers would get shutdown when the parent document closes regardless, so there's no actual leak). Unless someone has some idea of how to make this GC more deterministic, my recommendation would be to just disable this test and close this bug, since seemingly it's the test itself that is not reliable, not the underlying worker code. That said, there's a sporadic worker crash that seems to have started happening in the last week or so which I believe is a different issue - eseidel has glommed that crash onto the same bug, and I think we should move that to its own issue and see if we can collect more information about it. For what it's worth, we'd likely consider a resolution to this bug a blocker to shipping the next Safari release, but we would not consider disabling dedicated Workers as a solution, since we have shipped them already. If this bug could be shown to be an error in the test and inappropriate dependence on GC, that would be a satisfactory resolution. However, the sporadic failure here is a at least sometimes a crash. A crash definitely indicates a bug in WebKit, not in the test, even if the test itself is also flawed. Therefore, I would not consider disabling the test a solution until we have at least addressed the crash. That makes sense to me - we should try to address the crash, and if keeping this test enabled is the only way to do that, that's fine. I would recommend changing the test, however, so the test itself always passes so it's not making the bots red while we're waiting to reproduce the crash. As long as we have some bug report tracking the crash and add a new test when we fix it that would detect it, I'm fine with that. Regarding Geoff's comments - I'm as convinced as I can be (based on my own and ap's efforts stepping through the GC code - see ap's comments in the bug) that the original failures were a "conservative GC" issue. I'm honestly stumped as to how to move forward on that. We should definitely change the test to not depend on that, if we can. I am a little dubious of the sporadic failure truly being a conservative GC issue and not being fixable; in the past we have managed to make GC-related tests reliable even if they could be affected by conservative GC in theory. I'll look at the test myself and see if any changes seem like they would help. But as I said, I'm more concerned with the crash than the sporadic non- crash failure. 3) Potential race conditions with Document.postTask() (https://bugs.webkit.org/show_bug.cgi?id=31633 ) I've suggested a simple solution to the potential race condition with SharedWorkers in the bug. I'd be interested in hearing whether people think it's a good approach or not - if so, I'm happy to submit a patch for review in short order. There's still the question as to whether dispatching tasks on a detached Document is OK or not, but I'm assuming it is (since that's what Dedicated workers have always done). Are there other issues we should be addressing as a high priority? This one would probably be a ship blocker. We would consider disabling SharedWorkers to ship if that addressed the issue. Any objections to the solution I proposed? Haven't had a chance to study it yet, but I'll comment in the bug if I have any thoughts. I'm not an expert in this code though so I'm not sure my own input is the most useful here. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Issues with WebWorkers that may block Safari support
On Fri, Dec 4, 2009 at 2:00 PM, Maciej Stachowiak wrote: > > On Dec 4, 2009, at 1:40 PM, Drew Wilson wrote: > > Hi all, > > There have been various side discussions regarding the stability/viability > of Workers and SharedWorkers, and I wanted to make sure we understand all of > the concerns, since as far as I know there isn't any one person who has been > privy to all of the conversations (or maybe it's just me that's been out of > the loop :). So I'll enumerate the issues I'm aware of below, and people can > chime in on any that I've missed that may impact the ability to ship these > features, and we can figure out how to get them addressed ASAP. > > 1) worker-lifecycle.html fails intermittently ( > https://bugs.webkit.org/show_bug.cgi?id=29344) > > The original report doesn't seem to be a true worker failure, but rather an > artifact of the way GC works in JSC. Since JSC uses conservative GC, it's > quite possible for the VM to think that there's a dangling reference to a > Worker even though there isn't actually one, and that seems to be what's > happening here (workers would get shutdown when the parent document closes > regardless, so there's no actual leak). Unless someone has some idea of how > to make this GC more deterministic, my recommendation would be to just > disable this test and close this bug, since seemingly it's the test itself > that is not reliable, not the underlying worker code. > > That said, there's a sporadic worker crash that seems to have started > happening in the last week or so which I believe is a different issue - > eseidel has glommed that crash onto the same bug, and I think we should move > that to its own issue and see if we can collect more information about it. > > > For what it's worth, we'd likely consider a resolution to this bug a > blocker to shipping the next Safari release, but we would not consider > disabling dedicated Workers as a solution, since we have shipped them > already. > > If this bug could be shown to be an error in the test and inappropriate > dependence on GC, that would be a satisfactory resolution. However, the > sporadic failure here is a at least sometimes a crash. A crash definitely > indicates a bug in WebKit, not in the test, even if the test itself is also > flawed. Therefore, I would not consider disabling the test a solution until > we have at least addressed the crash. > > That makes sense to me - we should try to address the crash, and if keeping this test enabled is the only way to do that, that's fine. I would recommend changing the test, however, so the test itself always passes so it's not making the bots red while we're waiting to reproduce the crash. Regarding Geoff's comments - I'm as convinced as I can be (based on my own and ap's efforts stepping through the GC code - see ap's comments in the bug) that the original failures were a "conservative GC" issue. I'm honestly stumped as to how to move forward on that. > > 2) SharedWorkers proxy their network access to a parent document (no bug > for this currently) > > The result of this is the application can get a network error if the user > has multiple windows open that are using the same SharedWorker, then closes > the parent document that is acting as a network proxy while a network > request is in progress. While this is something that would need to change > eventually in order to properly support exposing the appcache APIs to > SharedWorker context, it doesn't seem like this would be any kind of ship > blocker. Any robust application would need to deal with sporadic network > hiccups anyway by retrying, and in practice this situation will almost never > be encountered in the field. Perhaps people have other concerns that make > this a ship blocker? > > > I don't believe we consider this a ship blocker. Though we did discuss it > at the same time as the other two items on the list. > > > 3) Potential race conditions with Document.postTask() ( > https://bugs.webkit.org/show_bug.cgi?id=31633) > > I've suggested a simple solution to the potential race condition with > SharedWorkers in the bug. I'd be interested in hearing whether people think > it's a good approach or not - if so, I'm happy to submit a patch for review > in short order. There's still the question as to whether dispatching tasks > on a detached Document is OK or not, but I'm assuming it is (since that's > what Dedicated workers have always done). > > Are there other issues we should be addressing as a high priority? > > > This one would probably be a ship blocker. We would consider disabling > SharedWorkers to ship if that addressed the issue. > Any objections to the solution I proposed? > > Regards, > Maciej > > ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Issues with WebWorkers that may block Safari support
On Dec 4, 2009, at 1:40 PM, Drew Wilson wrote: Hi all, There have been various side discussions regarding the stability/ viability of Workers and SharedWorkers, and I wanted to make sure we understand all of the concerns, since as far as I know there isn't any one person who has been privy to all of the conversations (or maybe it's just me that's been out of the loop :). So I'll enumerate the issues I'm aware of below, and people can chime in on any that I've missed that may impact the ability to ship these features, and we can figure out how to get them addressed ASAP. 1) worker-lifecycle.html fails intermittently (https://bugs.webkit.org/show_bug.cgi?id=29344 ) The original report doesn't seem to be a true worker failure, but rather an artifact of the way GC works in JSC. Since JSC uses conservative GC, it's quite possible for the VM to think that there's a dangling reference to a Worker even though there isn't actually one, and that seems to be what's happening here (workers would get shutdown when the parent document closes regardless, so there's no actual leak). Unless someone has some idea of how to make this GC more deterministic, my recommendation would be to just disable this test and close this bug, since seemingly it's the test itself that is not reliable, not the underlying worker code. That said, there's a sporadic worker crash that seems to have started happening in the last week or so which I believe is a different issue - eseidel has glommed that crash onto the same bug, and I think we should move that to its own issue and see if we can collect more information about it. For what it's worth, we'd likely consider a resolution to this bug a blocker to shipping the next Safari release, but we would not consider disabling dedicated Workers as a solution, since we have shipped them already. If this bug could be shown to be an error in the test and inappropriate dependence on GC, that would be a satisfactory resolution. However, the sporadic failure here is a at least sometimes a crash. A crash definitely indicates a bug in WebKit, not in the test, even if the test itself is also flawed. Therefore, I would not consider disabling the test a solution until we have at least addressed the crash. 2) SharedWorkers proxy their network access to a parent document (no bug for this currently) The result of this is the application can get a network error if the user has multiple windows open that are using the same SharedWorker, then closes the parent document that is acting as a network proxy while a network request is in progress. While this is something that would need to change eventually in order to properly support exposing the appcache APIs to SharedWorker context, it doesn't seem like this would be any kind of ship blocker. Any robust application would need to deal with sporadic network hiccups anyway by retrying, and in practice this situation will almost never be encountered in the field. Perhaps people have other concerns that make this a ship blocker? I don't believe we consider this a ship blocker. Though we did discuss it at the same time as the other two items on the list. 3) Potential race conditions with Document.postTask() (https://bugs.webkit.org/show_bug.cgi?id=31633 ) I've suggested a simple solution to the potential race condition with SharedWorkers in the bug. I'd be interested in hearing whether people think it's a good approach or not - if so, I'm happy to submit a patch for review in short order. There's still the question as to whether dispatching tasks on a detached Document is OK or not, but I'm assuming it is (since that's what Dedicated workers have always done). Are there other issues we should be addressing as a high priority? This one would probably be a ship blocker. We would consider disabling SharedWorkers to ship if that addressed the issue. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Fri, Dec 4, 2009 at 12:13 AM, Chris Jerdonek wrote: > An alternative policy is as follows: > > (1) If-else control structures must have either braces around all > clauses or braces around no clauses. > > (2) A clause with more than one line must be surrounded by braces. > This is actually the precise policy used by the Google C++ Style Guide, which governs the Chromium repository. The problem with this is that it fails to ensure consistency, and the difference between consistently using braces everywhere and consistently not using them on one-line bodies is significant enough that this comes up as a bone of contention in code changes. On Fri, Dec 4, 2009 at 6:30 AM, Adam Treat wrote: > It seems we keep changing the style guide as fashion changes. It is meant > to > ensure consistency and readability. This is a purely subjective change and > I > think unwarranted. Obviously if I agreed I wouldn't have proposed this change, but it's worth detailing why I disagree. You mention that the style guide is meant to ensure readability. It seems clear, then, that you would support changes to improve readability if they did not decrease consistency; the main problems are that readability is significantly subjective (see some of the expressed opinions in this thread), and changing the rules currently decreases consistency (at least over the short to medium term). I think safety and maintenance cost are also worth considering. My opinion is that this change (as well as Maciej's proposed variant) does not merely affect readability but also improves safety very slightly. (Requiring braces at all times would improve safety more, and decrease maintenance cost, but at perhaps some cost to readability.) I agree with you that our codebase should be consistent. Personally, I would prefer that when we make style guide changes, we actually change the codebase to comply with them. Most of the work here could probably be automated, and I think if we did this it would address the main concern you have. The counterargument to this is that it can obscure blame (probably moreso for a change like the namespace indenting one than this change, but noticeably in both cases). On the other hand, gradual changes obscure blame (gradually, at the changed locations) too, and it's already the case that when digging up the history of a block of code you need to repeatedly skip past uninteresting changes. On Fri, Dec 4, 2009 at 9:39 AM, Adam Treat wrote: > I don't think we should be changing the style guide for anything besides > clarifications of currently unwritten rules. No matter how the fashion may > change or how developers may change. Changing the rules throws consistency > out the window which is, I believe, the greatest benefit of having a style > guide. Again, I think the consistency argument would be addressed if we actually made code consistent at rule changes. Changing code to make it more readable by the current developer set, when that developer set changes in makeup or opinion, seems like a good thing to me, as it improves productivity and code quality, regardless of whether the change is subjective in nature. As an example I give you the SQLite codebase, which has a consistent set of style rules that are so different from any other code I have ever encountered that I struggle to read and patch the code (and have been told the same by SQLite developers here at Google). You may argue that such a case is far outside any cognitive burden of the rule in question in this thread, but tiny amounts of cognitive friction, spread over a large codebase with many developers and a long lifetime, add up. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Friday 04 December 2009 04:22:57 pm Dirk Pranke wrote: > On Fri, Dec 4, 2009 at 9:39 AM, Adam Treat wrote: > > I don't think we should be changing the style guide for anything besides > > clarifications of currently unwritten rules. No matter how the fashion > > may change or how developers may change. Changing the rules throws > > consistency out the window which is, I believe, the greatest benefit of > > having a style guide. > > While I agree with your points re: subjectivity, and I agree that any > two competent programmers can disagree on any > points, it is also true that some practices can be shown to be more > reliable, maintainable, or readable, and those > practices do change over time, partially as technology changes and > partially because this is a social process. You can't show that any practice is more readable than another because it is subjective as you admit. People can argue over the readability of different style options and come to different conclusions much as has been done in this long thread. As for technology changing, what do you think drives this particular style change other than the subjective opinions of a set of developers? > Dunno if this changes any of your thinking or not ... Sorry, not really. Cheers, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Issues with WebWorkers that may block Safari support
> The original report doesn't seem to be a true worker failure, but rather an > artifact of the way GC works in JSC. Since JSC uses conservative GC, it's > quite possible for the VM to think that there's a dangling reference to a > Worker even though there isn't actually one, and that seems to be what's > happening here (workers would get shutdown when the parent document closes > regardless, so there's no actual leak). Unless someone has some idea of how > to make this GC more deterministic, my recommendation would be to just > disable this test and close this bug, since seemingly it's the test itself > that is not reliable, not the underlying worker code. If the test just tries to determine whether an object has been collected, and it discovers that the object hasn't been collected in JSC, and the reason it hasn't been collected is that the conservative mark method finds a pointer to it on the stack, then disabling the test seems reasonable. However, the I don't think anyone has proven those things, and the code involved looks pretty wonky, so something else may be going wrong here. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Issues with WebWorkers that may block Safari support
Hi all, There have been various side discussions regarding the stability/viability of Workers and SharedWorkers, and I wanted to make sure we understand all of the concerns, since as far as I know there isn't any one person who has been privy to all of the conversations (or maybe it's just me that's been out of the loop :). So I'll enumerate the issues I'm aware of below, and people can chime in on any that I've missed that may impact the ability to ship these features, and we can figure out how to get them addressed ASAP. 1) worker-lifecycle.html fails intermittently ( https://bugs.webkit.org/show_bug.cgi?id=29344) The original report doesn't seem to be a true worker failure, but rather an artifact of the way GC works in JSC. Since JSC uses conservative GC, it's quite possible for the VM to think that there's a dangling reference to a Worker even though there isn't actually one, and that seems to be what's happening here (workers would get shutdown when the parent document closes regardless, so there's no actual leak). Unless someone has some idea of how to make this GC more deterministic, my recommendation would be to just disable this test and close this bug, since seemingly it's the test itself that is not reliable, not the underlying worker code. That said, there's a sporadic worker crash that seems to have started happening in the last week or so which I believe is a different issue - eseidel has glommed that crash onto the same bug, and I think we should move that to its own issue and see if we can collect more information about it. 2) SharedWorkers proxy their network access to a parent document (no bug for this currently) The result of this is the application can get a network error if the user has multiple windows open that are using the same SharedWorker, then closes the parent document that is acting as a network proxy while a network request is in progress. While this is something that would need to change eventually in order to properly support exposing the appcache APIs to SharedWorker context, it doesn't seem like this would be any kind of ship blocker. Any robust application would need to deal with sporadic network hiccups anyway by retrying, and in practice this situation will almost never be encountered in the field. Perhaps people have other concerns that make this a ship blocker? 3) Potential race conditions with Document.postTask() ( https://bugs.webkit.org/show_bug.cgi?id=31633) I've suggested a simple solution to the potential race condition with SharedWorkers in the bug. I'd be interested in hearing whether people think it's a good approach or not - if so, I'm happy to submit a patch for review in short order. There's still the question as to whether dispatching tasks on a detached Document is OK or not, but I'm assuming it is (since that's what Dedicated workers have always done). Are there other issues we should be addressing as a high priority? -atw ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Fri, Dec 4, 2009 at 9:39 AM, Adam Treat wrote: > I don't think we should be changing the style guide for anything besides > clarifications of currently unwritten rules. No matter how the fashion may > change or how developers may change. Changing the rules throws consistency > out the window which is, I believe, the greatest benefit of having a style > guide. > While I agree with your points re: subjectivity, and I agree that any two competent programmers can disagree on any points, it is also true that some practices can be shown to be more reliable, maintainable, or readable, and those practices do change over time, partially as technology changes and partially because this is a social process. Hence I believe that if there was a significant consensus that rules should be changed, that is okay, even if there is no semantic difference in the rule change. I agree that having style guidelines that are not followed *in the existing codebase* are pretty much useless, even if they're used for new code. But, changing the rules only throws consistency out the window if the code is not updated to conform with the rule, so an alternative to an inconsistently-formatted codebase (and a useless style guide) or style-guide stais is to require "whitespace-only" CLs to update the codebase (and yes, I know some people object to such things). Accordingly, I believe that whitespace CLs are an acceptable cost to impose as a part of this, and people proposing the change should be willing to pay the cost. Of course, I think we should attempt to determine the cost before deciding to make the change. Dunno if this changes any of your thinking or not ... Peter, were you thinking that you (or at least someone) would attempt to bring the code into compliance with the new rule? -- Dirk ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Strings on multiple threads
On 03.12.2009, at 18:13, Jeremy Orlow wrote: Easier said than done in many cases. For DOM Storage and Databases, there are several classes that are used on multiple threads. And several of these classes contain strings (or contain things that contain strings). It seems very difficult to be absolutely sure these strings are only shared in a safe way unless you make a threadsafeCopy every time you access it. Doing so would probably be negligible performance wise for all the cases I'm looking at, so maybe that's the right answer, but these issues are very subtle. And that's what concerns me most. We should aim to gradually redesign these classes to only use message passing, and generally cleaning up what happens on which thread. It's indeed very difficult to work with strings in this code, but I don't think that's the only problem that arises from using objects on multiple threads there, and it's not necessarily one to tackle in isolation. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Fri, Dec 4, 2009 at 9:39 AM, Adam Treat wrote: > On Friday 04 December 2009 12:17:03 pm Jeremy Orlow wrote: > > I'm not necessarily disagreeing with your conclusion, but I have to ask: > > can you think of an example of a style change that isn't subjective > and/or > > something that can change as "the fashion" (or, more likely, the > developers > > working on the project) changes? Even stuff like the namespace change > > really depends on your development style (to determine whether or not you > > care about those 4 extra spaces "wasted"). > > Very little of it is not subjective. The main thing the style guide gives > us > is consistency. And that I am fully supportive of. But a lot of it is > merely > taste. The compiler excepts both. Two developers fully informed and both > of > great technical prowess can reasonably disagree on almost all points of the > style guide. That meets my definition of subjective for this purpose. > > Regarding the namespace change, I objected to that change too (and for the > very same reasons) if you look through the archives. > > I don't think we should be changing the style guide for anything besides > clarifications of currently unwritten rules. No matter how the fashion may > change or how developers may change. Changing the rules throws consistency > out the window which is, I believe, the greatest benefit of having a style > guide. > > Cheers, > Adam > You make a very compelling argument. Consistency first. I recant my support for the proposed change. -Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] SVG Filters
On Dec 4, 2009, at 11:02 AM, Dirk Schulze wrote: What kinds of tests do we have for the code already? Do we have code that tries to exercise edge cases? Do we have a fuzzer of some sort? -- Darin Every effect that was implemented has at least one test. They are mostly simple test cases that just test one effect at once but there are also more complex tests, to see the behavior on combining different effects. I try to address the different edge cases of every filter effect and add more tests if necessary. Mainly effects with pixel manipulation already have more than one test to target different edge cases. I think the feature is ready to be enabled by default. One thing that would strongly increase my confidence in actually shipping it would be some form of fuzz testing. Design review by security experts would also help, but that is hard to arrange. Whereas anyone can write a fuzz tester. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] SVG Filters
> What kinds of tests do we have for the code already? Do we have code that > tries to exercise edge cases? Do we have a fuzzer of some sort? > > -- Darin Every effect that was implemented has at least one test. They are mostly simple test cases that just test one effect at once but there are also more complex tests, to see the behavior on combining different effects. I try to address the different edge cases of every filter effect and add more tests if necessary. Mainly effects with pixel manipulation already have more than one test to target different edge cases. - Dirk ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] SVG Filters
On Dec 1, 2009, at 6:07 PM, Nikolas Zimmermann wrote: > I'd like to enable SVG FIlters support by default. This is the last remaining > piece before we can officially claim SVG 1.0/1.1 support, in our SVG DOM > implementation (through SVG requiredFeatures/requiredExtensions > functionality). > > Dirk has done an amazing job, providing most of our new cross-platform filter > support. In previous discussions, security concerns have been raised, as the > code is doing pixel-manipulations, with web content as input, so it's a place > that needs special attention. Oliver specifically asked for a person not > involved in reviewing the patches, but a 3rd party to check the code for > potential problems. > > What do you think about this approach? Would anyone volunteer, for having a > look over the existing filters code in trunk? > Does anyone see other problems with turning on filters? If this is in good shape, I’d love to see this turned on in nightly builds, especially if have lots of good regression tests for it. It’s good to have the code tested and lived on for a while. I think it would be great for us to figure out what type of testing and reviewing we need to do to be confident enough of the security of the code to turn it on for releases such as the WebKit that comes with a future version of Safari. At a high level it sounds great for someone to check this for security problems, but it’s not obvious to me that someone will be available and have the skills to do it. What kinds of tests do we have for the code already? Do we have code that tries to exercise edge cases? Do we have a fuzzer of some sort? -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Friday 04 December 2009 12:17:03 pm Jeremy Orlow wrote: > I'm not necessarily disagreeing with your conclusion, but I have to ask: > can you think of an example of a style change that isn't subjective and/or > something that can change as "the fashion" (or, more likely, the developers > working on the project) changes? Even stuff like the namespace change > really depends on your development style (to determine whether or not you > care about those 4 extra spaces "wasted"). Very little of it is not subjective. The main thing the style guide gives us is consistency. And that I am fully supportive of. But a lot of it is merely taste. The compiler excepts both. Two developers fully informed and both of great technical prowess can reasonably disagree on almost all points of the style guide. That meets my definition of subjective for this purpose. Regarding the namespace change, I objected to that change too (and for the very same reasons) if you look through the archives. I don't think we should be changing the style guide for anything besides clarifications of currently unwritten rules. No matter how the fashion may change or how developers may change. Changing the rules throws consistency out the window which is, I believe, the greatest benefit of having a style guide. Cheers, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] TARGET_OS_EMBEDDED
On Dec 4, 2009, at 5:51 AM, Zoltan Herczeg wrote: Hi, it would be a great to have a macro in WebKit, which would be enabled on embedded systems. We could replace macros like PLATFORM(SYMBIAN) in TextCodecQt.cpp to this new macro. However, TARGET_OS_EMBEDDED macro enables WTF_PLATFORM_IPHONE. Well, not only symbian and iPhone exist in embedded domain. What is your suggestion? I think we should probably phase out TARGET_OS_EMBEDDED, as it seems too general to be useful. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Fri, Dec 4, 2009 at 6:30 AM, Adam Treat wrote: > On Thursday 03 December 2009 02:30:17 pm Kenneth Russell wrote: > > In the WebKit WebGL implementation I've frequently encountered the > > case where the else clause in a single if/else is a one-liner, and I > > find it both ugly and error-prone to have to remove its braces. I'd > > really like to be able to use braces on both arms. > > > > Perhaps existing code could be grandfathered in but new or modified > > code required to conform to the rule Peter proposes. > > > > -Ken > > I'd like to lodge an objection on the grounds that the style guide is a > matter > of subjective opinion and rationally only serves to make our code > consistent. > I do not like changing this style guide as the fashion changes. > > However, as Hyatt might note I do not have a personally substantive problem > with this particular change as I'd actually prefer this if the style guide > were being made up today. > > It seems we keep changing the style guide as fashion changes. It is meant > to > ensure consistency and readability. This is a purely subjective change and > I > think unwarranted. > I'm not necessarily disagreeing with your conclusion, but I have to ask: can you think of an example of a style change that isn't subjective and/or something that can change as "the fashion" (or, more likely, the developers working on the project) changes? Even stuff like the namespace change really depends on your development style (to determine whether or not you care about those 4 extra spaces "wasted"). ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] TARGET_OS_EMBEDDED
I think this would suffer from lack of clarity. Not all embedded systems are alike and not all will wish to be treated in the same way. On Friday 04 December 2009 08:51:12 am Zoltan Herczeg wrote: > Hi, > > it would be a great to have a macro in WebKit, which would be enabled on > embedded systems. We could replace macros like PLATFORM(SYMBIAN) in > TextCodecQt.cpp to this new macro. However, TARGET_OS_EMBEDDED macro > enables WTF_PLATFORM_IPHONE. Well, not only symbian and iPhone exist in > embedded domain. What is your suggestion? > > Zoltan > > > ___ > 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] Proposing style guide change regarding braces on conditional arms
On Thursday 03 December 2009 02:30:17 pm Kenneth Russell wrote: > In the WebKit WebGL implementation I've frequently encountered the > case where the else clause in a single if/else is a one-liner, and I > find it both ugly and error-prone to have to remove its braces. I'd > really like to be able to use braces on both arms. > > Perhaps existing code could be grandfathered in but new or modified > code required to conform to the rule Peter proposes. > > -Ken I'd like to lodge an objection on the grounds that the style guide is a matter of subjective opinion and rationally only serves to make our code consistent. I do not like changing this style guide as the fashion changes. However, as Hyatt might note I do not have a personally substantive problem with this particular change as I'd actually prefer this if the style guide were being made up today. It seems we keep changing the style guide as fashion changes. It is meant to ensure consistency and readability. This is a purely subjective change and I think unwarranted. Cheers, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] TARGET_OS_EMBEDDED
Hi, it would be a great to have a macro in WebKit, which would be enabled on embedded systems. We could replace macros like PLATFORM(SYMBIAN) in TextCodecQt.cpp to this new macro. However, TARGET_OS_EMBEDDED macro enables WTF_PLATFORM_IPHONE. Well, not only symbian and iPhone exist in embedded domain. What is your suggestion? Zoltan ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Thu, Dec 3, 2009 at 10:17 PM, Peter Kasting wrote: > On Thu, Dec 3, 2009 at 6:24 PM, Chris Jerdonek > wrote: >> >> For the people thinking about this, it would be nice if the final >> policy minimizes (and does not increase) the likelihood of having to >> modify several lines of surrounding code after touching one line of >> code. I don't think this consideration has been raised. > > Do you mean "in the steady state of modifying the code base" or "while files > aren't compliant with whatever change might get made"? I meant the former (though my alternative below also reduces the latter). > I think you mean the > former, but if so, I'm not sure what policy would serve you best. Ideally > you'd want "always use braces", but failing that, each proposal has a > different set of cases where you do/don't have to change as you touch > things. I don't feel strongly about this, but I will provide an example to illustrate what I mean. The illustrative case is several "else if" clauses with braces -- only one of which exceeds one line. If a code change removes the excess lines in that one clause, the two rules being considered say you have to remove the braces from all the clauses -- even though the clauses are already lined up. And this change can go back and forth indefinitely. Several people are already saying they value alignment within individual if-else control structures more than they value the number of braces. So removing the braces from all the clauses in the example above seems secondary. An alternative policy is as follows: (1) If-else control structures must have either braces around all clauses or braces around no clauses. (2) A clause with more than one line must be surrounded by braces. A consequence of this policy is that if-else statements may gradually converge to braces, but this change would take place only as needed. --Chris ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
