88222 2012-06-04 04:00:22 -0700 Improve the performance of pushScope in StyleResolver 2012-06-07 19:06:38 -0700 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 tasak webkit-unassigned cmarcelo macpherson menard morrita tasak webkit.review.bot oldest_to_newest 640459 0 tasak 2012-06-04 04:00:22 -0700 As StyleResolver::setupScopeStack has a bug that m_scopeStackParent is always set to be NULL, setupScopeStack is always invoked if scoped author styles exist. 640465 1 145558 tasak 2012-06-04 04:24:11 -0700 Created attachment 145558 Patch 641076 2 145558 morrita 2012-06-04 18:10:40 -0700 Comment on attachment 145558 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145558&action=review > Source/WebCore/ChangeLog:10 > + Reviewed by NOBODY (OOPS!). The "Reviewed By" line should be jut after the bug URL. See others. > Source/WebCore/css/StyleResolver.cpp:563 > for (; parent; parent = parent->parentOrHostNode()) { It would be better to introduce new local variable for this loop instead of reusing the parameter. It's generally a bad style to reuse it and that is a source of this bug. > Source/WebCore/css/StyleResolver.cpp:597 > + const ScopeStackFrame& lastFrame = m_scopeStack[m_scopeStack.size() - 1]; Vector::last() is preferred. Then it's easy to pack the conditional into the single if clause. Also, why do we need isEmpty() check? 643407 3 146251 tasak 2012-06-07 04:29:04 -0700 Created attachment 146251 Patch 643409 4 tasak 2012-06-07 04:34:57 -0700 (In reply to comment #2) > (From update of attachment 145558 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145558&action=review > > > Source/WebCore/ChangeLog:10 > > + Reviewed by NOBODY (OOPS!). > > The "Reviewed By" line should be jut after the bug URL. See others. Done. However, I think, this is the default order generated by prepare-ChangeLog script. > > Source/WebCore/css/StyleResolver.cpp:563 > > for (; parent; parent = parent->parentOrHostNode()) { > > It would be better to introduce new local variable for this loop instead of reusing the parameter. > It's generally a bad style to reuse it and that is a source of this bug. I see. Done. > > Source/WebCore/css/StyleResolver.cpp:597 > > + const ScopeStackFrame& lastFrame = m_scopeStack[m_scopeStack.size() - 1]; > > Vector::last() is preferred. Then it's easy to pack the conditional into the single if clause. Done. > Also, why do we need isEmpty() check? Yes, we need. Because pushScope doesn't add any ruleset if a scope doesn't have any rules, but m_scopeStackParent is always updated to avoid ruleset recalculation. And I also tested whether we could remove isEmpty() check or not and obtained many crash logs. Best regards, Takashi Sakamoto 644179 5 146251 morrita 2012-06-07 18:34:01 -0700 Comment on attachment 146251 Patch Okay, let's land this. 644211 6 146251 webkit.review.bot 2012-06-07 19:06:32 -0700 Comment on attachment 146251 Patch Clearing flags on attachment: 146251 Committed r119782: <http://trac.webkit.org/changeset/119782> 644213 7 webkit.review.bot 2012-06-07 19:06:38 -0700 All reviewed patches have been landed. Closing bug. 145558 2012-06-04 04:24:11 -0700 2012-06-07 04:28:57 -0700 Patch bug-88222-20120604202410.patch text/plain 2968 tasak U3VidmVyc2lvbiBSZXZpc2lvbjogMTE5Mzc2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNDYyZTMwZWUyMDE5ZDQ2 NWRlZTM1MjBhOTFmYTBmYWY5YjY1NmEyMi4uNDE2ZGFjZjRjODlkMzE0NTI1NTM5Yjg4OWIyNDIz MzgxOTVlMTVkZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI4IEBACisyMDEyLTA2LTA0ICBUYWth c2hpIFNha2Ftb3RvICA8dGFzYWtAZ29vZ2xlLmNvbT4KKworICAgICAgICBJbXByb3ZlIHRoZSBw ZXJmb3JtYW5jZSBvZiBwdXNoU2NvcGUgaW4gU3R5bGVSZXNvbHZlcgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODgyMjIKKworICAgICAgICBBcyBzZXR1 cFNjb3BlU3RhY2sgYWx3YXlzIHNldHMgbV9zY29wZVN0YWNrUGFyZW50IHRvIGJlIE5VTEwsCisg ICAgICAgIG1fc2NvcGVTdGFjayBpcyBuZXZlciByZXVzZWQuIG1fc2NvcGVTdGFja1BhcmVudCBz aG91bGQgYmUgdGhlIGxhc3QKKyAgICAgICAgZWxlbWVudCBvZiBtX3Njb3BlU3RhY2suCisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3Rz LCBiZWNhdXNlIGZhc3QvY3NzL3N0eWxlLXNjb3BlZC8gdXNlcyBwdXNoU2NvcGUsCisgICAgICAg IHBvcFNjb3BlIGFuZCBzZXR1cFNjb3BlU3RhY2sgYW5kIGNoZWNrcyB3aGV0aGVyIHRoZXJlIGV4 aXN0cworICAgICAgICBhbnkgY3Jhc2ggYnVnIG9yIG5vdC4gQW5kIHRoaXMgcGF0Y2ggZG9lc24n dCBjaGFuZ2UgYW55IGJlaGF2aW9yCisgICAgICAgIG9mIHNjb3BlZCBhdXRob3Igc3lsdGVzLgor CisgICAgICAgICogY3NzL1N0eWxlUmVzb2x2ZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U3R5 bGVSZXNvbHZlcjo6c2V0dXBTY29wZVN0YWNrKToKKyAgICAgICAgTW9kaWZpZWQgdG8gc2V0IG1f c2NvcGVTYWNrUGFyZW50IHRvIGJlIHRoZSBsYXN0IGVsZW1lbnQgb2YKKyAgICAgICAgbV9zY29w ZVN0YWNrLgorICAgICAgICAoV2ViQ29yZTo6U3R5bGVSZXNvbHZlcjo6cG9wU2NvcGUpOgorICAg ICAgICBNb2RpZmllZCB0byByZW1vdmUgdGhlIGxhc3QgZWxlbWVudCBpZiBtX3Njb3BlU3RhY2sg aXMgbm90IGVtcHR5IGFuZAorICAgICAgICB0aGUgbGFzdCBlbGVtZW50IG9mIG1fc2NvcGVTdGFj ayBoYXMgdGhlIHNhbWUgc2NvcGUgYXMgdGhlIHNjb3BpbmcKKyAgICAgICAgZWxlbWVudCBnaXZl biBieSB0aGUgYXJndW1lbnQuCisKIDIwMTItMDYtMDQgIEtlbnRhcm8gSGFyYSAgPGhhcmFrZW5A Y2hyb21pdW0ub3JnPgogCiAgICAgICAgIFVzZSBTZWxlY3RvclF1ZXJ5IGluIEVsZW1lbnQ6Ondl YmtpdE1hdGNoZXNTZWxlY3RvcigpCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9jc3MvU3R5 bGVSZXNvbHZlci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9jc3MvU3R5bGVSZXNvbHZlci5jcHAKaW5k ZXggMDk4ZTg1NjcwMTY2ZTk3OTQxMGNjMzJiMDU5NWI5YWVjMGVhMDQyZC4uMmY1YmI5ZTM0ZWRh NzQyMDIzNzAyYTA2NGNjMmIyNDkzYzMyOTBlZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUv Y3NzL1N0eWxlUmVzb2x2ZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2Nzcy9TdHlsZVJlc29s dmVyLmNwcApAQCAtNTU4LDYgKzU1OCw3IEBAIHZvaWQgU3R5bGVSZXNvbHZlcjo6c2V0dXBTY29w ZVN0YWNrKGNvbnN0IENvbnRhaW5lck5vZGUqIHBhcmVudCkKICAgICAvLyBUaGUgc2NvcGluZyBl bGVtZW50IHN0YWNrIHNob3VsZG4ndCBiZSB1c2VkIGlmIDxzdHlsZSBzY29wZWQ+IGlzbid0IHVz ZWQgYW55d2hlcmUuCiAgICAgQVNTRVJUKCFtX3Njb3BlZEF1dGhvclN0eWxlcy5pc0VtcHR5KCkp OwogCisgICAgbV9zY29wZVN0YWNrUGFyZW50ID0gcGFyZW50OwogICAgIG1fc2NvcGVTdGFjay5z aHJpbmsoMCk7CiAgICAgZm9yICg7IHBhcmVudDsgcGFyZW50ID0gcGFyZW50LT5wYXJlbnRPckhv c3ROb2RlKCkpIHsKICAgICAgICAgUnVsZVNldCogcnVsZVNldCA9IHJ1bGVTZXRGb3JTY29wZShw YXJlbnQpOwpAQCAtNTY1LDcgKzU2Niw2IEBAIHZvaWQgU3R5bGVSZXNvbHZlcjo6c2V0dXBTY29w ZVN0YWNrKGNvbnN0IENvbnRhaW5lck5vZGUqIHBhcmVudCkKICAgICAgICAgICAgIG1fc2NvcGVT dGFjay5hcHBlbmQoU2NvcGVTdGFja0ZyYW1lKHBhcmVudCwgcnVsZVNldCkpOwogICAgIH0KICAg ICBtX3Njb3BlU3RhY2sucmV2ZXJzZSgpOwotICAgIG1fc2NvcGVTdGFja1BhcmVudCA9IHBhcmVu dDsKIH0KIAogdm9pZCBTdHlsZVJlc29sdmVyOjpwdXNoU2NvcGUoY29uc3QgQ29udGFpbmVyTm9k ZSogc2NvcGUsIGNvbnN0IENvbnRhaW5lck5vZGUqIHNjb3BlUGFyZW50KQpAQCAtNTkzLDcgKzU5 MywxMSBAQCB2b2lkIFN0eWxlUmVzb2x2ZXI6OnBvcFNjb3BlKGNvbnN0IENvbnRhaW5lck5vZGUq IHNjb3BlKQogewogICAgIC8vIE9ubHkgYm90aGVyIHRvIHVwZGF0ZSB0aGUgc2NvcGluZyBlbGVt ZW50IHN0YWNrIGlmIGl0IGlzIGNvbnNpc3RlbnQuCiAgICAgaWYgKHNjb3BlU3RhY2tJc0NvbnNp c3RlbnQoc2NvcGUpKSB7Ci0gICAgICAgIG1fc2NvcGVTdGFjay5yZW1vdmVMYXN0KCk7CisgICAg ICAgIGlmICghbV9zY29wZVN0YWNrLmlzRW1wdHkoKSkgeworICAgICAgICAgICAgY29uc3QgU2Nv cGVTdGFja0ZyYW1lJiBsYXN0RnJhbWUgPSBtX3Njb3BlU3RhY2tbbV9zY29wZVN0YWNrLnNpemUo KSAtIDFdOworICAgICAgICAgICAgaWYgKGxhc3RGcmFtZS5tX3Njb3BlID09IHNjb3BlKQorICAg ICAgICAgICAgICAgIG1fc2NvcGVTdGFjay5yZW1vdmVMYXN0KCk7CisgICAgICAgIH0KICAgICAg ICAgbV9zY29wZVN0YWNrUGFyZW50ID0gc2NvcGUtPnBhcmVudE9ySG9zdE5vZGUoKTsKICAgICB9 CiB9Cg== 146251 2012-06-07 04:29:04 -0700 2012-06-07 19:06:32 -0700 Patch bug-88222-20120607202903.patch text/plain 2822 tasak U3VidmVyc2lvbiBSZXZpc2lvbjogMTE5NzAzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOWQ4NjlkN2QwNzMxZjgz ZTRmNDBkYWZmNmY5Y2UwNGQ0NzcwOTU3OC4uMTZmYTYyNDdmY2FkZjE1Y2RmNTgwNDhlYmU4ZTdk ZTc1YWRjZTNiMyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI4IEBACisyMDEyLTA2LTA0ICBUYWth c2hpIFNha2Ftb3RvICA8dGFzYWtAZ29vZ2xlLmNvbT4KKworICAgICAgICBJbXByb3ZlIHRoZSBw ZXJmb3JtYW5jZSBvZiBwdXNoU2NvcGUgaW4gU3R5bGVSZXNvbHZlcgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODgyMjIKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBBcyBzZXR1cFNjb3BlU3RhY2sgYWx3YXlz IHNldHMgbV9zY29wZVN0YWNrUGFyZW50IHRvIGJlIE5VTEwsCisgICAgICAgIG1fc2NvcGVTdGFj ayBpcyBuZXZlciByZXVzZWQuIG1fc2NvcGVTdGFja1BhcmVudCBzaG91bGQgYmUgdGhlIGxhc3QK KyAgICAgICAgZWxlbWVudCBvZiBtX3Njb3BlU3RhY2suCisKKyAgICAgICAgTm8gbmV3IHRlc3Rz LCBiZWNhdXNlIGZhc3QvY3NzL3N0eWxlLXNjb3BlZC8gdXNlcyBwdXNoU2NvcGUsCisgICAgICAg IHBvcFNjb3BlIGFuZCBzZXR1cFNjb3BlU3RhY2sgYW5kIGNoZWNrcyB3aGV0aGVyIHRoZXJlIGV4 aXN0cworICAgICAgICBhbnkgY3Jhc2ggYnVnIG9yIG5vdC4gQW5kIHRoaXMgcGF0Y2ggZG9lc24n dCBjaGFuZ2UgYW55IGJlaGF2aW9yCisgICAgICAgIG9mIHNjb3BlZCBhdXRob3Igc3lsdGVzLgor CisgICAgICAgICogY3NzL1N0eWxlUmVzb2x2ZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U3R5 bGVSZXNvbHZlcjo6c2V0dXBTY29wZVN0YWNrKToKKyAgICAgICAgTW9kaWZpZWQgdG8gc2V0IG1f c2NvcGVTYWNrUGFyZW50IHRvIGJlIHRoZSBsYXN0IGVsZW1lbnQgb2YKKyAgICAgICAgbV9zY29w ZVN0YWNrLgorICAgICAgICAoV2ViQ29yZTo6U3R5bGVSZXNvbHZlcjo6cG9wU2NvcGUpOgorICAg ICAgICBNb2RpZmllZCB0byByZW1vdmUgdGhlIGxhc3QgZWxlbWVudCBpZiBtX3Njb3BlU3RhY2sg aXMgbm90IGVtcHR5IGFuZAorICAgICAgICB0aGUgbGFzdCBlbGVtZW50IG9mIG1fc2NvcGVTdGFj ayBoYXMgdGhlIHNhbWUgc2NvcGUgYXMgdGhlIHNjb3BpbmcKKyAgICAgICAgZWxlbWVudCBnaXZl biBieSB0aGUgYXJndW1lbnQuCisKIDIwMTItMDYtMDcgIExpIFlpbiAgPGxpLnlpbkBpbnRlbC5j b20+CiAKICAgICAgICAgRmlsZUFQSTogSWYgdHlwZSBjb25zaXN0cyBvZiBub24tQVNDSUkgY2hh cmFjdGVycyBpbiBCbG9iIGNvbnN0cnVjdG9yLCBpdCBzaG91bGQgdGhyb3cgYSBTeW50YXhFcnJv ci4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2Nzcy9TdHlsZVJlc29sdmVyLmNwcCBiL1Nv dXJjZS9XZWJDb3JlL2Nzcy9TdHlsZVJlc29sdmVyLmNwcAppbmRleCA3MGM5OTZmOTI3OWU5MTcy MWJkNTY1MDZiMTViNDUzMDRmNDc0ZjRhLi42NDhmNjFiYmFlOTQ3MTcwNjQ1OTMxZGY3YzU5NDg5 MGFiZDBjYjQ2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9jc3MvU3R5bGVSZXNvbHZlci5j cHAKKysrIGIvU291cmNlL1dlYkNvcmUvY3NzL1N0eWxlUmVzb2x2ZXIuY3BwCkBAIC01NTksMTAg KzU1OSwxMCBAQCB2b2lkIFN0eWxlUmVzb2x2ZXI6OnNldHVwU2NvcGVTdGFjayhjb25zdCBDb250 YWluZXJOb2RlKiBwYXJlbnQpCiAgICAgQVNTRVJUKCFtX3Njb3BlZEF1dGhvclN0eWxlcy5pc0Vt cHR5KCkpOwogCiAgICAgbV9zY29wZVN0YWNrLnNocmluaygwKTsKLSAgICBmb3IgKDsgcGFyZW50 OyBwYXJlbnQgPSBwYXJlbnQtPnBhcmVudE9ySG9zdE5vZGUoKSkgewotICAgICAgICBSdWxlU2V0 KiBydWxlU2V0ID0gcnVsZVNldEZvclNjb3BlKHBhcmVudCk7CisgICAgZm9yIChjb25zdCBDb250 YWluZXJOb2RlKiBzY29wZSA9IHBhcmVudDsgc2NvcGU7IHNjb3BlID0gc2NvcGUtPnBhcmVudE9y SG9zdE5vZGUoKSkgeworICAgICAgICBSdWxlU2V0KiBydWxlU2V0ID0gcnVsZVNldEZvclNjb3Bl KHNjb3BlKTsKICAgICAgICAgaWYgKHJ1bGVTZXQpCi0gICAgICAgICAgICBtX3Njb3BlU3RhY2su YXBwZW5kKFNjb3BlU3RhY2tGcmFtZShwYXJlbnQsIHJ1bGVTZXQpKTsKKyAgICAgICAgICAgIG1f c2NvcGVTdGFjay5hcHBlbmQoU2NvcGVTdGFja0ZyYW1lKHNjb3BlLCBydWxlU2V0KSk7CiAgICAg fQogICAgIG1fc2NvcGVTdGFjay5yZXZlcnNlKCk7CiAgICAgbV9zY29wZVN0YWNrUGFyZW50ID0g cGFyZW50OwpAQCAtNTkzLDcgKzU5Myw4IEBAIHZvaWQgU3R5bGVSZXNvbHZlcjo6cG9wU2NvcGUo Y29uc3QgQ29udGFpbmVyTm9kZSogc2NvcGUpCiB7CiAgICAgLy8gT25seSBib3RoZXIgdG8gdXBk YXRlIHRoZSBzY29waW5nIGVsZW1lbnQgc3RhY2sgaWYgaXQgaXMgY29uc2lzdGVudC4KICAgICBp ZiAoc2NvcGVTdGFja0lzQ29uc2lzdGVudChzY29wZSkpIHsKLSAgICAgICAgbV9zY29wZVN0YWNr LnJlbW92ZUxhc3QoKTsKKyAgICAgICAgaWYgKCFtX3Njb3BlU3RhY2suaXNFbXB0eSgpICYmIG1f c2NvcGVTdGFjay5sYXN0KCkubV9zY29wZSA9PSBzY29wZSkKKyAgICAgICAgICAgIG1fc2NvcGVT dGFjay5yZW1vdmVMYXN0KCk7CiAgICAgICAgIG1fc2NvcGVTdGFja1BhcmVudCA9IHNjb3BlLT5w YXJlbnRPckhvc3ROb2RlKCk7CiAgICAgfQogfQo=