Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Description • 10 years ago

The keypad creates a lot of reflows when a key is entered. One of the reason is because we try to determine the best size for the text. The proposed helper used the measureText method of a canvas context to determine the best size.

There is also an attached POC demonstrating how to use it in the dialer app.

	Gabriele Svelto [:gsvelto] (PTO)
	Comment 2 • 10 years ago

While trying to find a fix for bug 917193 I've found out that an effective way of removing most reflows caused by the font sizing algorithm would be to tweak the font's starting size. We start from 18px currently which requires 6 steps with associated layouts to get us to 40px (on devices with GAIA_DEV_PIXELS_PER_PX=1). There's nothing preventing us from starting with the correct size for the device and it's such a small change that I think it's worth it.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 3 • 10 years ago

Here an updated patch that should handle both the keypad and the call screen.

I did not yet use the suggestion from Gabriele, but this actually makes a lot of sense to do it.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 4 • 10 years ago

Here is an updated text_utils.js file that use some caching since in some of my profile I realized that calling context.font = ... is expensive.

	Anthony Ricaud (:rik) Assignee
	Comment 5 • 10 years ago

Comment on attachment 8373016 [details] [diff] [review]
foo.dialer.patch

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

Some measurements on the keypad:
- For the first key tap: before ~140ms; after ~50ms
- For every tap with no resize: before ~40ms; after ~5ms
That looks like a good improvement :)

As we discussed, I think passing an array of sizes will improve performance a bit more (no fine tuning of the steps, the app is in control) and give more design control to the app.
We should use getComputedStyle when querying the existing font size.
Instead of getOverflowCount, we should have something like getLongestStringWithEllipsis.

And unit tests are very nice delicacies, we should have more of them.

	[:fabrice] Fabrice Desré
	Comment 6 • 10 years ago

Rik, do you have to tackle that and provide a 1.3t version?

	Anthony Ricaud (:rik) Assignee
	Comment 7 • 10 years ago

I'm not sure to understand the exact question. (Maybe the word time is missing in the question?)

Before a potential 1.3T version, we still need to finish this patch for master. I'm currently focused on DSDS work so I cannot work on this at the moment. But happy to review if someone else provides a patch.

	[:fabrice] Fabrice Desré
	Comment 8 • 10 years ago

thanks Rik. Do you know who could work on that?

	Anthony Ricaud (:rik) Assignee
	Comment 9 • 10 years ago

Anyone :) We have Vivien's patch, my feedback, so all we need is someone to address those and add some tests. No prior knowledge of Dialer is needed.

	Steven Yang [:styang]
	Comment 10 • 10 years ago

Thomas, could your team help to verify on Tarako? thanks!

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 11 • 10 years ago

If you can confirm that the first 2 attachments help for Tarako I started to fix the review comments and write some tests for the helper.

I think the getOverflowCount versus getLongestStringWithEllipsis can be fixed in a followup as it is an implementation detail and the first goal here is to reduce the latency.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 12 • 10 years ago

Add a shared/js/text_utils.js file and some tests into apps/gallery/test/unit/text_utils_test.js since bug 841422 still prevent us to land the tests into shared/.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 13 • 10 years ago

Let's start the review cycle with the shared/js/text_utils.js file.

As I mentioned I agree that a better method than getOverflowCount would be nice, but since this code is used only for the Dialer at the moment, it sounds OK to address that in a followup.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 14 • 10 years ago

