147630 2015-08-04 09:53:46 -0700 shouldParseTelephoneNumbersInNode() does not need to check for Comment nodes 2015-08-08 19:17:53 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 cdumez cdumez commit-queue esprehn+autocc gyuyoung.kim kling koivisto rniwa oldest_to_newest 1114622 0 cdumez 2015-08-04 09:53:46 -0700 shouldParseTelephoneNumbersInNode() does not need to check for Comment nodes. We already know the input is a ContainerNode and a Comment is not a ContainerNode. 1114624 1 258179 cdumez 2015-08-04 10:04:16 -0700 Created attachment 258179 Patch 1114783 2 258179 commit-queue 2015-08-04 14:47:57 -0700 Comment on attachment 258179 Patch Clearing flags on attachment: 258179 Committed r187893: <http://trac.webkit.org/changeset/187893> 1114784 3 commit-queue 2015-08-04 14:48:02 -0700 All reviewed patches have been landed. Closing bug. 1116045 4 258179 darin 2015-08-08 14:34:15 -0700 Comment on attachment 258179 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258179&action=review > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2352 > + for (const ContainerNode* ancestor = &node; ancestor; ancestor = ancestor->parentNode()) { Modern way to write this is: for (auto& container : lineage(node)) { 1116047 5 258179 darin 2015-08-08 14:37:40 -0700 Comment on attachment 258179 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258179&action=review >> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2352 >> + for (const ContainerNode* ancestor = &node; ancestor; ancestor = ancestor->parentNode()) { > > Modern way to write this is: > > for (auto& container : lineage(node)) { Well, not exactly, I think it’s: for (auto& container : lineageOfType<Element>(node)) { } The reason I say Element here instead of ContainerNode is that only an element can have isLink set to true, even though the function is in Node. Links are all HTMLAnchorElement, HTMLLinkElement, or SVGAElement. The rest if the checks in disallowTelephoneNumberParsing are all clearly only relevant for Element, not other kinds of ContainerNode. 1116076 6 cdumez 2015-08-08 19:17:53 -0700 (In reply to comment #5) > Comment on attachment 258179 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=258179&action=review > > >> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2352 > >> + for (const ContainerNode* ancestor = &node; ancestor; ancestor = ancestor->parentNode()) { > > > > Modern way to write this is: > > > > for (auto& container : lineage(node)) { > > Well, not exactly, I think it’s: > > for (auto& container : lineageOfType<Element>(node)) { > } > > The reason I say Element here instead of ContainerNode is that only an > element can have isLink set to true, even though the function is in Node. > Links are all HTMLAnchorElement, HTMLLinkElement, or SVGAElement. The rest > if the checks in disallowTelephoneNumberParsing are all clearly only > relevant for Element, not other kinds of ContainerNode. Funny that you mention it, in an earlier version of my patch I had: static inline bool shouldParseTelephoneNumbersInNode(const ContainerNode& node) { const Element* element = is<Element>(node) ? &downcast<Element>(node) : node.parentElement(); if (!element) return true; for (auto& ancestor : lineageOfType<Element>(*element)) { if (disallowTelephoneNumberParsing(ancestor)) return false; } return true; } However, as you can see it is not as nice as one would think because lineageOfType() expects a non-null element. The code ended being longer and a bit more complicated so I kept the old-style for-loop in this case. However, if you prefer this (or have a better way to write this), I can upload a patch to make this change. Also note that there is no performance win to traverse Elements only in this case as all the checks in disallowTelephoneNumberParsing() are efficient on Node (i.e. isLink() / hasTagName() are not any faster on an Element than on a Node) so the implicit isElement() checks are redundant. 258179 2015-08-04 10:04:16 -0700 2015-08-04 14:47:57 -0700 Patch bug-147630-20150804100409.patch text/plain 2812 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMTg3ODU0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNzc0OWU1MDU3ZGQ1OGEx ZjhlZTIzYzFiZmI2ZGUzMWNjMDg2MmEwMy4uNGMzZjVhOWM4ZjJjMDU3OThmYmE1ZTFhMzk1YjU1 NzMxMjE2ZTk4ZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDE1LTA4LTA0ICBDaHJp cyBEdW1leiAgPGNkdW1lekBhcHBsZS5jb20+CisKKyAgICAgICAgc2hvdWxkUGFyc2VUZWxlcGhv bmVOdW1iZXJzSW5Ob2RlKCkgZG9lcyBub3QgbmVlZCB0byBjaGVjayBmb3IgQ29tbWVudCBub2Rl cworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ3NjMw CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgc2hvdWxk UGFyc2VUZWxlcGhvbmVOdW1iZXJzSW5Ob2RlKCkgZG9lcyBub3QgbmVlZCB0byBjaGVjayBmb3Ig Q29tbWVudAorICAgICAgICBub2Rlcy4gV2UgYWxyZWFkeSBrbm93IHRoZSBpbnB1dCBpcyBhIENv bnRhaW5lck5vZGUgYW5kIGEgQ29tbWVudCBpcworICAgICAgICBOT1QgYSBDb250YWluZXJOb2Rl LiBBbHNvLCB3ZSB3ZXJlIGFscmVhZHkgZG9pbmcgYSBpc0VsZW1lbnROb2RlKCkKKyAgICAgICAg Y2hlY2sgYmVmb3JlIGNhbGxpbmcgZGlzYWxsb3dUZWxlcGhvbmVOdW1iZXJQYXJzaW5nKCkuCisK KyAgICAgICAgSSBhbHNvIHVwZGF0ZWQgdGhlIGZ1bmN0aW9uIHRvIHVzZSBhIGZvciBsb29wIGZv ciBjbGFyaXR5IGFuZCBkcm9wcGVkCisgICAgICAgIHRoZSBpc0VsZW1lbnROb2RlKCkgY2hlY2sg YXMgY2FsbGluZyBoYXNUYWdOYW1lKCkgLyBpc0xpbmsoKSBvbiBhCisgICAgICAgIENvbnRhaW5l ck5vZGUgaXMgYXMgZmFzdCBhbGwgY2FsbGluZyBpdCBvbiBhbiBFbGVtZW50IG5vd2FkYXlzLgor CisgICAgICAgICogaHRtbC9wYXJzZXIvSFRNTFRyZWVCdWlsZGVyLmNwcDoKKyAgICAgICAgKFdl YkNvcmU6OmRpc2FsbG93VGVsZXBob25lTnVtYmVyUGFyc2luZyk6CisgICAgICAgIChXZWJDb3Jl OjpzaG91bGRQYXJzZVRlbGVwaG9uZU51bWJlcnNJbk5vZGUpOgorCiAyMDE1LTA4LTAzICBDc2Fi YSBPc3p0cm9nb27DoWMgIDxvc3N5QHdlYmtpdC5vcmc+CiAKICAgICAgICAgSW50cm9kdWNlIENP TVBJTEVSKEdDQ19PUl9DTEFORykgZ3VhcmQgYW5kIG1ha2UgQ09NUElMRVIoR0NDKSB0cnVlIG9u bHkgZm9yIEdDQwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvaHRtbC9wYXJzZXIvSFRNTFRy ZWVCdWlsZGVyLmNwcCBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvcGFyc2VyL0hUTUxUcmVlQnVpbGRl ci5jcHAKaW5kZXggNjNiMmMzNzBiZTJkYzhiZGE5ZWYzYzk0ZmI2OGIxYThiOWM3NzBlOC4uNjRi MjY3OGIwY2I2OTg0MTI2ZWU0NzBmMzMzZmM4NWI0MjMzOTM2MSAxMDA2NDQKLS0tIGEvU291cmNl L1dlYkNvcmUvaHRtbC9wYXJzZXIvSFRNTFRyZWVCdWlsZGVyLmNwcAorKysgYi9Tb3VyY2UvV2Vi Q29yZS9odG1sL3BhcnNlci9IVE1MVHJlZUJ1aWxkZXIuY3BwCkBAIC0yMzM2LDEwICsyMzM2LDkg QEAgdm9pZCBIVE1MVHJlZUJ1aWxkZXI6OmxpbmtpZnlQaG9uZU51bWJlcnMoY29uc3QgU3RyaW5n JiBzdHJpbmcpCiB9CiAKIC8vIExvb2tzIGF0IHRoZSBhbmNlc3RvcnMgb2YgdGhlIGVsZW1lbnQg dG8gZGV0ZXJtaW5lIHdoZXRoZXIgd2UncmUgaW5zaWRlIGFuIGVsZW1lbnQgd2hpY2ggZGlzYWxs b3dzIHBhcnNpbmcgcGhvbmUgbnVtYmVycy4KLXN0YXRpYyBpbmxpbmUgYm9vbCBkaXNhbGxvd1Rl bGVwaG9uZU51bWJlclBhcnNpbmcoY29uc3QgTm9kZSYgbm9kZSkKK3N0YXRpYyBpbmxpbmUgYm9v bCBkaXNhbGxvd1RlbGVwaG9uZU51bWJlclBhcnNpbmcoY29uc3QgQ29udGFpbmVyTm9kZSYgbm9k ZSkKIHsKICAgICByZXR1cm4gbm9kZS5pc0xpbmsoKQotICAgICAgICB8fCBub2RlLm5vZGVUeXBl KCkgPT0gTm9kZTo6Q09NTUVOVF9OT0RFCiAgICAgICAgIHx8IG5vZGUuaGFzVGFnTmFtZShzY3Jp cHRUYWcpCiAgICAgICAgIHx8IGlzPEhUTUxGb3JtQ29udHJvbEVsZW1lbnQ+KG5vZGUpCiAgICAg ICAgIHx8IG5vZGUuaGFzVGFnTmFtZShzdHlsZVRhZykKQEAgLTIzNTAsMTIgKzIzNDksMTAgQEAg c3RhdGljIGlubGluZSBib29sIGRpc2FsbG93VGVsZXBob25lTnVtYmVyUGFyc2luZyhjb25zdCBO b2RlJiBub2RlKQogCiBzdGF0aWMgaW5saW5lIGJvb2wgc2hvdWxkUGFyc2VUZWxlcGhvbmVOdW1i ZXJzSW5Ob2RlKGNvbnN0IENvbnRhaW5lck5vZGUmIG5vZGUpCiB7Ci0gICAgY29uc3QgQ29udGFp bmVyTm9kZSogY3VycmVudE5vZGUgPSAmbm9kZTsKLSAgICBkbyB7Ci0gICAgICAgIGlmIChjdXJy ZW50Tm9kZS0+aXNFbGVtZW50Tm9kZSgpICYmIGRpc2FsbG93VGVsZXBob25lTnVtYmVyUGFyc2lu ZygqY3VycmVudE5vZGUpKQorICAgIGZvciAoY29uc3QgQ29udGFpbmVyTm9kZSogYW5jZXN0b3Ig PSAmbm9kZTsgYW5jZXN0b3I7IGFuY2VzdG9yID0gYW5jZXN0b3ItPnBhcmVudE5vZGUoKSkgewor ICAgICAgICBpZiAoZGlzYWxsb3dUZWxlcGhvbmVOdW1iZXJQYXJzaW5nKCphbmNlc3RvcikpCiAg ICAgICAgICAgICByZXR1cm4gZmFsc2U7Ci0gICAgICAgIGN1cnJlbnROb2RlID0gY3VycmVudE5v ZGUtPnBhcmVudE5vZGUoKTsKLSAgICB9IHdoaWxlIChjdXJyZW50Tm9kZSk7CisgICAgfQogICAg IHJldHVybiB0cnVlOwogfQogCg==