Download progress on attachment elements sometimes exceeds element bounds
Created attachment 278298 [details] Patch
<rdar://problem/25245440>
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. > 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
(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.
http://trac.webkit.org/changeset/200582