[webkit-reviews] review granted: [Bug 137014] NSButtonCell leak allocated under WebCore::paintToggleButton : [Attachment 238557] [PATCH] Proposed Fix - RetainPtr

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 137014: NSButtonCell leak allocated under WebCore::paintToggleButton
https://bugs.webkit.org/show_bug.cgi?id=137014

Attachment 238557: [PATCH] Proposed Fix - RetainPtr
https://bugs.webkit.org/attachment.cgi?id=238557&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=238557&action=review


> Source/WebCore/platform/mac/ThemeMac.mm:366
>      static NSButtonCell *radioCell;
>      if (!radioCell)
> -	   radioCell = createToggleButtonCell(RadioPart);
> +	   radioCell = createToggleButtonCell(RadioPart).leakRef();

A simpler way to write this is:

static NSButtonCell *radioCell = createToggleButtonCell(RadioPart).leakRef();

> Source/WebCore/platform/mac/ThemeMac.mm:376
>      static NSButtonCell *checkboxCell;
>      if (!checkboxCell)
> -	   checkboxCell = createToggleButtonCell(CheckboxPart);
> +	   checkboxCell = createToggleButtonCell(CheckboxPart).leakRef();

Ditto.

> Source/WebCore/platform/mac/ThemeMac.mm:392
> +    RetainPtr<NSButtonCell> toggleButtonCell =
static_cast<NSButtonCell*>(controlStates->platformControl());

Misplaced star, should be static_cast<NSButtonCell *>.

Looking at the function as a whole, I suspect that there is a better way to
write it (not something for this patch). What happens now:
- controlStates object may or may not have an NSButtonCell;
- when it does, we use it;
- when it doesn't, we create a new one;
- and at the end, we sometimes put the new cell into controlStates, and
sometimes don't.

This is mysterious.