| Summary: | Fix build failures due to using deprecated AppKit symbols | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | ap, commit-queue, ews-watchlist, mitz, rniwa, ryanhaddad, thorton, webkit-bug-importer | ||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
|
Description
Wenson Hsieh
2017-12-21 17:05:48 PST
Created attachment 330081 [details]
Test pass for EWS
Attachment 330081 [details] did not pass style-queue:
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1167: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1167: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1169: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1174: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1174: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1176: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1181: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1181: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1183: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1188: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1188: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1190: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1195: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1195: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1197: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1209: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1209: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1211: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1216: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1216: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1218: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/PasteboardWriter.mm:81: Extra space in capture list. [whitespace/brackets] [4]
ERROR: Source/WebCore/platform/mac/PasteboardWriter.mm:83: Extra space in capture list. [whitespace/brackets] [4]
ERROR: Source/WebCore/platform/mac/PasteboardWriter.mm:85: Extra space in capture list. [whitespace/brackets] [4]
Total errors found: 24 in 39 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 330086 [details]
Try to fix the build for 10.11
Comment on attachment 330086 [details] Try to fix the build for 10.11 View in context: https://bugs.webkit.org/attachment.cgi?id=330086&action=review rs=me once you get the build green > Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:860 > +#pragma GCC diagnostic push why GCC instead of clang :P (In reply to Tim Horton from comment #4) > Comment on attachment 330086 [details] > Try to fix the build for 10.11 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330086&action=review > > rs=me once you get the build green Thanks! Though, I might need to make a few slightly more aggressive tweaks to get everything green...it would be nice to get a second opinion :P > > > Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:860 > > +#pragma GCC diagnostic push > > why GCC instead of clang :P ‼️ Good catch, fixed Created attachment 330094 [details]
More SDK compat fixes.
I guess I don’t understand how you can not handle the deprecated pasteboard types—couldn’t clients still be vending or expecting them, especially clients built with older SDKs? Is something else along the way (AppKit?) doing the mapping between the types? Created attachment 330097 [details]
Try to get El Capitan to build (3)
(In reply to mitz from comment #7) > I guess I don’t understand how you can not handle the deprecated pasteboard > types—couldn’t clients still be vending or expecting them, especially > clients built with older SDKs? Is something else along the way (AppKit?) > doing the mapping between the types? I reached out to the AppKit folks who made the change, and confirmed that AppKit maps between between modern (UTI) and legacy pasteboard types. I also specifically pointed out the scenario in which a client iterates over the list of registered pasteboard types in search of a deprecated pasteboard type string, and it appears that case is handled as well. Created attachment 330101 [details]
Try to get El Capitan to build (4)
Comment on attachment 330101 [details] Try to get El Capitan to build (4) Attachment 330101 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5798059 New failing tests: fast/events/ondrop-text-html.html editing/pasteboard/drag-drop-input-textarea.html fast/events/input-events-insert-by-drop.html editing/pasteboard/get-data-text-plain-drop.html editing/pasteboard/drop-text-events.html fast/forms/drag-out-of-textarea.html fast/events/input-events-drag-and-drop.html fast/forms/drag-into-textarea.html editing/pasteboard/drop-inputtext-acquires-style.html fast/events/5056619.html editing/pasteboard/data-transfer-get-data-on-drop-rich-text.html fast/events/dropzone-003.html editing/pasteboard/drag-drop-url-text.html editing/pasteboard/data-transfer-get-data-on-drop-plain-text.html editing/pasteboard/data-transfer-get-data-on-drop-url.html Created attachment 330108 [details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
(In reply to Build Bot from comment #11) > Comment on attachment 330101 [details] > Try to get El Capitan to build (4) > > Attachment 330101 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.webkit.org/results/5798059 > > New failing tests: > fast/events/ondrop-text-html.html > editing/pasteboard/drag-drop-input-textarea.html > fast/events/input-events-insert-by-drop.html > editing/pasteboard/get-data-text-plain-drop.html > editing/pasteboard/drop-text-events.html > fast/forms/drag-out-of-textarea.html > fast/events/input-events-drag-and-drop.html > fast/forms/drag-into-textarea.html > editing/pasteboard/drop-inputtext-acquires-style.html > fast/events/5056619.html > editing/pasteboard/data-transfer-get-data-on-drop-rich-text.html > fast/events/dropzone-003.html > editing/pasteboard/drag-drop-url-text.html > editing/pasteboard/data-transfer-get-data-on-drop-plain-text.html > editing/pasteboard/data-transfer-get-data-on-drop-url.html Curiously, these are all drag and drop related; copy and paste tests look OK. I'm also not seeing these test failures when running against the latest SDK. I tried manually running some of these test cases in Safari, and it doesn't seem broken. This makes me a little suspicious of DumpRenderTreePasteboard. Perhaps behavior there doesn't match up with the platform after this change? Comment on attachment 330101 [details] Try to get El Capitan to build (4) Attachment 330101 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5798385 New failing tests: fast/events/ondrop-text-html.html editing/pasteboard/drag-drop-input-textarea.html fast/events/input-events-insert-by-drop.html editing/pasteboard/get-data-text-plain-drop.html editing/pasteboard/drop-text-events.html fast/forms/drag-out-of-textarea.html fast/events/input-events-drag-and-drop.html fast/forms/drag-into-textarea.html editing/pasteboard/drop-inputtext-acquires-style.html fast/events/5056619.html editing/pasteboard/data-transfer-get-data-on-drop-rich-text.html fast/events/dropzone-003.html editing/pasteboard/drag-drop-url-text.html editing/pasteboard/data-transfer-get-data-on-drop-plain-text.html editing/pasteboard/data-transfer-get-data-on-drop-url.html Created attachment 330110 [details]
Archive of layout-test-results from ews117 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
> I tried manually running some of these test cases in Safari, and it doesn't seem broken.
Were you testing El Capitan, like EWS does?
Created attachment 330111 [details]
Fix MiniBrowser
(In reply to Alexey Proskuryakov from comment #16) > > I tried manually running some of these test cases in Safari, and it doesn't seem broken. > > Were you testing El Capitan, like EWS does? I was not. After building/testing on El Capitan, I realized that most of the modern pasteboard types (except URL) are present on 10.11, so AppKitCompatibilityDeclarations.h should not be trying to redefine them to be the deprecated versions. Fixing this fixed the test failures for me. Comment on attachment 330111 [details] Fix MiniBrowser Attachment 330111 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5799523 New failing tests: editing/pasteboard/data-transfer-get-data-on-paste-as-plain-text-when-custom-pasteboard-data-disabled.html Created attachment 330113 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
(In reply to Build Bot from comment #19) > Comment on attachment 330111 [details] > Fix MiniBrowser > > Attachment 330111 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.webkit.org/results/5799523 > > New failing tests: > editing/pasteboard/data-transfer-get-data-on-paste-as-plain-text-when-custom- > pasteboard-data-disabled.html This failure is harder to explain. Logging in this test shows that this fails because Pasteboard::addHTMLClipboardTypesForCocoaType uses utiTypeFromCocoaType to transform "Cocoa" types into either UTIs or MIME types (which, as an aside, would probably be a more accurate name for this function). Before this change, NSHTMLPboardType is passed in, which UTTypeCreatePreferredIdentifierForTag understands. After this change, "public.html" is given, so our logic to convert to a MIME type fails. This case in particular is trivial to address by teaching utiTypeFromCocoaType to go directly from UTI to MIME, rather than go from legacy Pboard type to UTI to MIME. However, this makes me wonder why -[NSPasteboard types] didn't return a list of types containing both legacy Pboard and modern UTI-based types, like it does in a simple test app. Without understanding why the list of pasteboard types isn't being duplicated in this scenario, I'm afraid compatibility might be a bigger risk than previously thought. I'm going to need more time to investigate this, and will also probably need to poke around in AppKit to understand their code for duplicating pasteboard types. In the interest of fixing the build (and mitigating risk), we should probably resort to just suppressing pasteboard deprecation warnings... Created attachment 330117 [details]
Keep using legacy pasteboard constants.
Created attachment 330123 [details]
Don't spam deprecation guards.
Comment on attachment 330123 [details] Don't spam deprecation guards. View in context: https://bugs.webkit.org/attachment.cgi?id=330123&action=review > Source/WebCore/platform/mac/PasteboardMac.mm:-208 > - Why did we lose this newline? > Source/WebCore/platform/mac/PasteboardMac.mm:664 > + Oh, it went here :p > Source/WebCore/platform/mac/PasteboardWriter.mm:80 > + // NSURLPboardType(). ? > Source/WebCore/platform/mac/PasteboardWriter.mm:82 > + [pasteboardItem setPropertyList:@[cocoaURL.relativeString, baseCocoaURL.absoluteString] forType:toUTI(WebCore::legacyURLPasteboardType()).get()]; Pretty sure the prevailing style is with spaces, similarly to C struct and array initializers. > Source/WebKitLegacy/mac/WebView/WebView.h:173 > + including legacyURLPasteboardType() to find a URL on the pasteboard. The comments in the API header shouldn’t refer to this WebCore-internal concept! Comment on attachment 330123 [details] Don't spam deprecation guards. View in context: https://bugs.webkit.org/attachment.cgi?id=330123&action=review Thanks for the review! >> Source/WebCore/platform/mac/PasteboardMac.mm:-208 >> - > > Why did we lose this newline? Whoops, that wasn't intended. Fixed! >> Source/WebCore/platform/mac/PasteboardMac.mm:664 >> + > > Oh, it went here :p :P Removed. >> Source/WebCore/platform/mac/PasteboardWriter.mm:80 >> + // NSURLPboardType(). > > ? Ah, find-and-replace error. Fixed! >> Source/WebCore/platform/mac/PasteboardWriter.mm:82 >> + [pasteboardItem setPropertyList:@[cocoaURL.relativeString, baseCocoaURL.absoluteString] forType:toUTI(WebCore::legacyURLPasteboardType()).get()]; > > Pretty sure the prevailing style is with spaces, similarly to C struct and array initializers. Oh, ok. Restored spaces in @[ ] I had removed these because check-webkit-style complained here with: ERROR: Source/WebCore/platform/mac/PasteboardWriter.mm:82: Extra space in capture list. [whitespace/brackets] [4] ERROR: Source/WebCore/platform/mac/PasteboardWriter.mm:84: Extra space in capture list. [whitespace/brackets] [4] ERROR: Source/WebCore/platform/mac/PasteboardWriter.mm:86: Extra space in capture list. [whitespace/brackets] [4] >> Source/WebKitLegacy/mac/WebView/WebView.h:173 >> + including legacyURLPasteboardType() to find a URL on the pasteboard. > > The comments in the API header shouldn’t refer to this WebCore-internal concept! Oops, another unintended find and replace! Fixed. Created attachment 330141 [details]
Patch for landing
Comment on attachment 330141 [details] Patch for landing Rejecting attachment 330141 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 330141, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ac/WebView/WebView.mm patching file Tools/ChangeLog patching file Tools/MiniBrowser/AppKitCompatibilityDeclarations.h patching file Tools/MiniBrowser/MiniBrowser.xcodeproj/project.pbxproj patching file Tools/MiniBrowser/mac/SettingsController.m patching file Tools/MiniBrowser/mac/WK1BrowserWindowController.m patching file Tools/MiniBrowser/mac/WK2BrowserWindowController.m Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/5804798 Created attachment 330142 [details]
Hold for EWS
Attachment 330142 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/mac/PasteboardWriter.mm:82: Extra space in capture list. [whitespace/brackets] [4]
ERROR: Source/WebCore/platform/mac/PasteboardWriter.mm:84: Extra space in capture list. [whitespace/brackets] [4]
ERROR: Source/WebCore/platform/mac/PasteboardWriter.mm:86: Extra space in capture list. [whitespace/brackets] [4]
Total errors found: 3 in 47 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 330142 [details] Hold for EWS Clearing flags on attachment: 330142 Committed r226277: <https://trac.webkit.org/changeset/226277> |