https://github.com/mozilla-b2g/gaia/blob/e554e48e95f2af0816be13a0976a3899c4545ec0/shared/js/dialer/font_size_manager.js#L99 We're missing a test to check that we're putting the ellipsis on the correct side for RTL languages.

Hi Rik, Would you mind reviewing my patch? :) Thanks

Comment on attachment 8469882 [details] Pull request Redirecting to Doug.

Comment on attachment 8469882 [details] Pull request This patch could use some rethinking. Here's how I would structure it: * MockL10n should be loaded with the rest of the MockXXX files, in the root `suiteSetup()`, and then unloaded again in the root `suiteTeardown()`. If there isn't a `mTeardown()` method, we should add one so that the LTR/RTL setting gets cleared for each test. * Instead of adding two new tests for RTL, we can re-use the existing tests and pass LTR/RTL into them. Here's an example of what I mean: ``` ['ltr', 'rtl'].forEach(function(direction) { test('adds ellipses to the end - ' + direction, function() { MockL10n.language.direction = direction; /* ... */ FontSizeManager.adaptToSpace( FontSizeManager.DIAL_PAD, view, true, direction == 'ltr' ? 'end' : ''); }); test(/* ... */); }); ``` You can stick also stick these tests in a new combined "ellipses in LTR/RTL modes" suite. The advantage of this is that it requires less code, and it's easier for reviewers and people new to the code to know that they should be supporting RTL, and how to do it. If we stick nearly-identical code in another suite, it can easily be missed. Some additional comments: * There needs to be more whitespace. Please keep the same code style as the code surrounding the changes. In particular, all `test()`, `suite()`, `setup()`, etc. calls should have a line break after them. * The comment is a little bit inspecific and didn't immediately help me understand the code below it. I would suggest writing something like "MockL10n does not invert text when in RTL mode, but FSM will treat the right side as the beginning for the purpose of ellipses."

Comment on attachment 8469882 [details] Pull request Hi Doug, Thanks for reviewing. I addressed your comment and also did some refactoring on new unit tests I introduced in bug 1047998. Thanks again.

Comment on attachment 8469882 [details] Pull request This is better, especially structurally, but there are still some problems. See the PR for my comments.

Comment on attachment 8469882 [details] Pull request Hi Doug, I've addressed your comment. However I am not quite sure if I get your idea on `viewElements`. So I leave my explanation on github. Thanks again.

Comment on attachment 8469882 [details] Pull request I left some more comments on the PR.

Comment on attachment 8469882 [details] Pull request Hi Doug, Comment addressed. Thanks

Comment on attachment 8469882 [details] Pull request Looks good now. I left a few last comments for whitespace issues. In the future, please fix review comments in a new commit so that it's easier to see what you've changed. You can squash them all once you're ready to land your patch.

Thanks, I'll keep that in mind. landed on master https://github.com/mozilla-b2g/gaia/commit/f07a308156862045aac8197698e33888c89452c7