Closed Bug 1316225 Opened 8 years ago Closed 8 years ago

Add rep for Errors

Categories

(DevTools :: Shared Components, defect)

Product:
DevTools
Component:
Shared Components
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: ntim, Assigned: nchevobbe)

References

Details

Attachments

(1 file, 2 obsolete files)

Bug 1316225 - Add a Rep for Error objects;
58 bytes, text/x-review-board-request
Honza
: review+
Details 
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 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 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 ?
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)
Attached patch Bug_1316225.diff (obsolete) — — Splinter Review 
jryans, as discussed on IRC here's a WIP patch
Attachment #8809527 - Attachment is obsolete: true
Attachment #8809869 - Flags: feedback?
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 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+
> 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
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.
Attachment #8809869 - Attachment is obsolete: true
(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
See Also: → 1317325
Filed Bug 1317325 to enhance the ErrorRep
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
https://hg.mozilla.org/mozilla-central/rev/9c15c0a68e00
Status: ASSIGNED → RESOLVED
Closed: 8 years 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)
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]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: