901216 - [l10n.js] make consoleWarn string concatenation-free

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Julien Wajsberg [:julienw]]

In l10n.js there are a lot of calls to the internal `consoleWarn` function, with string concatenation. That means that even if gDebug is false, we'll do those string concatenations, which is a waste of cpu power.

But console.warn can take several arguments: this concatenates the args separating with a space character, so basically what we're doing manually with the string concatenations. And this has the additional interesting side effect that we're not doing string concatenation if gDebug is false.

So it might make sense to modify the consoleWarn utility function to take advantage of this:

   function consoleWarn() {
     if (gDEBUG) {
       var args = [].slice.apply(arguments);
       args.unshift('[l10n]');
       console.warn.apply(console, args);
     }
   }

(untested)

and then modifying all calls to use several arguments instead of string concatenation.

Comment 1

[Fabien Cazenave [:kaze]]

The same should apply to consoleLog, too.

AFAIK we currently always keep gDEBUG=1, even on optimized builds (⇒ console.log() is ignored but console.warn() applies). We probably should think of a way to let gDEBUG to 1 on development builds and set it to 0 on optimized builds?

Comment 2

[Julien Wajsberg [:julienw]]

I'd favor always putting it to false, it's easy to put it to true if you want to test.

another possibility could be to put it to true while building (I'm sure there could be a way to check this).

Comment 3

[vijayanand.hongal]

Hi Julien

As per your comment, modified the l10n.js file to use variable args for the console warn.

Needed your comment on using the option available.

To use the built in splice as mentioned in comment #0, or using looping to process each argument and appending to string variable and passing the string to console.warn. 

Will push the changes as needed.

Comment 4

[vijayanand.hongal]

Attachment: consolewarn_option1.patch

Using slice method.

Comment 5

[vijayanand.hongal]

Attachment: consolewarn_option2.patch

Using for loop and build the string message to warn

Comment 6

[Julien Wajsberg [:julienw]]

option 1 is definitely better :)

Can you do the same for consoleLog too? Also I saw some places where you didn't change consoleWarn's parameters.

Note that I'm not a peer of l10n.js though, you better ask Kaze next time.

Comment 7

[vijayanand.hongal]

Attachment: PR

Updated consoleLog and  consoleWarn functions

Comment 8

[Fabien Cazenave [:kaze]]

Comment on attachment 8379076 [details] [review]
PR

Looks good to me, thanks for the patch!

Please address my nitpick (see PR) and update your PR so we can merge it.

Comment 9

[vijayanand.hongal]

Attachment: PR

Updated the consoleWarn and consoleLog function to be concatenation-free.

Comment 10

[vijayanand.hongal]

Attachment: PR

Updated the consoleWarn and consoleLog function to be concatenation-free.

Comment 11

[Fabien Cazenave [:kaze]]

Comment on attachment 8379541 [details] [review]
PR

Thanks for addressing my nitpick. I’m afraid I missed a bigger issue in my first review: the console[Log|Warn] text arguments should still be concatenated when the gDEBUG value requires it; the point of this patch is to avoid concatenating when gDEBUG is zero.

Feel free to update your PR by amending your commit rather than opening a new PR:

  git add l10n.js
  git commit --amend
  git push origin bldconsolewarn -f

… and re-request r? when your PR is updated (or if you propose a new PR).

Comment 12

[vijayanand.hongal]

Attachment: PR

Fixed the review comments given on github.

Comment 13

[vijayanand.hongal]

Travis build is green.

https://travis-ci.org/mozilla-b2g/gaia/builds/19328390

Comment 14

[Fabien Cazenave [:kaze]]

Comment on attachment 8379666 [details] [review]
PR

LGTM, thanks!

Comment 15

[Fabien Cazenave [:kaze]]

I’ve just tested this patch manually on my Inari, everything works as expected. I see we still miss a few l10n entities in Gaia right now. :-)

As this patch is not related to any 1.4-committed features, I’m not sure if I can check it in
 ⇒ checkin-needed tag + self-ni? to keep this on my radar.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Master: 1b3e92dee4d161156d7cf3ba9089766b12f59104