[Webkit-unassigned] [Bug 119356] [Forms: color] <input type='color'> popover color well implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=119356





--- Comment #13 from Ruth Fong <ruthiecftg at gmail.com>  2013-08-01 08:25:57 PST ---
(From update of attachment 207901)
View in context: https://bugs.webkit.org/attachment.cgi?id=207901&action=review

>> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:65
>>  @end
> 
> I think you should transform WKColorPickerMac to a protocol, and move the two attributes to the subclass.
> 
> WKColorPickerMac does not have ownership of _lastChangedByUser which leads to improper encapsulation. You should try to avoid inheritance as a way to share code, using encapsulation/aggregation leads to better designs.

Moving the private variables makes  sense.

I like WKColorPickerMac as a base class because that way m_colorPickerUI can be of type WKColorPickerMac and I can maintain more abstraction in WebColorPickerMac.

>> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:74
>> +    NSPopoverColorWell* popoverWell;
> 
> You could use a RetainPtr here to avoid risking leaking this.

Got it.

>> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:177
>> +
> 
> I would also add a destructor just to be safe. You could call invalidate from it, or just assert that _picker and popoverWell are nil.

Got it.

>> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:183
>> +    _picker = nil;
> 
> I believe you also want to remove the target of the popowerWell. Otherwise, if an other reference to the object exist, you may still get called.

Got it.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.