1025595 - Duplicate comparison: aWidgetType == NS_THEME_WINDOW_BUTTON_MINIMIZE

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Mardeg]

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsNativeThemeWin.cpp#2482

The 'aWidgetType' variable is compared to the NS_THEME_WINDOW_BUTTON_MINIMIZE constant twice.
This is a typo: the variable should be compared to the constant NS_THEME_WINDOW_BUTTON_MAXIMIZE at the above URL.

Comment 1

[Nochum Sossonko [:Natch]]

(In reply to Mardeg from comment #0)
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/
> nsNativeThemeWin.cpp#2482

This line points to this "case":

case NS_THEME_MENUARROW:
     {
       // Use the width of the arrow glyph as padding. See the drawing
       // code for details.
       aResult->width *= 2;
       break;
     }
> The 'aWidgetType' variable is compared to the
> NS_THEME_WINDOW_BUTTON_MINIMIZE constant twice.

The function the aforementioned line points to only compares aWidgetType to NS_THEME_WINDOW_BUTTON_MINIMIZE once and returns immediately (see http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsNativeThemeWin.cpp#2397). I feel like I'm missing something here...?

Comment 2

[:Gijs (he/him)]

(In reply to Nochum Sossonko [:Natch] from comment #1)
> (In reply to Mardeg from comment #0)
> > http://mxr.mozilla.org/mozilla-central/source/widget/windows/
> > nsNativeThemeWin.cpp#2482
> 
> This line points to this "case":
> 
> case NS_THEME_MENUARROW:
>      {
>        // Use the width of the arrow glyph as padding. See the drawing
>        // code for details.
>        aResult->width *= 2;
>        break;
>      }
> > The 'aWidgetType' variable is compared to the
> > NS_THEME_WINDOW_BUTTON_MINIMIZE constant twice.
> 
> The function the aforementioned line points to only compares aWidgetType to
> NS_THEME_WINDOW_BUTTON_MINIMIZE once and returns immediately (see
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/
> nsNativeThemeWin.cpp#2397). I feel like I'm missing something here...?

Here's a better link:

http://hg.mozilla.org/mozilla-central/annotate/80431d4fd0da/widget/windows/nsNativeThemeWin.cpp#l2519

Comment 3

[Nochum Sossonko [:Natch]]

(In reply to :Gijs Kruitbosch from comment #2)
> Here's a better link:
> 
> http://hg.mozilla.org/mozilla-central/annotate/80431d4fd0da/widget/windows/
> nsNativeThemeWin.cpp#l2519

Yeah, that's pretty obvious. Dunno how I missed that.

Taking.

Comment 4

[Nochum Sossonko [:Natch]]

Attachment: Correct erroneous compare

This actually changes behavior I think. This changes aShouldRepaint from being whatever it was, to always being true for BUTTON_MAXIMIZE cases. I'm assuming this is correct, but not sure of all the ramifications. Take http://mxr.mozilla.org/mozilla-central/source/layout/base/RestyleManager.cpp#943 for example (not sure if |app| can ever be BUTTON_MAXIMIZE), this will cause a repaint.

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 8443110 [details] [diff] [review]
Correct erroneous compare

hey, nice find.

Comment 6

[Nochum Sossonko [:Natch]]

Attachment: Correct erroneous compare

Thanks

Comment 7

[:Gijs (he/him)]

https://hg.mozilla.org/integration/fx-team/rev/811295fbccb5

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/811295fbccb5