157440 2016-05-06 17:39:21 -0700 Download progress on attachment elements sometimes exceeds element bounds 2016-05-09 11:05:03 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 thorton thorton andersca commit-queue esprehn+autocc glenn kondapallykalyan simon.fraser oldest_to_newest 1190845 0 thorton 2016-05-06 17:39:21 -0700 Download progress on attachment elements sometimes exceeds element bounds 1190846 1 278298 thorton 2016-05-06 17:39:52 -0700 Created attachment 278298 Patch 1190847 2 thorton 2016-05-06 17:43:07 -0700 <rdar://problem/25245440> 1190964 3 278298 darin 2016-05-07 12:47:41 -0700 Comment on attachment 278298 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278298&action=review > Source/WebCore/rendering/RenderThemeIOS.mm:1400 > + void buildTitleLines(const RenderAttachment&, unsigned maximumLines); Not a great argument name; its name is "maximum lines", but its value is a maximum number of lines or a count of lines. I find that slight cognitive dissonance a little annoying. > Source/WebCore/rendering/RenderThemeIOS.mm:1449 > + for (; lineIndex < std::min(maximumLines - 1, static_cast<unsigned>(lineCount)); ++lineIndex) { Do have some guarantee that maximumLines is not zero? Looks like this fails to compile on iOS because CFIndex is a signed type. So should probably write something more like this: std::min<CFIndex>(maximumLines - 1, lineCount) Maybe also compute this limit outside the loop? > Source/WebCore/rendering/RenderThemeIOS.mm:1451 > CTLineRef line = (CTLineRef)CFArrayGetValueAtIndex(ctLines, lineIndex); > addLine(line); Better without the local variable I think. > Source/WebCore/rendering/RenderThemeIOS.mm:1576 > + BOOL forceSingleLineTitle = !action.isEmpty() || !subtitle.isEmpty() || hasProgress; bool, not BOOL, please 1191199 4 thorton 2016-05-09 09:52:34 -0700 (In reply to comment #3) > Comment on attachment 278298 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=278298&action=review > > > Source/WebCore/rendering/RenderThemeIOS.mm:1400 > > + void buildTitleLines(const RenderAttachment&, unsigned maximumLines); > > Not a great argument name; its name is "maximum lines", but its value is a > maximum number of lines or a count of lines. I find that slight cognitive > dissonance a little annoying. Will rename! > > Source/WebCore/rendering/RenderThemeIOS.mm:1449 > > + for (; lineIndex < std::min(maximumLines - 1, static_cast<unsigned>(lineCount)); ++lineIndex) { > > Do have some guarantee that maximumLines is not zero? We do, at the callsite; it's only ever 1 or 2. > Looks like this fails to compile on iOS because CFIndex is a signed type. So > should probably write something more like this: > > std::min<CFIndex>(maximumLines - 1, lineCount) OK. > Maybe also compute this limit outside the loop? Sure. > > Source/WebCore/rendering/RenderThemeIOS.mm:1451 > > CTLineRef line = (CTLineRef)CFArrayGetValueAtIndex(ctLines, lineIndex); > > addLine(line); > > Better without the local variable I think. > > > Source/WebCore/rendering/RenderThemeIOS.mm:1576 > > + BOOL forceSingleLineTitle = !action.isEmpty() || !subtitle.isEmpty() || hasProgress; > > bool, not BOOL, please I've been writing too much ObjC. 1191220 5 thorton 2016-05-09 11:05:03 -0700 http://trac.webkit.org/changeset/200582 278298 2016-05-06 17:39:52 -0700 2016-05-07 12:47:41 -0700 Patch bug-157440-20160506174034.patch text/plain 5337 thorton U3VidmVyc2lvbiBSZXZpc2lvbjogMjAwNTEyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZWM0NDM2MzFmZjYxZDFj ZjdlYzdlNGM4ZTE2OGRiMTM5M2I2MTU1Yy4uMWE3MWE2ZGY5ZWY1NThlNmJjNDg3ZmQ4YjJmOTk5 YzY3MDRhNjRkNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDMxIEBACiAyMDE2LTA1LTA2ICBUaW0g SG9ydG9uICA8dGltb3RoeV9ob3J0b25AYXBwbGUuY29tPgogCisgICAgICAgIERvd25sb2FkIHBy b2dyZXNzIG9uIGF0dGFjaG1lbnQgZWxlbWVudHMgc29tZXRpbWVzIGV4Y2VlZHMgZWxlbWVudCBi b3VuZHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1 NzQ0MAorICAgICAgICA8cmRhcjovL3Byb2JsZW0vMjUyNDU0NDA+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSW4gdGhlIGNhc2Ugb2YgdmVyeSBsYXJn ZSBkeW5hbWljIHR5cGUgc2l6ZXMsIHdlIGNhbiBvdmVyZmxvdyB0aGUKKyAgICAgICAgYm91bmRz IG9mIHRoZSBhdHRhY2htZW50LiBJdCB0dXJucyBvdXQgdGhhdCB3ZSB1c2VkIHRvIGxpbWl0IHRv IG9uZQorICAgICAgICBsaW5lIGluIG1hbnkgY2FzZXMgYW55d2F5LCBzbyBvbmx5IHdyYXAgaWYg d2UgaGF2ZSBvbmx5IGEgdGl0bGUgYW5kL29yIGljb24uCisgICAgICAgIFRoaXMgbG9va3MgYmV0 dGVyIHdoZW4geW91IGhhdmUgbWFueSBvZiB0aGUgb3RoZXIgZWxlbWVudHMgZXZlbiBpbgorICAg ICAgICBub24tbGFyZ2UtdHlwZSBtb2Rlcy4KKworICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJU aGVtZUlPUy5tbToKKyAgICAgICAgKFdlYkNvcmU6OkF0dGFjaG1lbnRJbmZvOjpidWlsZFRpdGxl TGluZXMpOgorICAgICAgICAoV2ViQ29yZTo6QXR0YWNobWVudEluZm86OkF0dGFjaG1lbnRJbmZv KToKKyAgICAgICAgTGltaXQgdGhlIHRpdGxlIHRvIGEgc2luZ2xlIGxpbmUgaWYgd2UgaGF2ZSBh bnl0aGluZworICAgICAgICBvdGhlciB0aGFuIGEgdGl0bGUgYW5kIGljb24uCisKKyAgICAgICAg KFdlYkNvcmU6OmF0dGFjaG1lbnRCb3JkZXJQYXRoKToKKyAgICAgICAgKFdlYkNvcmU6OnBhaW50 QXR0YWNobWVudEJvcmRlcik6CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJUaGVtZUlPUzo6cGFp bnRBdHRhY2htZW50KToKKyAgICAgICAgQ2xpcCB0byB0aGUgYm9yZGVyLCBzbyB0aGF0IGV2ZW4g aWYgc29tZWhvdyB3ZSBwYWludCBvdXRzaWRlIG9mCisgICAgICAgIHRoZSBib3VuZHMsIHdlIGRv bid0IHBhaW50IG92ZXIgb3RoZXIgcGFydHMgb2YgdGhlIHBhZ2UuCisKKzIwMTYtMDUtMDYgIFRp bSBIb3J0b24gIDx0aW1vdGh5X2hvcnRvbkBhcHBsZS5jb20+CisKICAgICAgICAgPGF0dGFjaG1l bnQ+IGVsZW1lbnQgc2hvdWxkIHVuZGVyc3RhbmQgVVRJcwogICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU3NDI1CiAgICAgICAgIDxyZGFyOi8vcHJvYmxl bS8yNTU4NTQwMT4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJU aGVtZUlPUy5tbSBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJUaGVtZUlPUy5tbQpp bmRleCBkMjAzZjAxZmI3NzM4OTgyMzYxMjBkNzZhNDI2NWRmYjViZTgwYzhlLi43MmEwNzg2ZWJh Mjc4YjEyNzljYWFlYTRhNDU5YWJhOGM5ZjNiNWFkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9yZW5kZXJpbmcvUmVuZGVyVGhlbWVJT1MubW0KKysrIGIvU291cmNlL1dlYkNvcmUvcmVuZGVy aW5nL1JlbmRlclRoZW1lSU9TLm1tCkBAIC0xMzk3LDcgKzEzOTcsNyBAQCBzdHJ1Y3QgQXR0YWNo bWVudEluZm8gewogICAgIENHRmxvYXQgY29udGVudFlPcmlnaW4geyAwIH07CiAKIHByaXZhdGU6 Ci0gICAgdm9pZCBidWlsZFRpdGxlTGluZXMoY29uc3QgUmVuZGVyQXR0YWNobWVudCYpOworICAg IHZvaWQgYnVpbGRUaXRsZUxpbmVzKGNvbnN0IFJlbmRlckF0dGFjaG1lbnQmLCB1bnNpZ25lZCBt YXhpbXVtTGluZXMpOwogICAgIHZvaWQgYnVpbGRTaW5nbGVMaW5lKGNvbnN0IFN0cmluZyYsIENU Rm9udFJlZiwgVUlDb2xvciAqKTsKIAogICAgIHZvaWQgYWRkTGluZShDVExpbmVSZWYpOwpAQCAt MTQxOCw3ICsxNDE4LDcgQEAgdm9pZCBBdHRhY2htZW50SW5mbzo6YWRkTGluZShDVExpbmVSZWYg bGluZSkKICAgICBsaW5lcy5hcHBlbmQobGFiZWxMaW5lKTsKIH0KIAotdm9pZCBBdHRhY2htZW50 SW5mbzo6YnVpbGRUaXRsZUxpbmVzKGNvbnN0IFJlbmRlckF0dGFjaG1lbnQmIGF0dGFjaG1lbnQp Cit2b2lkIEF0dGFjaG1lbnRJbmZvOjpidWlsZFRpdGxlTGluZXMoY29uc3QgUmVuZGVyQXR0YWNo bWVudCYgYXR0YWNobWVudCwgdW5zaWduZWQgbWF4aW11bUxpbmVzKQogewogICAgIFJldGFpblB0 cjxDVEZvbnRSZWY+IGZvbnQgPSBhdHRhY2htZW50VGl0bGVGb250KCk7CiAKQEAgLTE0NDQsOSAr MTQ0NCw5IEBAIHZvaWQgQXR0YWNobWVudEluZm86OmJ1aWxkVGl0bGVMaW5lcyhjb25zdCBSZW5k ZXJBdHRhY2htZW50JiBhdHRhY2htZW50KQogICAgIGlmICghbGluZUNvdW50KQogICAgICAgICBy ZXR1cm47CiAKLSAgICAvLyBMYXkgb3V0IGFuZCByZWNvcmQgdGhlIGZpcnN0IChhdHRhY2htZW50 VGl0bGVNYXhpbXVtTGluZUNvdW50IC0gMSkgbGluZXMuCisgICAgLy8gTGF5IG91dCBhbmQgcmVj b3JkIHRoZSBmaXJzdCAobWF4aW11bUxpbmVzIC0gMSkgbGluZXMuCiAgICAgQ0ZJbmRleCBsaW5l SW5kZXggPSAwOwotICAgIGZvciAoOyBsaW5lSW5kZXggPCBzdGQ6Om1pbihhdHRhY2htZW50VGl0 bGVNYXhpbXVtTGluZUNvdW50IC0gMSwgbGluZUNvdW50KTsgKytsaW5lSW5kZXgpIHsKKyAgICBm b3IgKDsgbGluZUluZGV4IDwgc3RkOjptaW4obWF4aW11bUxpbmVzIC0gMSwgc3RhdGljX2Nhc3Q8 dW5zaWduZWQ+KGxpbmVDb3VudCkpOyArK2xpbmVJbmRleCkgewogICAgICAgICBDVExpbmVSZWYg bGluZSA9IChDVExpbmVSZWYpQ0ZBcnJheUdldFZhbHVlQXRJbmRleChjdExpbmVzLCBsaW5lSW5k ZXgpOwogICAgICAgICBhZGRMaW5lKGxpbmUpOwogICAgIH0KQEAgLTE0NTQsNyArMTQ1NCw3IEBA IHZvaWQgQXR0YWNobWVudEluZm86OmJ1aWxkVGl0bGVMaW5lcyhjb25zdCBSZW5kZXJBdHRhY2ht ZW50JiBhdHRhY2htZW50KQogICAgIGlmIChsaW5lSW5kZXggPT0gbGluZUNvdW50KQogICAgICAg ICByZXR1cm47CiAKLSAgICAvLyBXZSBoYWQgdGV4dCB0aGF0IGRpZG4ndCBmaXQgaW4gdGhlIGZp cnN0IChhdHRhY2htZW50VGl0bGVNYXhpbXVtTGluZUNvdW50IC0gMSkgbGluZXMuCisgICAgLy8g V2UgaGFkIHRleHQgdGhhdCBkaWRuJ3QgZml0IGluIHRoZSBmaXJzdCAobWF4aW11bUxpbmVzIC0g MSkgbGluZXMuCiAgICAgLy8gQ29tYmluZSBpdCBpbnRvIG9uZSBsYXN0IGxpbmUsIGFuZCBjZW50 ZXItdHJ1bmNhdGUgaXQuCiAgICAgQ1RMaW5lUmVmIGZpcnN0UmVtYWluaW5nTGluZSA9IChDVExp bmVSZWYpQ0ZBcnJheUdldFZhbHVlQXRJbmRleChjdExpbmVzLCBsaW5lSW5kZXgpOwogICAgIENG SW5kZXggcmVtYWluaW5nUmFuZ2VTdGFydCA9IENUTGluZUdldFN0cmluZ1JhbmdlKGZpcnN0UmVt YWluaW5nTGluZSkubG9jYXRpb247CkBAIC0xNTczLDcgKzE1NzMsOCBAQCBBdHRhY2htZW50SW5m bzo6QXR0YWNobWVudEluZm8oY29uc3QgUmVuZGVyQXR0YWNobWVudCYgYXR0YWNobWVudCkKICAg ICB9IGVsc2UKICAgICAgICAgYnVpbGRTaW5nbGVMaW5lKGFjdGlvbiwgYXR0YWNobWVudEFjdGlv bkZvbnQoKS5nZXQoKSwgYXR0YWNobWVudEFjdGlvbkNvbG9yKGF0dGFjaG1lbnQpKTsKIAotICAg IGJ1aWxkVGl0bGVMaW5lcyhhdHRhY2htZW50KTsKKyAgICBCT09MIGZvcmNlU2luZ2xlTGluZVRp dGxlID0gIWFjdGlvbi5pc0VtcHR5KCkgfHwgIXN1YnRpdGxlLmlzRW1wdHkoKSB8fCBoYXNQcm9n cmVzczsKKyAgICBidWlsZFRpdGxlTGluZXMoYXR0YWNobWVudCwgZm9yY2VTaW5nbGVMaW5lVGl0 bGUgPyAxIDogYXR0YWNobWVudFRpdGxlTWF4aW11bUxpbmVDb3VudCk7CiAgICAgYnVpbGRTaW5n bGVMaW5lKHN1YnRpdGxlLCBhdHRhY2htZW50U3VidGl0bGVGb250KCkuZ2V0KCksIGF0dGFjaG1l bnRTdWJ0aXRsZUNvbG9yKCkpOwogCiAgICAgaWYgKCFsaW5lcy5pc0VtcHR5KCkpIHsKQEAgLTE2 NDMsMTAgKzE2NDQsMTUgQEAgc3RhdGljIHZvaWQgcGFpbnRBdHRhY2htZW50UHJvZ3Jlc3MoR3Jh cGhpY3NDb250ZXh0JiBjb250ZXh0LCBBdHRhY2htZW50SW5mbyYgaW4KICAgICBjb250ZXh0LmZp bGxQYXRoKHByb2dyZXNzUGF0aCk7CiB9CiAKLXN0YXRpYyB2b2lkIHBhaW50QXR0YWNobWVudEJv cmRlcihHcmFwaGljc0NvbnRleHQmIGNvbnRleHQsIEF0dGFjaG1lbnRJbmZvJiBpbmZvKQorc3Rh dGljIFBhdGggYXR0YWNobWVudEJvcmRlclBhdGgoQXR0YWNobWVudEluZm8mIGluZm8pCiB7CiAg ICAgUGF0aCBib3JkZXJQYXRoOwogICAgIGJvcmRlclBhdGguYWRkUm91bmRlZFJlY3QoaW5mby5h dHRhY2htZW50UmVjdCwgRmxvYXRTaXplKGF0dGFjaG1lbnRCb3JkZXJSYWRpdXMsIGF0dGFjaG1l bnRCb3JkZXJSYWRpdXMpKTsKKyAgICByZXR1cm4gYm9yZGVyUGF0aDsKK30KKworc3RhdGljIHZv aWQgcGFpbnRBdHRhY2htZW50Qm9yZGVyKEdyYXBoaWNzQ29udGV4dCYgY29udGV4dCwgUGF0aCYg Ym9yZGVyUGF0aCkKK3sKICAgICBjb250ZXh0LnNldFN0cm9rZUNvbG9yKGF0dGFjaG1lbnRCb3Jk ZXJDb2xvcigpKTsKICAgICBjb250ZXh0LnNldFN0cm9rZVRoaWNrbmVzcygxKTsKICAgICBjb250 ZXh0LnN0cm9rZVBhdGgoYm9yZGVyUGF0aCk7CkBAIC0xNjY2LDcgKzE2NzIsOSBAQCBib29sIFJl bmRlclRoZW1lSU9TOjpwYWludEF0dGFjaG1lbnQoY29uc3QgUmVuZGVyT2JqZWN0JiByZW5kZXJl ciwgY29uc3QgUGFpbnRJbgogCiAgICAgY29udGV4dC50cmFuc2xhdGUodG9GbG9hdFNpemUocGFp bnRSZWN0LmxvY2F0aW9uKCkpKTsKIAotICAgIHBhaW50QXR0YWNobWVudEJvcmRlcihjb250ZXh0 LCBpbmZvKTsKKyAgICBQYXRoIGJvcmRlclBhdGggPSBhdHRhY2htZW50Qm9yZGVyUGF0aChpbmZv KTsKKyAgICBwYWludEF0dGFjaG1lbnRCb3JkZXIoY29udGV4dCwgYm9yZGVyUGF0aCk7CisgICAg Y29udGV4dC5jbGlwUGF0aChib3JkZXJQYXRoKTsKIAogICAgIGNvbnRleHQudHJhbnNsYXRlKEZs b2F0U2l6ZSgwLCBpbmZvLmNvbnRlbnRZT3JpZ2luKSk7CiAK