Add rep for Errors

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Developer Tools: Shared Components
RESOLVED FIXED
8 months ago
4 months ago

People

(Reporter: ntim, Assigned: nchevobbe)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 months ago
Support the following:
Error
EvalError
InternalError
RangeError
ReferenceError
SyntaxError
TypeError
URIError


Testcase:
var e = new Error("foo");

Expected output (old console):
Error: foo
Stack trace:
@debugger eval code:1:5

Chrome does something similar as well.

Actual output (new console):
Error {  }
Comment hidden (mozreview-request)
(Reporter)

Comment 2

8 months ago
mozreview-review
Comment on attachment 8809527 [details]
Bug 1316225 - Add a Rep for Error objects;

https://reviewboard.mozilla.org/r/92090/#review92084

::: devtools/client/shared/components/reps/error.js:37
(Diff revision 1)
> +      let name = preview && preview.name
> +        ? preview.name
> +        : "Error";
> +
> +      let content = this.props.mode === "tiny"
> +        ? `${name}`

`${name}` is just name

::: devtools/client/shared/components/test/mochitest/test_reps_error.html:61
(Diff revision 1)
> +      "@debugger eval code:1:13\n",
> +      "Error Rep has expected text content for a simple error");
> +
> +    const tinyRenderedComponent = renderComponent(ErrorRep.rep, {object: stub, mode: "tiny"});
> +    is(tinyRenderedComponent.textContent,
> +      `Error`,

nit: Don't use template string here (eslint should complain about this)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

8 months ago
mozreview-review-reply
Comment on attachment 8809527 [details]
Bug 1316225 - Add a Rep for Error objects;

https://reviewboard.mozilla.org/r/92090/#review92084

> `${name}` is just name

urgh

> nit: Don't use template string here (eslint should complain about this)

I don't have any eslint error with this.
What's the rule name ?
(Assignee)

Comment 5

8 months ago
Comment on attachment 8809527 [details]
Bug 1316225 - Add a Rep for Error objects;

TRY revealed errors in the JSON view.
In the Error rep, we use L10N, it is not available for the JSON view.
There are some special webpack magic with require.context in it (http://searchfox.org/mozilla-central/source/devtools/shared/l10n.js#29), which I don't know how to deal with for now.

An ugly but quick workaround would be to require the l10N directly in the render function of the Error Rep, since I think we can't have Error objects in the JSON view.
Attachment #8809527 - Flags: review?(odvarko)
(Assignee)

Comment 6

8 months ago
Created attachment 8809869 [details] [diff] [review]
Bug_1316225.diff

jryans, as discussed on IRC here's a WIP patch
Attachment #8809527 - Attachment is obsolete: true
Attachment #8809869 - Flags: feedback?
Attachment #8809869 - Flags: feedback? → feedback?(jryans)
Comment on attachment 8809869 [details] [diff] [review]
Bug_1316225.diff

The main complication here is that the JSON View doesn't have an easy way to request the properties file.  l10n files are packaged as chrome:// in a regular build, so the JSON View can't just request arbitrary l10n files at the moment.

In the interest of moving this current bug forward, I would suggest just inlining the one string you want to use into the Error rep module, with a comment pointing to a follow up bug to make this better.  (To be honest, it's not clear to me that localizing the string "Stack trace:" is actually helping anyone, but surely we'll eventually have some other string we really do want to localize.)

Here are some approaches that would solve this for real:

1. For the JSON View's own l10n file, we load[1] those strings in the chrome module that manages the JSON View and pass it down to the content window, giving it access to the strings.  We could do something similar to make the "raw!" loader plugin work for general l10n files.

2. We could convert JSON View to use Webpack, similar to what's in progress for the Inspector in bug 1291049.  This would mean the properties get folded into the Webpack bundle and then can be loaded from there.  (Of course, this is my least favorite idea, but I am probably the only one who feels that way at this point...)

:bgrins, any thoughts about this?

[1]: http://searchfox.org/mozilla-central/rev/846adaea6ccd1428780ed47d0d94da18478acdbb/devtools/client/jsonview/converter-child.js#140
Flags: needinfo?(bgrinstead)
Attachment #8809869 - Flags: feedback?(jryans)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

7 months ago
TRY is fine https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f41b597b29b47186edb639ac6ff45613371e0dc and I filed Bug 1317038 to deal with localization in the JSON View.

Comment 10

7 months ago
mozreview-review
Comment on attachment 8810197 [details]
Bug 1316225 - Add a Rep for Error objects;

https://reviewboard.mozilla.org/r/92568/#review92694

R+, but one question.

How hard it would be to make the stack trace expandable?
The stack trace info is often big and can mess up all
the other logs.

Honza
Attachment #8810197 - Flags: review?(odvarko) → review+
(Assignee)

Comment 11

7 months ago
> How hard it would be to make the stack trace expandable? The stack trace info is often big and can mess up all the other logs.

I guess it wouldn't be too hard. We could have the same kind of mechanism we have to display errors in the console, or something like the longstring, where we crop the stack message and allow the user to expand it.
This also mean we should then implement the "expand" function in the panel that use Reps (except JSON View, we shouldn't have an error object there).

What I wonder now is why we do show them like this in the old console, maybe there was some kind of rationale behind that.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)
> > How hard it would be to make the stack trace expandable? The stack trace info is often big and can mess up all the other logs.
> 
> I guess it wouldn't be too hard. We could have the same kind of mechanism we
> have to display errors in the console, or something like the longstring,
> where we crop the stack message and allow the user to expand it.
Yeah, and it would be great if every stack-frame URL is clickable link that can navigate the user to the source (in Debugger panel).

> This also mean we should then implement the "expand" function in the panel
> that use Reps (except JSON View, we shouldn't have an error object there).
Yes, some kind of a generic component that can collapse/expand provided Rep.
Such component ould expect that provided rep is 'expandable' and is,
for example, able to provide child reps.

> What I wonder now is why we do show them like this in the old console, maybe
> there was some kind of rationale behind that.
I don't know (my feeling was always that it's just not done yet)

Honza
(Assignee)

Comment 13

7 months ago
FWIW, Chrome has a really simple text expander (like we have for long string in the old console), it might be good to have proper stack trace, using the frame component we're already using in the new console.
If you don't mind, I'm going to create a follow up for this bug, since this one is a blocker for shipping the new console in Developer Edition.
(Assignee)

Updated

7 months ago
Attachment #8809869 - Attachment is obsolete: true

Comment 14

7 months ago
Pushed by chevobbe.nicolas@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9c15c0a68e00
Add a Rep for Error objects; r=Honza
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13)
> FWIW, Chrome has a really simple text expander (like we have for long string
> in the old console), it might be good to have proper stack trace, using the
> frame component we're already using in the new console.
> If you don't mind, I'm going to create a follow up for this bug, since this
> one is a blocker for shipping the new console in Developer Edition.
Definitely agree, I was also thinking about a follow up for this functionality.

Honza
(Assignee)

Updated

7 months ago
See Also: → bug 1317325
(Assignee)

Comment 16

7 months ago
Filed Bug 1317325 to enhance the ErrorRep
(Assignee)

Updated

7 months ago
Blocks: 1317325
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Comment on attachment 8809869 [details] [diff] [review]
> Bug_1316225.diff
> 
> The main complication here is that the JSON View doesn't have an easy way to
> request the properties file.  l10n files are packaged as chrome:// in a
> regular build, so the JSON View can't just request arbitrary l10n files at
> the moment.
> 
> In the interest of moving this current bug forward, I would suggest just
> inlining the one string you want to use into the Error rep module, with a
> comment pointing to a follow up bug to make this better.  (To be honest,
> it's not clear to me that localizing the string "Stack trace:" is actually
> helping anyone, but surely we'll eventually have some other string we really
> do want to localize.)

I agree, let's inline the string at least for now - it's jargon and we'd likely leave a localization comment not to change it anyway.

> Here are some approaches that would solve this for real:
> 
> 1. For the JSON View's own l10n file, we load[1] those strings in the chrome
> module that manages the JSON View and pass it down to the content window,
> giving it access to the strings.  We could do something similar to make the
> "raw!" loader plugin work for general l10n files.
> 
> 2. We could convert JSON View to use Webpack, similar to what's in progress
> for the Inspector in bug 1291049.  This would mean the properties get folded
> into the Webpack bundle and then can be loaded from there.  (Of course, this
> is my least favorite idea, but I am probably the only one who feels that way
> at this point...)

Note that webpack packaging for the inspector is for local development in a tab only.  We aren't shipping a bundle inside the toolbox.

I'm going to forward the ni? to Julian to see if he has thoughts.  I will say as far as (1), if we could have the raw! syntax working for general l10 files I believe we could plug that into the properties parser and localization helper added for devtools.html.
Flags: needinfo?(bgrinstead) → needinfo?(jdescottes)
(In reply to Brian Grinstead [:bgrins] from comment #17)
> I'm going to forward the ni? to Julian to see if he has thoughts.  I will
> say as far as (1), if we could have the raw! syntax working for general l10
> files I believe we could plug that into the properties parser and
> localization helper added for devtools.html.

Right, :nchevobbe was already attempting to use the LocalizationHelper here in the Error rep as part of attachment 8809869 [details] [diff] [review].  I agree those systems should work assuming we get raw! loading to work for the JSON View.
Status: NEW → ASSIGNED

Comment 19

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9c15c0a68e00
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
[Not sure to really understand what you mean by "have the raw! syntax working for general l10n files", so I might be answering off-topic.]

I think here we will need, as jryans suggested, to download the l10n file from the chrome module. Then I see 2 options to reuse it in the JSON view:

1: LocalizationHelper is both responsible for loading and parsing the l10n files. We could easily change this to directly build a LocalizationHelper using a bundle.
2: we could preload a cache of raw files from the jsonview chrome module and create a requirejs addon that would lookup in this cache when encountering a raw! url? I don't know much about the JSON View, so maybe this is technically not feasible? But this way we wouldn't have to touch the LocalizationHelper.
Flags: needinfo?(jdescottes)

Comment 21

4 months ago
I have reproduced this bug with Firefox nightly 52.0a1 (build id:20161108030212)on
windows 7(64 bit)

I have verified this bug as fixed with Firefox aurora 53.0a2 (build id:20170302004002)
User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0

 [bugday-20170301]
You need to log in before you can comment on or make changes to this bug.