This bug tracks the work required to bring input[type=color] to iOS.
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.