Bug 197161 - Remove DeprecatedOptional
Summary: Remove DeprecatedOptional
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on: 197219
Blocks:
  Show dependency treegraph
 
Reported: 2019-04-22 09:07 PDT by Alex Christensen
Modified: 2019-04-25 10:37 PDT (History)
9 users (show)

See Also:


Attachments
Patch (9.68 KB, patch)
2019-04-22 09:09 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (4.91 MB, application/zip)
2019-04-22 14:17 PDT, Build Bot
no flags Details
Patch (17.98 KB, patch)
2019-04-24 11:58 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (17.98 KB, patch)
2019-04-24 15:33 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (16.33 KB, patch)
2019-04-25 10:35 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-04-22 09:07:59 PDT
Remove DeprecatedOptional
Comment 1 Alex Christensen 2019-04-22 09:09:46 PDT
Created attachment 367942 [details]
Patch
Comment 2 Chris Dumez 2019-04-22 09:14:24 PDT
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 3 Darin Adler 2019-04-22 12:45:44 PDT
Comment on attachment 367942 [details]
Patch

Oops, I didn’t see the comment from Chris when marking this as reviewed
Comment 4 Alex Christensen 2019-04-22 13:36:31 PDT
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.
Comment 5 Devin Rousso 2019-04-22 13:41:56 PDT
(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.
Comment 6 Chris Dumez 2019-04-22 13:48:10 PDT
(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.
Comment 7 Chris Dumez 2019-04-22 13:49:27 PDT
Anyway, I do not mind trying but please monitor the bots after landing. Worse case scenario we roll it out.
Comment 8 Build Bot 2019-04-22 14:17:17 PDT
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
Comment 9 Build Bot 2019-04-22 14:17:19 PDT
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 10 Alex Christensen 2019-04-23 12:58:29 PDT
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 11 WebKit Commit Bot 2019-04-23 13:26:05 PDT
Comment on attachment 367942 [details]
Patch

Clearing flags on attachment: 367942

Committed r244558: <https://trac.webkit.org/changeset/244558>
Comment 12 WebKit Commit Bot 2019-04-23 13:26:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-04-23 13:27:22 PDT
<rdar://problem/50141786>
Comment 14 Shawn Roberts 2019-04-23 17:25:21 PDT
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
Comment 15 WebKit Commit Bot 2019-04-23 17:26:39 PDT
Re-opened since this is blocked by bug 197219
Comment 16 Alex Christensen 2019-04-24 11:58:24 PDT
Created attachment 368154 [details]
Patch
Comment 17 Build Bot 2019-04-24 12:01:23 PDT
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.
Comment 18 Alex Christensen 2019-04-24 15:33:04 PDT
Created attachment 368192 [details]
Patch
Comment 19 Build Bot 2019-04-24 15:34:20 PDT
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.
Comment 20 Anders Carlsson 2019-04-25 08:01:41 PDT
"iOS12" should say "iOS 12".
Comment 21 Darin Adler 2019-04-25 08:59:11 PDT
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."
Comment 22 Alex Christensen 2019-04-25 10:35:34 PDT
(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!
Comment 23 Alex Christensen 2019-04-25 10:35:53 PDT
Created attachment 368246 [details]
Patch
Comment 24 Alex Christensen 2019-04-25 10:37:10 PDT
http://trac.webkit.org/r244652