Bug 226433

Summary: Remove WTF::Optional synonym for std::optional, using that class template directly instead
Product: WebKit Reporter: Darin Adler <darin>
Component: WebKit Misc.Assignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, alecflett, allan.jensen, apinheiro, bburg, beidson, benjamin, berto, calvaris, cdumez, cfleizach, cgarcia, changseok, cmarcelo, dbarton, dino, dmazzoni, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, fred.wang, galpeter, glenn, graouts, gustavo, gyuyoung.kim, hi, hta, jamesr, japhet, jbedard, jcraig, jdiggs, jer.noble, jfernandez, jiewen_tan, joepeck, jsbell, kangil.han, keith_miller, kondapallykalyan, luiz, macpherson, mark.lam, mcatanzaro, menard, mifenton, mmaxfield, msaboff, pdr, philipj, pnormand, rego, ryanhaddad, saam, sabouhallawa, samuel_white, sam, schenney, sergio, simon.fraser, svillar, tommyw, tonikitoo, toyoshim, tzagallo, vjaquez, webkit-bug-importer, youennf, yutak
Priority: P3 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 226437    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
cdumez: review+, ews-feeder: commit-queue-
Patch none

Description Darin Adler 2021-05-29 22:36:56 PDT
Remove WTF::Optional synonym for std::optional, using that class template directly instead
Comment 1 Darin Adler 2021-05-29 23:02:49 PDT Comment hidden (obsolete)
Comment 2 EWS Watchlist 2021-05-29 23:05:10 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)
Comment 3 Darin Adler 2021-05-29 23:31:09 PDT
Created attachment 430131 [details]
Patch
Comment 4 Chris Dumez 2021-05-29 23:37:49 PDT
Comment on attachment 430131 [details]
Patch

R=me once the bots are happy.
Comment 5 Darin Adler 2021-05-30 00:02:43 PDT
Created attachment 430132 [details]
Patch
Comment 6 EWS 2021-05-30 09:12:09 PDT
Committed r278253 (238290@main): <https://commits.webkit.org/238290@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430132 [details].
Comment 7 Radar WebKit Bug Importer 2021-05-30 09:13:20 PDT
<rdar://problem/78664888>
Comment 8 Chris Dumez 2021-05-30 12:02:53 PDT
Seems to have broken the Windows build:
https://build.webkit.org/#/builders/67/builds/3216/steps/8/logs/errors

The issue is not super obvious to me.
Comment 9 Chris Dumez 2021-05-30 12:10:15 PDT
(In reply to Chris Dumez from comment #8)
> Seems to have broken the Windows build:
> https://build.webkit.org/#/builders/67/builds/3216/steps/8/logs/errors
> 
> The issue is not super obvious to me.

The include is there so I am wondering if there is a `#define optional` somewhere that is polluting us?
Comment 10 Chris Dumez 2021-05-30 12:37:42 PDT
(In reply to Chris Dumez from comment #9)
> (In reply to Chris Dumez from comment #8)
> > Seems to have broken the Windows build:
> > https://build.webkit.org/#/builders/67/builds/3216/steps/8/logs/errors
> > 
> > The issue is not super obvious to me.
> 
> The include is there so I am wondering if there is a `#define optional`
> somewhere that is polluting us?

Or maybe that bot just needs a clean build. Some of the build output still refers to "Optional":
'Inspector::Protocol::Helpers::Optional': definition of dllimport data not allowed
Comment 11 Darin Adler 2021-05-30 13:51:38 PDT
Maybe Windows uses a different "erb" file? The EWS Windows build didn’t fail, though!
Comment 12 Chris Dumez 2021-05-30 13:57:26 PDT
(In reply to Darin Adler from comment #11)
> Maybe Windows uses a different "erb" file? The EWS Windows build didn’t
> fail, though!

And the boy recovered on its own on the next build. It was an incremental build issue I believe.
Comment 13 Chris Dumez 2021-06-01 12:04:43 PDT
Follow-up build fix in <https://commits.webkit.org/r278319>.
Comment 14 Chris Dumez 2021-06-01 12:09:48 PDT
(In reply to Chris Dumez from comment #13)
> Follow-up build fix in <https://commits.webkit.org/r278319>.

And <https://commits.webkit.org/r278320>
Comment 15 Michael Catanzaro 2021-06-01 18:09:01 PDT
Very nice!
Comment 16 Simon Fraser (smfr) 2021-06-01 20:43:49 PDT
Will the style checker tell me if I use the old thing? A webkit-dev email is probably justified too.
Comment 17 Darin Adler 2021-06-01 20:45:37 PDT
We can add it to the style checker, but also it won’t compile.
Comment 18 Simon Fraser (smfr) 2021-06-01 20:47:58 PDT
Not compiling seems sufficient. But a webkit-dev heads-up would be welcome.
Comment 19 Darin Adler 2021-06-01 20:56:17 PDT
(In reply to Simon Fraser (smfr) from comment #18)
> Not compiling seems sufficient. But a webkit-dev heads-up would be welcome.

https://lists.webkit.org/pipermail/webkit-dev/2021-June/031875.html