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

Issue 2856093003: Update TextSelection for non-user initiated events (Closed)

Created:
3 years, 7 months ago by Peter Varga
Modified:
3 years, 7 months ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nasko+codewatch_chromium.org, jam, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, James Su
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Update TextSelection for non-user initiated events This makes Chromium Content API to be able to notify about text selection changes triggered by non-user events (eg. JavaScript, IME, autofill). R=creis@chromium.org,nasko@chromium.org,ekaramad@chromium.org BUG=671986 TEST=interactive_ui_tests --gtest_filter=SitePerProcessTextInputManagerTest.NonUserInitiatedTextSelection CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2856093003 Cr-Commit-Position: refs/heads/master@{#473834} Committed: https://chromium.googlesource.com/chromium/src/+/d48794b9728276c1183b0c56f8cb7afe71b7bca4

Patch Set 1 #

Total comments: 25

Patch Set 2 : Update TextSelection for non-user initiated events #

Total comments: 2

Patch Set 3 : Update TextSelection for non-user initiated events #

Patch Set 4 : Rework and split test #

Patch Set 5 : Workaround mac fail #

Total comments: 8

Patch Set 6 : Update TextSelection for non-user initiated events #

Total comments: 4

Patch Set 7 : Update TextSelection for non-user initiated events #

Unified diffs Side-by-side diffs Delta from patch set Stats (+283 lines, -107 lines) Patch
M AUTHORS View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc View 1 2 3 4 5 6 4 chunks +148 lines, -24 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.h View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 1 chunk +3 lines, -2 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_guest.h View 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/frame_host/render_widget_host_view_guest.cc View 1 2 3 1 chunk +3 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.h View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.cc View 1 2 3 4 1 chunk +6 lines, -3 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura_unittest.cc View 1 2 3 4 chunks +4 lines, -4 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_base.h View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_view_base.cc View 1 2 3 1 chunk +6 lines, -3 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_mac_unittest.mm View 1 2 4 chunks +6 lines, -6 lines 0 comments Download
M content/browser/renderer_host/text_input_manager.h View 1 3 chunks +10 lines, -4 lines 0 comments Download
M content/browser/renderer_host/text_input_manager.cc View 1 3 chunks +7 lines, -5 lines 0 comments Download
M content/common/frame_messages.h View 1 2 3 1 chunk +3 lines, -2 lines 0 comments Download
M content/public/renderer/render_frame.h View 1 2 3 1 chunk +0 lines, -5 lines 0 comments Download
M content/public/test/text_input_test_utils.h View 1 chunk +6 lines, -0 lines 0 comments Download
M content/public/test/text_input_test_utils.cc View 1 chunk +15 lines, -0 lines 0 comments Download
M content/renderer/pepper/pepper_plugin_instance_impl.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/render_frame_impl.h View 1 2 3 4 3 chunks +7 lines, -4 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 8 chunks +48 lines, -37 lines 0 comments Download

Messages

Total messages: 56 (21 generated)
EhsanK
A few nits and suggestions mostly looks good to me except for the test location ...
3 years, 7 months ago (2017-05-04 00:16:31 UTC) #2
Peter Varga
I agree with the listed suggestions and I'm going to fix them when we agree ...
3 years, 7 months ago (2017-05-04 07:30:40 UTC) #3
nasko
https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc#newcode278 chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:278: : TextInputManagerObserverBase(web_contents), callback_called_(false) { Has this CL been run ...
3 years, 7 months ago (2017-05-04 16:10:24 UTC) #4
Peter Varga
https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc#newcode278 chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:278: : TextInputManagerObserverBase(web_contents), callback_called_(false) { On 2017/05/04 16:10:24, nasko wrote: ...
3 years, 7 months ago (2017-05-04 16:33:45 UTC) #5
Peter Varga
https://codereview.chromium.org/2856093003/diff/1/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2856093003/diff/1/content/renderer/render_frame_impl.cc#newcode6310 content/renderer/render_frame_impl.cc:6310: if (!selection.IsNull()) { On 2017/05/04 16:33:45, Peter Varga wrote: ...
3 years, 7 months ago (2017-05-05 13:09:11 UTC) #6
Peter Varga
Updated patch.
3 years, 7 months ago (2017-05-08 15:20:43 UTC) #7
EhsanK
https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc#newcode1041 chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:1041: CreateIframePage("a()"); On 2017/05/04 16:33:45, Peter Varga wrote: > On ...
3 years, 7 months ago (2017-05-08 16:56:26 UTC) #8
Peter Varga
https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc#newcode1041 chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:1041: CreateIframePage("a()"); On 2017/05/08 16:56:25, EhsanK wrote: > On 2017/05/04 ...
3 years, 7 months ago (2017-05-09 06:42:29 UTC) #9
EhsanK
Thanks. LGTM (you still need owner approval) I also had one question: If a page ...
3 years, 7 months ago (2017-05-09 15:16:16 UTC) #10
nasko
Deferring to ekaramad@ on this review. LGTM and welcome to the Chromium community!
3 years, 7 months ago (2017-05-09 22:34:53 UTC) #11
Peter Varga
On 2017/05/09 15:16:16, EhsanK wrote: > Thanks. LGTM (you still need owner approval) > > ...
3 years, 7 months ago (2017-05-10 06:35:07 UTC) #12
Peter Varga
On 2017/05/09 22:34:53, nasko wrote: > Deferring to ekaramad@ on this review. LGTM and welcome ...
3 years, 7 months ago (2017-05-10 06:36:55 UTC) #13
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/2856093003/20001
3 years, 7 months ago (2017-05-10 06:37:54 UTC) #15
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/432147)
3 years, 7 months ago (2017-05-10 06:46:55 UTC) #17
nasko
thestig@, can you do an OWNERS review of chrome/?
3 years, 7 months ago (2017-05-10 17:04:29 UTC) #20
Lei Zhang
lgtm, but there are a bunch of red bots. https://codereview.chromium.org/2856093003/diff/20001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2856093003/diff/20001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc#newcode1066 chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:1066: ...
3 years, 7 months ago (2017-05-10 21:06:03 UTC) #21
Peter Varga
On 2017/05/10 21:06:03, Lei Zhang wrote: > lgtm, but there are a bunch of red ...
3 years, 7 months ago (2017-05-10 21:12:12 UTC) #22
Peter Varga
https://codereview.chromium.org/2856093003/diff/20001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2856093003/diff/20001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc#newcode1066 chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:1066: EXPECT_TRUE(!!clipboard); On 2017/05/10 21:06:03, Lei Zhang wrote: > ASSERT_TRUE() ...
3 years, 7 months ago (2017-05-10 21:12:36 UTC) #23
Lei Zhang
On 2017/05/10 21:12:12, Peter Varga wrote: > If I'm not mistaken the bots are red ...
3 years, 7 months ago (2017-05-10 21:14:16 UTC) #24
Peter Varga
I have investigated the test failures on macOS and Windows. I've found that there are ...
3 years, 7 months ago (2017-05-16 16:16:12 UTC) #31
Lei Zhang
Looks like the mac bots are still failing.
3 years, 7 months ago (2017-05-16 18:40:48 UTC) #34
Peter Varga
I have debugged and workarounded the mac failure. See the comment in the test for ...
3 years, 7 months ago (2017-05-17 14:29:25 UTC) #37
Peter Varga
Tests seem to be passing with the mentioned changes. Is it ok to land this ...
3 years, 7 months ago (2017-05-17 15:31:15 UTC) #40
Lei Zhang
On 2017/05/17 15:31:15, Peter Varga wrote: > Tests seem to be passing with the mentioned ...
3 years, 7 months ago (2017-05-17 19:19:41 UTC) #41
Lei Zhang
https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc#newcode296 chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:296: bool last_user_initiated_; Should these bools be initialized in the ...
3 years, 7 months ago (2017-05-17 19:23:05 UTC) #42
Peter Varga
https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc#newcode296 chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:296: bool last_user_initiated_; On 2017/05/17 19:23:05, Lei Zhang wrote: > ...
3 years, 7 months ago (2017-05-18 08:27:20 UTC) #43
Lei Zhang
https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc#newcode296 chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:296: bool last_user_initiated_; On 2017/05/18 08:27:20, Peter Varga wrote: > ...
3 years, 7 months ago (2017-05-18 18:13:46 UTC) #44
Peter Varga
https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc#newcode296 chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:296: bool last_user_initiated_; > Can VerifyChange() get called before the ...
3 years, 7 months ago (2017-05-19 08:50:02 UTC) #45
Lei Zhang
Thanks for addressing my remaining concerns. ekaramad: Want to give this CL another look?
3 years, 7 months ago (2017-05-19 23:35:06 UTC) #46
EhsanK
LGTM (with a nit) for the test file. Thanks! https://codereview.chromium.org/2856093003/diff/100001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2856093003/diff/100001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc#newcode286 chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:286: ...
3 years, 7 months ago (2017-05-22 15:15:24 UTC) #47
Peter Varga
https://codereview.chromium.org/2856093003/diff/100001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2856093003/diff/100001/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc#newcode286 chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:286: if (!expected_text_.has_value()) On 2017/05/22 15:15:24, EhsanK wrote: > Is ...
3 years, 7 months ago (2017-05-23 06:59:41 UTC) #48
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/2856093003/120001
3 years, 7 months ago (2017-05-23 07:03:09 UTC) #51
commit-bot: I haz the power
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/d48794b9728276c1183b0c56f8cb7afe71b7bca4
3 years, 7 months ago (2017-05-23 08:05:38 UTC) #54
Changwan Ryu
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2901093003/ by changwan@chromium.org. ...
3 years, 7 months ago (2017-05-23 19:13:13 UTC) #55
Changwan Ryu
3 years, 7 months ago (2017-05-23 23:13:37 UTC) #56
Message was sent while issue was closed.
On 2017/05/23 19:13:13, Changwan Ryu wrote:
> A revert of this CL (patchset #7 id:120001) has been created in
> https://codereview.chromium.org/2901093003/ by mailto:changwan@chromium.org.
> 
> The reason for reverting is: We started to seeing terribly flaky ImeTest on
> Android after this commit (crbug.com/725532), so try reverting this one..

I don't see any test failure after reverting this CL, so now I think this was
indeed the culprit.
In most of the failures, the selection values were incorrect and were zeros.

I suspect that intermediate selection changes, for example changes in the middle
of a focus change or IME operation from InputMethodController were getting
propagated to the browser and were causing the flakiness.

Powered by Google App Engine
This is Rietveld 408576698