Bug 180664 – Web Inspector: fix test case failures in js-isLikelyStackTrace.html

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

Rewriting this to be async-friendly has revealed that either the test or implementation is incorrect in 1/2 the test cases.

[Joseph Pecoraro]

Created attachment 349439 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 349439 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Models/StackTrace.js:88
> +            WI.StackTrace.likelyStackTraceRegex = new RegExp(`^${stackTraceLine}([\\n\\r]${stackTraceLine})+$`, "g");

Err, given this is a /^...$/ regex the "g" part is unnecessary... I'll remove that.

[Joseph Pecoraro]

Created attachment 349441 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 349441 [details]
[PATCH] Proposed Fix

I'm going to add a few explicit test cases instead of generating most of them.

[Joseph Pecoraro]

Created attachment 349444 [details]
[PATCH] Proposed Fix

Reviewable now!

[Devin Rousso]

Comment on attachment 349444 [details]
[PATCH] Proposed Fix

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

> LayoutTests/inspector/console/js-isLikelyStackTrace.html:-5
> -function typeErrorWrap()

So I'm not sure how much of a difference this makes, but this function introduced another frame (both `typeErrorWrap` and `typeError`).  Does that make a difference?  The wrapped test used to fail whereas the "unwrapped" version doesn't, so I'm wondering if that made a difference to our heuristic somehow.

> LayoutTests/inspector/console/js-isLikelyStackTrace.html:10
>          console.trace();

Is this necessary?

> LayoutTests/inspector/console/js-isLikelyStackTrace.html:74
> +                "a@file:///ex.html:10:6\nb@file:///ex.html:7:6\nglobal code@file://ex.html:12:2",

Did you mean to repeat this twice?

> Source/WebInspectorUI/UserInterface/Models/StackTrace.js:83
> +        if (!WI.StackTrace.likelyStackTraceRegex) {

NIT: this should begin with a `_` since it is "private":

    WI.StackTrace._likelyStackTraceRegex

[Joseph Pecoraro]

Comment on attachment 349444 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/console/js-isLikelyStackTrace.html:-5
>> -function typeErrorWrap()
> 
> So I'm not sure how much of a difference this makes, but this function introduced another frame (both `typeErrorWrap` and `typeError`).  Does that make a difference?  The wrapped test used to fail whereas the "unwrapped" version doesn't, so I'm wondering if that made a difference to our heuristic somehow.

It shouldn't make a difference. There are still at least 2 frames (global code and typeError()).

>> LayoutTests/inspector/console/js-isLikelyStackTrace.html:10
>>          console.trace();
> 
> Is this necessary?

It doesn't affect the test, but it looks nice when loading the test locally and debugging it.

>> LayoutTests/inspector/console/js-isLikelyStackTrace.html:74
>> +                "a@file:///ex.html:10:6\nb@file:///ex.html:7:6\nglobal code@file://ex.html:12:2",
> 
> Did you mean to repeat this twice?

Err, no an undo issue I think. This one was supposed to have moved the "global code" to the first frame to ensure things continue to work like that.

>> Source/WebInspectorUI/UserInterface/Models/StackTrace.js:83
>> +        if (!WI.StackTrace.likelyStackTraceRegex) {
> 
> NIT: this should begin with a `_` since it is "private":
> 
>     WI.StackTrace._likelyStackTraceRegex

kk

[Joseph Pecoraro]

Created attachment 349490 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 349490 [details]
[PATCH] Proposed Fix

r=me

[WebKit Commit Bot]

Comment on attachment 349490 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 349490

Committed r235939: <https://trac.webkit.org/changeset/235939>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44385932>