Bug 157483 - Web Inspector: Console Evaluation links in the Console should not show normally
Summary: Web Inspector: Console Evaluation links in the Console should not show normally
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: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-09 12:41 PDT by Timothy Hatcher
Modified: 2017-03-16 19:14 PDT (History)
6 users (show)

See Also:


Attachments
Screenshot (15.70 KB, image/png)
2016-05-09 12:41 PDT, Timothy Hatcher
no flags Details
Patch (1.65 KB, patch)
2017-03-16 14:28 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (219.87 KB, image/png)
2017-03-16 14:28 PDT, Devin Rousso
no flags Details
Patch (6.37 KB, patch)
2017-03-16 14:55 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (6.08 KB, patch)
2017-03-16 16:27 PDT, Devin Rousso
mattbaker: review+
Details | Formatted Diff | Diff
Patch (5.46 KB, patch)
2017-03-16 18:33 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2016-05-09 12:41:21 PDT
Created attachment 278426 [details]
Screenshot

We should not show the console.log (etc) location for direct calls when a result has not been returned yet. This would let the location show up for async logs later still.
Comment 1 Radar WebKit Bug Importer 2016-05-09 12:41:57 PDT
<rdar://problem/26177460>
Comment 2 Devin Rousso 2017-03-16 14:28:03 PDT
Created attachment 304691 [details]
Patch
Comment 3 Devin Rousso 2017-03-16 14:28:17 PDT
Created attachment 304692 [details]
[Image] After Patch is applied
Comment 4 Timothy Hatcher 2017-03-16 14:35:34 PDT
I'd argue we drop the whole right hand thing, not jus the location. I think it is only useful when it is a function call defined in the console that gets called out of line.

Like:

> setTimeout(100, function() { console.log("Here") })
< undefined
L: "Here"    [S] Console Evaluation — ConsoleEvaluation:1:13


Nothing else would have the [S] Console Evaluation line.
Comment 5 Matt Baker 2017-03-16 14:36:37 PDT
Comment on attachment 304691 [details]
Patch

Subtitle should only be created if we're linking to a source location. Also the cursor when hovering Console Evaluation should be the default, instead of a pointer (hand).
Comment 6 Matt Baker 2017-03-16 14:37:15 PDT
(In reply to comment #4)
> I'd argue we drop the whole right hand thing, not jus the location. I think
> it is only useful when it is a function call defined in the console that
> gets called out of line.
> 
> Like:
> 
> > setTimeout(100, function() { console.log("Here") })
> < undefined
> L: "Here"    [S] Console Evaluation — ConsoleEvaluation:1:13
> 
> 
> Nothing else would have the [S] Console Evaluation line.

Let's drop it!
Comment 7 Devin Rousso 2017-03-16 14:55:51 PDT
Created attachment 304699 [details]
Patch
Comment 8 Matt Baker 2017-03-16 15:37:26 PDT
Comment on attachment 304699 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:266
> +        return new WebInspector.CallFrame(target, id, sourceCodeLocation, functionName, thisObject, scopeChain, nativeCode, programCode, isTailDeleted, isConsoleEvaluation);

I think this change is unnecessary. Why not have CallFrame determine `isConsoleEvaluation` in its constructor?
Comment 9 Matt Baker 2017-03-16 15:40:48 PDT
Comment on attachment 304699 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:259
>              programCode = true;

This can also move into CallFrame's constructor, guaranteeing that `isConsoleEvaluation`, `functionName`, and `programCode` are internally consistent.
Comment 10 Devin Rousso 2017-03-16 16:27:24 PDT
Created attachment 304717 [details]
Patch
Comment 11 Matt Baker 2017-03-16 16:43:00 PDT
Comment on attachment 304717 [details]
Patch

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

Looks good, r=me with a nit.

> Source/WebInspectorUI/UserInterface/Models/CallFrame.js:52
> +        this._isConsoleEvaluation = isConsoleEvaluation || false;

Don't need to OR with false, `isConsoleEvaluation` is a boolean by definition.
Comment 12 Devin Rousso 2017-03-16 18:33:09 PDT
Created attachment 304736 [details]
Patch
Comment 13 WebKit Commit Bot 2017-03-16 19:14:31 PDT
Comment on attachment 304736 [details]
Patch

Clearing flags on attachment: 304736

Committed r214093: <http://trac.webkit.org/changeset/214093>
Comment 14 WebKit Commit Bot 2017-03-16 19:14:43 PDT
All reviewed patches have been landed.  Closing bug.