Remove DeprecatedOptional
Created attachment 367942 [details] Patch
Did you check with Joe? I broke some builds (non-ews) when I tried to do it last because we need to maintain some binary compatibility is some system frameworks.
Comment on attachment 367942 [details] Patch Oops, I didn’t see the comment from Chris when marking this as reviewed
I think this is OK since it's been several years, this would only affect open source developers working on super old operating systems, and I think it's not needed any more because we're back to using WTF::Optional (see http://trac.webkit.org/r209326 ) but I'll wait for Joe or Devin to agree.
(In reply to Alex Christensen from comment #4) > I think this is OK since it's been several years, this would only affect open source developers working on super old operating systems, and I think it's not needed any more because we're back to using WTF::Optional (see http://trac.webkit.org/r209326 ) but I'll wait for Joe or Devin to agree. That's correct. If none of the bots complain, then I think this is fine to remove.
(In reply to Devin Rousso from comment #5) > (In reply to Alex Christensen from comment #4) > > I think this is OK since it's been several years, this would only affect open source developers working on super old operating systems, and I think it's not needed any more because we're back to using WTF::Optional (see http://trac.webkit.org/r209326 ) but I'll wait for Joe or Devin to agree. > That's correct. If none of the bots complain, then I think this is fine to > remove. You will be proven wrong very soon I fear. It tried to remove this file like 1 month ago when I renamed Optional and it passed EWS but failed on internal bots.
Anyway, I do not mind trying but please monitor the bots after landing. Worse case scenario we roll it out.
Comment on attachment 367942 [details] Patch Attachment 367942 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11963367 New failing tests: http/tests/paymentrequest/payment-response-payerName-attribute.https.html http/tests/paymentrequest/payment-response-payerPhone-attribute.https.html http/tests/paymentrequest/updateWith-method-pmi-handling.https.html http/tests/paymentrequest/payment-request-change-shipping-address.https.html http/tests/paymentrequest/payment-response-complete-method.https.html http/tests/paymentrequest/payment-address-attributes-and-toJSON-method.https.html http/tests/paymentrequest/payment-request-change-shipping-option.https.html http/tests/paymentrequest/payment-response-payerEmail-attribute.https.html http/tests/paymentrequest/payment-response-methodName-attribute.https.html http/tests/paymentrequest/payment-response-retry-method.https.html http/tests/paymentrequest/payment-request-merchant-validation.https.html http/tests/paymentrequest/payment-request-show-method.https.html
Created attachment 367972 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 367942 [details] Patch Joe said "Let's do this" in more elegant words. I believe the test failures are unrelated. I'll check the bots after this lands.
Comment on attachment 367942 [details] Patch Clearing flags on attachment: 367942 Committed r244558: <https://trac.webkit.org/changeset/244558>
All reviewed patches have been landed. Closing bug.
<rdar://problem/50141786>
The tests listed in EWS failure are actually crashing on iOS Simulator and Debug. https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/3750 https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20%28Tests%29/builds/3419 Debug is hitting assertion failures and causing 13 crashes ASSERTION FAILED: dlopen(/System/Library/Frameworks/PassKit.framework/PassKit, 2): Symbol not found: __ZN9Inspector17BackendDispatcher19reportProtocolErrorESt8optionalIlENS0_15CommonErrorCodeERKN3WTF6StringE https://build.webkit.org/results/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20(Tests)/r244565%20(3417)/fast/css/appearance-apple-pay-button-crash-log.txt
Re-opened since this is blocked by bug 197219
Created attachment 368154 [details] Patch
Attachment 368154 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/InspectorBackendDispatcherCompatibility.h:38: Use 'WTF::Optional<>' instead of 'std::optional<>'. [runtime/wtf_optional] [4] ERROR: Source/JavaScriptCore/inspector/InspectorBackendDispatcherCompatibility.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/JavaScriptCore/inspector/InspectorBackendDispatcherCompatibility.cpp:31: Use 'WTF::Optional<>' instead of 'std::optional<>'. [runtime/wtf_optional] [4] Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 368192 [details] Patch
Attachment 368192 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/InspectorBackendDispatcherCompatibility.h:38: Use 'WTF::Optional<>' instead of 'std::optional<>'. [runtime/wtf_optional] [4] ERROR: Source/JavaScriptCore/inspector/InspectorBackendDispatcherCompatibility.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/JavaScriptCore/inspector/InspectorBackendDispatcherCompatibility.cpp:31: Use 'WTF::Optional<>' instead of 'std::optional<>'. [runtime/wtf_optional] [4] Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
"iOS12" should say "iOS 12".
Comment on attachment 368192 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368192&action=review > Source/JavaScriptCore/ChangeLog:19 > + * inspector/InspectorBackendDispatcherCompatibility.cpp: Added. > + (Inspector::BackendDispatcher::reportProtocolError): > + * inspector/InspectorBackendDispatcherCompatibility.h: Added. I think the contents of InspectorBackendDispatcherCompatibility.h could all be entirely within InspectorBackendDispatcherCompatibility.cpp; we don’t need two files. > Source/JavaScriptCore/inspector/InspectorBackendDispatcherCompatibility.cpp:30 > +// This symbol must be exported from JavaScriptCore to maintain binary compatibility with iOS12. I’d like the other implicit half of this to be explicitly stated. "We can, and should, remove this entire file once we no longer need binary compatibility with iOS 12."
(In reply to Darin Adler from comment #21) > Comment on attachment 368192 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=368192&action=review > > > Source/JavaScriptCore/ChangeLog:19 > > + * inspector/InspectorBackendDispatcherCompatibility.cpp: Added. > > + (Inspector::BackendDispatcher::reportProtocolError): > > + * inspector/InspectorBackendDispatcherCompatibility.h: Added. > > I think the contents of InspectorBackendDispatcherCompatibility.h could all > be entirely within InspectorBackendDispatcherCompatibility.cpp; we don’t > need two files. Aha! I initially had it as an inline function that wasn't exporting a symbol and I thought I needed a header. I actually just needed the definition separate from the declaration, even if in the same file. > > Source/JavaScriptCore/inspector/InspectorBackendDispatcherCompatibility.cpp:30 > > +// This symbol must be exported from JavaScriptCore to maintain binary compatibility with iOS12. > > I’d like the other implicit half of this to be explicitly stated. "We can, > and should, remove this entire file once we no longer need binary > compatibility with iOS 12." Ok!
Created attachment 368246 [details] Patch
http://trac.webkit.org/r244652