Bug 137279 – Fixed the Inspector to be able to properly distinguish between scope types

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

The pre-existing code incorrectly labels Catch Scopes and Function Name Scopes as With Scopes.  This patch will fix this.

[Mark Lam]

Created attachment 238980 [details]
the patch.

[WebKit Commit Bot]

Attachment 238980 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/JSJavaScriptCallFrame.h:80:  FUNCTION_NAME_SCOPE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

(In reply to comment #2)
> ERROR: Source/JavaScriptCore/inspector/JSJavaScriptCallFrame.h:80:  FUNCTION_NAME_SCOPE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Total errors found: 1 in 15 files

I named it FUNCTION_NAME_SCOPE to be consistent with its pre-existing peers.  We can rename them all later if desired.

[Geoffrey Garen]

Comment on attachment 238980 [details]
the patch.

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

> Source/JavaScriptCore/runtime/JSNameScope.h:70
> +        m_isCatchScope = !!(attributes & CatchScope);

It's a bad abstraction to use a property attribute to discover that an object is a catch scope. The two abstractions are unrelated, and relating them makes for spaghetti code.

Instead, you should use an explicit parameter to the JSNameScope constructor. The VM knows when it allocates a catch scope, and it only does so in a few places.

[Joseph Pecoraro]

Comment on attachment 238980 [details]
the patch.

You could add a test for this. Do we have any existing tests for Scope Chain?

The inspector pieces look fine to me.

[Mark Lam]

Created attachment 239052 [details]
patch 2: rework JSNameScope to take an explicit type argument.

[WebKit Commit Bot]

Attachment 239052 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/JSJavaScriptCallFrame.h:80:  FUNCTION_NAME_SCOPE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 239052 [details]
patch 2: rework JSNameScope to take an explicit type argument.

This breaks the ARMv7 build.  Will fix and then post another patch.

[Mark Lam]

Created attachment 239069 [details]
patch 3: fixed ARM builds

[WebKit Commit Bot]

Attachment 239069 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/JSJavaScriptCallFrame.h:80:  FUNCTION_NAME_SCOPE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 239069 [details]
patch 3: fixed ARM builds

r=me

[Mark Lam]

Thanks for the reviews.  Landed in r174216: <http://trac.webkit.org/r174216>.

[Brent Fulgham]

This broke the 64-bit Windows build.

[Brent Fulgham]

argumentGPR4 is not defined for 64-bit Windows. This is "regT6" on other platforms, but "regT6" is called "argumentGPR2" on Windows.

How do we fix this?

Should I roll it out?

[Mark Lam]

(In reply to comment #14)
> argumentGPR4 is not defined for 64-bit Windows. This is "regT6" on other platforms, but "regT6" is called "argumentGPR2" on Windows.

I'll have a fix coming soon.  Currently testing.

[Mark Lam]

Build fix landed in r174360: <http://trac.webkit.org/r174360>.