[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 #9 from Benjamin Poulain <benjamin at webkit.org>  2013-08-01 00:46:37 PST ---
(From update of attachment 207901)
View in context: https://bugs.webkit.org/attachment.cgi?id=207901&action=review

Some comments:

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:65
> + at interface WKColorPickerMac  : NSObject<NSWindowDelegate> {
> + at protected
> +BOOL _lastChangedByUser;
> +WebColorPickerMac* _picker;
>  }
> -
> -- (id)init;
> -- (void)setAndShowPicker:(WebKit::WebColorPickerMac*)picker withColor:(NSColor *)color;
> -- (void)didChooseColor:(NSColorPanel *)panel;
> +- (void)didChooseColor:(id)sender;
>  - (void)invalidate;
> -
> -// Sets color to the NSColorPanel as a non user change.
>  - (void)setColor:(NSColor *)color;
> + at end
>  
> + at implementation WKColorPickerMac
> +- (void)didChooseColor:(id)sender { }
> +- (void)invalidate { }
> +- (void)setColor:(NSColor *)color { }
>  @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.

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:74
> +    NSPopoverColorWell* popoverWell;

You could use a RetainPtr here to avoid risking leaking this.

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:159
> +            return self;

Indent.

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:167
> +    [popoverWell retain];

This is odd!

> 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.

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:183
> +    [popoverWell removeFromSuperviewWithoutNeedingDisplay];
> +    [popoverWell deactivate];
> +    [popoverWell release];
> +    _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.

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