Dialer changes using the text_utils.js helper file. I still need to address the getComputedStyle comment (I also need to check that it won't create any additional reflows), and the call screen use font-weight which is not supported yet by the helper.

I have to admit that I'm tempted to just fix the keypad for now and fix the rest in a followup as it introduce a lot of changes and I would be happy to minimize the risk of regressions for Tarako.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 15 • 10 years ago

I made a dialer patch version that targets only the keypad for now. The call screen changes and its various dependencies can be done in a followup for master.

The only changes with this patch is the logic of |formatPhoneNumber|.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 16 • 10 years ago

One of the overflow test for text_utils fails on Travis. Likely because of the different font configuration options between platforms...  I guess I can modify the test in a different way so it does not fails before landing by s/129/ > 2/

	Steven Yang [:styang]
	Comment 17 • 10 years ago

Hi William, we want to check how much will the patch improve on Tarako? could you help to check it? Thanks!

	Danny Liang [:dliang]
	Comment 18 • 10 years ago

(In reply to Anthony Ricaud (:rik) from comment #5)
> Comment on attachment 8373016 [details] [diff] [review]
> foo.dialer.patch
> 
> Review of attachment 8373016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some measurements on the keypad:
> - For the first key tap: before ~140ms; after ~50ms
> - For every tap with no resize: before ~40ms; after ~5ms
> That looks like a good improvement :)

Hi Anthony, may I know how did you measure above number? I used the build on 3/10 with your patch, but I didn't feel the obvious improvement. So I'd like to sync the test steps with you for precise measurement. Thanks.

	William Hsu [:whsu]
	Comment 19 • 10 years ago

(In reply to Steven Yang [:styang] from comment #17)
> Hi William, we want to check how much will the patch improve on Tarako?
> could you help to check it? Thanks!

Hi, Steven,

Sorry for the late reply.
Sure! I can help on this but I need Vivien to land this patch on Gaia master.
Because I don't prefer to measure local build performance, it is usually inaccuracy.
Thanks.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 20 • 10 years ago

(In reply to William Hsu [:whsu] from comment #19)
> Sure! I can help on this but I need Vivien to land this patch on Gaia master.

We all need something. For my part I need someone to review them ;)

	Anthony Ricaud (:rik) Assignee
	Comment 21 • 10 years ago

Comment on attachment 8388070 [details] [diff] [review]
bug967440.text_utils.patch

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

r+ with the jshint change.

::: shared/js/text_utils.js
@@ +1,1 @@
> +/* jshint unused: false */

/* exported TextUtils */ is better

	Anthony Ricaud (:rik) Assignee
	Comment 22 • 10 years ago

Comment on attachment 8388072 [details] [diff] [review]
bug967440.keypad.patch

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

This looks good but keypad.js and utils.js needs some new tests.

	Anthony Ricaud (:rik) Assignee
	Comment 23 • 10 years ago

(In reply to Danny Liang [:dliang] from comment #18)
> Hi Anthony, may I know how did you measure above number? I used the build on
> 3/10 with your patch, but I didn't feel the obvious improvement. So I'd like
> to sync the test steps with you for precise measurement. Thanks.
I should have explained when I put those measurements :(

If I remember correcly, I just added some new Date() around |this.formatPhoneNumber(ellipsisSide, maxFontSize);| in keypad.js. I added some document.body.offsetLeft to ask for a reflow to make sure I was measuring the reflow in there.

	Joe Cheng [:jcheng] (please needinfo)
	Comment 24 • 10 years ago

Can Danny confirm again after know how rik measures the numbers? Thanks 
Also 1.3T? again since end users may not be able to tell the difference per earlier comments

	James Zhang (Spreadtrum)
	Comment 26 • 10 years ago

When can we land this patch?

	Joe Cheng [:jcheng] (please needinfo)
	Comment 27 • 10 years ago

Hi James, do you feel the current dialpad response is still unacceptable? thanks

	Anthony Ricaud (:rik) Assignee
	Comment 28 • 10 years ago

Comment on attachment 8394084 [details] [diff] [review]
bug967440.dialer.patch

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

::: apps/communications/dialer/test/unit/keypad_test.js
@@ +414,5 @@
>          MockMultiSimActionButtonSingleton._phoneNumberGetter());
>      });
>    });
> +
> +  suite('Phone Number View: Font Size', function() {

We don't need all those tests. We just need to check that we call Utils.adjustTextForElement with the proper parameters.

That way, you only have to include mock_utils.js earlier.

::: apps/communications/dialer/test/unit/utils_test.js
@@ +68,5 @@
>        });
>      });
>    });
> +
> +  suite('Font Size utilities', function() {

Same here, we don't need those tests.

We just need to check that we set the proper fontSize based on what a mock textutils returns.

Keep the ellipsis tests (but convert to use a mock of textutils). We also need to check that the length of the final string is respecting the counter value returned.

	Anthony Ricaud (:rik) Assignee
	Comment 29 • 10 years ago

(In reply to James Zhang from comment #26)
> When can we land this patch?
As soon as it has a r+. But you can take the content of attachment 8394084 [details] [diff] [review] and apply it to check if it's helping.

	John Hammink
	Comment 30 • 10 years ago

I'll try to test the user scenario around this.  Will need to ferret out an STR also.

	James Zhang (Spreadtrum)
	Comment 31 • 10 years ago

(In reply to Joe Cheng [:jcheng] from comment #27)
> Hi James, do you feel the current dialpad response is still unacceptable?
> thanks

Yes, slower than 6821 android version.

	Joe Cheng [:jcheng] (please needinfo)
	Comment 32 • 10 years ago

1.3T+ to improve performance

	John Hammink
	Comment 33 • 10 years ago

Mason - I could use your help.  I need to come up with an STR for this bug.  What tools are available and what's a useful STR for testing these reflows?  Thanks.

	Mason Chang [Inactive] [:mchang]
	Comment 34 • 10 years ago

Hi John,

You can use the reflow counter in the developer tools - https://wiki.mozilla.org/B2G/Performance/App_Performance_Validation#2._Reflows.2FRestyles 

As you type, you should see reflow counts in adb logcat as well, which will tell you how much time is spent in the reflow. In terms of STR, just try typing in an app and see if we're reflowing or not. Or was there a specific workload?

Anthony, what STR did you use to measure in comment 5?

	Anthony Ricaud (:rik) Assignee
	Comment 35 • 10 years ago

(In reply to Mason Chang [:mchang] from comment #34)
> Anthony, what STR did you use to measure in comment 5?
That should be in comment 23.

	Joe Cheng [:jcheng] (please needinfo)
	Comment 37 • 10 years ago

triage: should not block release. please ask for approval to land in 1.3T when ready .thanks

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 38 • 10 years ago

Vivien, I'd like to use shared TextUtils for bug 908300. Is this going to land soon? It looks like it's ready to go.

Also, as an aside question: why do we create a new canvas for each fontSize+fontFamily combination? Is it faster than setting `ctx.font` for each iteration of the resize loop? Seem like you are allocating a bunch of extra memory/causing garbage collection when doing the resize (especially if you are clearing the cache after each element resize).

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 39 • 10 years ago

(In reply to Michael Henretty [:mhenretty] from comment #38)
> Vivien, I'd like to use shared TextUtils for bug 908300. Is this going to
> land soon? It looks like it's ready to go.
>

Sounds like I need to rebase this and land it. I forgot it...

If you don't want to wait for me and land it that's fine too ;) 

> Also, as an aside question: why do we create a new canvas for each
> fontSize+fontFamily combination? Is it faster than setting `ctx.font` for
> each iteration of the resize loop? Seem like you are allocating a bunch of
> extra memory/causing garbage collection when doing the resize (especially if
> you are clearing the cache after each element resize).

Yeah, setting ctx.font was very expensive on the tests I made. So I favor performance against memory, but the app author can just reset all the canvases when the app is going in the background (visibilitychange) or when it does not need it anymore.

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 40 • 10 years ago

I tried to land this patch, but it failed on travis. It seems that the reflow count for large text gives different results locally as it does in travis. In any case, this is the only test that consistently fails:

https://github.com/mozilla-b2g/gaia/pull/19005/files#diff-7cfe73a7cfe695190d4eba65afd3c059R174

Vivien, do you know what is going on here?

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 41 • 10 years ago

link to failing travis build:
https://travis-ci.org/mozilla-b2g/gaia/jobs/24585869#L2193

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 42 • 10 years ago

(In reply to Michael Henretty [:mhenretty] from comment #40)
> Created attachment 8418469 [details] [review]
> Github PR, text utils, also moving to shared
> 
> I tried to land this patch, but it failed on travis. It seems that the
> reflow count for large text gives different results locally as it does in
> travis. In any case, this is the only test that consistently fails:
> 
> https://github.com/mozilla-b2g/gaia/pull/19005/files#diff-
> 7cfe73a7cfe695190d4eba65afd3c059R174
> 
> Vivien, do you know what is going on here?

Likely different fonts :(.

Janx and some of the fonts guys are pushing to have a solution for that in bug 992210

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 43 • 10 years ago

Comment on attachment 8418469 [details] [review]
Github PR, text utils, also moving to shared

In general, I think testing for an exact count of reflows makes these tests overly brittle. What if we get a reflow optimization from the platform, should that cause these tests to go red? I think testing for *less than* a certain amount of reflows is sufficient here. Vivien, what do you think?

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 44 • 10 years ago

(In reply to Michael Henretty [:mhenretty] from comment #43)
> Comment on attachment 8418469 [details] [review]
> Github PR, text utils, also moving to shared
> 
> In general, I think testing for an exact count of reflows makes these tests
> overly brittle. What if we get a reflow optimization from the platform,
> should that cause these tests to go red? I think testing for *less than* a
> certain amount of reflows is sufficient here. Vivien, what do you think?

In the theory I think I agree. On the practice, it may depends on what exactly you are trying to do. For this particular test, you're probably right, unless there is 0 reflows.

For some other tests, we want to make sure that we know the exact amount of reflows. But that's probably mostly when it should be 0 or 1 reflow and that's it.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 45 • 10 years ago

Comment on attachment 8418469 [details] [review]
Github PR, text utils, also moving to shared

This particular test is flaky anyway because of cross-platform fonts.

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 46 • 10 years ago

master: https://github.com/mozilla-b2g/gaia/commit/3bfa8074c49441e59f82fe5929573a39cc6ff00a

I'll try to watch TBPL to make sure I didn't burn anything there.

Also, just to note, the whole point of text utils is attachment 8395665 [details] [diff] [review], which still needs to be rebased against the dialer app changes on master. So I'm leaving this bug open.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 47 • 10 years ago

Is this going to be landed before FL?

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 48 • 10 years ago

(In reply to Wesley Huang [:wesley_huang] from comment #47)
> Is this going to be landed before FL?

Probably not. In bug 908300, we made some significant changes to the TextUtils class, not to mention the dialer portion of this patch has to be rebased against recent changes in the dialer. Vivien says this is still on his radar, but he probably won't get the free time to work on it before Friday.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 49 • 10 years ago

Hi Vivien,
Any concern if we defer it to 2.1?
I didn't hear strong request that this must be in 2.0.

	Anthony Ricaud (:rik) Assignee
	Comment 50 • 10 years ago

There's been some big font changes recently in bug 1007709 so this patch probably has a lot of work to finish. I'll take it for the next sprint.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 51 • 10 years ago

this one shouldn't be a feature blocker for 2.0. 
I'm removing the flag and mark [priority] instead. 
Approval is required when patch is ready to uplift.

	Anthony Ricaud (:rik) Assignee
	Comment 53 • 10 years ago

This was more work than anticipated. I refactored a bunch of stuff (listed in the commit message).

I think the test coverage is good but let me know if there's more to add.

Germán: Since you worked on this very recently, could you check that I didn't break stuff?

	Doug Sherk (:drs) (inactive)
	Comment 55 • 10 years ago

Comment on attachment 8463418 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/22219

Generally really good. The comments on the PR that I left are mostly nits. Thanks for all the cleanup you did and tests you added here.

I didn't test this since Germán is going to.

	Germán Toro del Valle (:gtorodelvalle)
	Comment 56 • 10 years ago

Comment on attachment 8463418 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/22219

Wow! Awesome work going on here :O Thank you, guys!

I have only a minor comment you may have not noticed. It is related to using 4 pixel steps in https://github.com/mozilla-b2g/gaia/pull/22219/files#diff-bc0feadf5c699af9ea9684b6b18be649R48 You will notice the effect if you try the Dialer using master and using the proposed patch. You will see that the size adaptation is not that smooth and the final (minimum) size (when using the left ellipsis) is not as precise as it could be :) Just mentioning if you want to rethink these steps ;)

	Germán Toro del Valle (:gtorodelvalle)
	Comment 57 • 10 years ago

You use the Dialer and you tap on many keys so the size adaptation takes place, I mean :p

	Anthony Ricaud (:rik) Assignee
	Comment 58 • 10 years ago

Let's go with a step of 4px in this patch. We can always change it later if needed.

	Anthony Ricaud (:rik) Assignee
	Comment 59 • 10 years ago

I've made a new commit on top of the previous one for the review fixes.

	Doug Sherk (:drs) (inactive)
	Comment 60 • 10 years ago

Comment on attachment 8463418 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/22219

I left some comments on the PR. Mostly indentation issues.

	Anthony Ricaud (:rik) Assignee
	Comment 61 • 10 years ago

Woot, finally landed!

https://github.com/mozilla-b2g/gaia/commit/bc3e3b7f09a32c73d9ca285d44a143b51146501f