74435 2011-12-13 12:05:32 -0800 inline setting m_regionForStyling since region is rarely set 2011-12-13 15:53:25 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 74141 1 tony tony darin dglazkov kling macpherson webkit.review.bot oldest_to_newest 520266 0 tony 2011-12-13 12:05:32 -0800 inline setting m_regionForStyling since region is rarely set 520269 1 119059 tony 2011-12-13 12:06:08 -0800 Created attachment 119059 Patch 520355 2 119059 webkit.review.bot 2011-12-13 13:51:54 -0800 Comment on attachment 119059 Patch Attachment 119059 did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10844442 New failing tests: media/event-attributes.html 520423 3 tony 2011-12-13 15:38:24 -0800 Committed r102712: <http://trac.webkit.org/changeset/102712> 520429 4 darin 2011-12-13 15:40:46 -0800 What does “since region is rarely set” mean? This change is opaque to me. 520432 5 tony 2011-12-13 15:42:22 -0800 (In reply to comment #4) > What does “since region is rarely set” mean? This change is opaque to me. Region is NULL unless there are CSS Regions in the page. Do you want me to rewrite the ChangeLog entry? 520434 6 darin 2011-12-13 15:46:17 -0800 (In reply to comment #5) > (In reply to comment #4) > > What does “since region is rarely set” mean? This change is opaque to me. > > Region is NULL unless there are CSS Regions in the page. Do you want me to rewrite the ChangeLog entry? Probably not worth it now, but there’s just too much left unsaid in that note. A good comment would be more like: Inline all of initForRegionStyling except for the rarely used part for non-null regions. 520439 7 kling 2011-12-13 15:50:11 -0800 (In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > What does “since region is rarely set” mean? This change is opaque to me. > > > > Region is NULL unless there are CSS Regions in the page. Do you want me to rewrite the ChangeLog entry? > > Probably not worth it now, but there’s just too much left unsaid in that note. A good comment would be more like: > > Inline all of initForRegionStyling except for the rarely used part for non-null regions. /me makes a mental note to increase his standards for ChangeLog entries. Given the code change, I thought this was fairly straightforward, but that comment is indeed more clear. 520441 8 tony 2011-12-13 15:51:31 -0800 ChangeLog updated in http://trac.webkit.org/changeset/102714. 520444 9 darin 2011-12-13 15:53:25 -0800 A comment to an even higher standard would say why the inlining is good. Such as “sped things up measurably” or “I am sure this is faster because the function is called in only one place” or whatever. Anyway, no need to try to polish this more. 119059 2011-12-13 12:06:08 -0800 2011-12-13 15:33:47 -0800 Patch bug-74435-20111213120621.patch text/plain 2414 tony U3VidmVyc2lvbiBSZXZpc2lvbjogMTAyNjg0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOWQ0M2MyMWZjM2UyMjRk NTg5ZjdiYjRkYWIzNjJmYTNmYzc1ZWJhNS4uMGEwMmQ0MzFmZWMwYmU0MzE0NmEwMTQxYjJmMmZh ODA3OTQ4NDlmZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDExLTEyLTEzICBUb255 IENoYW5nICA8dG9ueUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgaW5saW5lIHNldHRpbmcgbV9y ZWdpb25Gb3JTdHlsaW5nIHNpbmNlIHJlZ2lvbiBpcyByYXJlbHkgc2V0CisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03NDQzNQorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogY3NzL0NTU1N0eWxlU2VsZWN0b3Iu Y3BwOgorICAgICAgICAoV2ViQ29yZTo6Q1NTU3R5bGVTZWxlY3Rvcjo6aW5pdEZvclJlZ2lvblN0 eWxpbmcpOgorICAgICAgICAoV2ViQ29yZTo6Q1NTU3R5bGVTZWxlY3Rvcjo6aW5pdFJlZ2lvblJ1 bGVzKToKKyAgICAgICAgKiBjc3MvQ1NTU3R5bGVTZWxlY3Rvci5oOgorCiAyMDExLTEyLTEzICBU b3IgQXJuZSBWZXN0YsO4ICA8dG9yLmFybmUudmVzdGJvQG5va2lhLmNvbT4KIAogICAgICAgICBb UXRdIEdldCByaWQgb2YgbGF5ZXJpbmcgdmlvbGF0aW9ucyBpbiBpbmNsdWRlcwpkaWZmIC0tZ2l0 IGEvU291cmNlL1dlYkNvcmUvY3NzL0NTU1N0eWxlU2VsZWN0b3IuY3BwIGIvU291cmNlL1dlYkNv cmUvY3NzL0NTU1N0eWxlU2VsZWN0b3IuY3BwCmluZGV4IDk1YTRlM2M2OGY2Nzg3OTg3MzU0Y2Zi NWJkNjlkZDM5YTBmZWRhNWIuLjAxMWEwOGRjM2FmMWM5M2U3NmE0YmQ5NTVmZWMxMmNiMzM1ZTU3 OGMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2Nzcy9DU1NTdHlsZVNlbGVjdG9yLmNwcAor KysgYi9Tb3VyY2UvV2ViQ29yZS9jc3MvQ1NTU3R5bGVTZWxlY3Rvci5jcHAKQEAgLTg3MCwxMyAr ODcwLDE3IEBAIGlubGluZSB2b2lkIENTU1N0eWxlU2VsZWN0b3I6OmluaXRGb3JTdHlsZVJlc29s dmUoRWxlbWVudCogZSwgUmVuZGVyU3R5bGUqIHBhcmVuCiAgICAgbV9mb250RGlydHkgPSBmYWxz ZTsKIH0KIAotdm9pZCBDU1NTdHlsZVNlbGVjdG9yOjppbml0Rm9yUmVnaW9uU3R5bGluZyhSZW5k ZXJSZWdpb24qIHJlZ2lvbikKK2lubGluZSB2b2lkIENTU1N0eWxlU2VsZWN0b3I6OmluaXRGb3JS ZWdpb25TdHlsaW5nKFJlbmRlclJlZ2lvbiogcmVnaW9uKQogewogICAgIHNldFJlZ2lvbkZvclN0 eWxpbmcocmVnaW9uKTsKIAotICAgIGlmICghcmVnaW9uKQotICAgICAgICByZXR1cm47CisgICAg aWYgKHJlZ2lvbikKKyAgICAgICAgaW5pdFJlZ2lvblJ1bGVzKHJlZ2lvbik7Cit9CiAKK3ZvaWQg Q1NTU3R5bGVTZWxlY3Rvcjo6aW5pdFJlZ2lvblJ1bGVzKFJlbmRlclJlZ2lvbiogcmVnaW9uKQor eworICAgIEFTU0VSVChyZWdpb24pOwogICAgIC8vIE1hcmsgdGhhdCB0aGUgc2V0IG9mIHJ1bGVz IGNvbWVzIGZyb20gcmVnaW9uIHN0eWxpbmcgc2luY2Ugd2UgbmVlZCB0byBmaWx0ZXIKICAgICAv LyB0aGUgcHJvcGVydGllcyB0aGF0IGNhbiBiZSBhcHBsaWVkLgogICAgIG1fcmVnaW9uUnVsZXMg PSBhZG9wdFB0cihuZXcgUnVsZVNldCh0cnVlKSk7CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29y ZS9jc3MvQ1NTU3R5bGVTZWxlY3Rvci5oIGIvU291cmNlL1dlYkNvcmUvY3NzL0NTU1N0eWxlU2Vs ZWN0b3IuaAppbmRleCBiODM2Yjg2MTMyNzI3MmUxNTgxNmViZWZjY2FlZjUwMmU0YzgyZTcxLi5m ODE5ZTA1NjYxNTE0ZjI2MGM3MTBiZmIxM2VhMmE3ZTZiMWE5ODRiIDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViQ29yZS9jc3MvQ1NTU3R5bGVTZWxlY3Rvci5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL2Nz cy9DU1NTdHlsZVNlbGVjdG9yLmgKQEAgLTEzMSw2ICsxMzEsNyBAQCBwcml2YXRlOgogICAgIHZv aWQgaW5pdEZvclN0eWxlUmVzb2x2ZShFbGVtZW50KiwgUmVuZGVyU3R5bGUqIHBhcmVudFN0eWxl ID0gMCwgUHNldWRvSWQgPSBOT1BTRVVETyk7CiAgICAgdm9pZCBpbml0RWxlbWVudChFbGVtZW50 Kik7CiAgICAgdm9pZCBpbml0Rm9yUmVnaW9uU3R5bGluZyhSZW5kZXJSZWdpb24qKTsKKyAgICB2 b2lkIGluaXRSZWdpb25SdWxlcyhSZW5kZXJSZWdpb24qKTsKICAgICBSZW5kZXJTdHlsZSogbG9j YXRlU2hhcmVkU3R5bGUoKTsKICAgICBib29sIG1hdGNoZXNSdWxlU2V0KFJ1bGVTZXQqKTsKICAg ICBOb2RlKiBsb2NhdGVDb3VzaW5MaXN0KEVsZW1lbnQqIHBhcmVudCwgdW5zaWduZWQmIHZpc2l0 ZWROb2RlQ291bnQpIGNvbnN0Owo=