27001 2009-07-06 14:13:41 -0700 Fix improper use of PassRefPtr<Node> to RefPtr<Node> 2009-07-06 17:59:23 -0700 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 ddkilzer webkit-unassigned oldest_to_newest 130009 0 32318 ddkilzer 2009-07-06 14:13:41 -0700 Created attachment 32318 Patch v1 Fix improper use of PassRefPtr<Node> to RefPtr<Node> Reviewed by NOBODY (OOPS!). PassRefPtr<> should only be used for arguments to functions that take ownership of the object, or as return values from functions that relinquish ownership of the object. * editing/Editor.cpp: (WebCore::Editor::increaseSelectionListLevelOrdered): Changed stack-allocated PassRefPtr<Node> to RefPtr<Node> and call release() on returned object to prevent ref count churn. (WebCore::Editor::increaseSelectionListLevelUnordered): Ditto. 130012 1 32318 ggaren 2009-07-06 14:16:11 -0700 Comment on attachment 32318 Patch v1 > (WebCore::Editor::increaseSelectionListLevelOrdered): Changed > stack-allocated PassRefPtr<Node> to RefPtr<Node> and call > release() on returned object to prevent ref count churn. Technically, I don't think there would be refcount churn, since the default behavior of PassRefPtr is to give up its reference upon assignment: template <typename U> PassRefPtr(const PassRefPtr<U>& o) : m_ptr(o.releaseRef()) { } However, this is still a good change for the style reasons you cited. 130063 2 ddkilzer 2009-07-06 17:55:45 -0700 (In reply to comment #1) > (From update of attachment 32318 [details]) > > (WebCore::Editor::increaseSelectionListLevelOrdered): Changed > > stack-allocated PassRefPtr<Node> to RefPtr<Node> and call > > release() on returned object to prevent ref count churn. > > Technically, I don't think there would be refcount churn, since the default > behavior of PassRefPtr is to give up its reference upon assignment: > > template <typename U> PassRefPtr(const PassRefPtr<U>& o) : > m_ptr(o.releaseRef()) { } I'll adjust the changelog entry. (If you didn't call release(), though, and returned the RefPtr<Node> object, I do believe some ref count churn would be involved creating a PassRefPtr<Node> from a RefPtr<Node>. That's what I was referring to when using release().) 130065 3 ddkilzer 2009-07-06 17:59:23 -0700 $ git svn dcommit Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/editing/Editor.cpp Committed r45577 http://trac.webkit.org/changeset/45577 32318 2009-07-06 14:13:41 -0700 2009-07-06 14:16:11 -0700 Patch v1 0002--Fix-improper-use-of-PassRefPtr-Node-to-RefP.patch text/plain 3174 ddkilzer RnJvbSAyNGE3MzVlNDk3NjY4OTU3ZmRmYjFmM2EyNGYxOGE0YTQyMmQyMzJmIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBEYXZpZCBLaWx6ZXIgPGRka2lsemVyQGFwcGxlLmNvbT4KRGF0 ZTogTW9uLCA2IEp1bCAyMDA5IDA2OjQwOjM0IC0wNzAwClN1YmplY3Q6IFtQQVRDSCAyLzhdICAg ICAgICAgRml4IGltcHJvcGVyIHVzZSBvZiBQYXNzUmVmUHRyPE5vZGU+IHRvIFJlZlB0cjxOb2Rl PgoKICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KCiAgICAgICAgUGFzc1JlZlB0 cjw+IHNob3VsZCBvbmx5IGJlIHVzZWQgZm9yIGFyZ3VtZW50cyB0byBmdW5jdGlvbnMgdGhhdAog ICAgICAgIHRha2Ugb3duZXJzaGlwIG9mIHRoZSBvYmplY3QsIG9yIGFzIHJldHVybiB2YWx1ZXMg ZnJvbSBmdW5jdGlvbnMKICAgICAgICB0aGF0IHJlbGlucXVpc2ggb3duZXJzaGlwIG9mIHRoZSBv YmplY3QuCgogICAgICAgICogZWRpdGluZy9FZGl0b3IuY3BwOgogICAgICAgIChXZWJDb3JlOjpF ZGl0b3I6OmluY3JlYXNlU2VsZWN0aW9uTGlzdExldmVsT3JkZXJlZCk6IENoYW5nZWQKICAgICAg ICBzdGFjay1hbGxvY2F0ZWQgUGFzc1JlZlB0cjxOb2RlPiB0byBSZWZQdHI8Tm9kZT4gYW5kIGNh bGwKICAgICAgICByZWxlYXNlKCkgb24gcmV0dXJuZWQgb2JqZWN0IHRvIHByZXZlbnQgcmVmIGNv dW50IGNodXJuLgogICAgICAgIChXZWJDb3JlOjpFZGl0b3I6OmluY3JlYXNlU2VsZWN0aW9uTGlz dExldmVsVW5vcmRlcmVkKTogRGl0dG8uCi0tLQogV2ViQ29yZS9DaGFuZ2VMb2cgICAgICAgICAg fCAgIDE2ICsrKysrKysrKysrKysrKysKIFdlYkNvcmUvZWRpdGluZy9FZGl0b3IuY3BwIHwgICAg OCArKysrLS0tLQogMiBmaWxlcyBjaGFuZ2VkLCAyMCBpbnNlcnRpb25zKCspLCA0IGRlbGV0aW9u cygtKQoKZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cK aW5kZXggNTNkODkyNy4uNzQ5MTU1OSAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysr IGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsNSArMSwyMSBAQAogMjAwOS0wNy0wNiAgRGF2aWQg S2lsemVyICA8ZGRraWx6ZXJAYXBwbGUuY29tPgogCisgICAgICAgIEZpeCBpbXByb3BlciB1c2Ug b2YgUGFzc1JlZlB0cjxOb2RlPiB0byBSZWZQdHI8Tm9kZT4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBQYXNzUmVmUHRyPD4gc2hvdWxkIG9ubHkgYmUg dXNlZCBmb3IgYXJndW1lbnRzIHRvIGZ1bmN0aW9ucyB0aGF0CisgICAgICAgIHRha2Ugb3duZXJz aGlwIG9mIHRoZSBvYmplY3QsIG9yIGFzIHJldHVybiB2YWx1ZXMgZnJvbSBmdW5jdGlvbnMKKyAg ICAgICAgdGhhdCByZWxpbnF1aXNoIG93bmVyc2hpcCBvZiB0aGUgb2JqZWN0LgorCisgICAgICAg ICogZWRpdGluZy9FZGl0b3IuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RWRpdG9yOjppbmNyZWFz ZVNlbGVjdGlvbkxpc3RMZXZlbE9yZGVyZWQpOiBDaGFuZ2VkCisgICAgICAgIHN0YWNrLWFsbG9j YXRlZCBQYXNzUmVmUHRyPE5vZGU+IHRvIFJlZlB0cjxOb2RlPiBhbmQgY2FsbAorICAgICAgICBy ZWxlYXNlKCkgb24gcmV0dXJuZWQgb2JqZWN0IHRvIHByZXZlbnQgcmVmIGNvdW50IGNodXJuLgor ICAgICAgICAoV2ViQ29yZTo6RWRpdG9yOjppbmNyZWFzZVNlbGVjdGlvbkxpc3RMZXZlbFVub3Jk ZXJlZCk6IERpdHRvLgorCisyMDA5LTA3LTA2ICBEYXZpZCBLaWx6ZXIgIDxkZGtpbHplckBhcHBs ZS5jb20+CisKICAgICAgICAgTWlub3IgY2xlYW4gdXAgdG8gcnVudGltZV9yb290LntjcHB8aH0K IAogICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KZGlmZiAtLWdpdCBhL1dlYkNv cmUvZWRpdGluZy9FZGl0b3IuY3BwIGIvV2ViQ29yZS9lZGl0aW5nL0VkaXRvci5jcHAKaW5kZXgg MmFkNWNjZS4uNDkwZDJlZiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9lZGl0aW5nL0VkaXRvci5jcHAK KysrIGIvV2ViQ29yZS9lZGl0aW5nL0VkaXRvci5jcHAKQEAgLTY1NSw5ICs2NTUsOSBAQCBQYXNz UmVmUHRyPE5vZGU+IEVkaXRvcjo6aW5jcmVhc2VTZWxlY3Rpb25MaXN0TGV2ZWxPcmRlcmVkKCkK ICAgICBpZiAoIWNhbkVkaXRSaWNobHkoKSB8fCBtX2ZyYW1lLT5zZWxlY3Rpb24oKS0+aXNOb25l KCkpCiAgICAgICAgIHJldHVybiAwOwogICAgIAotICAgIFBhc3NSZWZQdHI8Tm9kZT4gbmV3TGlz dCA9IEluY3JlYXNlU2VsZWN0aW9uTGlzdExldmVsQ29tbWFuZDo6aW5jcmVhc2VTZWxlY3Rpb25M aXN0TGV2ZWxPcmRlcmVkKG1fZnJhbWUtPmRvY3VtZW50KCkpOworICAgIFJlZlB0cjxOb2RlPiBu ZXdMaXN0ID0gSW5jcmVhc2VTZWxlY3Rpb25MaXN0TGV2ZWxDb21tYW5kOjppbmNyZWFzZVNlbGVj dGlvbkxpc3RMZXZlbE9yZGVyZWQobV9mcmFtZS0+ZG9jdW1lbnQoKSk7CiAgICAgcmV2ZWFsU2Vs ZWN0aW9uQWZ0ZXJFZGl0aW5nT3BlcmF0aW9uKCk7Ci0gICAgcmV0dXJuIG5ld0xpc3Q7CisgICAg cmV0dXJuIG5ld0xpc3QucmVsZWFzZSgpOwogfQogCiBQYXNzUmVmUHRyPE5vZGU+IEVkaXRvcjo6 aW5jcmVhc2VTZWxlY3Rpb25MaXN0TGV2ZWxVbm9yZGVyZWQoKQpAQCAtNjY1LDkgKzY2NSw5IEBA IFBhc3NSZWZQdHI8Tm9kZT4gRWRpdG9yOjppbmNyZWFzZVNlbGVjdGlvbkxpc3RMZXZlbFVub3Jk ZXJlZCgpCiAgICAgaWYgKCFjYW5FZGl0UmljaGx5KCkgfHwgbV9mcmFtZS0+c2VsZWN0aW9uKCkt PmlzTm9uZSgpKQogICAgICAgICByZXR1cm4gMDsKICAgICAKLSAgICBQYXNzUmVmUHRyPE5vZGU+ IG5ld0xpc3QgPSBJbmNyZWFzZVNlbGVjdGlvbkxpc3RMZXZlbENvbW1hbmQ6OmluY3JlYXNlU2Vs ZWN0aW9uTGlzdExldmVsVW5vcmRlcmVkKG1fZnJhbWUtPmRvY3VtZW50KCkpOworICAgIFJlZlB0 cjxOb2RlPiBuZXdMaXN0ID0gSW5jcmVhc2VTZWxlY3Rpb25MaXN0TGV2ZWxDb21tYW5kOjppbmNy ZWFzZVNlbGVjdGlvbkxpc3RMZXZlbFVub3JkZXJlZChtX2ZyYW1lLT5kb2N1bWVudCgpKTsKICAg ICByZXZlYWxTZWxlY3Rpb25BZnRlckVkaXRpbmdPcGVyYXRpb24oKTsKLSAgICByZXR1cm4gbmV3 TGlzdDsKKyAgICByZXR1cm4gbmV3TGlzdC5yZWxlYXNlKCk7CiB9CiAKIHZvaWQgRWRpdG9yOjpk ZWNyZWFzZVNlbGVjdGlvbkxpc3RMZXZlbCgpCi0tIAoxLjYuMgoK