Attachment 346886[details] did not pass style-queue:
ERROR: Source/WebCore/page/EventHandler.cpp:3705: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebCore/page/EventHandler.cpp:3707: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 2 in 50 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 346889[details]
Archive of layout-test-results from ews116 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 346898[details]
Archive of layout-test-results from ews115 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Attachment 346911[details] did not pass style-queue:
ERROR: Source/WebCore/page/EventHandler.cpp:3705: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebCore/page/EventHandler.cpp:3707: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 2 in 55 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 346911[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=346911&action=review> Source/WebCore/page/DragController.cpp:348
> +static bool isColorInput(Node& node)
This function is slightly misnamed, because it's also checking whether the input element is not disabled.
> Source/WebCore/platform/ios/DragImageIOS.mm:258
> + RetainPtr<UIGraphicsImageRenderer> render = adoptNS([allocUIGraphicsImageRendererInstance() initWithSize:imageRect.size()]);
This would be a good place to use auto.
> Source/WebCore/platform/mac/DragImageMac.mm:346
> + RetainPtr<NSImage> dragImage = adoptNS([[NSImage alloc] initWithSize:NSMakeSize(swatchWidth, swatchWidth)]);
Ditto.
> Source/WebKit/UIProcess/ios/DragDropInteractionState.mm:297
> + RetainPtr<NSString> title = (NSString *)source.linkTitle;
> + RetainPtr<NSURL> url = (NSURL *)source.linkURL;
> + dragItem.previewProvider = [title, url, draggingCenter] () -> UIDragPreview * {
You can avoid some retain count churn by WTFMove()ing title and url in the capture list. Even better, you could avoid these local variables entirely by writing the capture list like this: [title = retainPtr((NSString *)source.linkTitle), url = retainPtr((NSURL *)source.linkURL)].
> Source/WebKit/UIProcess/ios/DragDropInteractionState.mm:307
> + RetainPtr<UIImage> image = source.image;
> + dragItem.previewProvider = [image] () -> UIDragPreview * {
Ditto for image.
> Source/WebKit/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:280
> + WebProcess::singleton().parentProcessConnection()->sendSync(Messages::WebPasteboardProxy::SetPasteboardColor(pasteboardName, color), Messages::WebPasteboardProxy::SetPasteboardColor::Reply(newChangeCount), 0);
sendSync() can time out, and if so I think you'd end up with an uninitialized newChangeCount. You should either initialize newChangeCount to 0 or handle sendSync() failing.
Created attachment 346915[details]
Archive of layout-test-results from ews103 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 346917[details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 346918[details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Created attachment 346927[details]
Archive of layout-test-results from ews116 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 346928[details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Created attachment 346943[details]
Archive of layout-test-results from ews103 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 346947[details]
Archive of layout-test-results from ews112 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 346938[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=346938&action=review> Source/WebCore/ChangeLog:18
> + Implemented createDragImageForColor to display a particular color swatch once
Nit - these are good comments to put next to the relevant functions a bit further down.
> Source/WebCore/page/DragController.cpp:350
> + Node* n = node.deprecatedShadowAncestorNode();
For variable names, we usually prefer full words over single letters or abbreviations (https://webkit.org/code-style-guidelines/#names-full-words)
> Source/WebCore/platform/mac/PasteboardMac.mm:230
> + platformStrategies()->pasteboardStrategy()->setTypes(types, m_pasteboardName);
Nit - this could be made a tiny bit cleaner by using an initializer list to create the types vector.
Comment on attachment 346938[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=346938&action=review
I haven’t read through the entire patch. Here are some comment so far.
> Source/WebCore/page/DragController.cpp:353
> + if (input.isColorControl() && !input.isDisabledFormControl())
We should not allow dragging when the control is read-only.
> Source/WebCore/page/DragController.cpp:1179
> + Color color = input.valueAsColor();
Auto?
> Source/WebCore/page/DragController.cpp:1185
> + dragImageOffset = IntPoint(dragImageSize(dragImage.get()));
Uniform intialization syntax?
> Source/WebCore/platform/ios/DragImageIOS.mm:256
> + FloatRect imageRect(0, 0, elementRect.width() * pageScaleFactor, elementRect.height() * pageScaleFactor);
Uniform initialization syntax?
> Source/WebCore/platform/ios/DragImageIOS.mm:259
> + UIImage *image = [render.get() imageWithActions:^(UIGraphicsImageRendererContext *rendererContext) {
The .get() is not needed.
> Source/WebCore/platform/ios/DragImageIOS.mm:260
> + GraphicsContext context(rendererContext.CGContext);
Uniform initialization syntax.
> Source/WebCore/platform/ios/DragImageIOS.mm:263
> + context.fillRoundedRect(FloatRoundedRect(imageRect, FloatRoundedRect::Radii(4 * pageScaleFactor)), color);
Ditto. Also, we should cache the rounded rect in a local and then reference it here and in line 266 below as opposed to computing it twice.
> Source/WebCore/platform/ios/DragImageIOS.mm:266
> + visiblePath.addRoundedRect(FloatRoundedRect(imageRect, FloatRoundedRect::Radii(4 * pageScaleFactor)));
How did you come up with 4 here and above?
> Source/WebCore/platform/mac/DragDataMac.mm:94
> + return String(UIColorPboardType);
Uniform initialization syntax?
Created attachment 346961[details]
Archive of layout-test-results from ews205 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to Daniel Bates from comment #26)
> Comment on attachment 346938[details]
> Patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346938&action=review
>
> > Source/WebCore/platform/ios/DragImageIOS.mm:266
> > + visiblePath.addRoundedRect(FloatRoundedRect(imageRect, FloatRoundedRect::Radii(4 * pageScaleFactor)));
>
> How did you come up with 4 here and above?
This value matches NSColorWell's drag-and-drop implementation.
Comment on attachment 347168[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=347168&action=review> Source/WebKit/UIProcess/ios/DragDropInteractionState.mm:305
> + UIDragPreviewParameters *parameters = [[[UIDragPreviewParameters alloc] initWithTextLineRects:@[[NSValue valueWithCGRect:[imageView bounds]]]] autorelease];
Nit - I think we usually put spaces around the inside of the outer square brackets in array initialized like this: `@[ … ]`.
> Tools/TestWebKitAPI/Tests/WebKitCocoa/DragAndDropTests.mm:111
> +TEST(DragAndDropTests, ColorInputInputEvent)
I think you could consider consolidating this and ColorInputChangeEvent into a single test that checks for both events, since their setup and steps are identical.
> Tools/TestWebKitAPI/Tests/WebKitCocoa/color-drop.html:10
> + setTimeout(() => window.webkit.messageHandlers.testHandler.postMessage("change"), 0);
Hm...I don't see why we need to schedule these on a timer.
> Tools/TestWebKitAPI/Tests/mac/DragAndDropTestsMac.mm:62
> +TEST(DragAndDropPasteboardTests, DropColor)
Nit - s/DragAndDropPasteboardTests/DragAndDropTests/
> Tools/TestWebKitAPI/Tests/mac/DragAndDropTestsMac.mm:72
> + auto hostWindow = adoptNS([[NSWindow alloc] initWithContentRect:NSMakeRect(0, 0, 400, 400) styleMask:0 backing:NSBackingStoreBuffered defer:NO]);
I don't think we need to add the web view in this window, since TestWKWebView is hosted in an NSWindow by default.
Comment on attachment 347173[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=347173&action=review> Source/WebCore/page/DragController.cpp:917
> + bool isColorControl = (is<HTMLInputElement>(state.source.get()) && downcast<HTMLInputElement>(*state.source.get()).isColorControl());
Nit - I don't think the extra ()'s around the RHS are needed. (The second .get() can also be removed).
> Source/WebCore/page/DragController.cpp:1177
> + auto& input = downcast<HTMLInputElement>(*state.source.get());
Nit - The .get() is unnecessary here.
> Source/WebCore/page/mac/DragControllerMac.mm:148
> + supportedTypes.append(String(UIColorPboardType));
Nit - The explicit call to String() shouldn't be needed here.
MSVC Debug evidently does not support #ifs inside an ASSERT macro:
https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/10949/steps/compile-webkit/logs/stdio> Source\WebCore\page/EventHandler.cpp(3707): error C2121: '#': invalid character: possibly the result of a macro expansion
> Source\WebCore\page/EventHandler.cpp(3699): error C2059: syntax error: 'if'
> Source\WebCore\page/EventHandler.cpp(3707): error C2143: syntax error: missing ';' before '{'
(In reply to Ross Kirsling from comment #42)
> MSVC Debug evidently does not support #ifs inside an ASSERT macro:
> https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/
> 10949/steps/compile-webkit/logs/stdio
>
> > Source\WebCore\page/EventHandler.cpp(3707): error C2121: '#': invalid character: possibly the result of a macro expansion
> > Source\WebCore\page/EventHandler.cpp(3699): error C2059: syntax error: 'if'
> > Source\WebCore\page/EventHandler.cpp(3707): error C2143: syntax error: missing ';' before '{'
Note that Clang too will give the following warning with -Wembedded-directive:
> warning: embedding a directive within macro arguments has undefined behavior
2018-08-09 19:31 PDT, Aditya Keerthi
2018-08-09 21:38 PDT, EWS Watchlist
2018-08-09 23:45 PDT, EWS Watchlist
2018-08-10 10:28 PDT, Aditya Keerthi
2018-08-10 12:04 PDT, EWS Watchlist
2018-08-10 12:12 PDT, EWS Watchlist
2018-08-10 12:42 PDT, EWS Watchlist
2018-08-10 14:39 PDT, EWS Watchlist
2018-08-10 14:40 PDT, EWS Watchlist
2018-08-10 16:21 PDT, Aditya Keerthi
2018-08-10 17:17 PDT, EWS Watchlist
2018-08-10 18:32 PDT, EWS Watchlist
2018-08-11 08:25 PDT, EWS Watchlist
2018-08-12 18:23 PDT, Aditya Keerthi
2018-08-14 10:24 PDT, Aditya Keerthi
2018-08-15 09:37 PDT, Aditya Keerthi
2018-08-15 10:14 PDT, Aditya Keerthi
2018-08-15 16:51 PDT, Aditya Keerthi