[webkit-dev] WTF::currentThread() and non-WTF threads
Hi webkit-dev!
Trying to add ASSERTS to RefCounted objects (
https://bugs.webkit.org/show_bug.cgi?id=31639) I've added some more calls to
WTF::currentThread() and started to get a lot of assertions here:
static ThreadIdentifier establishIdentifierForPthreadHandle(pthread_t&
pthreadHandle)
{
ASSERT(!identifierByPthreadHandle(pthreadHandle));
Here is what happens: A thread that is not created by WTF::CreateThread is
calling currentThread() in hope to get a ThreadIdentifier. It doesn't have
it so we create it and put into ThreadMap. Normally, WTF threads call
WTF::detachThread() that removes the ThreadIdentifier from the map. The
'other' threads do not do so - so the TheradIdentifier remains in the map.
On OSX, pthreads reuse system handles when one thread terminates and another
starts... That easily leads to threads that run sequentially to have the
same handle, and the same ThreadIdentifier - since it is still in the
threadMap, which makes currentThread() kind of useless since it returns the
same id for different threads, the very thing the threadMap is tryign to
avoid.
I'm thinking about changing the implementation to stop "auto-register"
non-WTF threads. Instead, lets add a new function, WTF::registerThread()
that would establish the ThreadIdentifier for a thread not created by
WTF::createThread - and assert in currentThread() if the current thread is
unknown to WTF. This way, we could find all the cases of such threads and
equip them with registerThread()/detachThread() pair that will keep the
thread map in a good state.
The currentThread() would look like this:
ThreadIdentifier currentThread()
{
pthread_t currentThread = pthread_self();
if (ThreadIdentifier id = identifierByPthreadHandle(currentThread))
return id;
ASSERT_NOT_REACHED();
// Either the thread is not created by WTF::CreateThread() or registered
by WTF::registerThread(), or we are getting
// a call from thread-specific destructor after WTF::detachThread() was
called and ThreadIdentifier removed.
// Neither scenario permits reliable thread id tracking, so we can not
return a meaningful ThreadIdentifier here.
// Normally ThreadIdentifiers are compared so lets generate a fake
one-time ThreadIdentifier to fail comparison, if
// it is in fact done.
static ThreadIdentifier fakeId = minFakeIdentifierCount;
if (fakeId == maxFakeIdentifierCount)
fakeId = minFakeIdentifierCount;
return fakeId++;
}
What would you say? Any bad feelings about that?
Dmitry
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Database in Worker context
On Thu, Dec 10, 2009 at 1:09 PM, Dmitry Titov wrote: > On Thu, Dec 10, 2009 at 11:09 AM, Eric Uhrhane wrote: >> >> On Wed, Dec 9, 2009 at 6:55 PM, Dmitry Titov wrote: >> > Thanks for the link to the prototype code! >> > If I understand it right and the DatabaseManager can not have any other >> > relationship with ScriptExecutionContext but 1-1 and their lifetime is >> > the >> > same then it's not clear to me what benefit could be had by effectively >> > splitting a class in two. The fact that DatabaseManager is RefCounted >> > also >> > hints that some other object could take ownership of it as well outside >> > of >> > lifetime of ScriptExecutionContext, but is this true? >> >> That's there because the DatabaseThread [and therefore the Database] >> can stick around for a bit after the ScriptExecutionContext is deleted >> if I make it non-refcounted. If I move the DatabaseManager parts into >> the ScriptExecutionContext, then I have to make sure that the SEC >> sticks around until after the Database and DatabaseThread have gone >> away. > > It feels that there is a new class that does not have a specific lifetime or > relationship to other existing classes, as it is a mechanical part of some > other class rather... > I look at how m_document is used by current Database and it feels that if > SEC had a databaseThread() accessor, then all other methods like > addOpenDatabase etc could be part of DatabaseThread instead, with Database > keeping a pointer to its DatabaseThread. What do you think about this? You mean that DatabaseManager would merge into DatabaseThread, which would then have DocumentDatabaseThread and WorkerDatabaseThread to cover the DocumentDatabaseManager and WorkerDatabaseManager split? That's doable. It stretches the DT class a bit, but it's not too bad. It looks like there's some redundancy between the two [m_openDatabaseSet in both] that I might be able to clear out, so that's a plus. >> > ScriptExecutionContext collects functionality common for Workers and >> > Documents, and as such is a home for a few 'groups' of methods, like a >> > few >> > methods to deal with MessagePorts. MessagePort, in turn, has a raw >> > pointer >> > back to ScriptExecutionContext - so it's clear that MessagePorts do not >> > outlive the SEC. Same pattern could be used for >> > ScriptExecutionContext/Database, for consistency. It also simplifies >> > design >> > a bit - no need for a special refcounted manager class, and things >> > like callOnJavaScriptThread could be replaced by SEC::postTask() which >> > is >> > already implemented. >> >> The point of CallOnJavascriptThread is that the JS thread is the Main >> Thread in the Document context, but [in Chromium's implementation] NOT >> in the Worker context. I needed a virtual method to distinguish >> between the two cases, since the Database code previously treated >> isMainThread as synonymous with isJavascriptThread. But of course >> that virtual method can be moved to Document and SEC instead of DDM >> and WDM. > > I understand the reason for callOnJavascriptThread... This is exactly as > SEC::postTask is defined - it posts a task from any thread to the one which > executes JavaScript code for a given SEC. So for Document it is the main > thread, for Workers - the thread associated with WorkerThread. I think they > are the same thing - you can just use postTask in place of > callOnJavascriptThread. Ah, I see--thanks, I had missed that function when implementing this. I've switched over to using it now. Eric ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Database in Worker context
On Thu, Dec 10, 2009 at 11:09 AM, Eric Uhrhane wrote: > On Wed, Dec 9, 2009 at 6:55 PM, Dmitry Titov wrote: > > Thanks for the link to the prototype code! > > If I understand it right and the DatabaseManager can not have any other > > relationship with ScriptExecutionContext but 1-1 and their lifetime is > the > > same then it's not clear to me what benefit could be had by effectively > > splitting a class in two. The fact that DatabaseManager is RefCounted > also > > hints that some other object could take ownership of it as well outside > of > > lifetime of ScriptExecutionContext, but is this true? > > That's there because the DatabaseThread [and therefore the Database] > can stick around for a bit after the ScriptExecutionContext is deleted > if I make it non-refcounted. If I move the DatabaseManager parts into > the ScriptExecutionContext, then I have to make sure that the SEC > sticks around until after the Database and DatabaseThread have gone > away. > It feels that there is a new class that does not have a specific lifetime or relationship to other existing classes, as it is a mechanical part of some other class rather... I look at how m_document is used by current Database and it feels that if SEC had a databaseThread() accessor, then all other methods like addOpenDatabase etc could be part of DatabaseThread instead, with Database keeping a pointer to its DatabaseThread. What do you think about this? > ScriptExecutionContext collects functionality common for Workers and > > Documents, and as such is a home for a few 'groups' of methods, like a > few > > methods to deal with MessagePorts. MessagePort, in turn, has a raw > pointer > > back to ScriptExecutionContext - so it's clear that MessagePorts do not > > outlive the SEC. Same pattern could be used for > > ScriptExecutionContext/Database, for consistency. It also simplifies > design > > a bit - no need for a special refcounted manager class, and things > > like callOnJavaScriptThread could be replaced by SEC::postTask() which is > > already implemented. > > The point of CallOnJavascriptThread is that the JS thread is the Main > Thread in the Document context, but [in Chromium's implementation] NOT > in the Worker context. I needed a virtual method to distinguish > between the two cases, since the Database code previously treated > isMainThread as synonymous with isJavascriptThread. But of course > that virtual method can be moved to Document and SEC instead of DDM > and WDM. I understand the reason for callOnJavascriptThread... This is exactly as SEC::postTask is defined - it posts a task from any thread to the one which executes JavaScript code for a given SEC. So for Document it is the main thread, for Workers - the thread associated with WorkerThread. I think they are the same thing - you can just use postTask in place of callOnJavascriptThread. Dmitry ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Should we ever change style guidelines?
On Thursday 10 December 2009 03:16:51 pm Peter Kasting wrote: > Yeah, I'm not sold. But oh well. I think I was getting too irked, when > really we're just both giving our opinions on how we want the style guide > to work. There's nothing wrong with having opinions, or disagreeing. > > Besides, I've been on enough bugs with you to know that your judgement is > solid. If it comes down to trusting that judgement, that's not too bad a > fate. Thanks Peter :) Cheers, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Should we ever change style guidelines?
On Thu, Dec 10, 2009 at 11:55 AM, Adam Treat wrote: > But I think I've been clear?! > It is always possible that my confusion is due to my own ignorance/misreading/stupidity. Maybe my brain is just slow today. I believe I've given good examples of why. You disagree? > Yeah, I'm not sold. But oh well. I think I was getting too irked, when really we're just both giving our opinions on how we want the style guide to work. There's nothing wrong with having opinions, or disagreeing. Besides, I've been on enough bugs with you to know that your judgement is solid. If it comes down to trusting that judgement, that's not too bad a fate. Cheers, PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Should we ever change style guidelines?
On Thursday 10 December 2009 02:35:38 pm Peter Kasting wrote: > On Thu, Dec 10, 2009 at 11:28 AM, Adam Treat wrote: > > both are confusing and in my mind call for exercising reviewer > > discretion over the style guide. > > > > And I *do not* think the > > style guide is wrong for the *guide* to prefer "!foo" over "foo == 0"! > > OK. Then I have no idea what your standards are. Uses of !foo are > confusing, but foo == 0 shouldn't be preferred, except when it should. > Whatever. But I think I've been clear?! I think the style guideline for preferring '!foo' vs 'foo == 0' is a good one for the vast majority of cases. And it also happens to be the current rule which is a bonus :) However, I don't think it is a good rule to follow dogmatically. And I believe I've given good examples of why. You disagree? If a person thinks the style guidelines should only consist of those rules where there are no possible exceptions, then I guess such a person would advocate against removing this guideline. I however don't feel that they should be interpreted so dogmatically. > In several dozen emails we have progressed no further. Stalemates like > this and feelings like "this is inexplicable" as above are precisely why I > don't want reviewer judgement guiding style on patches that I or anyone > else submits. I guess the reverse is true for me. Stalemates like this are a good example of why I do want good reviewer judgement to be used when evaluating the style as well as the rest of the code in a patch. Cheers, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Should we ever change style guidelines?
On Thu, Dec 10, 2009 at 11:28 AM, Adam Treat wrote: > both are confusing and in my mind call for exercising reviewer > discretion over the style guide. > > And I *do not* think the > style guide is wrong for the *guide* to prefer "!foo" over "foo == 0"! > OK. Then I have no idea what your standards are. Uses of !foo are confusing, but foo == 0 shouldn't be preferred, except when it should. Whatever. In several dozen emails we have progressed no further. Stalemates like this and feelings like "this is inexplicable" as above are precisely why I don't want reviewer judgement guiding style on patches that I or anyone else submits. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Database in Worker context
> not clear to me what benefit could be had by effectively splitting a class in two Perhaps more easily worked on by multiple hands in parallel with fewer conflicts. Various bits of "database stuff" are better encapsulated in database specific classes (so maybe easier to learn how the system works when code snorkling). Either way would work of course, the direction eric's heading seems like a clean way to factor things. On Thu, Dec 10, 2009 at 11:09 AM, Eric Uhrhane wrote: > On Wed, Dec 9, 2009 at 6:55 PM, Dmitry Titov wrote: > > Thanks for the link to the prototype code! > > If I understand it right and the DatabaseManager can not have any other > > relationship with ScriptExecutionContext but 1-1 and their lifetime is > the > > same then it's not clear to me what benefit could be had by effectively > > splitting a class in two. The fact that DatabaseManager is RefCounted > also > > hints that some other object could take ownership of it as well outside > of > > lifetime of ScriptExecutionContext, but is this true? > > That's there because the DatabaseThread [and therefore the Database] > can stick around for a bit after the ScriptExecutionContext is deleted > if I make it non-refcounted. If I move the DatabaseManager parts into > the ScriptExecutionContext, then I have to make sure that the SEC > sticks around until after the Database and DatabaseThread have gone > away. > > I can do that--the benefit I thought I was getting wasn't one of > object lifetime. I just thought the classes were cleaner this way. > But if you really think it's better to have it all in together, I can > certainly move it. > > > ScriptExecutionContext collects functionality common for Workers and > > Documents, and as such is a home for a few 'groups' of methods, like a > few > > methods to deal with MessagePorts. MessagePort, in turn, has a raw > pointer > > back to ScriptExecutionContext - so it's clear that MessagePorts do not > > outlive the SEC. Same pattern could be used for > > ScriptExecutionContext/Database, for consistency. It also simplifies > design > > a bit - no need for a special refcounted manager class, and things > > like callOnJavaScriptThread could be replaced by SEC::postTask() which is > > already implemented. > > The point of CallOnJavascriptThread is that the JS thread is the Main > Thread in the Document context, but [in Chromium's implementation] NOT > in the Worker context. I needed a virtual method to distinguish > between the two cases, since the Database code previously treated > isMainThread as synonymous with isJavascriptThread. But of course > that virtual method can be moved to Document and SEC instead of DDM > and WDM. > > > On Wed, Dec 9, 2009 at 5:21 PM, Eric Uhrhane wrote: > >> > >> On Wed, Dec 9, 2009 at 4:11 PM, Dmitry Titov > wrote: > >> > On Wed, Dec 9, 2009 at 12:58 PM, Eric Uhrhane > >> > wrote: > >> >> > >> >> I've pulled the database-related members out of Document and made a > >> >> new class for them, DatabaseManager. An instance of that is owned by > >> >> each ScriptExecutionContext. There are two flavors, > >> >> DocumentDatabaseManager and WorkerDatabaseManager. They're not very > >> >> different, but in a few cases it was necessary to distinguish between > >> >> them > >> > > >> > I don't see your code, just generic thought: If those classes are > small, > >> > then perhaps ScriptExecutionContext could absorb a couple of methods > to > >> > deal > >> > with that? Some of them could be virtual and implemented differently > on > >> > Document and WorkerContext. > >> > Dmitry > >> > >> You don't see the code because I sent this as an early > >> warning--there's nothing checked in yet. > >> The classes aren't huge, and in fact most of the code was in Document > >> before, but given that it's all to do with a separable concept [the > >> Database], I thought it nicer to pull it out. Let's > >> see...DatabaseManger.cpp is 172 lines [including copyright headers, > >> etc.] and the header is 107. Seems like a lot to sprinkle through > >> ScriptExecutionContext, Database, and WorkerContext. You can take a > >> look at my current code [where I'm backing it up--this won't actually > >> be sent for review from this repository] at > >> http://codereview.chromium.org/401016/show [mostly storage + v8 > >> bindings] and http://codereview.chromium.org/464018/show [mostly > >> Chrome browser process stuff] and see what you think. > >> > >> Thanks, > >> > >> Eric > > > > > > ___ > > 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 > ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Should we ever change style guidelines?
On Thursday 10 December 2009 01:34:12 pm Peter Kasting wrote: > But if "!color.green()" is potentially confusing, then certainly it is just > as confusing (in fact moreso) without the surrounding "color.blue()" and > "color.red()" tests in Adam's example. Yet Adam cited "consistency with > surroundings" as the reason to prefer == 0 in this case, which suggests > that he'd be fine with an isolated test of this value. It doesn't suggest anything of the sort. The two cases are not mutually incompatible; both are confusing and in my mind call for exercising reviewer discretion over the style guide. > My point is that your argument (and Adam's) is not actually an argument for > reviewer discretion, or promoting consistency over whatever the style guide > says; it's an argument that the style guide is wrong to prefer "!foo" over > "foo == 0". No, *it is* an argument for reviewer discretion. And I *do not* think the style guide is wrong for the *guide* to prefer "!foo" over "foo == 0"! I just think there are exceptions. And that in the end we have to trust in the compiler and the reviewer's judgement. Cheers, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Database in Worker context
On Wed, Dec 9, 2009 at 6:55 PM, Dmitry Titov wrote: > Thanks for the link to the prototype code! > If I understand it right and the DatabaseManager can not have any other > relationship with ScriptExecutionContext but 1-1 and their lifetime is the > same then it's not clear to me what benefit could be had by effectively > splitting a class in two. The fact that DatabaseManager is RefCounted also > hints that some other object could take ownership of it as well outside of > lifetime of ScriptExecutionContext, but is this true? That's there because the DatabaseThread [and therefore the Database] can stick around for a bit after the ScriptExecutionContext is deleted if I make it non-refcounted. If I move the DatabaseManager parts into the ScriptExecutionContext, then I have to make sure that the SEC sticks around until after the Database and DatabaseThread have gone away. I can do that--the benefit I thought I was getting wasn't one of object lifetime. I just thought the classes were cleaner this way. But if you really think it's better to have it all in together, I can certainly move it. > ScriptExecutionContext collects functionality common for Workers and > Documents, and as such is a home for a few 'groups' of methods, like a few > methods to deal with MessagePorts. MessagePort, in turn, has a raw pointer > back to ScriptExecutionContext - so it's clear that MessagePorts do not > outlive the SEC. Same pattern could be used for > ScriptExecutionContext/Database, for consistency. It also simplifies design > a bit - no need for a special refcounted manager class, and things > like callOnJavaScriptThread could be replaced by SEC::postTask() which is > already implemented. The point of CallOnJavascriptThread is that the JS thread is the Main Thread in the Document context, but [in Chromium's implementation] NOT in the Worker context. I needed a virtual method to distinguish between the two cases, since the Database code previously treated isMainThread as synonymous with isJavascriptThread. But of course that virtual method can be moved to Document and SEC instead of DDM and WDM. > On Wed, Dec 9, 2009 at 5:21 PM, Eric Uhrhane wrote: >> >> On Wed, Dec 9, 2009 at 4:11 PM, Dmitry Titov wrote: >> > On Wed, Dec 9, 2009 at 12:58 PM, Eric Uhrhane >> > wrote: >> >> >> >> I've pulled the database-related members out of Document and made a >> >> new class for them, DatabaseManager. An instance of that is owned by >> >> each ScriptExecutionContext. There are two flavors, >> >> DocumentDatabaseManager and WorkerDatabaseManager. They're not very >> >> different, but in a few cases it was necessary to distinguish between >> >> them >> > >> > I don't see your code, just generic thought: If those classes are small, >> > then perhaps ScriptExecutionContext could absorb a couple of methods to >> > deal >> > with that? Some of them could be virtual and implemented differently on >> > Document and WorkerContext. >> > Dmitry >> >> You don't see the code because I sent this as an early >> warning--there's nothing checked in yet. >> The classes aren't huge, and in fact most of the code was in Document >> before, but given that it's all to do with a separable concept [the >> Database], I thought it nicer to pull it out. Let's >> see...DatabaseManger.cpp is 172 lines [including copyright headers, >> etc.] and the header is 107. Seems like a lot to sprinkle through >> ScriptExecutionContext, Database, and WorkerContext. You can take a >> look at my current code [where I'm backing it up--this won't actually >> be sent for review from this repository] at >> http://codereview.chromium.org/401016/show [mostly storage + v8 >> bindings] and http://codereview.chromium.org/464018/show [mostly >> Chrome browser process stuff] and see what you think. >> >> Thanks, >> >> Eric > > > ___ > 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] Should we ever change style guidelines?
On Thu, Dec 10, 2009 at 7:57 AM, Joe Mason wrote: > > Off the top of my head as a reviewer I'd accept: > > > > if (color.red() == 255 && color.green() == 0 && color.blue() == > > 255) // pink! > > > > over > > > > if (color.red() == 255 && !color.green() && color.blue() == 255) > > // pink! > > > > most days of the week... Consistency and all that. > > The actual potential bug in this comes when "if (!color.green())" comes > on its own - it looks to a casual glance like green() returns a bool > saying whether this color is green or not. But if "!color.green()" is potentially confusing, then certainly it is just as confusing (in fact moreso) without the surrounding "color.blue()" and "color.red()" tests in Adam's example. Yet Adam cited "consistency with surroundings" as the reason to prefer == 0 in this case, which suggests that he'd be fine with an isolated test of this value. My point is that your argument (and Adam's) is not actually an argument for reviewer discretion, or promoting consistency over whatever the style guide says; it's an argument that the style guide is wrong to prefer "!foo" over "foo == 0". PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Should we ever change style guidelines?
Let's try this again now that I've resubscribed with the correct email address: > On Wednesday 09 December 2009 07:52:22 pm Peter Kasting wrote: > > You haven't really said why. The closest you got was the vague "It > is also > > true that the current style guidelines if > > practiced pedantically in every case can lead to potential bugs." > Bugs > > like what? Perhaps if there are some, we should change the > appropriate > > guide, instead of leaving the choice up to a reviewer, who, if the > rule > > really _can_ be problematic, might erroneously enforce it. > > Off the top of my head as a reviewer I'd accept: > > if (color.red() == 255 && color.green() == 0 && color.blue() == > 255) // pink! > > over > > if (color.red() == 255 && !color.green() && color.blue() == 255) > // pink! > > most days of the week... Consistency and all that. I was trying to come up with an example for exactly that situation last night. The actual potential bug in this comes when "if (!color.green())" comes on its own - it looks to a casual glance like green() returns a bool saying whether this color is green or not. Obviously nobody would make that mistake, but in a more obscure API (possibly to a 3rd-party component, so we can't sanitize it) the distinction could trip people up. This is a corner case and one that's unlikely to come up, which is why it's not worth putting into the coding standard, but it's exactly the sort of thing where a submitter should be able to say, "I've broken the style guide deliberately here because I think this expression is much clearer with an explicit test against 0". Joe - This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Should we ever change style guidelines?
On Wednesday 09 December 2009 07:52:22 pm Peter Kasting wrote: > You haven't really said why. The closest you got was the vague "It is also > true that the current style guidelines if > practiced pedantically in every case can lead to potential bugs." Bugs > like what? Perhaps if there are some, we should change the appropriate > guide, instead of leaving the choice up to a reviewer, who, if the rule > really _can_ be problematic, might erroneously enforce it. Off the top of my head as a reviewer I'd accept: if (color.red() == 255 && color.green() == 0 && color.blue() == 255) // pink! over if (color.red() == 255 && !color.green() && color.blue() == 255) // pink! most days of the week... Consistency and all that. Cheers, Adam PS: Maybe not on a monday. I really hate mondays :) ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
