Bug 148603 - Web Inspector: Debugger Popovers should work for object literal shorthand variables
Summary: Web Inspector: Debugger Popovers should work for object literal shorthand var...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-08-28 20:07 PDT by Joseph Pecoraro
Modified: 2015-08-31 15:50 PDT (History)
9 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (5.00 KB, patch)
2015-08-28 20:10 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-08-28 20:07:56 PDT
* SUMMARY
Debugger Popovers should work for object literal shorthand variables.

* TEST
>  1. var lineNumber = 10;
>  2. var columnNumber = 20;
>  3. var o0 = { lineNumber: lineNumber, columnNumber: columnNumber };
>  4. var o1 = { lineNumber, columnNumber };
>  5. var o2 = { lineNumber , columnNumber };
>  6. var o2 = {
>  7.     lineNumber
>  8.     ,
>  9.     columnNumber
> 10. };
> 11. console.log(o1, o2);

* STEPS TO REPRODUCE
1. Pause on line 11 in the test case.
2. Hover lineNumber/columnNumber anywhere it is treated as a variable
  => Expected to see their values, but got nothing on lines 4-10

* NOTES
- We should really eventually move to having an Esprima AST where it makes sense, like here. The AST Node for this position would just give us the info we want!
Comment 1 Radar WebKit Bug Importer 2015-08-28 20:08:36 PDT
<rdar://problem/22487471>
Comment 2 Joseph Pecoraro 2015-08-28 20:10:18 PDT
Created attachment 260206 [details]
[PATCH] Proposed Fix
Comment 3 Timothy Hatcher 2015-08-31 10:12:18 PDT
Comment on attachment 260206 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:558
> +    let state = CodeMirror.copyState(mode, cm.getTokenAt(initialPosition).state);

Why does it need to be a copy? Because you use StringStream?

Why can't we walk the existing tokens that are already parsed by the CodeMirror instance? CodeMirrorCompletionController and CodeMirrorTokenTrackingController loop using getTokenAt.
Comment 4 Joseph Pecoraro 2015-08-31 12:23:35 PDT
Comment on attachment 260206 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:558
>> +    let state = CodeMirror.copyState(mode, cm.getTokenAt(initialPosition).state);
> 
> Why does it need to be a copy? Because you use StringStream?
> 
> Why can't we walk the existing tokens that are already parsed by the CodeMirror instance? CodeMirrorCompletionController and CodeMirrorTokenTrackingController loop using getTokenAt.

I suppose it doesn't need to be a copy, but as a generic algorithm I wanted it to be safe.

When I tried getTokenAt(...) it was skipping over whitespace and just giving me the next token. In this case, it wasn't giving me a token for the comma. mode.token() gives me the token type but more importantly each substring, so I could detect the "," or "}" needed here.
Comment 5 WebKit Commit Bot 2015-08-31 15:50:02 PDT
Comment on attachment 260206 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 260206

Committed r189190: <http://trac.webkit.org/changeset/189190>
Comment 6 WebKit Commit Bot 2015-08-31 15:50:05 PDT
All reviewed patches have been landed.  Closing bug.