Fix a ?: precedence error in ScrollbarThemeMac and ScrollbarThemeChromium

[Nico Weber]

Fix a ?: precedence error in ScrollbarThemeMac and ScrollbarThemeChromium

[Nico Weber]

Created attachment 96438 [details]
Patch

[Darin Adler]

Comment on attachment 96438 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96438&action=review

Please post a new patch that gets the ScrollbarThemeMac.mm part right.

Anyone got information on what the symptom of this was? Clearly the old code was incorrect. But we normally want regression testing too.

> Source/WebCore/platform/mac/ScrollbarThemeMac.mm:295
> -            result = IntRect(scrollbar->x(), scrollbar->y(), cOuterButtonLength[scrollbar->controlSize()] + painting ? cOuterButtonOverlap : 0, thickness);
> +            result = IntRect(scrollbar->x(), scrollbar->y(), cOuterButtonLength[scrollbar->controlSize()] + painting ? (cOuterButtonOverlap : 0), thickness);

This looks wrong. The parentheses are added in the wrong place here!

[Nico Weber]

Comment on attachment 96438 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96438&action=review

>> Source/WebCore/platform/mac/ScrollbarThemeMac.mm:295
>> +            result = IntRect(scrollbar->x(), scrollbar->y(), cOuterButtonLength[scrollbar->controlSize()] + painting ? (cOuterButtonOverlap : 0), thickness);
> 
> This looks wrong. The parentheses are added in the wrong place here!

|painting| is a bool. What this code did before I added the parens was |(int + bool) ? overlap : 0|. I would have thought it's supposed to be |int + (bool ? overlap : 0)|, which is what this patch does.

Am I misunderstanding the code?

[Nico Weber]

Thanks for your comment, Darin. I replied to it – I'm afraid I don't get it :-/

[Darin Adler]

(In reply to comment #3)
> (From update of attachment 96438 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96438&action=review
> 
> >> Source/WebCore/platform/mac/ScrollbarThemeMac.mm:295
> >> +            result = IntRect(scrollbar->x(), scrollbar->y(), cOuterButtonLength[scrollbar->controlSize()] + painting ? (cOuterButtonOverlap : 0), thickness);
> > 
> > This looks wrong. The parentheses are added in the wrong place here!
> 
> |painting| is a bool. What this code did before I added the parens was |(int + bool) ? overlap : 0|. I would have thought it's supposed to be |int + (bool ? overlap : 0)|, which is what this patch does.
> 
> Am I misunderstanding the code?

The above says:

    painting ? (cOuterButtonOverlap : 0)

But you need to say:

    (painting ? cOuterButtonOverlap : 0)

Maybe you just had trouble seeing it.

[Nico Weber]

Created attachment 96439 [details]
Patch

[Nico Weber]

9_9

Sorry.

[Darin Adler]

Still would be good to find out what the symptom of this was.

[Nico Weber]

This codepath was hit only when

    (buttonsPlacement() == ScrollbarButtonsDoubleStart || buttonsPlacement() == ScrollbarButtonsDoubleBoth

is true which I think isn't true for most people in practice. The only interesting called of this function is RenderScrollbarTheme::trackRect(), so from reading the code it looks like for doublestart / doubleboth scrollbars, the track would appear slightly too large.

[WebKit Review Bot]

Comment on attachment 96439 [details]
Patch

Clearing flags on attachment: 96439

Committed r88364: <http://trac.webkit.org/changeset/88364>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.