Bug 217390 - Refactor TestOptions code in WebKitTestRunner to make it easier to rationalize and extend
Summary: Refactor TestOptions code in WebKitTestRunner to make it easier to rationaliz...
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: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks: 217297
  Show dependency treegraph
 
Reported: 2020-10-06 09:44 PDT by Sam Weinig
Modified: 2020-10-08 17:45 PDT (History)
3 users (show)

See Also:


Attachments
Patch (98.59 KB, patch)
2020-10-06 09:52 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (99.39 KB, patch)
2020-10-06 20:41 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Path (99.39 KB, patch)
2020-10-07 15:17 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (103.19 KB, patch)
2020-10-08 09:04 PDT, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (104.71 KB, patch)
2020-10-08 12:40 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (104.12 KB, patch)
2020-10-08 17:05 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2020-10-06 09:44:39 PDT
Refactor TestOptions code in WebKitTestRunner to make it easier to rationalize and extend
Comment 1 Sam Weinig 2020-10-06 09:52:05 PDT
Created attachment 410649 [details]
Patch
Comment 2 Darin Adler 2020-10-06 18:09:21 PDT
Comment on attachment 410649 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410649&action=review

I approve the idea of using the standard library in our test tools. However I don’t know what specifically motivates doing more of that here while still having so much code using WTF.

> Tools/WebKitTestRunner/Options.cpp:116
> +static bool parseFeature(String featureString, std::unordered_map<std::string, bool>& features)

Funny that this takes String, not StringView or const String&

> Tools/WebKitTestRunner/TestOptions.cpp:101
> +enum class KeyType {

Perhaps : uint8_t?
Comment 3 Sam Weinig 2020-10-06 20:40:59 PDT
(In reply to Darin Adler from comment #2)
> Comment on attachment 410649 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410649&action=review
> 
> I approve the idea of using the standard library in our test tools. However
> I don’t know what specifically motivates doing more of that here while still
> having so much code using WTF.

I'm mostly trying to ween WebKitTestRunner off of WTF where there are stdlib equivalents we can use, but I haven't been super consistent. Going to do another pass where I do just that.

> 
> > Tools/WebKitTestRunner/Options.cpp:116
> > +static bool parseFeature(String featureString, std::unordered_map<std::string, bool>& features)
> 
> Funny that this takes String, not StringView or const String&

I made it take std::string_view :).

> 
> > Tools/WebKitTestRunner/TestOptions.cpp:101
> > +enum class KeyType {
> 
> Perhaps : uint8_t?

Done.
Comment 4 Sam Weinig 2020-10-06 20:41:11 PDT
Created attachment 410730 [details]
Patch
Comment 5 EWS 2020-10-06 21:20:24 PDT
Committed r268115: <https://trac.webkit.org/changeset/268115>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410730 [details].
Comment 6 Radar WebKit Bug Importer 2020-10-06 21:21:19 PDT
<rdar://problem/70031230>
Comment 7 Karl Rackler 2020-10-07 13:10:31 PDT
Reverted r268115 for reason:

This reverts commit r268115 while investigating ews test failure.

Committed r268143: <https://trac.webkit.org/changeset/268143>
Comment 8 Sam Weinig 2020-10-07 15:17:28 PDT
Created attachment 410790 [details]
Path
Comment 9 Sam Weinig 2020-10-08 09:04:04 PDT
Created attachment 410848 [details]
Patch
Comment 10 Sam Weinig 2020-10-08 10:54:45 PDT
I do not understand this iOS-WebKit2 failure. What in my change is affecting anti-aliasing results?
Comment 11 Sam Weinig 2020-10-08 12:40:21 PDT
Created attachment 410872 [details]
Patch
Comment 12 EWS 2020-10-08 15:35:38 PDT
Tools/Scripts/svn-apply failed to apply attachment 410872 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 13 Sam Weinig 2020-10-08 17:05:21 PDT
Created attachment 410896 [details]
Patch
Comment 14 EWS 2020-10-08 17:45:34 PDT
Committed r268238: <https://trac.webkit.org/changeset/268238>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410896 [details].