Bug 187871

Created attachment 345486 [details]
Patch

Comment on attachment 345486 [details]
Patch

Attachment 345486 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8604867

Number of test failures exceeded the failure limit.

Created attachment 345495 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment on attachment 345486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345486&action=review

> Source/WebCore/html/ColorInputType.cpp:173
> +#if PLATFORM(MAC)

It feels like this ifdef is more appropriate in the platform-y Chrome code instead of here.

> Source/WebKit/UIProcess/ios/forms/WKFormColorControl.h:33
> +- (instancetype)initWithView:(WKContentView *)view;

Can this just be a UIView? It’s kind of weird to spread knowledge of WKContentView, and I don’t think it’s important in this case.

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:109
> +        [colorButtons addObject:[colorButtonsRow copy]];

This looks like a leak. But it’s not, because you release them in dealloc. But the array was retaining them, so the extra ref was for naught. If you did things the WebKit way this would be

auto row = adoptNS([colorButtonsRow copy]);
[colorButtons addObject:row.get()];

and then nothing in dealloc.

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:145
> +    [_colorButtons release];
> +    [_colorMatrix release];

Why not RetainPtr instead of this?

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:219
> +    const CGFloat *components = CGColorGetComponents(uiColor.CGColor);

Why are you doing all this yourself? I’m sure we have UIColor->Color code elsewhere.

(In reply to Tim Horton from comment #4)
> Comment on attachment 345486 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345486&action=review
> 
> > Source/WebKit/UIProcess/ios/forms/WKFormColorControl.h:33
> > +- (instancetype)initWithView:(WKContentView *)view;
> 
> Can this just be a UIView? It’s kind of weird to spread knowledge of
> WKContentView, and I don’t think it’s important in this case.

I made it a WKContentView because the view is passed through to another class which has WKContentView specific logic. If I changed it to a UIView here, I would need to cast to WKContentView later on. Do you prefer casting or keeping the definition as-is?

(In reply to Aditya Keerthi from comment #5)
> (In reply to Tim Horton from comment #4)
> > Comment on attachment 345486 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=345486&action=review
> > 
> > > Source/WebKit/UIProcess/ios/forms/WKFormColorControl.h:33
> > > +- (instancetype)initWithView:(WKContentView *)view;
> > 
> > Can this just be a UIView? It’s kind of weird to spread knowledge of
> > WKContentView, and I don’t think it’s important in this case.
> 
> I made it a WKContentView because the view is passed through to another
> class which has WKContentView specific logic. If I changed it to a UIView
> here, I would need to cast to WKContentView later on. Do you prefer casting
> or keeping the definition as-is?

No, no, leave it as is. I didn't see the WKContentView specific bit.

Created attachment 345585 [details]
Patch

Comment on attachment 345585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345585&action=review

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:114
> +    _colorButtons = adoptNS([colorButtons copy]);

In retrospect, there's not even a real reason for this copy.

Created attachment 345591 [details]
Patch

Comment on attachment 345591 [details]
Patch

Clearing flags on attachment: 345591

Committed r234105: <https://trac.webkit.org/changeset/234105>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/42509854>

Comment on attachment 345591 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345591&action=review

> Source/WebCore/css/html.css:887
>  #endif // defined(ENABLE_DATALIST_ELEMENT) && ENABLE_DATALIST_ELEMENT

This #endif got left behind. :) It seems to be what's rendering text diffs invalid on WinCairo right now:
https://build.webkit.org/results/WinCairo%2064-bit%20WKL%20Release%20(Tests)/r234105%20(694)/results.html

Sorry about that! I'll put up a patch to remove it.