171486 2017-04-30 14:46:52 -0700 [Cocoa] Have check-webkit-style advise against use of [get…Class() alloc] 2017-05-01 14:32:54 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Local Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 171493 171514 1 mitz mitz ap buildbot dbates ddkilzer ggaren glenn lforschler sam oldest_to_newest 1303149 0 mitz 2017-04-30 14:46:52 -0700 It’s safer to use alloc…Instance() so the style checker could suggest that. Patch forthcoming. 1303150 1 308701 mitz 2017-04-30 14:49:57 -0700 Created attachment 308701 Add style check 1303159 2 mitz 2017-04-30 16:40:04 -0700 Committed <https://trac.webkit.org/r216000>. 1303160 3 dbates 2017-04-30 17:11:22 -0700 Can we add a unit test for this change? Notice that we have unit tests in <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py>. To run the unit tests, run Tools/Scripts/test-webkitpy. 1303219 4 ddkilzer 2017-05-01 06:11:28 -0700 The webkitpy unit test started to fail with this change: [1068/1576] webkitpy.style.checkers.cpp_unittest.CppStyleTest.test_spacing_before_brackets failed: Traceback (most recent call last): File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 1885, in test_spacing_before_brackets self.assert_lint(' return [get_##framework##_##className##Class() alloc]; \\', '') File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 317, in assert_lint self.assertEqual(expected_message, self.perform_single_line_lint(code, file_name)) File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 270, in perform_single_line_lint return self.perform_lint(code, filename, basic_error_rules) File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 259, in perform_lint self.process_file_data(filename, extension, lines, error_collector, unit_test_config) File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 253, in process_file_data checker.check(lines) File "./Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3986, in check self.handle_style_error, self.min_confidence) File "./Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3846, in _process_lines enum_state, asm_state, error) File "./Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3791, in process_line check_style(clean_lines, line, file_extension, class_state, file_state, enum_state, error) File "./Tools/Scripts/webkitpy/style/checkers/cpp.py", line 2842, in check_style check_soft_link_class_alloc(clean_lines, line_number, error) File "./Tools/Scripts/webkitpy/style/checkers/cpp.py", line 2746, in check_soft_link_class_alloc 'Using +alloc with a soft-linked class. Use alloc%sInstance() instead.' % matched.group(1)) File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 69, in __call__ ' which is not in CppChecker.categories' % (message, category)) AssertionError: Message "Using +alloc with a soft-linked class. Use alloc_##framework##_##className##Instance() instead." has category "runtime/soft-linked-alloc", which is not in CppChecker.categories Ran 1576 tests in 5.895s FAILED (failures=1, errors=0) And fixing that results in a false-positive match for the macro: [1056/1576] webkitpy.style.checkers.cpp_unittest.CppStyleTest.test_spacing_before_brackets failed: Traceback (most recent call last): File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 1885, in test_spacing_before_brackets self.assert_lint(' return [get_##framework##_##className##Class() alloc]; \\', '') File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 317, in assert_lint self.assertEqual(expected_message, self.perform_single_line_lint(code, file_name)) AssertionError: '' != 'Using +alloc with a soft-linked class. Use alloc_##framework##_##className##Instance() instead. [runtime/soft-linked-alloc] [4]' Ran 1576 tests in 5.756s FAILED (failures=1, errors=0) Fixed in r216013: <https://trac.webkit.org/r216013> 1303221 5 ddkilzer 2017-05-01 06:26:46 -0700 And FWIW, TestWebKitAPI has a few hits left: $ ./Tools/Scripts/check-webkit-style --filter=-,+runtime/soft-linked-alloc Tools/TestWebKitAPI ERROR: Tools/TestWebKitAPI/Tests/WebKit/ios/AudioSessionCategoryIOS.mm:69: Using +alloc with a soft-linked class. Use allocUIWindowInstance() instead. [runtime/soft-linked-alloc] [4] ERROR: Tools/TestWebKitAPI/Tests/WebKit/ios/AudioSessionCategoryIOS.mm:70: Using +alloc with a soft-linked class. Use allocUIWebViewInstance() instead. [runtime/soft-linked-alloc] [4] ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/RequiresUserActionForPlayback.mm:76: Using +alloc with a soft-linked class. Use allocUIWindowInstance() instead. [runtime/soft-linked-alloc] [4] Total errors found: 3 in 755 files Filed: Bug 171493: [Cocoa] Replace uses of [get…Class() alloc] with alloc…Instance() in TestWebKitAPI <https://bugs.webkit.org/show_bug.cgi?id=171493> 1303409 6 ddkilzer 2017-05-01 14:32:54 -0700 And Source/WebCore has a couple left, too: $ ./Tools/Scripts/check-webkit-style --filter=-,+runtime/soft-linked-alloc Source/WebCore ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:447: Using +alloc with a soft-linked class. Use allocWebAVPictureInPicturePlayerLayerViewInstance() instead. [runtime/soft-linked-alloc] [4] ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:616: Using +alloc with a soft-linked class. Use allocWebAVPlayerLayerViewInstance() instead. [runtime/soft-linked-alloc] [4] Total errors found: 2 in 8492 files Filed: Bug 171514: [Cocoa] Replace uses of [get…Class() alloc] in Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm 308701 2017-04-30 14:49:57 -0700 2017-04-30 15:17:23 -0700 Add style check bug-171486-20170430144956.patch text/plain 2339 mitz SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDIxNTk5OSkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE1IEBACisyMDE3LTA0LTMwICBEYW4gQmVybnN0ZWluICA8bWl0ekBhcHBsZS5jb20+CisK KyAgICAgICAgW0NvY29hXSBIYXZlIGNoZWNrLXdlYmtpdC1zdHlsZSBhZHZpc2UgYWdhaW5zdCB1 c2Ugb2YgW2dldOKApkNsYXNzKCkgYWxsb2NdCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0xNzE0ODYKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvc3R5bGUvY2hlY2tlcnMvY3Bw LnB5OgorICAgICAgICAoY2hlY2tfc29mdF9saW5rX2NsYXNzX2FsbG9jKTogQWRkZWQuIExvb2tz IGZvciBbZ2V04oCmQ2xhc3MoKSBhbGxvY10gYW5kIHN1Z2dlc3RzCisgICAgICAgICAgYWxsb2Pi gKZJbnN0YW5jZSgpIGluc3RlYWQuCisgICAgICAgIChjaGVja19zdHlsZSk6IEludm9rZSBuZXcg Y2hlY2suCisKIDIwMTctMDQtMzAgIEJyYWR5IEVpZHNvbiAgPGJlaWRzb25AYXBwbGUuY29tPgog CiAgICAgICAgIE1vcmUgZml4aW5nIGFmdGVyIHIyMTU5OTEKSW5kZXg6IFRvb2xzL1NjcmlwdHMv d2Via2l0cHkvc3R5bGUvY2hlY2tlcnMvY3BwLnB5Cj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFRvb2xzL1Njcmlw dHMvd2Via2l0cHkvc3R5bGUvY2hlY2tlcnMvY3BwLnB5CShyZXZpc2lvbiAyMTU5OTIpCisrKyBU b29scy9TY3JpcHRzL3dlYmtpdHB5L3N0eWxlL2NoZWNrZXJzL2NwcC5weQkod29ya2luZyBjb3B5 KQpAQCAtMjcyOSw2ICsyNzI5LDIzIEBAIGRlZiBjaGVja19mb3JfbnVsbChjbGVhbl9saW5lcywg bGluZV9udW0KICAgICAgICAgZXJyb3IobGluZV9udW1iZXIsICdyZWFkYWJpbGl0eS9udWxsJywg NCwgJ1VzZSBudWxscHRyIGluc3RlYWQgb2YgTlVMTCAoZXZlbiBpbiAqY29tbWVudHMqKS4nKQog CiAKK2RlZiBjaGVja19zb2Z0X2xpbmtfY2xhc3NfYWxsb2MoY2xlYW5fbGluZXMsIGxpbmVfbnVt YmVyLCBlcnJvcik6CisgICAgIiIiQ2hlY2tzIHRoYXQgYWxsb2NhdGluZyBhbiBpbnN0YW5jZSBv ZiBhIHNvZnQtbGlua2VkIGNsYXNzIHVzZXMgYWxsb2NbQ2xhc3NdSW5zdGFuY2UuCisKKyAgICBB cmdzOgorICAgICAgY2xlYW5fbGluZXM6IEEgQ2xlYW5zZWRMaW5lcyBpbnN0YW5jZSBjb250YWlu aW5nIHRoZSBmaWxlLgorICAgICAgbGluZV9udW1iZXI6IFRoZSBudW1iZXIgb2YgdGhlIGxpbmUg dG8gY2hlY2suCisgICAgICBlcnJvcjogVGhlIGZ1bmN0aW9uIHRvIGNhbGwgd2l0aCBhbnkgZXJy b3JzIGZvdW5kLgorICAgICIiIgorCisgICAgbGluZSA9IGNsZWFuX2xpbmVzLmVsaWRlZFtsaW5l X251bWJlcl0KKworICAgIG1hdGNoZWQgPSBzZWFyY2gocidcW2dldChbXlxzXSspQ2xhc3NcKFwp XHMrYWxsb2NcXScsIGxpbmUpCisgICAgaWYgbWF0Y2hlZDoKKyAgICAgICAgZXJyb3IobGluZV9u dW1iZXIsICdydW50aW1lL3NvZnQtbGlua2VkLWFsbG9jJywgNCwKKyAgICAgICAgICAgICAgJ1Vz aW5nICthbGxvYyB3aXRoIGEgc29mdC1saW5rZWQgY2xhc3MuIFVzZSBhbGxvYyVzSW5zdGFuY2Uo KSBpbnN0ZWFkLicgJSBtYXRjaGVkLmdyb3VwKDEpKQorCisKIGRlZiBnZXRfbGluZV93aWR0aChs aW5lKToKICAgICAiIiJEZXRlcm1pbmVzIHRoZSB3aWR0aCBvZiB0aGUgbGluZSBpbiBjb2x1bW4g cG9zaXRpb25zLgogCkBAIC0yODIyLDYgKzI4MzksNyBAQCBkZWYgY2hlY2tfc3R5bGUoY2xlYW5f bGluZXMsIGxpbmVfbnVtYmVyCiAgICAgY2hlY2tfY2hlY2soY2xlYW5fbGluZXMsIGxpbmVfbnVt YmVyLCBlcnJvcikKICAgICBjaGVja19mb3JfY29tcGFyaXNvbnNfdG9femVybyhjbGVhbl9saW5l cywgbGluZV9udW1iZXIsIGVycm9yKQogICAgIGNoZWNrX2Zvcl9udWxsKGNsZWFuX2xpbmVzLCBs aW5lX251bWJlciwgZmlsZV9zdGF0ZSwgZXJyb3IpCisgICAgY2hlY2tfc29mdF9saW5rX2NsYXNz X2FsbG9jKGNsZWFuX2xpbmVzLCBsaW5lX251bWJlciwgZXJyb3IpCiAgICAgY2hlY2tfaW5kZW50 YXRpb25fYW1vdW50KGNsZWFuX2xpbmVzLCBsaW5lX251bWJlciwgZXJyb3IpCiAgICAgY2hlY2tf ZW51bV9jYXNpbmcoY2xlYW5fbGluZXMsIGxpbmVfbnVtYmVyLCBlbnVtX3N0YXRlLCBlcnJvcikK IAo=