Re: [webkit-dev] 194 bugs in pending-commit
On Fri, Jun 17, 2011 at 10:56 PM, Adam Barth aba...@webkit.org wrote: 2) Mark the patch as obsolete / clear the review flag if we're not going to land the patch. Does the slash mean do both? I have https://bugs.webkit.org/show_bug.cgi?id=47036 on that list and the only r+ed patch on it is already marked obsolete. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] 194 bugs in pending-commit
On Fri, Jun 17, 2011 at 11:13 PM, Peter Kasting pkast...@chromium.org wrote: On Fri, Jun 17, 2011 at 10:56 PM, Adam Barth aba...@webkit.org wrote: 2) Mark the patch as obsolete / clear the review flag if we're not going to land the patch. Does the slash mean do both? I have https://bugs.webkit.org/show_bug.cgi?id=47036 on that list and the only r+ed patch on it is already marked obsolete. Yeah, Bugzilla kind of sucks. That page isn't smart enough to hide the obsolete patches. If you have EditBugs, you can run webkit-patch clean-pending-commit, which will automatically remove the review flags from obsolete patches. Eric and I have been meaning to having one of the bots do that periodically, but we haven't set that up yet. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Adding ENABLE_BATTERY_STATUS to WebCore
The batterystatus can be unimportant for pages or apps. Or, detail info is needed more than this. But, before the spec is changed, I think there is no problem to support this. It is better to support to do not. Can you be sure that this will be not used? I think It is good to support to someone try to use this for their apps or pages after spec is published, if it is lacked or not enough now. 2011/6/18 Eric Seidel e...@webkit.org Given how many desktop applications do this, I think we're well off into the land of wishes and fairy tales. :) But it's also possible that libraries like jquery or Google's closure could do this... but again, I'm skeptical. Then again, if we don't expose information like this, they don't ever have a chance to prove me wrong. -eric On Fri, Jun 17, 2011 at 10:54 AM, Darin Fisher da...@chromium.org wrote: I think there are web app developers that would do things differently if they knew their user was running on battery power. An app might scale back its CPU usage, or run a timer at a lower frequency. Crazy idea: Maybe an advertising network could be nice and not show animated ads to such users? ;-) -Darin On Fri, Jun 17, 2011 at 10:47 AM, Eric Seidel e...@webkit.org wrote: My 2¢: I'm confused by who the client of this API would be. It seems that web sites don't really need to know my battery state. But web applications that are on mobile phone (like WebOS, or Apple's original vision for iPhone apps) would want battery state information, but would want *more* information than this spec provides (imagine trying to write any power-management application like the zillion examples available for Apple's Dashboard, or iPhone). I'm also not sure that I want all web sites knowing this information. I wonder what fingerprinting vectors this API would expose (if any?). Certainly websites could know if their visitors are laptop users or not (but I suspect they can already figure that out from screen size and UA strings). It's also possible that I'm just spreading FUD here, and that smarter people than I have already hashed this all out on the spec list... -eric On Fri, Jun 17, 2011 at 10:40 AM, Darin Fisher da...@chromium.org wrote: On Fri, Jun 17, 2011 at 10:27 AM, Andrei Popescu andr...@google.com wrote: On Fri, Jun 17, 2011 at 4:21 PM, Darin Fisher da...@chromium.org wrote: On Fri, Jun 17, 2011 at 4:11 AM, Anssi Kostiainen anssi.kostiai...@nokia.com wrote: Hi, On 16.6.2011, at 19.02, ext Darin Fisher wrote: On Thu, Jun 16, 2011 at 5:12 AM, Anssi Kostiainen anssi.kostiai...@nokia.com wrote: On 15.6.2011, at 21.29, ext Darin Fisher wrote: There should probably be a way to poll the current state. Much as you can poll the document.readyState and respond to progress events, it would seem to make sense to have a way to poll the battery state as well as respond to battery state change events. The current design is quite similar to that of the Device Orientation Event. We discussed the polling option but settled on the current design. Let me know if you'd like me to dig into archives for some pointers on that design decision. I'd be curious to read them. Off-hand it seems like device orientation suffers from the same problem. You wouldn't want the application to do too much work that would be discarded once it finds out that the orientation is X instead of Y. I think the design guidelines introduced in the following Mozilla position paper are still valid. In this context, specifically: [[ Device APIs should be asynchronous; in particular, user agents should not have to block on return values of Device API function calls, and Device APIs should be driven by callbacks. http://www.w3.org/2008/security-ws/papers/mozilla.html#asynchronous ]] The proposal wasn't to make blocking APIs that query any devices. Instead, you would be able to get synchronous access to the last known value for a device property (last known battery state, last known device orientation, etc.). In particular, you would get access to the last known value prior to your page being loaded. Synchronous access to this value seems helpful for the reasons stated in this thread. But, let me expand on that for a moment. Suppose an application wanted to know both the battery state and the device orientation before generating its content. It would need to asynchronously query both states before proceeding. That seems quite awkward, especially when the information could be provided synchronously. In the case of device orientation, having such a
Re: [webkit-dev] Reopening bugs is why many of the bugs are pending-commit
There seems to be some confusion about what to do when you discover that a code change caused a bug. Please do *not* reopen the original bug report to say that the code change caused a problem. This makes the bug database state that the original bug is not fixed, and also makes the patch to fix the bug look like it’s a reviewed-but-not-landed patch. That’s not what we want! Instead of reopening the bug, please file a new bug report about the problem you are now observing. You can add a comment in the original bug pointing out the patch was the cause of the new problem. On the other hand, if you roll a patch out, then it’s important to reopen the bug that patch fixed. Please clear the review flag on the patch that you rolled out and any older reviewed versions of the same patch attached to the bug. And if you discover that the patch turned out to not fix the original reported bug, then it’s fine to reopen the bug. Please clear the review flag on the patch that failed to fix the bug and any other older patches already landed or obsolete attached to the bug. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Are roll-outs getting a little out of hand?
I noticed these three roll-outs: http://trac.webkit.org/changeset/89190 http://trac.webkit.org/changeset/89191 http://trac.webkit.org/changeset/89192 Were all of these necessary? Was there a way to fix the problem instead of rolling out in any of these cases? -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Are roll-outs getting a little out of hand?
On Jun 18, 2011, at 12:24 PM, Darin Adler wrote: I noticed these three roll-outs: http://trac.webkit.org/changeset/89190 http://trac.webkit.org/changeset/89191 In the case of these first two patches was any attempt made to actually fix the qt build? Every other build was apparently happy with the change so it seems like a Qt engineer (like Ossy_) could have attempted to fix the Qt build themselves, rather than just rolling out the patch and requiring someone else without familiarity with Qt to fix the Qt build through the slow and painful EWS process. --Oliver ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] 105 bugs in pending-commit
On Jun 17, 2011, at 10:56 PM, Adam Barth wrote: There are a 194 open bugs with an R+ patches attached to them: I worked on this and some others must have been working on it as well. The number is down to 105 now, with quite a few in the commit queue. I ran out of steam. I hope more others can join in. -- Dari ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] 105 bugs in pending-commit
I'm running clean-pending-commit and clean-review-queue now. Adam and I will make sure to set up a bot to run both next week. -eric On Sat, Jun 18, 2011 at 1:03 PM, Darin Adler da...@apple.com wrote: On Jun 17, 2011, at 10:56 PM, Adam Barth wrote: There are a 194 open bugs with an R+ patches attached to them: I worked on this and some others must have been working on it as well. The number is down to 105 now, with quite a few in the commit queue. I ran out of steam. I hope more others can join in. -- Dari ___ 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] Are roll-outs getting a little out of hand?
On Sat, Jun 18, 2011 at 1:15 PM, Darin Adler da...@apple.com wrote: On Jun 18, 2011, at 12:58 PM, Oliver Hunt wrote: On Jun 18, 2011, at 12:24 PM, Darin Adler wrote: http://trac.webkit.org/changeset/89190 Looks like Ossy did look at this one; it was not a Qt issue at all, broken on all non-V8 platforms. Now that the commit-bot compiles only with V8, this kind of thing can get through. Our current thinking on this is that fast EWS bots will prevent this kind of trouble. We don't have any particularly fast JSC-building EWS bots however. Probably not too hard to fix, but probably also reasonable to roll out. http://trac.webkit.org/changeset/89191 This was a Qt-specific change, so clearly OK to roll that out if a Qt expert can’t quickly fix it. -- Darin ___ 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] 105 bugs in pending-commit
I should note that webkit-patch has some (hidden, see webkit-patch help -a) commands for dealing with the review and commit queue overflow: clean-pending-commit -- obsoletes patches with r+ on closed bugs, clears r+ on obsolete patches clean-review-queue -- obsoletes r? patches on closed bugs assign-to-committer -- assigns bugs with an r+'d patch to patch author (if they're a committer) (I just ran all 3 of those just now, they hadn't been run in probably a month.) webkit-patch also has a couple other review/commit-related query commands: patches-to-commit-queue -- lists patches which should be cq+'d (have cq? or were posted by non-committers) patches-to-review -- lists patches in pending-review These commands are all hidden as part of our effort to keep webkit-patch simple. The first 3 should probably be run on a bot somewhere (something Adam and I will look at next week). -eric On Sat, Jun 18, 2011 at 1:12 PM, Eric Seidel e...@webkit.org wrote: I'm running clean-pending-commit and clean-review-queue now. Adam and I will make sure to set up a bot to run both next week. -eric On Sat, Jun 18, 2011 at 1:03 PM, Darin Adler da...@apple.com wrote: On Jun 17, 2011, at 10:56 PM, Adam Barth wrote: There are a 194 open bugs with an R+ patches attached to them: I worked on this and some others must have been working on it as well. The number is down to 105 now, with quite a few in the commit queue. I ran out of steam. I hope more others can join in. -- Dari ___ 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] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
1: Recently, Alexey has encouraged me to use PassRefPtr less for function arguments. The PassRefPtr optimization pays off when the object in question is possibly being handed off from one PassRefPtr to another. For an argument, that can happen in two ways: 1) The argument can be the result of a function that returns a PassRefPtr. 2) The argument can be the result of calling release a local or data member that is a RefPtr. In both of those cases, we are transferring ownership. Mechanically speaking, PassRefPtr only pays off if we are actually getting that optimization. If we are passing a raw pointer, then using PassRefPtr for the function argument type doesn’t help much. It just puts a ref at the call site, a ref that otherwise would happen inside the function. It may even cause a bit of code bloat if there is more than one call site. Conceptually speaking, PassRefPtr only pays off if the context is a clear transfer of ownership. Passing an object that the recipient *might* later choose to take shared ownership of is not enough. Clients are always welcome to take shared ownership of something passed with a raw pointer. Because there are also costs to PassRefPtr, we should reserve PassRefPtr arguments for cases where the optimization will really pay off and for where the function definitely is taking ownership. Those functions do exist, but many current uses of PassRefPtr for arguments do not qualify. 2: Recently, I’ve noticed that many bugs simply would cease to exist if we used RefPtr more and raw pointer less for things like local variables and data members. The time it’s safe to use a raw pointer for a local variable or data member for a reference counted object pointer is when there is some guarantee that someone else is holding a reference. It can be difficult to have such a guarantee and those guarantees are fragile as code executes. Nowhere is that more clear than in loader-related code. Some are loathe to use RefPtr for data members because they are concerned about reference cycles. Generally speaking, we can eliminate the worry about reference cycles by making destruction include a “closing” process, which can null out all the references and break cycles. If we find it’s necessary we can also look into an efficient WeakPtr implementation. Some have been enthusiastic about this in the past. I have been a bit less so because I don’t know of an efficient implementation strategy. Conclusion: A specific example where an argument has type PassRefPtr, but that does not seem like the correct design, is the the node argument in the constructors of the various HTMLCollection classes. Examples of where we are using raw pointers, but should use RefPtr instead abound. Sadly there is no way to qualify a member function to make “this” a RefPtr, so we are stuck with the “protector” idiom for the this pointer. Maybe we can even find a way to discuss these two issues in the RefPtr document without making it too confusing. Comments are welcome. But lets not bikeshed http://en.wikipedia.org/wiki/Parkinson's_Law_of_Triviality this! -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
I've fixed many a security bug cause by not refing local variables. Generally, you need to ref your local variables if any function you call can end up executing JavaScript (and many can). If you're not sure, please feel encouraged to ask around. Adam On Jun 18, 2011 8:54 PM, Darin Adler da...@apple.com wrote: 1: Recently, Alexey has encouraged me to use PassRefPtr less for function arguments. The PassRefPtr optimization pays off when the object in question is possibly being handed off from one PassRefPtr to another. For an argument, that can happen in two ways: 1) The argument can be the result of a function that returns a PassRefPtr. 2) The argument can be the result of calling release a local or data member that is a RefPtr. In both of those cases, we are transferring ownership. Mechanically speaking, PassRefPtr only pays off if we are actually getting that optimization. If we are passing a raw pointer, then using PassRefPtr for the function argument type doesn’t help much. It just puts a ref at the call site, a ref that otherwise would happen inside the function. It may even cause a bit of code bloat if there is more than one call site. Conceptually speaking, PassRefPtr only pays off if the context is a clear transfer of ownership. Passing an object that the recipient *might* later choose to take shared ownership of is not enough. Clients are always welcome to take shared ownership of something passed with a raw pointer. Because there are also costs to PassRefPtr, we should reserve PassRefPtr arguments for cases where the optimization will really pay off and for where the function definitely is taking ownership. Those functions do exist, but many current uses of PassRefPtr for arguments do not qualify. 2: Recently, I’ve noticed that many bugs simply would cease to exist if we used RefPtr more and raw pointer less for things like local variables and data members. The time it’s safe to use a raw pointer for a local variable or data member for a reference counted object pointer is when there is some guarantee that someone else is holding a reference. It can be difficult to have such a guarantee and those guarantees are fragile as code executes. Nowhere is that more clear than in loader-related code. Some are loathe to use RefPtr for data members because they are concerned about reference cycles. Generally speaking, we can eliminate the worry about reference cycles by making destruction include a “closing” process, which can null out all the references and break cycles. If we find it’s necessary we can also look into an efficient WeakPtr implementation. Some have been enthusiastic about this in the past. I have been a bit less so because I don’t know of an efficient implementation strategy. Conclusion: A specific example where an argument has type PassRefPtr, but that does not seem like the correct design, is the the node argument in the constructors of the various HTMLCollection classes. Examples of where we are using raw pointers, but should use RefPtr instead abound. Sadly there is no way to qualify a member function to make “this” a RefPtr, so we are stuck with the “protector” idiom for the this pointer. Maybe we can even find a way to discuss these two issues in the RefPtr document without making it too confusing. Comments are welcome. But lets not bikeshed http://en.wikipedia.org/wiki/Parkinson's_Law_of_Triviality this! -- Darin ___ 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] 194 bugs in pending-commit
IIRC, Mozilla's bugzilla can hide obsolete patches (?). If so, why can not webkit's bugzilla? I actually do not like the way the review flags are cleared today only in order to make the tools and pending-xxx pages happier. IMO the review flags give much about the history of the bug. In that matter, I dislike webkit-patch's ways of clearing r- flags of patches while it marks it as obsolete and uploads a new one. Reason: an easy-to-see r-'ed patch is very helpful to me to understand the chronological progresses in the bug. What is the reason for clearing r- flag while uploading a new one, instead of only making it obsolete? On Sat, Jun 18, 2011 at 2:23 AM, Adam Barth aba...@webkit.org wrote: On Fri, Jun 17, 2011 at 11:13 PM, Peter Kasting pkast...@chromium.org wrote: On Fri, Jun 17, 2011 at 10:56 PM, Adam Barth aba...@webkit.org wrote: 2) Mark the patch as obsolete / clear the review flag if we're not going to land the patch. Does the slash mean do both? I have https://bugs.webkit.org/show_bug.cgi?id=47036 on that list and the only r+ed patch on it is already marked obsolete. Yeah, Bugzilla kind of sucks. That page isn't smart enough to hide the obsolete patches. If you have EditBugs, you can run webkit-patch clean-pending-commit, which will automatically remove the review flags from obsolete patches. Eric and I have been meaning to having one of the bots do that periodically, but we haven't set that up yet. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev -- --Antonio Gomes ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] 194 bugs in pending-commit
webkit-patch upload clears all flags when obsoleting a patch. We could make it not clear r- if you like. I know of no way to construct a query like http://webkit.org/pending-commit in our current bugzilla without clearing r+ on obsolete patches/closed bugs. We clear flags (specifically r+) to make the life of those going through http://webkit.org/pending-commit easier (like sever folks tried to do this afternoon). Similarly, our current bugzilla doesn't automatically clear r? on closed bugs. So we have webkit-patch clean-review-queue to do that. The history information for any bug is always easily accessed via the history link in the upper-right corner of a bug, like: https://bugs.webkit.org/show_activity.cgi?id=62114 -eric On Sat, Jun 18, 2011 at 9:01 PM, Antonio Gomes toniki...@gmail.com wrote: IIRC, Mozilla's bugzilla can hide obsolete patches (?). If so, why can not webkit's bugzilla? I actually do not like the way the review flags are cleared today only in order to make the tools and pending-xxx pages happier. IMO the review flags give much about the history of the bug. In that matter, I dislike webkit-patch's ways of clearing r- flags of patches while it marks it as obsolete and uploads a new one. Reason: an easy-to-see r-'ed patch is very helpful to me to understand the chronological progresses in the bug. What is the reason for clearing r- flag while uploading a new one, instead of only making it obsolete? On Sat, Jun 18, 2011 at 2:23 AM, Adam Barth aba...@webkit.org wrote: On Fri, Jun 17, 2011 at 11:13 PM, Peter Kasting pkast...@chromium.org wrote: On Fri, Jun 17, 2011 at 10:56 PM, Adam Barth aba...@webkit.org wrote: 2) Mark the patch as obsolete / clear the review flag if we're not going to land the patch. Does the slash mean do both? I have https://bugs.webkit.org/show_bug.cgi?id=47036 on that list and the only r+ed patch on it is already marked obsolete. Yeah, Bugzilla kind of sucks. That page isn't smart enough to hide the obsolete patches. If you have EditBugs, you can run webkit-patch clean-pending-commit, which will automatically remove the review flags from obsolete patches. Eric and I have been meaning to having one of the bots do that periodically, but we haven't set that up yet. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev -- --Antonio Gomes ___ 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] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
On Jun 18, 2011, at 5:52 PM, Darin Adler wrote: 1: Recently, Alexey has encouraged me to use PassRefPtr less for function arguments. The PassRefPtr optimization pays off when the object in question is possibly being handed off from one PassRefPtr to another. For an argument, that can happen in two ways: 1) The argument can be the result of a function that returns a PassRefPtr. 2) The argument can be the result of calling release a local or data member that is a RefPtr. In both of those cases, we are transferring ownership. Mechanically speaking, PassRefPtr only pays off if we are actually getting that optimization. If we are passing a raw pointer, then using PassRefPtr for the function argument type doesn’t help much. It just puts a ref at the call site, a ref that otherwise would happen inside the function. It may even cause a bit of code bloat if there is more than one call site. Conceptually speaking, PassRefPtr only pays off if the context is a clear transfer of ownership. Passing an object that the recipient *might* later choose to take shared ownership of is not enough. Clients are always welcome to take shared ownership of something passed with a raw pointer. Because there are also costs to PassRefPtr, we should reserve PassRefPtr arguments for cases where the optimization will really pay off and for where the function definitely is taking ownership. Those functions do exist, but many current uses of PassRefPtr for arguments do not qualify. A few thoughts on this: - The benefit of PassRefPtr at any individual call site is probably too small to be measurable, but I believe taken together they all add up to somewhere in the ballpark of .2%-.5% on some benchmarks by avoiding refcount churn. - I think having a rule for using PassRefPtr for arguments that depends on how callers use the function is likely to be hard to use in practice, since it requires global knowledge of all callers to pick the right function signature. The rule I prefer is that if a function takes ownership of the argument, whether or not we currently have a caller that gives away ownership, the function should take PassRefPtr. If it does not take ownership, it should take a raw pointer. That way you can decide the right thing to do based solely on the definition of the function itself. - Longer term we can replace PassRefPtr with use of C++0x rvalue references. I wonder if it's possible to do this in a way where we use rvalue references on compilers that support them and classic PassRefPtr elsewhere. 2: Recently, I’ve noticed that many bugs simply would cease to exist if we used RefPtr more and raw pointer less for things like local variables and data members. The time it’s safe to use a raw pointer for a local variable or data member for a reference counted object pointer is when there is some guarantee that someone else is holding a reference. It can be difficult to have such a guarantee and those guarantees are fragile as code executes. Nowhere is that more clear than in loader-related code. Some are loathe to use RefPtr for data members because they are concerned about reference cycles. Generally speaking, we can eliminate the worry about reference cycles by making destruction include a “closing” process, which can null out all the references and break cycles. If we find it’s necessary we can also look into an efficient WeakPtr implementation. Some have been enthusiastic about this in the past. I have been a bit less so because I don’t know of an efficient implementation strategy. We could probably use RefPtr for at least some data members in the DOM, but it would be tricky to avoid introducing needless refcount churn. I guess this gets back to the point about PassRefPtr. Using RefPtr for locals is probably a good idea in some cases, but it can also lead to refcount churn, and refcount churn tends to make a program slower by death of a thousand cuts, so it's hard to find the source of the problem after the fact. I've wondered at times whether it might be a good idea to use a RefPtrT (note the reference) for cases where it's desirable to be guaranteed that someone holds a ref but you don't want to add an extra ref yourself. Conclusion: A specific example where an argument has type PassRefPtr, but that does not seem like the correct design, is the the node argument in the constructors of the various HTMLCollection classes. That's probably so, but I'd also guess the cost is smaller than maintaining clear understanding of why PassRefPtr should not be used here but should in other similar cases. i would still prefer to adhere to the simpler rule even though PassRefPtr doesn't add anything in this specific case. PassRefPtr also makes it harder to accidentally put an incoming pointer into a raw pointer data member or local variable, so it has a potential typechecking benefit even when it isn't helping your
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
18.06.2011, в 22:15, Maciej Stachowiak написал(а): - I think having a rule for using PassRefPtr for arguments that depends on how callers use the function is likely to be hard to use in practice, since it requires global knowledge of all callers to pick the right function signature. The rule I prefer is that if a function takes ownership of the argument, whether or not we currently have a caller that gives away ownership, the function should take PassRefPtr. If it does not take ownership, it should take a raw pointer. That way you can decide the right thing to do based solely on the definition of the function itself. Using PassRefPtr for arguments is a fairly common source of bugs. The way it's zeroed out after being used has caused crashes in code paths that weren't tested before commit for some reason (see e.g. https://bugs.webkit.org/show_bug.cgi?id=52981). Another example that we've just seen (https://bugs.webkit.org/show_bug.cgi?id=62836) was when a different compiler had a different order of evaluation, so building with it suddenly exposed a crash that we didn't need to have. Looking at either bug, using PassRefPtr is pointless. For trace(), we could theoretically pass ownership from jsConsolePrototypeFunctionTrace(), but we do not, and micro-optimizing Console functions for performance isn't worth the cost of having had this crasher bug in my opinion. For bug 62836, it's quite clearly impossible to pass ownership to HTMLTableRowsCollection. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
