Descriptive error messages in l10n.js



5 years ago
5 years ago


(Reporter: timdream, Assigned: stas)


Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

The error thrown in l10n.js is not descriptive enough in adb log. I spent non-trivial time looking for the cause here...

I would like to append a "l10n.js" label in the message.
Summary: Descript error messages in l10n.js → Descriptive error messages in l10n.js
Tim: which message was confusing?

I'm ok with the change, but I'm wondering if we can unify the places where we emit things to adb log.

Stas, what do you think?
Flags: needinfo?(timdream)
Flags: needinfo?(stas)
I am getting message like

E/GeckoConsole( 1527): [JavaScript Error: "ContextError: Context not ready"]

and no lines before or after, when working on bug 1013155. Took me a while to figure out where that is because the object referenced to the remote input field in the keyboard scope is called InputContext.
Flags: needinfo?(timdream)
Ah, good catch. Yeah, we should make it more descriptive. Also, I see a few of those bugs and would love to start hunting them down once we get bug 1000806 fixed because this bug means that there's code that tries to access l10n resources without waiting for them to be ready.

Maybe we can get this message more descriptive in particular - basically saying what I wrote above.
Yeah sure, no worries. Do you guys want to pick up this bug instead of taking my patch?

Comment 6

5 years ago
Posted patch L10nErrorSplinter Review
Thanks for the report and for the patch, Tim!  I'll be happy to take this bug over.

We used to have more descriptive error classes, but due to memory usage constraints, we remove them.  ContextError was the only one that we left, and I agree that the name might not be immediately helpful.

I tried a different approach:  remove ContextError and replace it with L10nError which is available to all of l10n.js.

Unexpected errors now look like this in adb logcat:

  E/GeckoConsole(  974): [JavaScript Error: "L10nError: Context not ready"]
Assignee: timdream → stas
Attachment #8425383 - Attachment is obsolete: true
Attachment #8425383 - Flags: review?(gandalf)
Attachment #8427764 - Flags: review?(gandalf)
Flags: needinfo?(stas)
Comment on attachment 8427764 [details] [diff] [review]

Review of attachment 8427764 [details] [diff] [review]:

can you do a quick b2gperf check on this?
(In reply to Staś Małolepszy :stas from comment #6)
>   E/GeckoConsole(  974): [JavaScript Error: "L10nError: Context not ready"]

Yeah that's good. Thanks!

Comment 9

5 years ago
(In reply to Zibi Braniecki [:gandalf] from comment #7)

> can you do a quick b2gperf check on this?

App       Δ median  Δ mean  Sig?
--------  --------  ------  ----
Browser          1       7      
Calendar        -5     -17      
Camera           1      -1      
Clock            7       3      
Contacts        14      38      
Email           -7      21      
Messages         6      19      
Music           -1       5      
Phone           -3       3      
Settings         5     -24      
Usage            5      18      
Video            2       5      
Gallery          4      43      

No statistically significant differences.  Were you suspecting any?
Comment on attachment 8427764 [details] [diff] [review]

Review of attachment 8427764 [details] [diff] [review]:

Yes. Glad to see I was wrong :) r+
Attachment #8427764 - Flags: review?(gandalf) → review+
You need to log in before you can comment on or make changes to this bug.