Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2267933002:
Move text/selection update to the back of pipeline (Closed)
|
Created:
4 years, 4 months ago by dglazkov Modified:
3 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove text/selection update to the back of pipeline
Instead of forcing the spurious rendering pipeline flush to get an updated selection and text input information (for IME), do after the layout and style information had updated.
BUG=640310
Committed: https://crrev.com/aa7d9e29b66032b9289769d2948c2b8a394ed4b0
Cr-Commit-Position: refs/heads/master@{#415483}
Patch Set 1 #Patch Set 2 : Blunt-force approach. #Patch Set 3 : Even simpler blunt force. #Messages
Total messages: 23 (14 generated)
The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Blunt-force approach.
The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Even simpler blunt force.
The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== WIP BUG= ========== to ========== Move text/selection update to the back of pipeline Instead of forcing the spurious rendering pipeline flush to get an updated selection and text input information (for IME), do after the layout and style information had updated. BUG=640310 ==========
dglazkov@chromium.org changed reviewers: + chrishtr@chromium.org, esprehn@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
PTAL. Would love your thoughts/ideas on testability of this change. I am pretty sure we have IME tests, but all of them seemed silent even when I simply removed the two lines. However, TextInputManagerTest.* tests started failing, which is a good thing. I fixed those by moving the lines to the back after the Layout stage. As for whether this will break IME in real world... ¯\_(ツ)_/¯
On 2016/08/27 at 04:31:18, dglazkov wrote: > PTAL. > > Would love your thoughts/ideas on testability of this change. I am pretty sure we have IME tests, but all of them seemed silent even when I simply removed the two lines. However, TextInputManagerTest.* tests started failing, which is a good thing. I fixed those by moving the lines to the back after the Layout stage. As for whether this will break IME in real world... > > ¯\_(ツ)_/¯ This lgtm, I think you'd need to write a content browser test that sends real input? You need so much of the system to have UpdateVisualState() run at all :/
The CQ bit was checked by dglazkov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Agreed, it seems hard to test this. I guess you could do it with lots of mock interfaces, but not sure that is a win.
Message was sent while issue was closed.
Description was changed from ========== Move text/selection update to the back of pipeline Instead of forcing the spurious rendering pipeline flush to get an updated selection and text input information (for IME), do after the layout and style information had updated. BUG=640310 ========== to ========== Move text/selection update to the back of pipeline Instead of forcing the spurious rendering pipeline flush to get an updated selection and text input information (for IME), do after the layout and style information had updated. BUG=640310 Committed: https://crrev.com/aa7d9e29b66032b9289769d2948c2b8a394ed4b0 Cr-Commit-Position: refs/heads/master@{#415483} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/aa7d9e29b66032b9289769d2948c2b8a394ed4b0 Cr-Commit-Position: refs/heads/master@{#415483}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2295013002/ by dglazkov@chromium.org. The reason for reverting is: Need to do more testing/thinking.. |
