User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.153 Safari/537.36

Hey Gandalf, Can you help me here, i replaced https://github.com/mozilla-b2g/gaia/blob/master/apps/costcontrol/js/common.js#L128 with "return navigator.mozL10n.setAttributes;". Is this correct? I found 1 instance of "mozL10n.translate" in https://github.com/mozilla-b2g/gaia/blob/master/apps/costcontrol/js/view_manager.js#L136. What should i do with this?

(In reply to gauravmittal from comment #1) > Hey Gandalf, Can you help me here, i replaced > https://github.com/mozilla-b2g/gaia/blob/master/apps/costcontrol/js/common. > js#L128 with "return navigator.mozL10n.setAttributes;". Is this correct? Yes, rename the function to setL10nAttributes as well, please. > I found 1 instance of "mozL10n.translate" in > https://github.com/mozilla-b2g/gaia/blob/master/apps/costcontrol/js/ > view_manager.js#L136. What should i do with this? Just remove it. It's a no-op at this point https://github.com/mozilla-b2g/gaia/blob/ea93363a8c424d65a9ad91438ce6961377a20f98/shared/js/l10n.js#L1305-L1307

Pullrequest added. Mention any changes needed.

Comment on attachment 8479227 [details] Pull request for the bug Hi gauravmittal, always ask for the owner / peer review when modifying a module [1], please. Here is ok. I only want to be aware about changes in the code. Thank you. [1] https://wiki.mozilla.org/Modules/FirefoxOS

Hi Salva, I am mentoring gauravimittal. I wanted to guide the owner through the process of selecting reviewer once the TBPL is green :) Thanks for stepping in!

