70223 2011-10-17 03:13:13 -0700 CSSStyleSheet: finding the owner node should be in its own method 2011-11-07 21:47:00 -0800 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 67720 1 rolandsteiner rolandsteiner darin dglazkov kling koivisto macpherson morrita webkit.review.bot oldest_to_newest 485027 0 rolandsteiner 2011-10-17 03:13:13 -0700 CSSStyleSheet::document() tries to find the owner node and - if it's a document - return it. This should be in its own function, which will come in useful for https://bugs.webkit.org/show_bug.cgi?id=67720. Also, the function itself can be streamlined. 485029 1 111235 rolandsteiner 2011-10-17 03:28:32 -0700 Created attachment 111235 Patch 485062 2 111235 kling 2011-10-17 05:01:37 -0700 Comment on attachment 111235 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111235&action=review > Source/WebCore/css/CSSStyleSheet.cpp:285 > + Unnecessary newline. > Source/WebCore/css/CSSStyleSheet.h:91 > + Node* findOwnerNode() const; I think this name is way too easily confused with StyleSheet::ownerNode() which doesn't climb. 485652 3 rolandsteiner 2011-10-17 17:15:59 -0700 (In reply to comment #2) > > Source/WebCore/css/CSSStyleSheet.h:91 > > + Node* findOwnerNode() const; > > I think this name is way too easily confused with StyleSheet::ownerNode() which doesn't climb. Well, that's what 'find' is supposed to imply, but I'm of course open to suggestions. 486079 4 111235 dglazkov 2011-10-18 09:02:27 -0700 Comment on attachment 111235 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111235&action=review >> Source/WebCore/css/CSSStyleSheet.h:91 >> + Node* findOwnerNode() const; > > I think this name is way too easily confused with StyleSheet::ownerNode() which doesn't climb. Kling-san is right. Are there any examples of similar searching in the code? Perhaps we could steal naming convention from there? 496789 5 111235 kling 2011-11-06 01:40:25 -0800 Comment on attachment 111235 Patch Removing from review queue since this patch doesn't apply anymore. 496991 6 113827 rolandsteiner 2011-11-06 23:33:08 -0800 Created attachment 113827 de-conflicted patch AFAICT there's no naming convention for tree climbing - e.g., CSSRule's parentStyleSheet() implicitly climbs. Renamed the function to 'styleSheetOwnerNode'. 497189 7 113827 webkit.review.bot 2011-11-07 08:32:08 -0800 Comment on attachment 113827 de-conflicted patch Clearing flags on attachment: 113827 Committed r99426: <http://trac.webkit.org/changeset/99426> 497190 8 webkit.review.bot 2011-11-07 08:32:13 -0800 All reviewed patches have been landed. Closing bug. 497624 9 darin 2011-11-07 17:26:48 -0800 (In reply to comment #6) > AFAICT there's no naming convention for tree climbing - e.g., CSSRule's parentStyleSheet() implicitly climbs. Renamed the function to 'styleSheetOwnerNode'. In the past, we have sometimes used the word “find” as the prefix for “climbing”. Heh, now that I write this it seems that’s what you used and Andreas talked you out of it! 497633 10 kling 2011-11-07 17:35:29 -0800 (In reply to comment #9) > (In reply to comment #6) > > AFAICT there's no naming convention for tree climbing - e.g., CSSRule's parentStyleSheet() implicitly climbs. Renamed the function to 'styleSheetOwnerNode'. > > In the past, we have sometimes used the word “find” as the prefix for “climbing”. Heh, now that I write this it seems that’s what you used and Andreas talked you out of it! D'oh! Well I'm certainly not opposed to the "find" prefix, especially if that's an existing convention. I was merely pointing out the possibly confusing similarity between "ownerNode" and "findOwnerNode" :-) 497638 11 darin 2011-11-07 17:40:17 -0800 (In reply to comment #10) > I was merely pointing out the possibly confusing similarity between "ownerNode" and "findOwnerNode" :-) Yes, I agree completely with that. We can use the “find” prefix to allude to the fact that we’re not just getting something we have directly, say in a data member, and instead having to do a bit of searching. But when it comes to the thing after “find” it should not just be the same as some other function in the same class. 497653 12 rolandsteiner 2011-11-07 18:19:24 -0800 Want me to rename it to findStyleSheetOwnerNode? 497695 13 darin 2011-11-07 20:13:30 -0800 (In reply to comment #12) > Want me to rename it to findStyleSheetOwnerNode? That might be good. On the other hand, since in turn it’s called in a function named just “document”, I am not sure that adding a find just to this function’s name pays off all that well in reminding people about performance cost. 497728 14 rolandsteiner 2011-11-07 21:47:00 -0800 Ask, and ye shall receive! ;) -> https://bugs.webkit.org/show_bug.cgi?id=71764 111235 2011-10-17 03:28:32 -0700 2011-11-06 23:33:03 -0800 Patch bug-70223-20111017192830.patch text/plain 2918 rolandsteiner U3VidmVyc2lvbiBSZXZpc2lvbjogOTc1ODAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBhZmZiMTUyZDc4YWQwZjAy NzMzODRmYjVjMTM4MmEyOWZkOWEwNjVlLi42YjQ2Y2RiZTc4NmIzMmY1YWFmOWVhOTZlYTBhNTE2 NGZlMmM0N2NiIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjEgQEAKKzIwMTEtMTAtMTcgIFJvbGFu ZCBTdGVpbmVyICA8cm9sYW5kc3RlaW5lckBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTcwMjIzCisgICAgICAgIENTU1N0eWxl U2hlZXQ6IGZpbmRpbmcgdGhlIG93bmVyIG5vZGUgc2hvdWxkIGJlIGluIGl0cyBvd24gbWV0aG9k CisKKyAgICAgICAgQWRkIGZpbmRPd25lck5vZGUoKSwgc2ltcGxpZnkgdGhlIGltcGxlbWVudGF0 aW9uCisgICAgICAgIChDU1NSdWxlOjpwYXJlbnRTdHlsZVNoZWV0IGp1c3QgZG9lcyB0aGUgc2Ft ZSBwYXJlbnQgY2xpbWJpbmcgLSBubyBuZWVkIHRvIHVzZSBpdC4pCisgICAgICAgIENoYW5nZSBk b2N1bWVudCgpIHRvIHVzZSBmaW5kT3duZXJOb2RlKCkuCisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLiAocmVmYWN0b3JpbmcpCisK KyAgICAgICAgKiBjc3MvQ1NTU3R5bGVTaGVldC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpDU1NT dHlsZVNoZWV0OjpmaW5kT3duZXJOb2RlKToKKyAgICAgICAgKFdlYkNvcmU6OkNTU1N0eWxlU2hl ZXQ6OmRvY3VtZW50KToKKyAgICAgICAgKiBjc3MvQ1NTU3R5bGVTaGVldC5oOgorCiAyMDExLTEw LTE2ICBLZW50YXJvIEhhcmEgIDxoYXJha2VuQGNocm9taXVtLm9yZz4KIAogICAgICAgICBHZW5l cmF0ZSBYU0xUUHJvY2Vzc29yIGNvbnN0cnVjdG9yIGZvciBKU0MgYnkgW0NvbnN0cnVjdG9yXSBJ REwuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9jc3MvQ1NTU3R5bGVTaGVldC5jcHAgYi9T b3VyY2UvV2ViQ29yZS9jc3MvQ1NTU3R5bGVTaGVldC5jcHAKaW5kZXggMzM1NDczMDA3MGEzNjZm YzNhNzdiY2FkNjE0Mzg5ZmU3MjI2M2RmYS4uMTAzMTJmMWZkMmEyYzk1NWI5MjczMzA5MTgwODBm MTM0ZWUxZTM5ZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvY3NzL0NTU1N0eWxlU2hlZXQu Y3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2Nzcy9DU1NTdHlsZVNoZWV0LmNwcApAQCAtMjY3LDI0 ICsyNjcsMjYgQEAgdm9pZCBDU1NTdHlsZVNoZWV0OjpzdGFydExvYWRpbmdEeW5hbWljU2hlZXQo KQogICAgICAgICBvd25lci0+c3RhcnRMb2FkaW5nRHluYW1pY1NoZWV0KCk7CiB9CiAKLURvY3Vt ZW50KiBDU1NTdHlsZVNoZWV0Ojpkb2N1bWVudCgpCitOb2RlKiBDU1NTdHlsZVNoZWV0OjpmaW5k T3duZXJOb2RlKCkgY29uc3QKIHsKLSAgICBTdHlsZUJhc2UqIHN0eWxlT2JqZWN0ID0gdGhpczsK LSAgICB3aGlsZSAoc3R5bGVPYmplY3QpIHsKKyAgICBmb3IgKGNvbnN0IFN0eWxlQmFzZSogc3R5 bGVPYmplY3QgPSB0aGlzOyBzdHlsZU9iamVjdDsgc3R5bGVPYmplY3QgPSBzdHlsZU9iamVjdC0+ cGFyZW50KCkpIHsKICAgICAgICAgaWYgKHN0eWxlT2JqZWN0LT5pc0NTU1N0eWxlU2hlZXQoKSkg ewotICAgICAgICAgICAgTm9kZSogb3duZXJOb2RlID0gc3RhdGljX2Nhc3Q8Q1NTU3R5bGVTaGVl dCo+KHN0eWxlT2JqZWN0KS0+b3duZXJOb2RlKCk7CisgICAgICAgICAgICBOb2RlKiBvd25lck5v ZGUgPSBzdGF0aWNfY2FzdDxjb25zdCBDU1NTdHlsZVNoZWV0Kj4oc3R5bGVPYmplY3QpLT5vd25l ck5vZGUoKTsKICAgICAgICAgICAgIGlmIChvd25lck5vZGUpCi0gICAgICAgICAgICAgICAgcmV0 dXJuIG93bmVyTm9kZS0+ZG9jdW1lbnQoKTsKKyAgICAgICAgICAgICAgICByZXR1cm4gb3duZXJO b2RlOwogICAgICAgICB9Ci0gICAgICAgIGlmIChzdHlsZU9iamVjdC0+aXNSdWxlKCkpCi0gICAg ICAgICAgICBzdHlsZU9iamVjdCA9IHN0YXRpY19jYXN0PENTU1J1bGUqPihzdHlsZU9iamVjdCkt PnBhcmVudFN0eWxlU2hlZXQoKTsKLSAgICAgICAgZWxzZQotICAgICAgICAgICAgc3R5bGVPYmpl Y3QgPSBzdHlsZU9iamVjdC0+cGFyZW50KCk7CiAgICAgfQotCiAgICAgcmV0dXJuIDA7CiB9CiAK K0RvY3VtZW50KiBDU1NTdHlsZVNoZWV0Ojpkb2N1bWVudCgpCit7CisgICAgTm9kZSogb3duZXJO b2RlID0gZmluZE93bmVyTm9kZSgpOworCisgICAgcmV0dXJuIG93bmVyTm9kZSA/IG93bmVyTm9k ZS0+ZG9jdW1lbnQoKSA6IDA7Cit9CisKKwogdm9pZCBDU1NTdHlsZVNoZWV0OjpzdHlsZVNoZWV0 Q2hhbmdlZCgpCiB7CiAgICAgU3R5bGVCYXNlKiByb290ID0gdGhpczsKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL2Nzcy9DU1NTdHlsZVNoZWV0LmggYi9Tb3VyY2UvV2ViQ29yZS9jc3MvQ1NT U3R5bGVTaGVldC5oCmluZGV4IDZkY2IzNDI3MTU4NmIyZjc3NDZhZjY5ZWJmNDY1YWU3Y2RmMGZh NmYuLjJhNzU5NDhiYzU5ZWQwMjdlYjE5OTU1YmUzYTBiYWNjYjZhYzEwYjIgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJDb3JlL2Nzcy9DU1NTdHlsZVNoZWV0LmgKKysrIGIvU291cmNlL1dlYkNvcmUv Y3NzL0NTU1N0eWxlU2hlZXQuaApAQCAtODgsNiArODgsNyBAQCBwdWJsaWM6CiAgICAgdmlydHVh bCB2b2lkIGNoZWNrTG9hZGVkKCk7CiAgICAgdm9pZCBzdGFydExvYWRpbmdEeW5hbWljU2hlZXQo KTsKIAorICAgIE5vZGUqIGZpbmRPd25lck5vZGUoKSBjb25zdDsKICAgICBEb2N1bWVudCogZG9j dW1lbnQoKTsKIAogICAgIGNvbnN0IFN0cmluZyYgY2hhcnNldCgpIGNvbnN0IHsgcmV0dXJuIG1f Y2hhcnNldDsgfQo= 113827 2011-11-06 23:33:08 -0800 2011-11-07 08:32:08 -0800 de-conflicted patch bug-70223-20111107163307.patch text/plain 2425 rolandsteiner U3VidmVyc2lvbiBSZXZpc2lvbjogOTkzOTQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBjYjA4MjU0YWQ3MzA1Mjdl ZDczNzhiNGNjMDU2NjUwYmFjMTA3NWRiLi4wZGQ5ODFjYjkxMTJhNDc2OGMxYjlmZDMzYThkZTky OTc2NGVmMDQ2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTkgQEAKKzIwMTEtMTEtMDcgIFJvbGFu ZCBTdGVpbmVyICA8cm9sYW5kc3RlaW5lckBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTcwMjIzCisgICAgICAgIENTU1N0eWxl U2hlZXQ6IGZpbmRpbmcgdGhlIG93bmVyIG5vZGUgc2hvdWxkIGJlIGluIGl0cyBvd24gbWV0aG9k CisKKyAgICAgICAgQWRkIHN0eWxlU2hlZXRPd25lck5vZGUoKSBmdW5jdGlvbiB0aGF0IHJldHVy bnMgdGhlIG93bmVyIE5vZGUgb2YgdGhlIHN0eWxlIHNoZWV0LCBvciAwLgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE5vIG5ldyB0ZXN0cy4gKHJlZmFj dG9yaW5nKQorCisgICAgICAgICogY3NzL0NTU1N0eWxlU2hlZXQuY3BwOgorICAgICAgICAoV2Vi Q29yZTo6Q1NTU3R5bGVTaGVldDo6c3R5bGVTaGVldE93bmVyTm9kZSk6CisgICAgICAgIChXZWJD b3JlOjpDU1NTdHlsZVNoZWV0Ojpkb2N1bWVudCk6CisgICAgICAgICogY3NzL0NTU1N0eWxlU2hl ZXQuaDoKKwogMjAxMS0xMS0wNiAgS2Vpc2hpIEhhdHRvcmkgIDxrZWlzaGlAd2Via2l0Lm9yZz4K IAogICAgICAgICBSZW1vdmUgQ29sb3JJbnB1dFR5cGU6OmhhbmRsZUNsaWNrRXZlbnQKZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJDb3JlL2Nzcy9DU1NTdHlsZVNoZWV0LmNwcCBiL1NvdXJjZS9XZWJD b3JlL2Nzcy9DU1NTdHlsZVNoZWV0LmNwcAppbmRleCBiNmU2MTBlZmEwZmQ4MWQwOGEwODE5Zjdl OWE4MDdmODg0MDQwN2Y0Li44MzgxOWVmZDFkYWU0OGIyYzYzMzM1NzMxMjVmZDMxYmUwYzdlZjJj IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9jc3MvQ1NTU3R5bGVTaGVldC5jcHAKKysrIGIv U291cmNlL1dlYkNvcmUvY3NzL0NTU1N0eWxlU2hlZXQuY3BwCkBAIC0yNDUsMTUgKzI0NSwyMyBA QCB2b2lkIENTU1N0eWxlU2hlZXQ6OnN0YXJ0TG9hZGluZ0R5bmFtaWNTaGVldCgpCiAgICAgICAg IG93bmVyLT5zdGFydExvYWRpbmdEeW5hbWljU2hlZXQoKTsKIH0KIAotRG9jdW1lbnQqIENTU1N0 eWxlU2hlZXQ6OmRvY3VtZW50KCkKK05vZGUqIENTU1N0eWxlU2hlZXQ6OnN0eWxlU2hlZXRPd25l ck5vZGUoKSBjb25zdAogewotICAgIGZvciAoQ1NTU3R5bGVTaGVldCogc2hlZXQgPSB0aGlzOyBz aGVldDsgc2hlZXQgPSBzaGVldC0+cGFyZW50U3R5bGVTaGVldCgpKSB7CisgICAgZm9yIChjb25z dCBDU1NTdHlsZVNoZWV0KiBzaGVldCA9IHRoaXM7IHNoZWV0OyBzaGVldCA9IHNoZWV0LT5wYXJl bnRTdHlsZVNoZWV0KCkpIHsKICAgICAgICAgaWYgKE5vZGUqIG93bmVyTm9kZSA9IHNoZWV0LT5v d25lck5vZGUoKSkKLSAgICAgICAgICAgIHJldHVybiBvd25lck5vZGUtPmRvY3VtZW50KCk7Cisg ICAgICAgICAgICByZXR1cm4gb3duZXJOb2RlOwogICAgIH0KICAgICByZXR1cm4gMDsKIH0KIAor RG9jdW1lbnQqIENTU1N0eWxlU2hlZXQ6OmRvY3VtZW50KCkKK3sKKyAgICBOb2RlKiBvd25lck5v ZGUgPSBzdHlsZVNoZWV0T3duZXJOb2RlKCk7CisKKyAgICByZXR1cm4gb3duZXJOb2RlID8gb3du ZXJOb2RlLT5kb2N1bWVudCgpIDogMDsKK30KKworCiB2b2lkIENTU1N0eWxlU2hlZXQ6OnN0eWxl U2hlZXRDaGFuZ2VkKCkKIHsKICAgICBDU1NTdHlsZVNoZWV0KiByb290U2hlZXQgPSB0aGlzOwpk aWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvY3NzL0NTU1N0eWxlU2hlZXQuaCBiL1NvdXJjZS9X ZWJDb3JlL2Nzcy9DU1NTdHlsZVNoZWV0LmgKaW5kZXggYTU3N2JkMmMxZTRiZDBmYWI3YjJkN2Mz Y2E4Zjc2NTIwMGJjZTdlMC4uZjdiYmNhZDE4NzFlNGM2Y2M0ODI4ZWMzZjg4ZWFhZTQyZjllMzY0 ZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvY3NzL0NTU1N0eWxlU2hlZXQuaAorKysgYi9T b3VyY2UvV2ViQ29yZS9jc3MvQ1NTU3R5bGVTaGVldC5oCkBAIC05NSw2ICs5NSw3IEBAIHB1Ymxp YzoKICAgICB2aXJ0dWFsIHZvaWQgY2hlY2tMb2FkZWQoKTsKICAgICB2b2lkIHN0YXJ0TG9hZGlu Z0R5bmFtaWNTaGVldCgpOwogCisgICAgTm9kZSogc3R5bGVTaGVldE93bmVyTm9kZSgpIGNvbnN0 OwogICAgIERvY3VtZW50KiBkb2N1bWVudCgpOwogCiAgICAgY29uc3QgU3RyaW5nJiBjaGFyc2V0 KCkgY29uc3QgeyByZXR1cm4gbV9jaGFyc2V0OyB9Cg==