Bug 197133

Summary: Make it possible to control the renderTreeAsText output by setting options on testRunner
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, commit-queue, darin, ews-watchlist, koivisto, rniwa, ryanhaddad, sam, simon.fraser, sroberts, thorton, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 197134    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews200 for win-future
none
Patch
sam: review+, ews-watchlist: commit-queue-
Archive of layout-test-results from ews101 for mac-highsierra
none
Archive of layout-test-results from ews200 for win-future
none
Archive of layout-test-results from ews114 for mac-highsierra
none
Patch
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews204 for win-future none

Description Simon Fraser (smfr) 2019-04-19 20:58:56 PDT 
Make it possible to control the renderTreeAsText output by setting options on testRunner
Comment 1 Simon Fraser (smfr) 2019-04-19 20:59:34 PDT Comment hidden (obsolete)
Comment 2 Simon Fraser (smfr) 2019-04-19 21:02:08 PDT 
Don't have the JSC C API to do this yet (bug 197134).
Comment 3 Simon Fraser (smfr) 2019-04-19 22:29:47 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-04-19 22:32:26 PDT Comment hidden (obsolete)
Comment 5 Simon Fraser (smfr) 2019-04-19 22:34:49 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-04-19 22:37:58 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2019-04-20 06:31:03 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2019-04-20 06:31:05 PDT Comment hidden (obsolete)
Comment 9 Simon Fraser (smfr) 2019-04-20 10:38:28 PDT Comment hidden (obsolete)
Comment 10 EWS Watchlist 2019-04-20 10:42:15 PDT Comment hidden (obsolete)
Comment 11 Simon Fraser (smfr) 2019-04-20 11:02:55 PDT Comment hidden (obsolete)
Comment 12 EWS Watchlist 2019-04-20 11:04:50 PDT Comment hidden (obsolete)
Comment 13 Simon Fraser (smfr) 2019-04-20 11:17:33 PDT Comment hidden (obsolete)
Comment 14 EWS Watchlist 2019-04-20 11:20:22 PDT Comment hidden (obsolete)
Comment 15 Simon Fraser (smfr) 2019-04-20 11:30:13 PDT Comment hidden (obsolete)
Comment 16 EWS Watchlist 2019-04-20 11:32:59 PDT Comment hidden (obsolete)
Comment 17 EWS Watchlist 2019-04-22 02:48:03 PDT Comment hidden (obsolete)
Comment 18 EWS Watchlist 2019-04-22 02:48:13 PDT Comment hidden (obsolete)
Comment 19 Shawn Roberts 2019-04-22 09:09:22 PDT 
EWS bots are hanging after processing this patch. 

Obsoleting the Patch so bots will stop processing.
Comment 20 Simon Fraser (smfr) 2019-04-22 15:38:58 PDT 
Created attachment 367988 [details]
Patch
Comment 21 EWS Watchlist 2019-04-22 15:42:20 PDT Comment hidden (obsolete)
Comment 22 EWS Watchlist 2019-04-23 18:46:32 PDT Comment hidden (obsolete)
Comment 23 EWS Watchlist 2019-04-23 18:46:34 PDT Comment hidden (obsolete)
Comment 24 EWS Watchlist 2019-04-23 19:08:29 PDT Comment hidden (obsolete)
Comment 25 EWS Watchlist 2019-04-23 19:08:40 PDT Comment hidden (obsolete)
Comment 26 EWS Watchlist 2019-04-23 19:41:31 PDT Comment hidden (obsolete)
Comment 27 EWS Watchlist 2019-04-23 19:41:33 PDT Comment hidden (obsolete)
Comment 28 Sam Weinig 2019-04-23 22:26:33 PDT 
Comment on attachment 367988 [details]
Patch

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

> Tools/DumpRenderTree/TestRunner.cpp:115
> +    double options = JSValueToNumber(context, arguments[0], exception);
> +    ASSERT(!*exception);

Seems like this could throw if the value passed in is not convertible to a number. Seems like checking for the exception and returning early would be clearer to test writers than crashing. That may not be what we've done elsewhere, but seems like a better pattern to me.

> Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:95
> +    # Underscores to camel case.
> +    $constantName =~ s{(\w+)}{
> +            ($a = lc $1) =~ s<(^[a-z]|_[a-z])><
> +                ($b = uc $1) =~ s/^_//;
> +                $b;
> +            >eg;
> +            $a;
> +        }eg;

This seems unnecessary. I'd just keep it using underscores, which are totally legal. This is generated code and doesn't have to conform to the normal rules.

> Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:621
> +    my $attributes = [];

Can you call this $attributesAndConstants to make me happier :).

> LayoutTests/fast/harness/render-tree-as-text-options.html:36
> +                    testRunner.RENDER_TREE_SHOW_ALL_LAYERS + 
> +                    testRunner.RENDER_TREE_SHOW_LAYER_NESTING + 

Why '+' rather than the more traditional '|'? Is this the syntax you want, or would rather it be TestRunner.RENDER_TREE_FOO, e.g. on the constructor?
Comment 29 Simon Fraser (smfr) 2019-04-24 09:38:45 PDT Comment hidden (obsolete)
Comment 30 EWS Watchlist 2019-04-24 09:42:27 PDT 
Attachment 368132 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestRunner.cpp:2087:  getRENDER_TREE_SHOW_ALL_LAYERS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2092:  getRENDER_TREE_SHOW_LAYER_NESTING is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2097:  getRENDER_TREE_SHOW_COMPOSITED_LAYERS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2102:  getRENDER_TREE_SHOW_OVERFLOW is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2107:  getRENDER_TREE_SHOW_SVG_GEOMETRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2112:  getRENDER_TREE_SHOW_LAYER_FRAGMENTS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/rendering/RenderTreeAsText.cpp:617:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 7 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 Simon Fraser (smfr) 2019-04-24 09:52:21 PDT 
Created attachment 368135 [details]
Patch
Comment 32 EWS Watchlist 2019-04-24 09:54:36 PDT 
Attachment 368135 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestRunner.cpp:2087:  getRENDER_TREE_SHOW_ALL_LAYERS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2092:  getRENDER_TREE_SHOW_LAYER_NESTING is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2097:  getRENDER_TREE_SHOW_COMPOSITED_LAYERS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2102:  getRENDER_TREE_SHOW_OVERFLOW is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2107:  getRENDER_TREE_SHOW_SVG_GEOMETRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/DumpRenderTree/TestRunner.cpp:2112:  getRENDER_TREE_SHOW_LAYER_FRAGMENTS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/rendering/RenderTreeAsText.cpp:617:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 7 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 33 WebKit Commit Bot 2019-04-24 11:25:32 PDT 
Comment on attachment 368135 [details]
Patch

Clearing flags on attachment: 368135

Committed r244599: <https://trac.webkit.org/changeset/244599>
Comment 34 EWS Watchlist 2019-04-24 11:42:47 PDT 
Comment on attachment 368135 [details]
Patch

Attachment 368135 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11985047

New failing tests:
fast/harness/render-tree-as-text-options.html
Comment 35 EWS Watchlist 2019-04-24 11:42:58 PDT 
Created attachment 368151 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 36 Darin Adler 2019-04-24 12:56:56 PDT 
Comment on attachment 368135 [details]
Patch

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

> Source/WebCore/rendering/RenderTreeAsText.h:41
> +enum class RenderAsTextFlag {

Was nice to modernize this a little. Maybe turn it into an OptionSet later?
Comment 37 Darin Adler 2019-04-24 12:57:26 PDT 
Comment on attachment 368135 [details]
Patch

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

>> Source/WebCore/rendering/RenderTreeAsText.h:41
>> +enum class RenderAsTextFlag {
> 
> Was nice to modernize this a little. Maybe turn it into an OptionSet later?

Oh, I see, that’s what you did!
Comment 38 Ryan Haddad 2019-04-25 12:47:25 PDT 
This landed in https://trac.webkit.org/changeset/244599/webkit
Comment 39 Radar WebKit Bug Importer 2019-04-25 12:48:30 PDT 
<rdar://problem/50215914>