(In reply to Zibi Braniecki [:gandalf] from comment #5) > Hi Salva, I am mentoring gauravimittal. I wanted to guide the owner through > the process of selecting reviewer once the TBPL is green :) > > Thanks for stepping in! Ok Zibi. No problem, next time I suggest to ask for owner / peer's review and your feedback as the code quality and maintainability is a matter of the owner yet. No hard feelings at all :) It is just what I do for my mentees to introduce them in the development procedure as soon as possible.

Comment on attachment 8479227 [details] Pull request for the bug I don't agree on changing the name of localize to setL10nAttributes. I think we are loosing meaning but let's finish :gandalf's review before asking for superreview again.

(In reply to Salvador de la Puente González [:salva] from comment #6) > It is just what I do for my mentees to > introduce them in the development procedure as soon as possible. Good call! I'll start introducing the mentee to the module owner earlier in the process. Thanks for the explanation.

Hi Gaurav, can you rebase your patch on top of current master and push? This will trigger new build on TBPL.

Hey Gandalf, I am not sure about the GU test in the B2G Desktop Linux x64 Opt. I didnt change anything in the test files for which its raising the errors. Can you guide me in solving this test? The other tests are being passed with exception of the GIP test.

(In reply to gauravmittal from comment #10) > Hey Gandalf, I am not sure about the GU test in the B2G Desktop Linux x64 > Opt. I didnt change anything in the test files for which its raising the > errors. Can you guide me in solving this test? You changed navigator.mozL10n.setAttributes to Common.localize in multiple files. Now their tests report "Common is not defined". Look at https://github.com/mozilla-b2g/gaia/blob/master/apps/costcontrol/test/unit/non_ready_screen_test.js for an example how they include mock_common and do the same.

hey, so m down to https://tbpl.mozilla.org/?rev=ecb16a9c27fc4c28d583efd2c6891cb847a62a9d&tree=Gaia-Try ... my Gu is still failing , but m nt able to understand y ... can u help, the third error will be solved if the forst two are solved, and if the first one is solved , i guess the second will be solved as well

Ok, there's a lot of intermingled code here. - widget_startup_test is testing for a lot of textContent. You would need to replace all of those calls to test for data-l10n-id and data-l10n-args attributes. - there's code in BalanceView.js and widget.js that takes values from textContent and puts it in other Node's textContent's. In result, I got to the point where I believe that debugging that requires more code refactoring than I believe you should be doing for your first bug. :salva - do you have an opinion on what Gaurav should do here? I'm leaning toward recommending him to revert changes to: - widget_startup_test.js - widget.js - BalanceView.js This makes the unit tests pass and gives us a smaller patch that cleans up at least a bit of the l10n code including the two most important cleanups: removal of mozL10n.translate and mozL10n.localize (and a bunch of .get's). That's a clear win in my book :) What do you think?

Gaurav: I believe you can follow my suggestion from the previous comment. revert changes to those three files and let's land what we have working :)

Zibi: Hey, After reverting the changes back, i got https://tbpl.mozilla.org/?rev=f5bd880c09664f57c1c17331c2e493d869186b88&tree=Gaia-Try .

Comment on attachment 8479227 [details] Pull request for the bug Cool! I believe you have a green build here (restarted one Gip because it timed out). Setting r? on :salva.

(In reply to Zibi Braniecki [:gandalf] from comment #13) > Ok, there's a lot of intermingled code here. > > - widget_startup_test is testing for a lot of textContent. You would need > to replace all of those calls to test for data-l10n-id and data-l10n-args > attributes. > - there's code in BalanceView.js and widget.js that takes values from > textContent and puts it in other Node's textContent's. > > In result, I got to the point where I believe that debugging that requires > more code refactoring than I believe you should be doing for your first bug. > > :salva - do you have an opinion on what Gaurav should do here? I'm leaning > toward recommending him to revert changes to: > - widget_startup_test.js > - widget.js > - BalanceView.js Agree but let's file a follow up bug because I think you're doing a good job and it would a pity to leave it partially done. Sorry for the delay, I was on PTO.

Comment on attachment 8479227 [details] Pull request for the bug The patch looks nice but I'm rejecting it because the branch in :gauravmittal's fork is master which is dangerous, hard to track in the history and uncomfortable for "live" review. Furthermore, it is against best practices for Gaia contribution [1]. In addition, the PR comment does not include the bug title and it must to [2]. [1] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Making_Gaia_code_changes#Git_best_practices [2] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Submitting_a_Gaia_patch#Patch_submission

@salva ... hey , i did the corrections like u asked in the PR. Can you check it??

:gauravmittal, please, move your changes to a brand new branch on your fork and edit the PR to to this branch. Change the commit comment (git commit --amend) to include the name of the bug and ask for my review again.

Comment on attachment 8489459 [details] PR for the bug You don't need to rerequest my review unless you change the content of the patch substantially. You're addressing Salva's comments, so just carry over my r+ and ask from him.

Comment on attachment 8489459 [details] PR for the bug New functionality was added by bug 1033549. Please, rebase your patch and provide a quick review to the new additions in case you find new l10n issues. You're almost there. Ask for my review again and good work!

Can u review it... i rebased it from upstream

You don't need to ni me if you're actually asking for my review.

Comment on attachment 8489459 [details] PR for the bug Working fine. Good work!

Awesome! Next steps: - add "checkin-needed" keyword ( https://bugzilla.mozilla.org/describekeywords.cgi ) to indicate that your PR is ready to be committed. - file a follow up bug with a changes listed in comment 13 (see comment 17) If nobody else lands it, I will land it for you tomorrow :)

Master: https://github.com/mozilla-b2g/gaia/commit/53e839e1f3f5edc4f0e1c48d9ae84609a3a97ddb

Congrats!

@salva and @gandalf .... thanks a lot for your guidance and mentor-ship!!!! :)

Comment on attachment 8489459 [details] PR for the bug [Approval Request Comment] This is part of the cleaning up of the API methods of mozL10n, that it was started on 2.0. [Bug caused by] (feature/regressing bug #): Feature [User impact] if declined: No user impact, this is a polish bug. [Testing completed]: Yes [Risk to taking this patch] (and alternatives if risky): Low risk [String changes made]:No

Comment on attachment 8489459 [details] PR for the bug Not enough user value to justify uplift at this point.

Correction of previous patch on https://github.com/mozilla-b2g/gaia/blob/master/apps/costcontrol/test/unit/balance_view_test.js#L87 and https://github.com/mozilla-b2g/gaia/blob/master/apps/costcontrol/test/unit/balance_view_test.js#L96

Comment on attachment 8496162 [details] Error fix of the previous patch lgtm! Glad you re-reviewed your own patch and caught it!

Comment on attachment 8496162 [details] Error fix of the previous patch Please, file another bug for this issue (and say it's a follow-up of this in the description). you can leave another comment here saying you're going to open a follow-up. What is incredible is the `assert.equal()` with only one parameter does not throw.

Follow up bug created : Bug 1077149