1013175 - Descriptive error messages in l10n.js

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::L10n, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 1

[Mozilla Shepherd (This account is used for automation and doesn't receive bugmail)]

Attachment: mozilla-b2g:master PR#19427

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

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?

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Yeah sure, no worries. Do you guys want to pick up this bug instead of taking my patch?

Comment 6

[Staś Małolepszy :stas]

Attachment: L10nError

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"]

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8427764 [details] [diff] [review]
L10nError

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

can you do a quick b2gperf check on this?

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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

[Staś Małolepszy :stas]

(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 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8427764 [details] [diff] [review]
L10nError

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

Yes. Glad to see I was wrong :) r+

Comment 11

[Staś Małolepszy :stas]

Commit: https://github.com/mozilla-b2g/gaia/commit/50991928704bfa5569b3a6a3ef12338fc4573b6c
Merged: https://github.com/mozilla-b2g/gaia/commit/8d8d34b01bec60940664bca8319413f84b1e51ea

L20n.js: https://github.com/l20n/l20n.js/commit/1c9fa39943def7c29ab5cbf44c58032b22024e7a