Bug 157440 – Download progress on attachment elements sometimes exceeds element bounds

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Download progress on attachment elements sometimes exceeds element bounds

[Tim Horton]

Created attachment 278298 [details]
Patch

[Tim Horton]

<rdar://problem/25245440>

[Darin Adler]

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

[Tim Horton]

(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.

[Tim Horton]

http://trac.webkit.org/changeset/200582