155482 2016-03-14 18:56:38 -0700 <attachment> on iOS should paint its progress indicator instead of a green square 2016-03-16 14:47:05 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 thorton thorton commit-queue enrica esprehn+autocc glenn kondapallykalyan sam simon.fraser oldest_to_newest 1174869 0 thorton 2016-03-14 18:56:38 -0700 <attachment> on iOS should paint its progress indicator instead of a green square 1174870 1 274068 thorton 2016-03-14 18:57:12 -0700 Created attachment 274068 Patch 1174888 2 thorton 2016-03-14 21:10:42 -0700 http://trac.webkit.org/changeset/198191 1174890 3 274068 dbates 2016-03-14 21:17:34 -0700 Comment on attachment 274068 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274068&action=review I know that this was already reviewed and landed. I had a suggestion to improve the readability of this code. > Source/WebCore/rendering/RenderThemeIOS.mm:1619 > + progressPath.addArc(center, info.progressRect.width() / 2, -M_PI_2, info.progress * 2 * M_PI - M_PI_2, 0); The last parameter of Path::addArc() is a boolean as to whether the arc should be drawn clockwise. Although 0 will be implicitly cast to boolean false I suggest that we either define a local boolean variable, say clockwise, that has value false and pass this variable for the last argument of Path::addArc() or pass false and add some kind of inline comment that conveys that the purpose of this argument is to draw the arc counterclockwise. 1174892 4 thorton 2016-03-14 21:22:40 -0700 (In reply to comment #3) > Comment on attachment 274068 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=274068&action=review > > I know that this was already reviewed and landed. I had a suggestion to > improve the readability of this code. > > > Source/WebCore/rendering/RenderThemeIOS.mm:1619 > > + progressPath.addArc(center, info.progressRect.width() / 2, -M_PI_2, info.progress * 2 * M_PI - M_PI_2, 0); > > The last parameter of Path::addArc() is a boolean as to whether the arc > should be drawn clockwise. Although 0 will be implicitly cast to boolean > false I suggest that we either define a local boolean variable, say > clockwise, that has value false and pass this variable for the last argument > of Path::addArc() That seems a bit excessive. > or pass false and add some kind of inline comment that > conveys that the purpose of this argument is to draw the arc > counterclockwise. More reasonable, but the proper fix would be to change addArc to take an enum instead of a boolean. 1175505 5 274068 darin 2016-03-16 14:47:05 -0700 Comment on attachment 274068 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274068&action=review > Source/WebCore/rendering/RenderThemeIOS.mm:1481 > + progress = std::max<float>(std::min<float>(progressString.toFloat(&validProgress), 1), 0); Tiny style comment: When clamping like this I like to put the lower bound on the left and the upper bound on the right, like this: progress = std::max(0.f, std::min(progressString.toFloat(&validProgress), 1.f)); 274068 2016-03-14 18:57:12 -0700 2016-03-14 20:55:00 -0700 Patch bug-155482-20160314185701.patch text/plain 3244 thorton U3VidmVyc2lvbiBSZXZpc2lvbjogMTk4MTU1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMWFmYTA4ZjFhZjg4ZjM5 N2Y2MzI0ZThmMzBjOTU4ZDEzZTNjNzFkOS4uMzJhOWFmZDg2ZjVmODE0MTA1NGM4NGRiY2ZjMGZk NGFjMDRhZTUxMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDE2LTAzLTE0ICBUaW0g SG9ydG9uICA8dGltb3RoeV9ob3J0b25AYXBwbGUuY29tPgorCisgICAgICAgIDxhdHRhY2htZW50 PiBvbiBpT1Mgc2hvdWxkIHBhaW50IGl0cyBwcm9ncmVzcyBpbmRpY2F0b3IgaW5zdGVhZCBvZiBh IGdyZWVuIHNxdWFyZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTU1NDgyCisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8yNDgwNTk5MT4KKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBuZXcgdGVzdHM7IHRo ZXJlIGFyZSBleGlzdGluZyB0ZXN0cyB0aGF0IHdpbGwgYmUgZW5hYmxlZCBzaG9ydGx5LgorCisg ICAgICAgICogcmVuZGVyaW5nL1JlbmRlclRoZW1lSU9TLm1tOgorICAgICAgICAoV2ViQ29yZTo6 Z2V0QXR0YWNobWVudFByb2dyZXNzKToKKyAgICAgICAgQ2xhbXAgcHJvZ3Jlc3MgdG8gMC0xLgor CisgICAgICAgIChXZWJDb3JlOjpwYWludEF0dGFjaG1lbnRQcm9ncmVzcyk6CisgICAgICAgIFBh aW50IGEgcGllLgorCiAyMDE2LTAzLTE0ICBQZXIgQXJuZSBWb2xsYW4gIDxwZWF2b0BvdXRsb29r LmNvbT4KIAogICAgICAgICBbV2luQ2Fpcm9dW01lZGlhRm91bmRhdGlvbl0gSW1wbGVtZW50IGZs b2F0IHZlcnNpb25zIG9mIE1lZGlhUGxheWVyIG1ldGhvZHMuCmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGhlbWVJT1MubW0gYi9Tb3VyY2UvV2ViQ29yZS9yZW5k ZXJpbmcvUmVuZGVyVGhlbWVJT1MubW0KaW5kZXggZGU5MGZhZWM0YmJlMmJjZDQzMmU5NWJkMzQ1 MTI5NzJiNzkwMWNmYS4uNTI4YjE0MzgzZTUxYzBiYTQwMWI1ZTI1MzVhNGQ2OTk5NjIwZWMyZCAx MDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclRoZW1lSU9TLm1tCisr KyBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJUaGVtZUlPUy5tbQpAQCAtMTM0MCw2 ICsxMzQwLDkgQEAgY29uc3QgQ0dTaXplIGF0dGFjaG1lbnRTaXplID0geyAxNjAsIDExOSB9Owog Y29uc3QgQ0dGbG9hdCBhdHRhY2htZW50Qm9yZGVyUmFkaXVzID0gMTY7CiBzdGF0aWMgQ29sb3Ig YXR0YWNobWVudEJvcmRlckNvbG9yKCkgeyByZXR1cm4gQ29sb3IoMjA0LCAyMDQsIDIwNCk7IH0K IAorc3RhdGljIENvbG9yIGF0dGFjaG1lbnRQcm9ncmVzc0NvbG9yKCkgeyByZXR1cm4gQ29sb3Io MjIyLCAyMjIsIDIyMik7IH0KK2NvbnN0IENHRmxvYXQgYXR0YWNobWVudFByb2dyZXNzQm9yZGVy VGhpY2tuZXNzID0gMzsKKwogY29uc3QgQ0dGbG9hdCBhdHRhY2htZW50UHJvZ3Jlc3NTaXplID0g MzY7CiBjb25zdCBDR0Zsb2F0IGF0dGFjaG1lbnRJY29uU2l6ZSA9IDQ4OwogCkBAIC0xNDc1LDcg KzE0NzgsNyBAQCBzdGF0aWMgQk9PTCBnZXRBdHRhY2htZW50UHJvZ3Jlc3MoY29uc3QgUmVuZGVy QXR0YWNobWVudCYgYXR0YWNobWVudCwgZmxvYXQmIHBybwogICAgIGlmIChwcm9ncmVzc1N0cmlu Zy5pc0VtcHR5KCkpCiAgICAgICAgIHJldHVybiBOTzsKICAgICBib29sIHZhbGlkUHJvZ3Jlc3M7 Ci0gICAgcHJvZ3Jlc3MgPSBwcm9ncmVzc1N0cmluZy50b0Zsb2F0KCZ2YWxpZFByb2dyZXNzKTsK KyAgICBwcm9ncmVzcyA9IHN0ZDo6bWF4PGZsb2F0PihzdGQ6Om1pbjxmbG9hdD4ocHJvZ3Jlc3NT dHJpbmcudG9GbG9hdCgmdmFsaWRQcm9ncmVzcyksIDEpLCAwKTsKICAgICByZXR1cm4gdmFsaWRQ cm9ncmVzczsKIH0KIApAQCAtMTU4Niw3ICsxNTg5LDYgQEAgc3RhdGljIHZvaWQgcGFpbnRBdHRh Y2htZW50SWNvbihHcmFwaGljc0NvbnRleHQmIGNvbnRleHQsIEF0dGFjaG1lbnRJbmZvJiBpbmZv KQogICAgIGNvbnRleHQuZHJhd0ltYWdlKCppY29uSW1hZ2UsIGluZm8uaWNvblJlY3QpOwogfQog Ci0KIHN0YXRpYyB2b2lkIHBhaW50QXR0YWNobWVudFRleHQoR3JhcGhpY3NDb250ZXh0JiBjb250 ZXh0LCBBdHRhY2htZW50SW5mbyYgaW5mbykKIHsKICAgICBmb3IgKGNvbnN0IGF1dG8mIGxpbmUg OiBpbmZvLmxpbmVzKSB7CkBAIC0xNjA0LDggKzE2MDYsMTkgQEAgc3RhdGljIHZvaWQgcGFpbnRB dHRhY2htZW50UHJvZ3Jlc3MoR3JhcGhpY3NDb250ZXh0JiBjb250ZXh0LCBBdHRhY2htZW50SW5m byYgaW4KIHsKICAgICBHcmFwaGljc0NvbnRleHRTdGF0ZVNhdmVyIHNhdmVyKGNvbnRleHQpOwog Ci0gICAgLy8gRklYTUU6IEltcGxlbWVudCBwcm9ncmVzcyBpbmRpY2F0b3IuCi0gICAgY29udGV4 dC5maWxsUmVjdChpbmZvLnByb2dyZXNzUmVjdCwgQ29sb3IoMCwgMjU1LCAwKSk7CisgICAgY29u dGV4dC5zZXRTdHJva2VUaGlja25lc3MoYXR0YWNobWVudFByb2dyZXNzQm9yZGVyVGhpY2tuZXNz KTsKKyAgICBjb250ZXh0LnNldFN0cm9rZUNvbG9yKGF0dGFjaG1lbnRQcm9ncmVzc0NvbG9yKCkp OworICAgIGNvbnRleHQuc2V0RmlsbENvbG9yKGF0dGFjaG1lbnRQcm9ncmVzc0NvbG9yKCkpOwor ICAgIGNvbnRleHQuc3Ryb2tlRWxsaXBzZShpbmZvLnByb2dyZXNzUmVjdCk7CisKKyAgICBGbG9h dFBvaW50IGNlbnRlciA9IGluZm8ucHJvZ3Jlc3NSZWN0LmNlbnRlcigpOworCisgICAgUGF0aCBw cm9ncmVzc1BhdGg7CisgICAgcHJvZ3Jlc3NQYXRoLm1vdmVUbyhjZW50ZXIpOworICAgIHByb2dy ZXNzUGF0aC5hZGRMaW5lVG8oRmxvYXRQb2ludChjZW50ZXIueCgpLCBpbmZvLnByb2dyZXNzUmVj dC55KCkpKTsKKyAgICBwcm9ncmVzc1BhdGguYWRkQXJjKGNlbnRlciwgaW5mby5wcm9ncmVzc1Jl Y3Qud2lkdGgoKSAvIDIsIC1NX1BJXzIsIGluZm8ucHJvZ3Jlc3MgKiAyICogTV9QSSAtIE1fUElf MiwgMCk7CisgICAgcHJvZ3Jlc3NQYXRoLmNsb3NlU3VicGF0aCgpOworICAgIGNvbnRleHQuZmls bFBhdGgocHJvZ3Jlc3NQYXRoKTsKIH0KIAogc3RhdGljIHZvaWQgcGFpbnRBdHRhY2htZW50Qm9y ZGVyKEdyYXBoaWNzQ29udGV4dCYgY29udGV4dCwgQXR0YWNobWVudEluZm8mIGluZm8pCg==