Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms

2009-12-04 Thread Dirk Pranke
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

2009-12-04 Thread David Kilzer
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

2009-12-04 Thread Maciej Stachowiak


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

2009-12-04 Thread Julie Parent
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

2009-12-04 Thread Maciej Stachowiak


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

2009-12-04 Thread Drew Wilson
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

2009-12-04 Thread Maciej Stachowiak


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

2009-12-04 Thread Peter Kasting
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

2009-12-04 Thread Adam Treat
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

2009-12-04 Thread Geoffrey Garen
> 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

2009-12-04 Thread Drew Wilson
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

2009-12-04 Thread Dirk Pranke
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

2009-12-04 Thread Alexey Proskuryakov


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

2009-12-04 Thread Darin Fisher
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

2009-12-04 Thread Maciej Stachowiak


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

2009-12-04 Thread Dirk Schulze
> 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

2009-12-04 Thread Darin Adler
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

2009-12-04 Thread Adam Treat
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

2009-12-04 Thread Maciej Stachowiak


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

2009-12-04 Thread Jeremy Orlow
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

2009-12-04 Thread Adam Treat
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

2009-12-04 Thread Adam Treat
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

2009-12-04 Thread Zoltan Herczeg
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

2009-12-04 Thread Chris Jerdonek
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