Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(717)

Issue 2121953002: Do not calculate composition bounds until IME requests. (Closed)

Created:
4 years, 5 months ago by Seigo Nonaka
Modified:
4 years, 4 months ago
CC:
Changwan Ryu, chromium-reviews, darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org, yosin_UTC9
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Do not calculate composition bounds until IME requests. Calculating composition bounds is not a light-weight operation. On the other hand, few IME uses this information. To skip this unnecessary calculation on certain environment, propagate cursor update mode from browser to the renderer. BUG=624714 Committed: https://crrev.com/dac0c7af14c29c9d8509b775d5044e5448b6263b Committed: https://crrev.com/fa291796686fef74a48f18061b9e36ccc9488a06 Cr-Original-Commit-Position: refs/heads/master@{#408900} Cr-Commit-Position: refs/heads/master@{#410927}

Patch Set 1 #

Total comments: 5

Patch Set 2 : [WIP] check with try bots #

Total comments: 8

Patch Set 3 : Addressed comments #

Total comments: 6

Patch Set 4 : [WIP] waiting trybots #

Total comments: 4

Patch Set 5 : Address comments #

Total comments: 19

Patch Set 6 : addressed comments #

Patch Set 7 : Add test #

Patch Set 8 : Make all tests passing #

Total comments: 4

Patch Set 9 : skip composition calculation when the focused not is not editable. #

Total comments: 2

Patch Set 10 : remove OS_ANDROID from render_widget.cc #

Total comments: 6

Patch Set 11 : Addresses Nasko's comments #

Patch Set 12 : fix test failures #

Unified diffs Side-by-side diffs Delta from patch set Stats (+155 lines, -61 lines) Patch
M content/browser/renderer_host/ime_adapter_android.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/renderer_host/ime_adapter_android.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +12 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +17 lines, -2 lines 0 comments Download
M content/common/input_messages.h View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +15 lines, -13 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +22 lines, -3 lines 0 comments Download
M content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java View 1 2 3 4 5 6 7 8 9 10 11 16 chunks +35 lines, -29 lines 0 comments Download
M content/public/android/javatests/src/org/chromium/content/browser/input/ImeLollipopTest.java View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -1 line 0 comments Download
M content/renderer/render_widget.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +10 lines, -1 line 0 comments Download
M content/renderer/render_widget.cc View 1 2 3 4 5 6 7 8 9 10 11 9 chunks +31 lines, -12 lines 0 comments Download

Messages

