Issue 2796703002: [Devtools] Execution context in console shows product experiment

Issue 2796703002: [Devtools] Execution context in console shows product experiment (Closed)

Created:
3 years, 8 months ago by allada

Modified:
3 years, 7 months ago

Reviewers:
caseq, pfeldman, luoe

CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org

Target Ref:
refs/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

[Devtools] Execution context in console shows product experiment New experiment to show product name for execution contexts in console dropdown. See: http://imgur.com/a/S7od6 R=caseq,pfeldman BUG=707880

Patch Set 1 : [Devtools] Execution context in console shows product experiment #

Patch Set 2 : [Devtools] Execution context in console shows product experiment #

Total comments: 21

Patch Set 3 : changes #

Created: 3 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+114 lines, -13 lines)			Patch
M	third_party/WebKit/Source/devtools/BUILD.gn	View	1	2 chunks	+3 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js	View	1 2	6 chunks	+47 lines, -13 lines	0 comments	Download
A	third_party/WebKit/Source/devtools/front_end/console_frame_product_lookup/ConsoleFrameProductLookup.js	View	1 2	1 chunk	+45 lines, -0 lines	0 comments	Download
A	third_party/WebKit/Source/devtools/front_end/console_frame_product_lookup/module.json	View		1 chunk	+17 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/devtools/front_end/inspector.json	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/Source/devtools/front_end/main/Main.js	View		1 chunk	+1 line, -0 lines	0 comments	Download

Depends on Patchset:

Issue 2788793002 Patch 160001

Messages

Total messages: 9 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

luoe

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Source/devtools/front_end/console_frame_product_lookup/ConsoleFrameProductLookup.js File third_party/WebKit/Source/devtools/front_end/console_frame_product_lookup/ConsoleFrameProductLookup.js (right): https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Source/devtools/front_end/console_frame_product_lookup/ConsoleFrameProductLookup.js#newcode15 third_party/WebKit/Source/devtools/front_end/console_frame_product_lookup/ConsoleFrameProductLookup.js:15: var frameParsedURL = new Common.ParsedURL(frame.url); nit, can we move ...

3 years, 8 months ago (2017-04-04 18:31:02 UTC) #5

caseq

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js File third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js (right): https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js#newcode62 third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:62: label = productName.trimMiddle(15) + ' / ' + label; ...

3 years, 8 months ago (2017-04-04 18:59:40 UTC) #6

pfeldman

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js File third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js (right): https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js#newcode33 third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:33: extension.instance().then(instance => { That's super early. We are creating ...

3 years, 8 months ago (2017-04-04 19:03:16 UTC) #7

pfeldman

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Source/devtools/front_end/inspector.json File third_party/WebKit/Source/devtools/front_end/inspector.json (right): https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Source/devtools/front_end/inspector.json#newcode21 third_party/WebKit/Source/devtools/front_end/inspector.json:21: { "name": "console_frame_product_lookup", "condition": "!v8only", "type": "remote" }, On ...

3 years, 8 months ago (2017-04-04 19:06:56 UTC) #8

allada

3 years, 8 months ago (2017-04-05 01:49:46 UTC) #9

PTaL

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(right):

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:33:
extension.instance().then(instance => {
On 2017/04/04 19:03:15, pfeldman wrote:
> That's super early. We are creating this all upon front-end creation. We
should
> delay this by default + trigger on demand (wasShown).

Done.

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:62:
label = productName.trimMiddle(15) + ' / ' + label;
On 2017/04/04 18:59:40, caseq wrote:
> Why not trimEnd()? Also,
> label = Common.UIString('%s / %s', ..., label);

We can trimEnd() I was playing around with it and I thought it looked better
when trimMiddle mostly because "..." does not look good next to "/".

Done.

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/console_frame_product_lookup/ConsoleFrameProductLookup.js
(right):

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_frame_product_lookup/ConsoleFrameProductLookup.js:15:
var frameParsedURL = new Common.ParsedURL(frame.url);
On 2017/04/04 19:03:16, pfeldman wrote:
> +1. also, we seem to not be saving much via passing parsed URL into the
> registry. Maybe pass a string there? Also enables better caching.

I wanted to keep it using a url because right now we only use domain out of it,
but we do have some data we might be able to use to be more specific with url
pattern matching.

Also, the primary user of the ProductRegistry are SDK.NetworkRequest's.
SDK.NetworkRequests have a parsedURL stored inside.

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_frame_product_lookup/ConsoleFrameProductLookup.js:15:
var frameParsedURL = new Common.ParsedURL(frame.url);
On 2017/04/04 18:31:02, luoe wrote:
> nit, can we move this inside the 'if' block, same as L38?

Done.

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_frame_product_lookup/ConsoleFrameProductLookup.js:20:
// We are not caching the frame url result because it may change.
On 2017/04/04 19:03:16, pfeldman wrote:
> What does this mean?

The frame's URL may change, but the creator of the frame never changes. So we
can cache the stuff below because it's only the "who created you" not the "who
are you" which is the code above. the "who are you" can be changed if the frame
gets navigated.

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_frame_product_lookup/ConsoleFrameProductLookup.js:32:
_lookupFrameStacktraceName(frame) {
On 2017/04/04 19:03:16, pfeldman wrote:
> Looks like static method.

Done.

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_frame_product_lookup/ConsoleFrameProductLookup.js:35:
while (stackTrace) {
On 2017/04/04 18:59:40, caseq wrote:
> nit: consider for instead of while.

Done.

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_frame_product_lookup/ConsoleFrameProductLookup.js:36:
for (var stack of stackTrace.callFrames) {
On 2017/04/04 18:31:02, luoe wrote:
> Can we rename 'stack' to 'callFrame' here?

Done.

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_frame_product_lookup/ConsoleFrameProductLookup.js:42:
stackTrace = frame.parent;
On 2017/04/04 18:59:40, caseq wrote:
> frame.parent does not exist. Also, what type do you expect it to be?

Sorry, I meant stackTrace.parent. Fixed. It should be a
{?Protocol.Runtime.StackTrace}.

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/inspector.json (right):

https://codereview.chromium.org/2796703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/inspector.json:21: { "name":
"console_frame_product_lookup", "condition": "!v8only", "type": "remote" },
On 2017/04/04 19:06:56, pfeldman wrote:
> On 2017/04/04 19:03:16, pfeldman wrote:
> > There is no reason for this to not work for node. Along with the product
> module
> > itself. I.e. drop the !v8only.
> 
> ignore me, there are no contexts we could name nicely in node.

Acknowledged.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages