Bug 181110 – Fix build failures due to using deprecated AppKit symbols

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Wenson Hsieh]

<rdar://problem/36162865>

[Wenson Hsieh]

Created attachment 330081 [details]
Test pass for EWS

[EWS Watchlist]

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.

[Wenson Hsieh]

Created attachment 330086 [details]
Try to fix the build for 10.11

[Tim Horton]

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

[Wenson Hsieh]

(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

[Wenson Hsieh]

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?

[Wenson Hsieh]

Created attachment 330097 [details]
Try to get El Capitan to build (3)

[Wenson Hsieh]

(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.

[Wenson Hsieh]

Created attachment 330101 [details]
Try to get El Capitan to build (4)

[EWS Watchlist]

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

[EWS Watchlist]

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

[Wenson Hsieh]

(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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Alexey Proskuryakov]

> 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?

[Wenson Hsieh]

Created attachment 330111 [details]
Fix MiniBrowser

[Wenson Hsieh]

(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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Wenson Hsieh]

(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...

[Wenson Hsieh]

Created attachment 330117 [details]
Keep using legacy pasteboard constants.

[Wenson Hsieh]

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!

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 330141 [details]
Patch for landing

[WebKit Commit Bot]

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

[Wenson Hsieh]

Created attachment 330142 [details]
Hold for EWS

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 330142 [details]
Hold for EWS

Clearing flags on attachment: 330142

Committed r226277: <https://trac.webkit.org/changeset/226277>

[Wenson Hsieh]

These deprecation warnings have been addressed.

See: r226277, r226279, r226281, r226284, r226286.

[Radar WebKit Bug Importer]

<rdar://problem/36307587>