Closed Bug 901216 Opened 8 years ago Closed 7 years ago

[l10n.js] make consoleWarn string concatenation-free

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-v1.4 fixed)

RESOLVED FIXED
1.4 S2 (28feb)
Tracking Status
b2g-v1.4 --- fixed

People

(Reporter: julienw, Assigned: kaze)

Details

Attachments

(1 file, 5 obsolete files)

PR
46 bytes, text/x-github-pull-request
kaze
: review+
Details | Review
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.
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?
Assignee: nobody → kaze
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).
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.
Flags: needinfo?(felash)
Attached patch consolewarn_option1.patch (obsolete) — Splinter Review
Using slice method.
Attached patch consolewarn_option2.patch (obsolete) — Splinter Review
Using for loop and build the string message to warn
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.
Flags: needinfo?(felash)
Attached file PR (obsolete) —
Updated consoleLog and  consoleWarn functions
Attachment #8378885 - Attachment is obsolete: true
Attachment #8378886 - Attachment is obsolete: true
Attachment #8379076 - Flags: review?(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.
Attachment #8379076 - Flags: review?(kaze) → review+
Attached file PR (obsolete) —
Updated the consoleWarn and consoleLog function to be concatenation-free.
Attachment #8379076 - Attachment is obsolete: true
Attachment #8379541 - Flags: review?(kaze)
Attached file PR (obsolete) —
Updated the consoleWarn and consoleLog function to be concatenation-free.
Attachment #8379541 - Attachment is obsolete: true
Attachment #8379541 - Flags: review?(kaze)
Attachment #8379557 - Flags: review?(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).
Attachment #8379541 - Flags: review-
Attachment #8379557 - Flags: review?(kaze) → review-
Attached file PR
Fixed the review comments given on github.
Attachment #8379557 - Attachment is obsolete: true
Attachment #8379666 - Flags: review?(kaze)
Comment on attachment 8379666 [details] [review]
PR

LGTM, thanks!
Attachment #8379666 - Flags: review?(kaze) → review+
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.
Flags: needinfo?(kaze)
Keywords: checkin-needed
Master: 1b3e92dee4d161156d7cf3ba9089766b12f59104
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(kaze)
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S2 (28feb)
You need to log in before you can comment on or make changes to this bug.