Bug 197161 – Remove DeprecatedOptional

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

[Alex Christensen]

Remove DeprecatedOptional

[Alex Christensen]

Created attachment 367942 [details]
Patch

[Chris Dumez]

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.

[Darin Adler]

Comment on attachment 367942 [details]
Patch

Oops, I didn’t see the comment from Chris when marking this as reviewed

[Alex Christensen]

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.

[Devin Rousso]

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

[Chris Dumez]

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

[Chris Dumez]

Anyway, I do not mind trying but please monitor the bots after landing. Worse case scenario we roll it out.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Alex Christensen]

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.

[WebKit Commit Bot]

Comment on attachment 367942 [details]
Patch

Clearing flags on attachment: 367942

Committed r244558: <https://trac.webkit.org/changeset/244558>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50141786>

[Shawn Roberts]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 197219

[Alex Christensen]

Created attachment 368154 [details]
Patch

[EWS Watchlist]

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.

[Alex Christensen]

Created attachment 368192 [details]
Patch

[EWS Watchlist]

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.

[Anders Carlsson]

"iOS12" should say "iOS 12".

[Darin Adler]

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

[Alex Christensen]

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

[Alex Christensen]

Created attachment 368246 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r244652