Do not calculate composition bounds until IME requests.

content/renderer/render_widget.cc

Index: content/renderer/render_widget.cc
diff --git a/content/renderer/render_widget.cc b/content/renderer/render_widget.cc
index 129ed07f6eb454e478f97327fc3e4969ea84065a..4dbc31cbafa4497412a20c37e987c3268f751798 100644
--- a/content/renderer/render_widget.cc
+++ b/content/renderer/render_widget.cc
@@ -252,6 +252,7 @@ RenderWidget::RenderWidget(CompositorDependencies* compositor_deps,
       device_scale_factor_(screen_info_.deviceScaleFactor),
 #if defined(OS_ANDROID)
       text_field_is_dirty_(false),
+      monitor_composition_info_(false),

[aelias_OOO_until_Jul13, 2016/07/07 00:13:35]

Please move this out of OS_ANDROID, as I don't see a strong reason to make this
platform-specific.

[Seigo Nonaka, 2016/07/07 13:38:26]

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?)

[aelias_OOO_until_Jul13, 2016/07/07 21:15:41]

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.

 #endif
       popup_origin_scale_for_emulation_(0.f),
       frame_swap_message_queue_(new FrameSwapMessageQueue()),
@@ -497,6 +498,8 @@ bool RenderWidget::OnMessageReceived(const IPC::Message& message) {
     IPC_MESSAGE_HANDLER(InputMsg_ImeEventAck, OnImeEventAck)
     IPC_MESSAGE_HANDLER(InputMsg_RequestTextInputStateUpdate,
                         OnRequestTextInputStateUpdate)
+    IPC_MESSAGE_HANDLER(InputMsg_RequestCompositionUpdate,
+                        OnRequestCompositionUpdate)
     IPC_MESSAGE_HANDLER(ViewMsg_ShowImeIfNeeded, OnShowImeIfNeeded)
 #endif
     IPC_MESSAGE_HANDLER(ViewMsg_HandleCompositorProto, OnHandleCompositorProto)
@@ -1371,7 +1374,8 @@ void RenderWidget::OnImeSetComposition(
     // sure we are in a consistent state.
     Send(new InputHostMsg_ImeCancelComposition(routing_id()));
   }
-  UpdateCompositionInfo(true);
+  UpdateCompositionInfo(true /* update composition range */,
+                        false /* force update */);
 }
 
 void RenderWidget::OnImeConfirmComposition(const base::string16& text,
@@ -1388,7 +1392,8 @@ void RenderWidget::OnImeConfirmComposition(const base::string16& text,
   else
     webwidget_->confirmComposition(WebWidget::DoNotKeepSelection);
   input_handler_->set_handling_input_event(false);
-  UpdateCompositionInfo(true);
+  UpdateCompositionInfo(true /* update composition range */,
+                        false /* force update */);
 }
 
 void RenderWidget::OnDeviceScaleFactorChanged() {
@@ -1469,7 +1474,12 @@ ui::TextInputType RenderWidget::GetTextInputType() {
   return ui::TEXT_INPUT_TYPE_NONE;
 }
 
-void RenderWidget::UpdateCompositionInfo(bool should_update_range) {
+void RenderWidget::UpdateCompositionInfo(
+        bool should_update_range, bool force_update) {

[aelias_OOO_until_Jul13, 2016/07/07 00:13:36]

Could you rename "force_update" to "immediate_request" to keep the terminology
consistent?

[Seigo Nonaka, 2016/07/07 13:38:26]

Done.

+#if defined(OS_ANDROID)
+  if (!monitor_composition_info_)
+    return;  // Do not calculate composition info if not requested.

[aelias_OOO_until_Jul13, 2016/07/07 00:13:36]

I don't think it's correct to early-exit here if "force_update == true", that is
not how the browser-side code worked.

[Seigo Nonaka, 2016/07/07 13:38:26]

Good catch! Yes, you are right!
Fixed. Thank you.

[aelias_OOO_until_Jul13, 2016/07/07 21:15:41]

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...

[Seigo Nonaka, 2016/07/13 07:12:07]

Sure added into ImeLolipopTest.java

+#endif  // OS_ANDROID
   TRACE_EVENT0("renderer", "RenderWidget::UpdateCompositionInfo");
   gfx::Range range = gfx::Range();
   if (should_update_range) {
@@ -1480,7 +1490,7 @@ void RenderWidget::UpdateCompositionInfo(bool should_update_range) {
   std::vector<gfx::Rect> character_bounds;
   GetCompositionCharacterBounds(&character_bounds);
 
-  if (!ShouldUpdateCompositionInfo(range, character_bounds))
+  if (!force_update && !ShouldUpdateCompositionInfo(range, character_bounds))
     return;
   composition_character_bounds_ = character_bounds;
   composition_range_ = range;
@@ -1538,6 +1548,15 @@ void RenderWidget::OnRequestTextInputStateUpdate() {
   UpdateSelectionBounds();
   UpdateTextInputState(ShowIme::HIDE_IME, ChangeSource::FROM_IME);
 }
+
+void RenderWidget::OnRequestCompositionUpdate(bool immediate_request,
+                                              bool monitor_request) {
+  monitor_composition_info_ = monitor_request;
+  if (!immediate_request)
+    return;
+  UpdateCompositionInfo(true /* update composition range */,
+                        true /* force update */);
+}
 #endif
 
 bool RenderWidget::ShouldHandleImeEvent() {
@@ -1718,7 +1737,8 @@ void RenderWidget::UpdateSelectionBounds() {
     }
   }
 
-  UpdateCompositionInfo(false);
+  UpdateCompositionInfo(false /* update composition range */,

[aelias_OOO_until_Jul13, 2016/07/07 00:13:35]

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?

[Seigo Nonaka, 2016/07/07 13:38:26]

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.

[aelias_OOO_until_Jul13, 2016/07/07 21:15:41]

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.

[Seigo Nonaka, 2016/07/13 07:12:07]

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.

[aelias_OOO_until_Jul13, 2016/07/15 01:58:48]

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 :).

[Seigo Nonaka, 2016/07/20 05:42:12]

Sure, finally I could make all test passing with removing this argument.

+                        false /* force update */);
 }
 
 void RenderWidget::SetDeviceColorProfileForTesting(
@@ -1818,7 +1838,8 @@ void RenderWidget::resetInputMethod() {
       Send(new InputHostMsg_ImeCancelComposition(routing_id()));
   }
 
-  UpdateCompositionInfo(true);
+  UpdateCompositionInfo(true /* update composition range */,
+                        false /* force update */);
 }
 
 #if defined(OS_ANDROID)