|
Description
Aditya Keerthi
2018-08-09 18:51:52 PDT
Created attachment 346886 [details]
Patch
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.
Comment on attachment 346886 [details] Patch Attachment 346886 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8815457 New failing tests: imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/valueMode.html imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/readwrite-readonly.html imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-willValidate.html accessibility/roles-exposed.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html fast/selectors/read-only-read-write-input-basics.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/selection.html editing/pasteboard/drag-and-drop-color-input.html imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-validity-badInput.html 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
Comment on attachment 346886 [details] Patch Attachment 346886 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8816200 New failing tests: imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/valueMode.html imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/readwrite-readonly.html imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-willValidate.html accessibility/roles-exposed.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html fast/selectors/read-only-read-write-input-basics.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/selection.html editing/pasteboard/drag-and-drop-color-input.html imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-validity-badInput.html 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
Created attachment 346911 [details]
Patch
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. Comment on attachment 346911 [details] Patch Attachment 346911 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8820766 New failing tests: imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/valueMode.html imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/readwrite-readonly.html imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-willValidate.html accessibility/roles-exposed.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html fast/selectors/read-only-read-write-input-basics.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/selection.html editing/pasteboard/drag-and-drop-color-input.html imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-validity-badInput.html imported/w3c/web-platform-tests/html/dom/reflection-forms.html 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
Comment on attachment 346911 [details] Patch Attachment 346911 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8820779 New failing tests: imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/selection.html imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/readwrite-readonly.html imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-willValidate.html accessibility/roles-exposed.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html fast/selectors/read-only-read-write-input-basics.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/valueMode.html imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-validity-badInput.html imported/w3c/web-platform-tests/html/dom/reflection-forms.html 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
Comment on attachment 346911 [details] Patch Attachment 346911 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8820777 New failing tests: imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application.html imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-willValidate.html fast/forms/color/input-appearance-color.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/valueMode.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/color.html imported/w3c/web-platform-tests/html/semantics/forms/the-form-element/form-elements-filter.html imported/w3c/web-platform-tests/html/dom/reflection-forms.html 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
Comment on attachment 346911 [details] Patch Attachment 346911 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8821532 New failing tests: imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/valueMode.html imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/readwrite-readonly.html imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-willValidate.html accessibility/roles-exposed.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html fast/selectors/read-only-read-write-input-basics.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/selection.html editing/pasteboard/drag-and-drop-color-input.html imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-validity-badInput.html 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
Comment on attachment 346911 [details] Patch Attachment 346911 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8821548 New failing tests: imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application.html imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-willValidate.html fast/forms/color/input-appearance-color.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/valueMode.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/color.html imported/w3c/web-platform-tests/html/semantics/forms/the-form-element/form-elements-filter.html imported/w3c/web-platform-tests/html/dom/reflection-forms.html 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 346938 [details]
Patch
Comment on attachment 346938 [details] Patch Attachment 346938 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8823661 New failing tests: editing/pasteboard/drag-and-drop-color-input.html 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
Comment on attachment 346938 [details] Patch Attachment 346938 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8824076 New failing tests: editing/pasteboard/drag-and-drop-color-input.html 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? Comment on attachment 346938 [details] Patch Attachment 346938 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8828474 New failing tests: editing/pasteboard/drag-and-drop-color-input.html 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
Created attachment 346994 [details]
Patch
(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. Created attachment 347088 [details]
Patch
(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/page/DragController.cpp:353 > > + if (input.isColorControl() && !input.isDisabledFormControl()) > > We should not allow dragging when the control is read-only. According to the specification, only text controls can be marked read-only. I've changed the code back to use isDisabledFormControl(). https://www.w3.org/TR/html5/sec-forms.html#the-readonly-attribute (In reply to Aditya Keerthi from comment #32) > (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/page/DragController.cpp:353 > > > + if (input.isColorControl() && !input.isDisabledFormControl()) > > > > We should not allow dragging when the control is read-only. > > According to the specification, only text controls can be marked read-only. > I've changed the code back to use isDisabledFormControl(). > > https://www.w3.org/TR/html5/sec-forms.html#the-readonly-attribute OK. Please add a test. Created attachment 347168 [details]
Patch
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. Created attachment 347173 [details]
Patch
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. Created attachment 347225 [details]
Patch
Comment on attachment 347225 [details] Patch Clearing flags on attachment: 347225 Committed r234930: <https://trac.webkit.org/changeset/234930> All reviewed patches have been landed. Closing bug. 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 |