Closed
Bug 967440
Opened 11 years ago
Closed 10 years ago
Remove some reflows on the keypad
Categories
(Firefox OS Graveyard :: Gaia::Dialer, defect)
Firefox OS Graveyard
Gaia::Dialer
Tracking
(blocking-b2g:-)
People
(Reporter: vingtetun, Assigned: rik)
References
Details
(Whiteboard: [priority][planned-sprint c=3][in-sprint=v2.0-S6])
Attachments
(3 files, 10 obsolete files)
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.
Updated•11 years ago
|
Attachment #8369974 -
Attachment mime type: application/javascript → text/plain
Reporter | ||
Updated•11 years ago
|
Comment 2•11 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.
Reporter | ||
Comment 3•11 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.
Attachment #8369975 -
Attachment is obsolete: true
Attachment #8369975 -
Flags: feedback?(anthony)
Attachment #8373016 -
Flags: feedback?(anthony)
Reporter | ||
Comment 4•11 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.
Attachment #8369974 -
Attachment is obsolete: true
Assignee | ||
Comment 5•11 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.
Attachment #8373016 -
Flags: feedback?(anthony)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → anthony
Updated•11 years ago
|
blocking-b2g: --- → 1.3T?
Comment 6•11 years ago
|
||
Rik, do you have to tackle that and provide a 1.3t version?
Flags: needinfo?(anthony)
Assignee | ||
Comment 7•11 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.
Flags: needinfo?(anthony)
Assignee | ||
Comment 9•11 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.
Assignee: anthony → nobody
Comment 10•11 years ago
|
||
Thomas, could your team help to verify on Tarako? thanks!
Flags: needinfo?(ttsai)
Reporter | ||
Comment 11•11 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.
Updated•11 years ago
|
Assignee: nobody → 21
Reporter | ||
Comment 12•11 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/.
Attachment #8387731 -
Attachment is obsolete: true
Reporter | ||
Comment 13•11 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.
Attachment #8373018 -
Attachment is obsolete: true
Attachment #8387948 -
Attachment is obsolete: true
Attachment #8388070 -
Flags: review?(anthony)
Reporter | ||
Comment 14•11 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.
Attachment #8373016 -
Attachment is obsolete: true
Reporter | ||
Comment 15•11 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|.
Attachment #8388071 -
Attachment is obsolete: true
Attachment #8388072 -
Flags: review?(anthony)
Reporter | ||
Comment 16•11 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/
Comment 17•11 years ago
|
||
Hi William, we want to check how much will the patch improve on Tarako? could you help to check it? Thanks!
Flags: needinfo?(whsu)
Updated•11 years ago
|
blocking-b2g: 1.3T? → 1.3T+
Comment 18•11 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.
Flags: needinfo?(anthony)
Comment 19•11 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.
Flags: needinfo?(whsu)
Reporter | ||
Comment 20•11 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 ;)
Assignee | ||
Comment 21•11 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
Attachment #8388070 -
Flags: review?(anthony) → review+
Assignee | ||
Comment 22•11 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.
Attachment #8388072 -
Flags: review?(anthony)
Assignee | ||
Comment 23•11 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.
Flags: needinfo?(anthony)
Updated•11 years ago
|
Flags: needinfo?(ttsai)
Comment 24•11 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
blocking-b2g: 1.3T+ → 1.3T?
Flags: needinfo?(dliang)
Comment 27•11 years ago
|
||
Hi James, do you feel the current dialpad response is still unacceptable? thanks
Flags: needinfo?(james.zhang)
Assignee | ||
Comment 28•11 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.
Attachment #8394084 -
Flags: review?(anthony) → review-
Assignee | ||
Comment 29•11 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.
Flags: needinfo?(styang)
Comment 30•11 years ago
|
||
I'll try to test the user scenario around this. Will need to ferret out an STR also.
Flags: needinfo?(jhammink)
Comment 31•11 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.
Flags: needinfo?(james.zhang)
Updated•11 years ago
|
Target Milestone: --- → 1.4 S4 (28mar)
Comment 33•11 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.
Flags: needinfo?(mchang)
Comment 34•11 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?
Flags: needinfo?(mchang) → needinfo?(anthony)
Assignee | ||
Comment 35•11 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.
Flags: needinfo?(anthony)
Reporter | ||
Comment 36•11 years ago
|
||
Attachment #8388072 -
Attachment is obsolete: true
Attachment #8394084 -
Attachment is obsolete: true
Attachment #8395665 -
Flags: review?(anthony)
Comment 37•11 years ago
|
||
triage: should not block release. please ask for approval to land in 1.3T when ready .thanks
blocking-b2g: 1.3T+ → -
Assignee | ||
Updated•11 years ago
|
Attachment #8395665 -
Flags: review?(anthony) → review+
Updated•11 years ago
|
Flags: needinfo?(dliang)
Comment 38•11 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).
Flags: needinfo?(21)
Reporter | ||
Comment 39•11 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.
Flags: needinfo?(21)
Comment 40•11 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?
Attachment #8388070 -
Attachment is obsolete: true
Flags: needinfo?(jhammink) → needinfo?(21)
Comment 41•11 years ago
|
||
link to failing travis build: https://travis-ci.org/mozilla-b2g/gaia/jobs/24585869#L2193
Reporter | ||
Comment 42•11 years ago
|
||
(In reply to Michael Henretty [:mhenretty] from comment #40) > Created attachment 8418469 [details] > 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
Flags: needinfo?(21)
Comment 43•11 years ago
|
||
Comment on attachment 8418469 [details]
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?
Attachment #8418469 -
Flags: review?(21)
Reporter | ||
Comment 44•11 years ago
|
||
(In reply to Michael Henretty [:mhenretty] from comment #43) > Comment on attachment 8418469 [details] > 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.
Reporter | ||
Comment 45•11 years ago
|
||
Comment on attachment 8418469 [details]
Github PR, text utils, also moving to shared
This particular test is flaky anyway because of cross-platform fonts.
Attachment #8418469 -
Flags: review?(21) → review+
Updated•11 years ago
|
feature-b2g: --- → 2.0
Comment 46•11 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.
Comment 47•10 years ago
|
||
Is this going to be landed before FL?
Target Milestone: 1.4 S4 (28mar) → 2.0 S3 (6june)
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.
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.
Flags: needinfo?(21)
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.
Assignee: 21 → anthony
Flags: needinfo?(21)
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
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.
feature-b2g: 2.0 → ---
Whiteboard: [priority]
Updated•10 years ago
|
Target Milestone: 2.0 S4 (20june) → ---
Updated•10 years ago
|
Whiteboard: [priority] → [priority][c=2]
Target Milestone: --- → 2.0 S6 (18july)
Updated•10 years ago
|
Whiteboard: [priority][c=2] → [priority][c=2][planned-sprint]
Updated•10 years ago
|
Assignee: anthony → drs+bugzilla
Updated•10 years ago
|
Blocks: dialer-keypad
Updated•10 years ago
|
Status: NEW → ASSIGNED
Updated•10 years ago
|
Whiteboard: [priority][c=2][planned-sprint] → [priority][c=2][planned-sprint][in-sprint=v2.0-S6]
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
Updated•10 years ago
|
Whiteboard: [priority][c=2][planned-sprint][in-sprint=v2.0-S6] → [priority][planned-sprint c=3][in-sprint=v2.0-S6]
Updated•10 years ago
|
Assignee: drs+bugzilla → anthony
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?
Attachment #8463418 -
Flags: review?(drs+bugzilla)
Attachment #8463418 -
Flags: feedback?(gtorodelvalle)
Comment hidden (typo) |
Comment 55•10 years ago
|
||
Comment on attachment 8463418 [details] 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.
Attachment #8463418 -
Flags: review+ → review-
Comment 56•10 years ago
|
||
Comment on attachment 8463418 [details] 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 ;)
Attachment #8463418 -
Flags: feedback?(gtorodelvalle) → feedback+
Comment 57•10 years ago
|
||
You use the Dialer and you tap on many keys so the size adaptation takes place, I mean :p
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8463418 -
Flags: review- → review?(drs+bugzilla)
Assignee | ||
Comment 59•10 years ago
|
||
I've made a new commit on top of the previous one for the review fixes.
Comment 60•10 years ago
|
||
Comment on attachment 8463418 [details] https://github.com/mozilla-b2g/gaia/pull/22219 I left some comments on the PR. Mostly indentation issues.
Attachment #8463418 -
Flags: review?(drs+bugzilla) → review+
Assignee | ||
Comment 61•10 years ago
|
||
Woot, finally landed! https://github.com/mozilla-b2g/gaia/commit/bc3e3b7f09a32c73d9ca285d44a143b51146501f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•