|
|
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. |
DescriptionDo 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 #Messages
Total messages: 165 (115 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== 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 rendere to stop calculation until it is requested. BUG=624714 ========== to ========== 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. And start calculation just when the IME requests. BUG=624714 ==========
Description was changed from ========== 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. And start calculation just when the IME requests. BUG=624714 ========== to ========== 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 ==========
nona@chromium.org changed reviewers: + kinaba@chromium.org, yukawa@chromium.org
Hi, Inaba-san, Yukawa-san, Could you kindly take a look? Thank you.
Thank you very much for taking over this! Here's the quick first round of comments. Also, could you try running the browser tests listed in crrev/1589953005? I suspect we might need to change some test expectations. https://codereview.chromium.org/2121953002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java (right): https://codereview.chromium.org/2121953002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:139: } We may have to invoke updateCursorAnchorInfo() from here. With this change, on the first IMMEDIATE case we don't receive onUpdateFrameInfo and only setCompositionCharacterBounds comes. https://codereview.chromium.org/2121953002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (left): https://codereview.chromium.org/2121953002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:196: mCursorAnchorInfoController.resetMonitoringState(); Is there any reason dropping the reset() call?
yosin@chromium.org changed reviewers: + yosin@chromium.org
Since Google C++ style guide doesn't mention about bool parameter, my comments about bool parameters are over kill. However, I feel multiple number of bool parameters are hard to read. https://codereview.chromium.org/2121953002/diff/40001/content/browser/rendere... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2121953002/diff/40001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:334: if (rwhi) nit: better to use early return style if (!rwhi) return; rwhi->Send(...); https://codereview.chromium.org/2121953002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2121953002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:196: onRequestCursorUpdates(0 /* disable any cursor update */); How about const int kDisableAnyCursorUpdate = 0; onRequrestCursorUpdate(kDisplayAnyCursorUpdate); https://codereview.chromium.org/2121953002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:664: if (mNativeImeAdapterAndroid != 0) { nit: Not need to have curly bracket. https://codereview.chromium.org/2121953002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2121953002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:1302: GetRenderWidget()->UpdateCompositionInfo(true, false /* force update */); How about const bool shouldUpdateRange = true; const bool kNoUpdate = false; GetRenderWidget()->UpdateCompositionInfo(shouldUpdateRange, kNoUpdate);
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Thank you for your review! PTAL https://codereview.chromium.org/2121953002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java (right): https://codereview.chromium.org/2121953002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:139: } On 2016/07/05 08:50:09, kinaba wrote: > We may have to invoke updateCursorAnchorInfo() from here. > > With this change, on the first IMMEDIATE case we don't receive onUpdateFrameInfo > and only setCompositionCharacterBounds comes. > Done. https://codereview.chromium.org/2121953002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (left): https://codereview.chromium.org/2121953002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:196: mCursorAnchorInfoController.resetMonitoringState(); On 2016/07/05 08:50:09, kinaba wrote: > Is there any reason dropping the reset() call? I replaced it with onRequestCursorUpdates(0) since now it resets both CursorAnchorInfoController and renderer's monitor state. https://codereview.chromium.org/2121953002/diff/40001/content/browser/rendere... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2121953002/diff/40001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:334: if (rwhi) On 2016/07/06 02:02:01, Yosi_UTC9 wrote: > nit: better to use early return style > > if (!rwhi) > return; > rwhi->Send(...); Done. https://codereview.chromium.org/2121953002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2121953002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:196: onRequestCursorUpdates(0 /* disable any cursor update */); On 2016/07/06 02:02:01, Yosi_UTC9 wrote: > How about > const int kDisableAnyCursorUpdate = 0; > onRequrestCursorUpdate(kDisplayAnyCursorUpdate); Done. https://codereview.chromium.org/2121953002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:664: if (mNativeImeAdapterAndroid != 0) { Thanks, but presubmit warns if I removed curly braces. Looks like we can drop curly braces only when it is single-line statement. https://codereview.chromium.org/2121953002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2121953002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:1302: GetRenderWidget()->UpdateCompositionInfo(true, false /* force update */); On 2016/07/06 02:02:01, Yosi_UTC9 wrote: > How about > > const bool shouldUpdateRange = true; > const bool kNoUpdate = false; > GetRenderWidget()->UpdateCompositionInfo(shouldUpdateRange, kNoUpdate); Done.
https://codereview.chromium.org/2121953002/diff/100001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/100001/content/renderer/rende... content/renderer/render_widget.cc:1548: void RenderWidget::OnRequestCursorUpdate(int mode) { How about using two bool parameters to match with UpdateCompositionInfo()? enum is better though. https://codereview.chromium.org/2121953002/diff/100001/content/renderer/rende... content/renderer/render_widget.cc:1554: if (mode & CURSOR_UPDATE_MONITOR) How about monitor_composition_info_ = mode & CURSOR_UPDATE_MONITOR; https://codereview.chromium.org/2121953002/diff/100001/content/renderer/rende... content/renderer/render_widget.cc:1559: if (mode & CURSOR_UPDATE_IMMEDIATE) How about if (!(mode & CURSOR_UPDATE_IMMEDIATE) return; const bool kForceUpdate = true; UpdateCompositionInfo(true, kForceUpdate);
yukawa@chromium.org changed reviewers: + aelias@chromium.org
Sorry, I cannot review this right now as I'm on vacation til next week and have only limited internet access. Also adding aelias@ as I think this should be in his radar.
Thank you for your review. Thank you Yohei for introducing aelias@. Alex, could you kindly take a look? This CL fixes the performance regression on Android with Hindi input. Thank you. https://codereview.chromium.org/2121953002/diff/100001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/100001/content/renderer/rende... content/renderer/render_widget.cc:1548: void RenderWidget::OnRequestCursorUpdate(int mode) { On 2016/07/06 03:33:08, Yosi_UTC9 wrote: > How about using two bool parameters to match with UpdateCompositionInfo()? > enum is better though. Sure, personally, enum is rarely used for this case here so I changed this to two bool parameters. https://codereview.chromium.org/2121953002/diff/100001/content/renderer/rende... content/renderer/render_widget.cc:1554: if (mode & CURSOR_UPDATE_MONITOR) On 2016/07/06 03:33:08, Yosi_UTC9 wrote: > How about > monitor_composition_info_ = mode & CURSOR_UPDATE_MONITOR; You are right, but I moved this calculation to Java side. Thank you for your comment. https://codereview.chromium.org/2121953002/diff/100001/content/renderer/rende... content/renderer/render_widget.cc:1559: if (mode & CURSOR_UPDATE_IMMEDIATE) On 2016/07/06 03:33:08, Yosi_UTC9 wrote: > How about > > if (!(mode & CURSOR_UPDATE_IMMEDIATE) > return; > const bool kForceUpdate = true; > UpdateCompositionInfo(true, kForceUpdate); Hmm, introducing const boolean sounds overkill to me and rarely used here. Let me add comments for the argument instead.
Thank you for your review. Thank you Yohei for introducing aelias@. Alex, could you kindly take a look? This CL fixes the performance regression on Android with Hindi input. Thank you.
From my side, lgtm with some nits https://codereview.chromium.org/2121953002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (left): https://codereview.chromium.org/2121953002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:196: mCursorAnchorInfoController.resetMonitoringState(); On 2016/07/06 03:25:56, Seigo Nonaka wrote: > On 2016/07/05 08:50:09, kinaba wrote: > > Is there any reason dropping the reset() call? > > I replaced it with onRequestCursorUpdates(0) since now it resets both > CursorAnchorInfoController and renderer's monitor state. Ah, I think I misunderstood the code. lg. https://codereview.chromium.org/2121953002/diff/120001/content/renderer/rende... File content/renderer/render_widget.h (right): https://codereview.chromium.org/2121953002/diff/120001/content/renderer/rende... content/renderer/render_widget.h:352: // changed. If they are changed, the new value will be sent to the browser Could you update the comment reflecting the force_update=true case? https://codereview.chromium.org/2121953002/diff/120001/content/renderer/rende... content/renderer/render_widget.h:487: void OnRequestCursorUpdate(bool immediate_request, bool monitor_request); How about naming "OnRequestCompositionUpdate"? At least for the renderer's point of view this is all about Composition, not Cursor.
Kinaba-san, thank you for your review! https://codereview.chromium.org/2121953002/diff/120001/content/renderer/rende... File content/renderer/render_widget.h (right): https://codereview.chromium.org/2121953002/diff/120001/content/renderer/rende... content/renderer/render_widget.h:352: // changed. If they are changed, the new value will be sent to the browser On 2016/07/06 06:08:33, kinaba wrote: > Could you update the comment reflecting the force_update=true case? Done. https://codereview.chromium.org/2121953002/diff/120001/content/renderer/rende... content/renderer/render_widget.h:487: void OnRequestCursorUpdate(bool immediate_request, bool monitor_request); On 2016/07/06 06:08:33, kinaba wrote: > How about naming "OnRequestCompositionUpdate"? > At least for the renderer's point of view this is all about Composition, not > Cursor. Make sense to me. Similarly, also I updated IPC name from Cursor to Composition since it is actually request composition info update.
https://codereview.chromium.org/2121953002/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java (left): https://codereview.chromium.org/2121953002/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:205: mMonitorModeEnabled = false; These two booleans mHasPendingImmediateRequest and mMonitorModeEnabled are now duplicative of state tracked in the render process, and I think they should be deleted. In the browser side, we should now be able to just blindly notify the IME app whenever we receive a new CompositionBounds from the renderer. https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:255: monitor_composition_info_(false), Please move this out of OS_ANDROID, as I don't see a strong reason to make this platform-specific. https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1478: bool should_update_range, bool force_update) { Could you rename "force_update" to "immediate_request" to keep the terminology consistent? https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1481: return; // Do not calculate composition info if not requested. I don't think it's correct to early-exit here if "force_update == true", that is not how the browser-side code worked. https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1740: UpdateCompositionInfo(false /* update composition range */, This scenario of setting update_range to false when selection bounds changed is confusing. At a minimum, I think we should rename "update_range" to "selection_only_changed" to clarify the distinction from "force_update". But also, I don't understand why we need to do anything at all when only selection changes, and what exactly would break if we simply removed this call from UpdateSelectionBounds(). Could you check if a unit test fails if this call is removed, and if so why?
changwan@chromium.org changed reviewers: + changwan@chromium.org
https://codereview.chromium.org/2121953002/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2121953002/diff/140001/content/public/android... 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 don't need to add a new constant.
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
Patchset #6 (id:200001) has been deleted
Thank you for your review! I addressed comments. Could you kindly take an another look? Thank you. https://codereview.chromium.org/2121953002/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java (left): https://codereview.chromium.org/2121953002/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:205: mMonitorModeEnabled = false; On 2016/07/07 00:13:35, aelias wrote: > These two booleans mHasPendingImmediateRequest and mMonitorModeEnabled are now > duplicative of state tracked in the render process, and I think they should be > deleted. In the browser side, we should now be able to just blindly notify the > IME app whenever we receive a new CompositionBounds from the renderer. I think we still need the both two members since we need to observe compositor coordination changes and need to update the cursor information if IME requests monitor mode. https://codereview.chromium.org/2121953002/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2121953002/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:199: onRequestCursorUpdates(DISABLE_CURSOR_UPDATE); On 2016/07/07 01:05:36, Changwan Ryu wrote: > How about just calling mCursorAnchorInfoController.onRequestCursorUpdate(false, > false)? > > Then we don't need to add a new constant. Sure fixed. https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:255: monitor_composition_info_(false), On 2016/07/07 00:13:35, aelias wrote: > Please move this out of OS_ANDROID, as I don't see a strong reason to make this > platform-specific. I don't have strong opinion to this, but this flag will be unused variable outside of Android platform. Is it acceptable? (I may be too nervous?) https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1478: bool should_update_range, bool force_update) { On 2016/07/07 00:13:36, aelias wrote: > Could you rename "force_update" to "immediate_request" to keep the terminology > consistent? Done. https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1481: return; // Do not calculate composition info if not requested. On 2016/07/07 00:13:36, aelias wrote: > I don't think it's correct to early-exit here if "force_update == true", that is > not how the browser-side code worked. Good catch! Yes, you are right! Fixed. Thank you. https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1740: UpdateCompositionInfo(false /* update composition range */, On 2016/07/07 00:13:35, aelias wrote: > This scenario of setting update_range to false when selection bounds changed is > confusing. At a minimum, I think we should rename "update_range" to > "selection_only_changed" to clarify the distinction from "force_update". > > But also, I don't understand why we need to do anything at all when only > selection changes, and what exactly would break if we simply removed this call > from UpdateSelectionBounds(). Could you check if a unit test fails if this call > is removed, and if so why? SelectionBoundsChanged is called when the bounding rectangle of the selection was changed. This can be called even if the selection range in the web content is not changed. For example, the offset of the text edit box is changed by timer or etc. IIRC, only IME can change the composition range except for blurring the text field, so we can assume we can re-use the previous composition range here. On the other hand, if the selection bounds (or caret bounds) are changed, the composition bounds can be also changed. Thus, call UpdateCompositionInfo without re-calculating composition range. I agree with "update_range" doesn't make sense here. I think "use_cached_range" is more appropriate name.
https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:255: monitor_composition_info_(false), On 2016/07/07 at 13:38:26, Seigo Nonaka wrote: > On 2016/07/07 00:13:35, aelias wrote: > > Please move this out of OS_ANDROID, as I don't see a strong reason to make this > > platform-specific. > > I don't have strong opinion to this, but this flag will be unused variable outside of Android platform. Is it acceptable? (I may be too nervous?) Well, I think this file in general greatly overuses OS_ANDROID and I have a strong preference to avoid platform-specific ifdefs (this makes unit testing much easier, among other things), but I suppose it might be cleaned up all at once in a future patch rather than exposing things piecemeal. I guess it's fine either way for this. https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1481: return; // Do not calculate composition info if not requested. On 2016/07/07 at 13:38:26, Seigo Nonaka wrote: > On 2016/07/07 00:13:36, aelias wrote: > > I don't think it's correct to early-exit here if "force_update == true", that is > > not how the browser-side code worked. > > Good catch! Yes, you are right! > Fixed. Thank you. Hmm, I'm concerned that no trybot caught this bug. It looks like all tests in CursorAnchorInfoControllerTest.java mock out the render process so the code here has no coverage. Could you write an integration test creating a real web page and exercising CursorAnchorInfo end-to-end from Java? This hopefully shouldn't be too hard since we have many existing tests doing this: just derive from ContentShellTestBase. ImeTest.java might be a good inspiration. To run the content_shell_test_apk locally on your phone, see instructions at https://chromium.googlesource.com/chromium/src/+/master/docs/android_test_ins... https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1740: UpdateCompositionInfo(false /* update composition range */, On 2016/07/07 at 13:38:26, Seigo Nonaka wrote: > IIRC, only IME can change the composition range except for blurring the text field, so we can assume we can re-use the previous composition range here. On the other hand, if the selection bounds (or caret bounds) are changed, the composition bounds can be also changed. > Thus, call UpdateCompositionInfo without re-calculating composition range. So the caching is a pure performance optimization? This optimization seems too small to be worth the logic complexity, given that we've observed the most expensive part is GetCompositionBounds, and that's unconditionally called. I'd prefer this caching logic be removed to keep this code simple to reason about, and because your new optimization should get us most of the benefit.
Patchset #7 (id:240001) has been deleted
Patchset #7 (id:260001) has been deleted
Patchset #8 (id:300001) has been deleted
Patchset #7 (id:280001) has been deleted
Patchset #9 (id:360001) has been deleted
Patchset #8 (id:340001) has been deleted
Patchset #7 (id:320001) has been deleted
I'm sorry for super late reply. I needed to work on another issue. Could you kindly take a look again? Thank you. https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1481: return; // Do not calculate composition info if not requested. On 2016/07/07 21:15:41, aelias wrote: > On 2016/07/07 at 13:38:26, Seigo Nonaka wrote: > > On 2016/07/07 00:13:36, aelias wrote: > > > I don't think it's correct to early-exit here if "force_update == true", > that is > > > not how the browser-side code worked. > > > > Good catch! Yes, you are right! > > Fixed. Thank you. > > Hmm, I'm concerned that no trybot caught this bug. It looks like all tests in > CursorAnchorInfoControllerTest.java mock out the render process so the code here > has no coverage. Could you write an integration test creating a real web page > and exercising CursorAnchorInfo end-to-end from Java? This hopefully shouldn't > be too hard since we have many existing tests doing this: just derive from > ContentShellTestBase. ImeTest.java might be a good inspiration. > > To run the content_shell_test_apk locally on your phone, see instructions at > https://chromium.googlesource.com/chromium/src/+/master/docs/android_test_ins... Sure added into ImeLolipopTest.java https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1740: UpdateCompositionInfo(false /* update composition range */, On 2016/07/07 21:15:41, aelias wrote: > On 2016/07/07 at 13:38:26, Seigo Nonaka wrote: > > IIRC, only IME can change the composition range except for blurring the text > field, so we can assume we can re-use the previous composition range here. On > the other hand, if the selection bounds (or caret bounds) are changed, the > composition bounds can be also changed. > > Thus, call UpdateCompositionInfo without re-calculating composition range. > > So the caching is a pure performance optimization? This optimization seems too > small to be worth the logic complexity, given that we've observed the most > expensive part is GetCompositionBounds, and that's unconditionally called. I'd > prefer this caching logic be removed to keep this code simple to reason about, > and because your new optimization should get us most of the benefit. Sure, I'm happy to remove this flag, but can I do it in subsequent CL? I'd like to keep this CL only for Android since this CL ideally doesn't affect other platforms.
Ugh, looks like looks like real failures, let me fix the test.
https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1740: UpdateCompositionInfo(false /* update composition range */, On 2016/07/13 at 07:12:07, Seigo Nonaka wrote: > Sure, I'm happy to remove this flag, but can I do it in subsequent CL? > I'd like to keep this CL only for Android since this CL ideally doesn't affect other platforms. I'd ask you to do it as part this CL; the risk of introducing bugs in other platforms seems quite low, and if there is such a bug it would probably affect android as well so the extra visibility/feedback will be nice :).
The CQ bit was checked by nona@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nona@chromium.org to run a CQ dry run
Patchset #8 (id:400001) has been deleted
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by nona@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...
Patchset #8 (id:420001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #8 (id:440001) has been deleted
The CQ bit was checked by nona@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nona@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nona@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...
Patchset #8 (id:460001) has been deleted
Patchset #8 (id:480001) has been deleted
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_...)
The CQ bit was checked by nona@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #9 (id:520001) has been deleted
The CQ bit was checked by nona@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...
Patchset #8 (id:500001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nona@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...
Patchset #9 (id:560001) has been deleted
The CQ bit was checked by nona@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...
Patchset #8 (id:540001) has been deleted
I'm sorry for the super slow update. Finally I could make all trybots passing. Could you kindly resume the review? Thank you. https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1740: UpdateCompositionInfo(false /* update composition range */, On 2016/07/15 01:58:48, aelias_OOO_until_July_25 wrote: > On 2016/07/13 at 07:12:07, Seigo Nonaka wrote: > > Sure, I'm happy to remove this flag, but can I do it in subsequent CL? > > I'd like to keep this CL only for Android since this CL ideally doesn't affect > other platforms. > > I'd ask you to do it as part this CL; the risk of introducing bugs in other > platforms seems quite low, and if there is such a bug it would probably affect > android as well so the extra visibility/feedback will be nice :). Sure, finally I could make all test passing with removing this argument.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2121953002/diff/580001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/580001/content/renderer/rende... content/renderer/render_widget.cc:260: monitor_composition_info_(true), Hmm, it looks like this was the way of fixing non-Android tests, but I don't like it because it adds unnecessary complexity/behavior change in production code. Could you change the tests themselves to invoke RequestCompositionUpdate?
https://codereview.chromium.org/2121953002/diff/580001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/580001/content/renderer/rende... content/renderer/render_widget.cc:260: monitor_composition_info_(true), On 2016/07/25 22:21:56, aelias wrote: > Hmm, it looks like this was the way of fixing non-Android tests, but I don't > like it because it adds unnecessary complexity/behavior change in production > code. Could you change the tests themselves to invoke RequestCompositionUpdate? This line is necessary not only for testing purpose but also for production on non-Android platforms. On desktop platform, renderer always monitors composition info. This is current behavior of ToT and I want to keep it. If we want to remove platform dependent initialization, we may need to invoke RequestCompositionUpdate on renderer startup time for disabling on Android or for enabling on non-Android platform. I guess it is more complex than this approach.
https://codereview.chromium.org/2121953002/diff/580001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/580001/content/renderer/rende... content/renderer/render_widget.cc:260: monitor_composition_info_(true), I agree tying this to render process creation would not be great, but having different platform-specific defaults is complex in its own way. What scenario does desktop actually need the monitoring for? Would it be possible to bracket RequestCompositionUpdate around that scenario? It could be as wide as when a textbox is focused/unfocused.
https://codereview.chromium.org/2121953002/diff/580001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/580001/content/renderer/rende... content/renderer/render_widget.cc:260: monitor_composition_info_(true), On 2016/07/26 01:22:24, aelias wrote: > I agree tying this to render process creation would not be great, but having > different platform-specific defaults is complex in its own way. > > What scenario does desktop actually need the monitoring for? Would it be > possible to bracket RequestCompositionUpdate around that scenario? It could be > as wide as when a textbox is focused/unfocused. Setting true on getting text input focus and false on blurring the text input focus sounds good idea. Will try to do it.
yukawa@chromium.org changed reviewers: + shuchen@chromium.org
> > What scenario does desktop actually need the monitoring for? This is for supporting platform specific IME callbacks that need to be handled in the browser process. - Windows: - COMPOSITIONFORM https://msdn.microsoft.com/en-us/library/aa741225.aspx - CANDIDATEFORM: https://msdn.microsoft.com/en-us/library/aa741223.aspx - IMR_QUERYCHARPOSITION: https://msdn.microsoft.com/en-us/library/windows/desktop/dd318634.aspx - mac: firstRect(forCharacterRange:actualRange:) https://developer.apple.com/reference/appkit/nstextinputclient/1438240-firstr... - Linux (snip) - Chromium OS (snip) For historical reasons, InputConnection#requestCursorUpdates() does not have any corresponding API in those desktop platforms. That's why this is true by default on desktop. Note that Mac one is indeed tricky, because it's a synchronous callback from the system to the browser process, and unfortunately there is no way to ask the system to callback again later hence it's important to keep pushing data from the renderer to the browser in order to avoid sync IPC from the browser process to the renderer process. See crbug.com/473850 about how this had caused lots of troubles. > Setting true on getting text input focus and false on blurring the text input > focus sounds good idea. Will try to do it. Adding shuchen@ just in case as it could affect desktop IMEs.
The CQ bit was checked by nona@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...
On 2016/07/26 01:58:07, yukawa wrote: > > > What scenario does desktop actually need the monitoring for? > > This is for supporting platform specific IME callbacks that need to be handled > in the browser process. > - Windows: > - COMPOSITIONFORM > https://msdn.microsoft.com/en-us/library/aa741225.aspx > - CANDIDATEFORM: > https://msdn.microsoft.com/en-us/library/aa741223.aspx > - IMR_QUERYCHARPOSITION: > https://msdn.microsoft.com/en-us/library/windows/desktop/dd318634.aspx > - mac: > firstRect(forCharacterRange:actualRange:) > > https://developer.apple.com/reference/appkit/nstextinputclient/1438240-firstr... > - Linux > (snip) > - Chromium OS > (snip) > > For historical reasons, InputConnection#requestCursorUpdates() does not have any > corresponding API in those desktop platforms. That's why this is true by > default on desktop. > > Note that Mac one is indeed tricky, because it's a synchronous callback from the > system to the browser process, and unfortunately there is no way to ask the > system to callback again later hence it's important to keep pushing data from > the renderer to the browser in order to avoid sync IPC from the browser process > to the renderer process. See crbug.com/473850 about how this had caused lots of > troubles. > > > Setting true on getting text input focus and false on blurring the text input > > focus sounds good idea. Will try to do it. > > Adding shuchen@ just in case as it could affect desktop IMEs. Thank you for your follow-up comments. Yukawa-san, as far as I know, there is no APIs compatible with InputConnection.requestCursorUpdate(MONITOR) in Windows, is this correct? I'm sure that there is no such API on Chrome OS and Linux, but sorry I'm not so familiar with Windows APIs. If there is such an interface, we can skip the composition calculation for better performance on Windows.
On 2016/07/26 02:15:40, Seigo Nonaka wrote: > On 2016/07/26 01:58:07, yukawa wrote: > > > > What scenario does desktop actually need the monitoring for? > > > > This is for supporting platform specific IME callbacks that need to be handled > > in the browser process. > > - Windows: > > - COMPOSITIONFORM > > https://msdn.microsoft.com/en-us/library/aa741225.aspx > > - CANDIDATEFORM: > > https://msdn.microsoft.com/en-us/library/aa741223.aspx > > - IMR_QUERYCHARPOSITION: > > https://msdn.microsoft.com/en-us/library/windows/desktop/dd318634.aspx > > - mac: > > firstRect(forCharacterRange:actualRange:) > > > > > https://developer.apple.com/reference/appkit/nstextinputclient/1438240-firstr... > > - Linux > > (snip) > > - Chromium OS > > (snip) > > > > For historical reasons, InputConnection#requestCursorUpdates() does not have > any > > corresponding API in those desktop platforms. That's why this is true by > > default on desktop. > > > > Note that Mac one is indeed tricky, because it's a synchronous callback from > the > > system to the browser process, and unfortunately there is no way to ask the > > system to callback again later hence it's important to keep pushing data from > > the renderer to the browser in order to avoid sync IPC from the browser > process > > to the renderer process. See crbug.com/473850 about how this had caused lots > of > > troubles. > > > > > Setting true on getting text input focus and false on blurring the text > input > > > focus sounds good idea. Will try to do it. > > > > Adding shuchen@ just in case as it could affect desktop IMEs. > > Thank you for your follow-up comments. > Yukawa-san, as far as I know, there is no APIs compatible with > InputConnection.requestCursorUpdate(MONITOR) in Windows, is this correct? > I'm sure that there is no such API on Chrome OS and Linux, but sorry I'm not so > familiar with Windows APIs. > If there is such an interface, we can skip the composition calculation for > better performance on Windows. No, Windows does not have such any API that corresponds to InputConnection#requestCursorUpdates().
On 2016/07/26 02:15:40, Seigo Nonaka wrote: > On 2016/07/26 01:58:07, yukawa wrote: > > > > What scenario does desktop actually need the monitoring for? > > > > This is for supporting platform specific IME callbacks that need to be handled > > in the browser process. > > - Windows: > > - COMPOSITIONFORM > > https://msdn.microsoft.com/en-us/library/aa741225.aspx > > - CANDIDATEFORM: > > https://msdn.microsoft.com/en-us/library/aa741223.aspx > > - IMR_QUERYCHARPOSITION: > > https://msdn.microsoft.com/en-us/library/windows/desktop/dd318634.aspx > > - mac: > > firstRect(forCharacterRange:actualRange:) > > > > > https://developer.apple.com/reference/appkit/nstextinputclient/1438240-firstr... > > - Linux > > (snip) > > - Chromium OS > > (snip) > > > > For historical reasons, InputConnection#requestCursorUpdates() does not have > any > > corresponding API in those desktop platforms. That's why this is true by > > default on desktop. > > > > Note that Mac one is indeed tricky, because it's a synchronous callback from > the > > system to the browser process, and unfortunately there is no way to ask the > > system to callback again later hence it's important to keep pushing data from > > the renderer to the browser in order to avoid sync IPC from the browser > process > > to the renderer process. See crbug.com/473850 about how this had caused lots > of > > troubles. > > > > > Setting true on getting text input focus and false on blurring the text > input > > > focus sounds good idea. Will try to do it. > > > > Adding shuchen@ just in case as it could affect desktop IMEs. > > Thank you for your follow-up comments. > Yukawa-san, as far as I know, there is no APIs compatible with > InputConnection.requestCursorUpdate(MONITOR) in Windows, is this correct? > I'm sure that there is no such API on Chrome OS and Linux, but sorry I'm not so > familiar with Windows APIs. > If there is such an interface, we can skip the composition calculation for > better performance on Windows. No, Windows does not have such any API that corresponds to InputConnection#requestCursorUpdates().
On 2016/07/26 02:20:57, yukawa wrote: > On 2016/07/26 02:15:40, Seigo Nonaka wrote: > > On 2016/07/26 01:58:07, yukawa wrote: > > > > > What scenario does desktop actually need the monitoring for? > > > > > > This is for supporting platform specific IME callbacks that need to be > handled > > > in the browser process. > > > - Windows: > > > - COMPOSITIONFORM > > > https://msdn.microsoft.com/en-us/library/aa741225.aspx > > > - CANDIDATEFORM: > > > https://msdn.microsoft.com/en-us/library/aa741223.aspx > > > - IMR_QUERYCHARPOSITION: > > > https://msdn.microsoft.com/en-us/library/windows/desktop/dd318634.aspx > > > - mac: > > > firstRect(forCharacterRange:actualRange:) > > > > > > > > > https://developer.apple.com/reference/appkit/nstextinputclient/1438240-firstr... > > > - Linux > > > (snip) > > > - Chromium OS > > > (snip) > > > > > > For historical reasons, InputConnection#requestCursorUpdates() does not have > > any > > > corresponding API in those desktop platforms. That's why this is true by > > > default on desktop. > > > > > > Note that Mac one is indeed tricky, because it's a synchronous callback from > > the > > > system to the browser process, and unfortunately there is no way to ask the > > > system to callback again later hence it's important to keep pushing data > from > > > the renderer to the browser in order to avoid sync IPC from the browser > > process > > > to the renderer process. See crbug.com/473850 about how this had caused > lots > > of > > > troubles. > > > > > > > Setting true on getting text input focus and false on blurring the text > > input > > > > focus sounds good idea. Will try to do it. > > > > > > Adding shuchen@ just in case as it could affect desktop IMEs. > > > > Thank you for your follow-up comments. > > Yukawa-san, as far as I know, there is no APIs compatible with > > InputConnection.requestCursorUpdate(MONITOR) in Windows, is this correct? > > I'm sure that there is no such API on Chrome OS and Linux, but sorry I'm not > so > > familiar with Windows APIs. > > If there is such an interface, we can skip the composition calculation for > > better performance on Windows. > > No, Windows does not have such any API that corresponds to > InputConnection#requestCursorUpdates(). Okay, then, skipping composition calculation on non editable node is the best effort as for now. Thank you very much!
Looks like now all trybots are passed. Could you kindly take a look again? Thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for following up, I appreciate it! https://codereview.chromium.org/2121953002/diff/600001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/600001/content/renderer/rende... content/renderer/render_widget.cc:906: #if !defined(OS_ANDROID) Sorry, I realize you're just following the conventions in this file, but putting one-liner behavior changes behind platform ifdefs is a bad practice that often causes pain and confusion later, and I am aiming to slowly remove all cases of that. The RequestCompositionUpdate IPC mechanism is powerful enough for all platforms, so it should not be needed to fork the logic here. It would be almost equivalent to have RenderWidgetHostView{Aura,Mac}::TextInputStateChanged implementation that sends a InputMsg_RequestCompositionUpdate, so can you do that? (If there is a timing problem with that, then feel free to widen it to another condition, but I just would like to avoid an ifdef in this file and have the behavior differ only in the actual browser-side platform files.)
The CQ bit was checked by nona@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by nona@chromium.org to run a CQ dry run
Patchset #10 (id:620001) has been deleted
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by nona@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 checked by nona@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nona@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...
Patchset #11 (id:660001) has been deleted
Patchset #10 (id:640001) has been deleted
Patchset #10 (id:680001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm sorry for slow update, but now bots are now green. Could you kindly take an another look? Thank you. https://codereview.chromium.org/2121953002/diff/600001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/600001/content/renderer/rende... content/renderer/render_widget.cc:906: #if !defined(OS_ANDROID) On 2016/07/26 19:26:02, aelias wrote: > Sorry, I realize you're just following the conventions in this file, but putting > one-liner behavior changes behind platform ifdefs is a bad practice that often > causes pain and confusion later, and I am aiming to slowly remove all cases of > that. The RequestCompositionUpdate IPC mechanism is powerful enough for all > platforms, so it should not be needed to fork the logic here. > > It would be almost equivalent to have > RenderWidgetHostView{Aura,Mac}::TextInputStateChanged implementation that sends > a InputMsg_RequestCompositionUpdate, so can you do that? (If there is a timing > problem with that, then feel free to widen it to another condition, but I just > would like to avoid an ifdef in this file and have the behavior differ only in > the actual browser-side platform files.) Sure, for Mac OSX, it is not a monitor implementation, so I fixed only for Aura.
lgtm, thanks a lot!
The CQ bit was checked by nona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinaba@chromium.org Link to the patchset: https://codereview.chromium.org/2121953002/#ps700001 (title: "remove OS_ANDROID from render_widget.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks a lot for your hard work, nona! On 2016/07/29 01:26:21, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... I guess we need content/ OWNER review too?
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...)
+nasko for the content/ owners. Hi, Nasko, could you kindly take a look? Thank you.
nona@chromium.org changed reviewers: + nasko@chromium.org
Sorry, forgot adding to the reviewer line. Nasko, could you kindly take a look as the content/ owner? Thank you.
content/ and IPC LGTM, provided the nits are fixed. https://codereview.chromium.org/2121953002/diff/700001/content/browser/render... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2121953002/diff/700001/content/browser/render... content/browser/renderer_host/ime_adapter_android.cc:333: bool immediate_request, bool monitor_request) { style: Each parameter should be on a separate line. https://codereview.chromium.org/2121953002/diff/700001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/700001/content/renderer/rende... content/renderer/render_widget.cc:506: OnRequestCompositionUpdate) nit: This should move before the #if defined(OS_ANDROID) section. https://codereview.chromium.org/2121953002/diff/700001/content/renderer/rende... content/renderer/render_widget.cc:906: Was this new empty line intentional?
The CQ bit was checked by nona@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: This issue passed the CQ dry run.
Thank you for your review, Nasko. Also thank you again Alex and Kazuhiro for the tough review! Submitting... https://codereview.chromium.org/2121953002/diff/700001/content/browser/render... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2121953002/diff/700001/content/browser/render... content/browser/renderer_host/ime_adapter_android.cc:333: bool immediate_request, bool monitor_request) { On 2016/07/29 16:26:10, nasko wrote: > style: Each parameter should be on a separate line. Done. https://codereview.chromium.org/2121953002/diff/700001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2121953002/diff/700001/content/renderer/rende... content/renderer/render_widget.cc:506: OnRequestCompositionUpdate) On 2016/07/29 16:26:10, nasko wrote: > nit: This should move before the #if defined(OS_ANDROID) section. Done. https://codereview.chromium.org/2121953002/diff/700001/content/renderer/rende... content/renderer/render_widget.cc:906: On 2016/07/29 16:26:10, nasko wrote: > Was this new empty line intentional? No, removed.
The CQ bit was checked by nona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, nasko@chromium.org, kinaba@chromium.org Link to the patchset: https://codereview.chromium.org/2121953002/#ps720001 (title: "Addresses Nasko's comments")
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.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:720001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Cr-Commit-Position: refs/heads/master@{#408900} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/dac0c7af14c29c9d8509b775d5044e5448b6263b Cr-Commit-Position: refs/heads/master@{#408900}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:720001) has been created in https://codereview.chromium.org/2210533004/ by aelias@chromium.org. The reason for reverting is: CursorAnchorInfoControllerTest.testMonitorMode consistently failing on downstream lollipop bots (and I also see the same failure in the trybots for patchset 11, I don't understand why CQ let this through...). BUG=633402.
Reopening this patchset. It's fine by me if you update a new patchset to fix the problem here reusing existing lgt'm, no need to create a new one in my opinion.
Thank you for letting me know the failures and I'm sorry for late reply. Let me investigate the failures. I may need to understand why the IPC unnecessary invoked.
The CQ bit was checked by nona@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 checked by nona@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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Patchset #12 (id:740001) has been deleted
It turned out that there are two issues. - updateCursorAnchorInfo should not emit before first frame update. - The original code doesn't invoke updateCursorAnchorInfo by CompositionUpdate IPC arrival but new my implementation requires to emit it. So need to update the test expectation. I fixed both in the latest CL.
Re-using lgtm from previous review. Alex, please let me know if you find any problems. Thank you.
The CQ bit was checked by nona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, nasko@chromium.org, kinaba@chromium.org Link to the patchset: https://codereview.chromium.org/2121953002/#ps760001 (title: "fix test failures")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Looks like the change was the new "if (mHasCoordinateInfo)", lgtm
Message was sent while issue was closed.
Committed patchset #12 (id:760001)
Message was sent while issue was closed.
Description was changed from ========== 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 Cr-Commit-Position: refs/heads/master@{#408900} ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/fa291796686fef74a48f18061b9e36ccc9488a06 Cr-Commit-Position: refs/heads/master@{#410927} |