135285 2014-07-25 02:13:13 -0700 There are unnecessary condition check on FontGlyphs.cpp 2014-07-27 23:29:06 -0700 1 1 1 Unclassified WebKit Text 528+ (Nightly build) Unspecified Unspecified RESOLVED INVALID P2 Minor --- 1 byeongha.cho webkit-unassigned commit-queue mmaxfield oldest_to_newest 1024805 0 byeongha.cho 2014-07-25 02:13:13 -0700 There are condition check for characterFontData followed by same condition check for characterFontData in the FontGlyphs::glyphDataAndPageForCharacter(). I think this is not necessary. 1024810 1 235507 byeongha.cho 2014-07-25 04:45:54 -0700 Created attachment 235507 Patch 1024824 2 235507 mmaxfield 2014-07-25 08:29:48 -0700 Comment on attachment 235507 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235507&action=review > Source/WebCore/platform/graphics/FontGlyphs.cpp:389 > characterFontData = characterFontData->variantFontData(description, variant); This function might return nullptr. 1024825 3 mmaxfield 2014-07-25 08:30:02 -0700 I'm not a reviewer but I would r- this. 1025053 4 byeongha.cho 2014-07-27 07:23:23 -0700 Yes, I agree that variantFontData() could rarely return nullptr. So I close this issue. 1025054 5 byeongha.cho 2014-07-27 07:40:38 -0700 Thank you for reviewing. 1025107 6 235507 darin 2014-07-27 23:28:49 -0700 Comment on attachment 235507 Patch As Myles says, this patch is only correct if variantFontData never returns null. Did you prove that variantFontData will never return null? 235507 2014-07-25 04:45:54 -0700 2014-07-27 23:29:06 -0700 Patch bug-135285-20140725204539.patch text/plain 1713 byeongha.cho U3VidmVyc2lvbiBSZXZpc2lvbjogMTcwODIyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZGY2NDAzZDMwNDdjYzQ4 ODg1MTQ0MDc0NzY1MGNjYWQ1NDQxNTk4Yy4uMGVhNTllOTJmZjA2NjM2NmU1NjA1MDAwZjZlYjBi YTkzMWI5NjM2YSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDE0LTA3LTI1ICBCeWVv bmdoYSBDaG8gIDxieWVvbmdoYS5jaG9Ac2Ftc3VuZy5jb20+CisKKyAgICAgICAgUmVtb3ZlIGR1 cGxpY2F0ZWQgY29uZGl0aW9uIGNoZWNrIGZvciBjaGFyYWN0ZXJGb250RGF0YS4KKyAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEzNTI4NQorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE5vIG5ldyBjaGFuZ2UgaW4g ZnVuY3Rpb25hbGl0eS4gbm8gbmV3IHRlc3RzLgorCisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhp Y3MvRm9udEdseXBocy5jcHA6CisgICAgICAgIChXZWJDb3JlOjpGb250R2x5cGhzOjpnbHlwaERh dGFBbmRQYWdlRm9yQ2hhcmFjdGVyKToKKwogMjAxNC0wNy0wNCAgQmVuamFtaW4gUG91bGFpbiAg PGJlbmphbWluQHdlYmtpdC5vcmc+CiAKICAgICAgICAgQ1NTIEpJVDogY29tcGlsZSA6aW4tcmFu Z2UgYW5kIDpvdXQtb2YtcmFuZ2UKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3Jt L2dyYXBoaWNzL0ZvbnRHbHlwaHMuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhp Y3MvRm9udEdseXBocy5jcHAKaW5kZXggOTQ4N2NjMGEwOGIxY2RiMDA4YmQxYzFkZDM2ZjBkMTA0 YTRkZDJmNy4uYTQ2YmQyOTM5NmNkMGM2NmM0NTI1NGZlMDk0NDJjY2MwZjg2NDEwNSAxMDA2NDQK LS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvRm9udEdseXBocy5jcHAKKysr IGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvRm9udEdseXBocy5jcHAKQEAgLTM4 Nyw4ICszODcsNyBAQCBzdGQ6OnBhaXI8R2x5cGhEYXRhLCBHbHlwaFBhZ2UqPiBGb250R2x5cGhz OjpnbHlwaERhdGFBbmRQYWdlRm9yQ2hhcmFjdGVyKGNvbnN0CiAgICAgICAgICAgICB2YXJpYW50 ID0gQnJva2VuSWRlb2dyYXBoVmFyaWFudDsKICAgICAgICAgaWYgKHZhcmlhbnQgIT0gTm9ybWFs VmFyaWFudCkKICAgICAgICAgICAgIGNoYXJhY3RlckZvbnREYXRhID0gY2hhcmFjdGVyRm9udERh dGEtPnZhcmlhbnRGb250RGF0YShkZXNjcmlwdGlvbiwgdmFyaWFudCk7Ci0gICAgfQotICAgIGlm IChjaGFyYWN0ZXJGb250RGF0YSkgeworCiAgICAgICAgIC8vIEdvdCB0aGUgZmFsbGJhY2sgZ2x5 cGggYW5kIGZvbnQuCiAgICAgICAgIEdseXBoUGFnZSogZmFsbGJhY2tQYWdlID0gR2x5cGhQYWdl VHJlZU5vZGU6OmdldFJvb3RDaGlsZChjaGFyYWN0ZXJGb250RGF0YS5nZXQoKSwgcGFnZU51bWJl ciktPnBhZ2UoKTsKICAgICAgICAgR2x5cGhEYXRhIGRhdGEgPSBmYWxsYmFja1BhZ2UgJiYgZmFs bGJhY2tQYWdlLT5mb250RGF0YUZvckNoYXJhY3RlcihjKSA/IGZhbGxiYWNrUGFnZS0+Z2x5cGhE YXRhRm9yQ2hhcmFjdGVyKGMpIDogY2hhcmFjdGVyRm9udERhdGEtPm1pc3NpbmdHbHlwaERhdGEo KTsK