Closed Bug 1025595 Opened 7 years ago Closed 7 years ago

Duplicate comparison: aWidgetType == NS_THEME_WINDOW_BUTTON_MINIMIZE

Categories

(Core :: Widget: Win32, defect)

All
Windows 7
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: mardeg, Assigned: Natch)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 1 obsolete file)

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.
Severity: normal → minor
Component: Theme → Widget: Win32
Product: Firefox → Core
Whiteboard: [good first bug]
(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...?
(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
(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.
Assignee: nobody → highmind63
Status: NEW → ASSIGNED
Attached patch Correct erroneous compare (obsolete) — Splinter Review
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.
Attachment #8443110 - Flags: review?(jmathies)
Comment on attachment 8443110 [details] [diff] [review]
Correct erroneous compare

hey, nice find.
Attachment #8443110 - Flags: review?(jmathies) → review+
Thanks
Attachment #8443110 - Attachment is obsolete: true
Attachment #8443631 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/811295fbccb5
Keywords: checkin-needed
Whiteboard: [good first bug] → [good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/811295fbccb5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][fixed-in-fx-team] → [good first bug]
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.