Total messages: 165 (115 generated)
Seigo Nonaka
Hi, Inaba-san, Yukawa-san, Could you kindly take a look? Thank you.
4 years, 5 months ago (2016-07-05 08:29:48 UTC) #5
kinaba
Thank you very much for taking over this! Here's the quick first round of comments. ...
4 years, 5 months ago (2016-07-05 08:50:09 UTC) #6
yosin_UTC9
Since Google C++ style guide doesn't mention about bool parameter, my comments about bool parameters ...
4 years, 5 months ago (2016-07-06 02:02:01 UTC) #8
Seigo Nonaka
Thank you for your review! PTAL https://codereview.chromium.org/2121953002/diff/20001/content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java File content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java (right): https://codereview.chromium.org/2121953002/diff/20001/content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java#newcode139 content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:139: } On 2016/07/05 ...
4 years, 5 months ago (2016-07-06 03:25:56 UTC) #11
yosin_UTC9
https://codereview.chromium.org/2121953002/diff/100001/content/renderer/render_widget.cc File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/100001/content/renderer/render_widget.cc#newcode1548 content/renderer/render_widget.cc:1548: void RenderWidget::OnRequestCursorUpdate(int mode) { How about using two bool ...
4 years, 5 months ago (2016-07-06 03:33:08 UTC) #12
yukawa
Sorry, I cannot review this right now as I'm on vacation til next week and ...
4 years, 5 months ago (2016-07-06 04:15:42 UTC) #14
Seigo Nonaka
Thank you for your review. Thank you Yohei for introducing aelias@. Alex, could you kindly ...
4 years, 5 months ago (2016-07-06 05:46:04 UTC) #15
Seigo Nonaka
Thank you for your review. Thank you Yohei for introducing aelias@. Alex, could you kindly ...
4 years, 5 months ago (2016-07-06 05:46:05 UTC) #16
kinaba
From my side, lgtm with some nits https://codereview.chromium.org/2121953002/diff/20001/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (left): https://codereview.chromium.org/2121953002/diff/20001/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java#oldcode196 content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:196: mCursorAnchorInfoController.resetMonitoringState(); On ...
4 years, 5 months ago (2016-07-06 06:08:33 UTC) #17
Seigo Nonaka
Kinaba-san, thank you for your review! https://codereview.chromium.org/2121953002/diff/120001/content/renderer/render_widget.h File content/renderer/render_widget.h (right): https://codereview.chromium.org/2121953002/diff/120001/content/renderer/render_widget.h#newcode352 content/renderer/render_widget.h:352: // changed. If ...
4 years, 5 months ago (2016-07-06 06:22:19 UTC) #18
aelias_OOO_until_Jul13
https://codereview.chromium.org/2121953002/diff/140001/content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java File content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java (left): https://codereview.chromium.org/2121953002/diff/140001/content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java#oldcode205 content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:205: mMonitorModeEnabled = false; These two booleans mHasPendingImmediateRequest and mMonitorModeEnabled ...
4 years, 5 months ago (2016-07-07 00:13:36 UTC) #19
Changwan Ryu
https://codereview.chromium.org/2121953002/diff/140001/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2121953002/diff/140001/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java#newcode199 content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:199: onRequestCursorUpdates(DISABLE_CURSOR_UPDATE); How about just calling mCursorAnchorInfoController.onRequestCursorUpdate(false, false)? Then we ...
4 years, 5 months ago (2016-07-07 01:05:36 UTC) #21
Seigo Nonaka
Thank you for your review! I addressed comments. Could you kindly take an another look? ...
4 years, 5 months ago (2016-07-07 13:38:26 UTC) #25
aelias_OOO_until_Jul13
https://codereview.chromium.org/2121953002/diff/140001/content/renderer/render_widget.cc File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/140001/content/renderer/render_widget.cc#newcode255 content/renderer/render_widget.cc:255: monitor_composition_info_(false), On 2016/07/07 at 13:38:26, Seigo Nonaka wrote: > ...
4 years, 5 months ago (2016-07-07 21:15:41 UTC) #26
Seigo Nonaka
I'm sorry for super late reply. I needed to work on another issue. Could you ...
4 years, 5 months ago (2016-07-13 07:12:07 UTC) #34
Seigo Nonaka
Ugh, looks like looks like real failures, let me fix the test.
4 years, 5 months ago (2016-07-14 02:50:15 UTC) #35
aelias_OOO_until_Jul13
https://codereview.chromium.org/2121953002/diff/140001/content/renderer/render_widget.cc File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/140001/content/renderer/render_widget.cc#newcode1740 content/renderer/render_widget.cc:1740: UpdateCompositionInfo(false /* update composition range */, On 2016/07/13 at ...
4 years, 5 months ago (2016-07-15 01:58:48 UTC) #36
Seigo Nonaka
I'm sorry for the super slow update. Finally I could make all trybots passing. Could ...
4 years, 5 months ago (2016-07-20 05:42:13 UTC) #82
aelias_OOO_until_Jul13
https://codereview.chromium.org/2121953002/diff/580001/content/renderer/render_widget.cc File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/580001/content/renderer/render_widget.cc#newcode260 content/renderer/render_widget.cc:260: monitor_composition_info_(true), Hmm, it looks like this was the way ...
4 years, 4 months ago (2016-07-25 22:21:56 UTC) #85
Seigo Nonaka
https://codereview.chromium.org/2121953002/diff/580001/content/renderer/render_widget.cc File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/580001/content/renderer/render_widget.cc#newcode260 content/renderer/render_widget.cc:260: monitor_composition_info_(true), On 2016/07/25 22:21:56, aelias wrote: > Hmm, it ...
4 years, 4 months ago (2016-07-26 01:08:15 UTC) #86
aelias_OOO_until_Jul13
https://codereview.chromium.org/2121953002/diff/580001/content/renderer/render_widget.cc File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/580001/content/renderer/render_widget.cc#newcode260 content/renderer/render_widget.cc:260: monitor_composition_info_(true), I agree tying this to render process creation ...
4 years, 4 months ago (2016-07-26 01:22:25 UTC) #87
Seigo Nonaka
https://codereview.chromium.org/2121953002/diff/580001/content/renderer/render_widget.cc File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/580001/content/renderer/render_widget.cc#newcode260 content/renderer/render_widget.cc:260: monitor_composition_info_(true), On 2016/07/26 01:22:24, aelias wrote: > I agree ...
4 years, 4 months ago (2016-07-26 01:30:54 UTC) #88
yukawa
> > What scenario does desktop actually need the monitoring for? This is for supporting ...
4 years, 4 months ago (2016-07-26 01:58:07 UTC) #90
Seigo Nonaka
On 2016/07/26 01:58:07, yukawa wrote: > > > What scenario does desktop actually need the ...
4 years, 4 months ago (2016-07-26 02:15:40 UTC) #93
yukawa
On 2016/07/26 02:15:40, Seigo Nonaka wrote: > On 2016/07/26 01:58:07, yukawa wrote: > > > ...
4 years, 4 months ago (2016-07-26 02:20:56 UTC) #94
yukawa
On 2016/07/26 02:15:40, Seigo Nonaka wrote: > On 2016/07/26 01:58:07, yukawa wrote: > > > ...
4 years, 4 months ago (2016-07-26 02:20:57 UTC) #95
Seigo Nonaka
On 2016/07/26 02:20:57, yukawa wrote: > On 2016/07/26 02:15:40, Seigo Nonaka wrote: > > On ...
4 years, 4 months ago (2016-07-26 02:23:27 UTC) #96
Seigo Nonaka
Looks like now all trybots are passed. Could you kindly take a look again? Thank ...
4 years, 4 months ago (2016-07-26 02:41:37 UTC) #97
aelias_OOO_until_Jul13
Thanks for following up, I appreciate it! https://codereview.chromium.org/2121953002/diff/600001/content/renderer/render_widget.cc File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/600001/content/renderer/render_widget.cc#newcode906 content/renderer/render_widget.cc:906: #if !defined(OS_ANDROID) ...
4 years, 4 months ago (2016-07-26 19:26:02 UTC) #100
Seigo Nonaka
I'm sorry for slow update, but now bots are now green. Could you kindly take ...
4 years, 4 months ago (2016-07-28 09:10:59 UTC) #123
aelias_OOO_until_Jul13
lgtm, thanks a lot!
4 years, 4 months ago (2016-07-28 18:50:40 UTC) #124
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2121953002/700001
4 years, 4 months ago (2016-07-29 01:26:21 UTC) #127
kinaba
Thanks a lot for your hard work, nona! On 2016/07/29 01:26:21, commit-bot: I haz the ...
4 years, 4 months ago (2016-07-29 01:28:26 UTC) #128
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/227468)
4 years, 4 months ago (2016-07-29 01:32:37 UTC) #130
Seigo Nonaka
+nasko for the content/ owners. Hi, Nasko, could you kindly take a look? Thank you.
4 years, 4 months ago (2016-07-29 01:44:35 UTC) #131
Seigo Nonaka
Sorry, forgot adding to the reviewer line. Nasko, could you kindly take a look as ...
4 years, 4 months ago (2016-07-29 01:46:00 UTC) #133
nasko
content/ and IPC LGTM, provided the nits are fixed. https://codereview.chromium.org/2121953002/diff/700001/content/browser/renderer_host/ime_adapter_android.cc File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2121953002/diff/700001/content/browser/renderer_host/ime_adapter_android.cc#newcode333 content/browser/renderer_host/ime_adapter_android.cc:333: ...
4 years, 4 months ago (2016-07-29 16:26:10 UTC) #134
Seigo Nonaka
Thank you for your review, Nasko. Also thank you again Alex and Kazuhiro for the ...
4 years, 4 months ago (2016-08-01 02:27:35 UTC) #139
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2121953002/720001
4 years, 4 months ago (2016-08-01 02:27:59 UTC) #142
commit-bot: I haz the power
Committed patchset #11 (id:720001)
4 years, 4 months ago (2016-08-01 02:31:20 UTC) #144
commit-bot: I haz the power
Patchset 11 (id:??) landed as https://crrev.com/dac0c7af14c29c9d8509b775d5044e5448b6263b Cr-Commit-Position: refs/heads/master@{#408900}
4 years, 4 months ago (2016-08-01 02:33:19 UTC) #146
aelias_OOO_until_Jul13
A revert of this CL (patchset #11 id:720001) has been created in https://codereview.chromium.org/2210533004/ by aelias@chromium.org. ...
4 years, 4 months ago (2016-08-04 19:46:57 UTC) #147
aelias_OOO_until_Jul13
Reopening this patchset. It's fine by me if you update a new patchset to fix ...
4 years, 4 months ago (2016-08-04 19:48:56 UTC) #148
Seigo Nonaka
Thank you for letting me know the failures and I'm sorry for late reply. Let ...
4 years, 4 months ago (2016-08-08 09:29:29 UTC) #149
Seigo Nonaka
It turned out that there are two issues. - updateCursorAnchorInfo should not emit before first ...
4 years, 4 months ago (2016-08-09 09:12:48 UTC) #157
Seigo Nonaka
Re-using lgtm from previous review. Alex, please let me know if you find any problems. ...
4 years, 4 months ago (2016-08-10 01:09:07 UTC) #158
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2121953002/760001
4 years, 4 months ago (2016-08-10 01:10:12 UTC) #161
aelias_OOO_until_Jul13
Looks like the change was the new "if (mHasCoordinateInfo)", lgtm
4 years, 4 months ago (2016-08-10 01:24:20 UTC) #162
commit-bot: I haz the power
Committed patchset #12 (id:760001)
4 years, 4 months ago (2016-08-10 02:36:39 UTC) #163
commit-bot: I haz the power
4 years, 4 months ago (2016-08-10 02:37:55 UTC) #165
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/fa291796686fef74a48f18061b9e36ccc9488a06
Cr-Commit-Position: refs/heads/master@{#410927}

Powered by Google App Engine
This is Rietveld 408576698