Last Comment Bug 1013175 - Descriptive error messages in l10n.js
: Descriptive error messages in l10n.js
Status: RESOLVED FIXED
:
Product: Firefox OS
Classification: Client Software
Component: Gaia::L10n (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: ---
Assigned To: Staś Małolepszy :stas
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-05-20 03:21 PDT by Tim Guan-tin Chien [:timdream] (please needinfo)
Modified: 2014-05-26 12:27 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
mozilla-b2g:master PR#19427 (46 bytes, text/x-github-pull-request)
2014-05-20 03:25 PDT, Mozilla Shepherd (This account is used for automation and doesn't receive bugmail)
no flags Details | Review | Splinter Review
L10nError (8.66 KB, patch)
2014-05-23 07:25 PDT, Staś Małolepszy :stas
gandalf: review+
Details | Diff | Splinter Review

Description User image Tim Guan-tin Chien [:timdream] (please needinfo) 2014-05-20 03:21:36 PDT
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 User image Mozilla Shepherd (This account is used for automation and doesn't receive bugmail) 2014-05-20 03:25:39 PDT
Created attachment 8425383 [details] [review]
mozilla-b2g:master PR#19427
Comment 2 User image Zibi Braniecki [:gandalf][:zibi] 2014-05-21 16:05:13 PDT
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 User image Tim Guan-tin Chien [:timdream] (please needinfo) 2014-05-21 21:35:31 PDT
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 User image Zibi Braniecki [:gandalf][:zibi] 2014-05-21 22:20:08 PDT
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 User image Tim Guan-tin Chien [:timdream] (please needinfo) 2014-05-22 00:20:27 PDT
Yeah sure, no worries. Do you guys want to pick up this bug instead of taking my patch?
Comment 6 User image Staś Małolepszy :stas 2014-05-23 07:25:17 PDT
Created attachment 8427764 [details] [diff] [review]
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 User image Zibi Braniecki [:gandalf][:zibi] 2014-05-23 08:43:10 PDT
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 User image Tim Guan-tin Chien [:timdream] (please needinfo) 2014-05-23 10:07:45 PDT
(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 User image Staś Małolepszy :stas 2014-05-26 06:17:10 PDT
(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 User image Zibi Braniecki [:gandalf][:zibi] 2014-05-26 07:39:38 PDT
Comment on attachment 8427764 [details] [diff] [review]
L10nError

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

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

Note You need to log in before you can comment on or make changes to this bug.