Update TextSelection for non-user initiated events

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

[Peter Varga, 2017-05-03 13:56:24]

Description was changed from

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

to

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

[EhsanK, 2017-05-04 00:16:31]

A few nits and suggestions mostly looks good to me except for the test location
which I guess does not belong to current test file. We tend to add
site-per-process specific tests here.

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:1041:
CreateIframePage("a()");
I think the test belongs to somewhere else but I leave that to creis@ to decide
(the scenario you are testing does not include any OOPIFs).

https://codereview.chromium.org/2856093003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/2856093003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_base.cc:125:
GetTextInputManager()->SelectionChanged(this, text, offset, range,
(nit) brackets given that the statement is longer than a line.

https://codereview.chromium.org/2856093003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/text_input_manager.cc (right):

https://codereview.chromium.org/2856093003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/text_input_manager.cc:317: : offset_(0),
range_(gfx::Range::InvalidRange()) {}
(nit) Initialize |user_initiated| (to false?).

https://codereview.chromium.org/2856093003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/text_input_manager.h (right):

https://codereview.chromium.org/2856093003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/text_input_manager.h:135: // True if text
selection is triggered by user input
(nit) dot at the end?

[Peter Varga, 2017-05-04 07:30:40]

I agree with the listed suggestions and I'm going to fix them when we agree in
the test location.

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:1041:
CreateIframePage("a()");
On 2017/05/04 00:16:31, EhsanK wrote:
> I think the test belongs to somewhere else but I leave that to creis@ to
decide
> (the scenario you are testing does not include any OOPIFs). 

I agree with that this file is probably not the best place for this test.
However, I couldn't find any better. This seems to be the only one which holds
"complex" tests based on TextInputManager.

I really need help or advice where to place this test.

[nasko, 2017-05-04 16:10:24]

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:278: :
TextInputManagerObserverBase(web_contents), callback_called_(false) {
Has this CL been run through "git cl format"?

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:1041:
CreateIframePage("a()");
On 2017/05/04 07:30:39, Peter Varga wrote:
> On 2017/05/04 00:16:31, EhsanK wrote:
> > I think the test belongs to somewhere else but I leave that to creis@ to
> decide
> > (the scenario you are testing does not include any OOPIFs). 
> 
> I agree with that this file is probably not the best place for this test.
> However, I couldn't find any better. This seems to be the only one which holds
> "complex" tests based on TextInputManager.
> 
> I really need help or advice where to place this test.

Why not extend the test to cover actual OOPIFs? That way not only we verify the
change itself, but we verify it works across OOPIFs too.

https://codereview.chromium.org/2856093003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2856093003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:1528: user_initiated);
Add {} to the if statement, as its body now spans more than one line.

https://codereview.chromium.org/2856093003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/text_input_manager.cc (right):

https://codereview.chromium.org/2856093003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/text_input_manager.cc:317: : offset_(0),
range_(gfx::Range::InvalidRange()) {}
On 2017/05/04 00:16:31, EhsanK wrote:
> (nit) Initialize |user_initiated| (to false?).

With C++11, we can now initialize it in the header file too.

https://codereview.chromium.org/2856093003/diff/1/content/public/renderer/ren...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/2856093003/diff/1/content/public/renderer/ren...
content/public/renderer/render_frame.h:231: bool user_initiated) = 0;
It looks like this API is never called outside of content/ code. Let's remove it
from the public API and leave it internal.

https://codereview.chromium.org/2856093003/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2856093003/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1385:
render_frame_->SetSelectedText(selected_text, 0, range, true);
Is it not possible for script running in a plugin to create a selection without
user input?

https://codereview.chromium.org/2856093003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2856093003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1384: SyncSelectionIfRequired(true);
Can we put some small comments in cases where we pass in a hardcoded value as to
why that's correct?

https://codereview.chromium.org/2856093003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:6310: if (!selection.IsNull()) {
This looks like a change in behavior as the block of code following the current
scope will now be executed if selection is null. Is this intentional? The CL
description says that it only exposes the user_initiated boolean.

[Peter Varga, 2017-05-04 16:33:45]

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
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:
> Has this CL been run through "git cl format"?

Yes it has been. Actually the script put this two initialization into the same
line :)

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:1041:
CreateIframePage("a()");
On 2017/05/04 16:10:24, nasko wrote:
> On 2017/05/04 07:30:39, Peter Varga wrote:
> > On 2017/05/04 00:16:31, EhsanK wrote:
> > > I think the test belongs to somewhere else but I leave that to creis@ to
> > decide
> > > (the scenario you are testing does not include any OOPIFs). 
> > 
> > I agree with that this file is probably not the best place for this test.
> > However, I couldn't find any better. This seems to be the only one which
holds
> > "complex" tests based on TextInputManager.
> > 
> > I really need help or advice where to place this test.
> 
> Why not extend the test to cover actual OOPIFs? That way not only we verify
the
> change itself, but we verify it works across OOPIFs too.

Because I'm not sure how to test OOPIF. Any hint/suggestion/doc how to test or
what to test?

https://codereview.chromium.org/2856093003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2856093003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:1528: user_initiated);
On 2017/05/04 16:10:24, nasko wrote:
> Add {} to the if statement, as its body now spans more than one line.

Acknowledged.

https://codereview.chromium.org/2856093003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/text_input_manager.cc (right):

https://codereview.chromium.org/2856093003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/text_input_manager.cc:317: : offset_(0),
range_(gfx::Range::InvalidRange()) {}
On 2017/05/04 16:10:24, nasko wrote:
> On 2017/05/04 00:16:31, EhsanK wrote:
> > (nit) Initialize |user_initiated| (to false?).
> 
> With C++11, we can now initialize it in the header file too.

ok, I will check this.

https://codereview.chromium.org/2856093003/diff/1/content/public/renderer/ren...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/2856093003/diff/1/content/public/renderer/ren...
content/public/renderer/render_frame.h:231: bool user_initiated) = 0;
On 2017/05/04 16:10:24, nasko wrote:
> It looks like this API is never called outside of content/ code. Let's remove
it
> from the public API and leave it internal.

Shouldn't this be done in another patch? This change doesn't really related to
the original issue.

https://codereview.chromium.org/2856093003/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2856093003/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1385:
render_frame_->SetSelectedText(selected_text, 0, range, true);
On 2017/05/04 16:10:24, nasko wrote:
> Is it not possible for script running in a plugin to create a selection
without
> user input?

I suppose it is possible but I wanted to keep the original behavior since I'm
not really familiar with the pepper plugin stuff and I didn't want to brake
anything.

https://codereview.chromium.org/2856093003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2856093003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1384: SyncSelectionIfRequired(true);
On 2017/05/04 16:10:24, nasko wrote:
> Can we put some small comments in cases where we pass in a hardcoded value as
to
> why that's correct?

I can't say this is correct. The only reason that I have for these hard coded
values is that this was the original behavior.

https://codereview.chromium.org/2856093003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:6310: if (!selection.IsNull()) {
On 2017/05/04 16:10:24, nasko wrote:
> This looks like a change in behavior as the block of code following the
current
> scope will now be executed if selection is null. Is this intentional? The CL
> description says that it only exposes the user_initiated boolean.

Yes it is intentional. I haven't found any test related to this. I've found that
clearing the selection from JavaScript doesn't update the selection text. In the
new test I tried to cover this for both the user and non-user initiated
selection clear. I haven't noticed duplicate SelectionChanged call so I don't
know why the SetSelectedText was guarded.

[Peter Varga, 2017-05-05 13:09:11]

https://codereview.chromium.org/2856093003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2856093003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:6310: if (!selection.IsNull()) {
On 2017/05/04 16:33:45, Peter Varga wrote:
> On 2017/05/04 16:10:24, nasko wrote:
> > This looks like a change in behavior as the block of code following the
> current
> > scope will now be executed if selection is null. Is this intentional? The CL
> > description says that it only exposes the user_initiated boolean.
> 
> Yes it is intentional. I haven't found any test related to this. I've found
that
> clearing the selection from JavaScript doesn't update the selection text. In
the
> new test I tried to cover this for both the user and non-user initiated
> selection clear. I haven't noticed duplicate SelectionChanged call so I don't
> know why the SetSelectedText was guarded.

While working on the new test I've found a case when this produce an unnecessary
SetSelectedText call. It seems I have to keep the early return.

[Peter Varga, 2017-05-08 15:20:43]

Updated patch.

[EhsanK, 2017-05-08 16:56:26]

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
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 2017/05/04 16:10:24, nasko wrote:
> > On 2017/05/04 07:30:39, Peter Varga wrote:
> > > On 2017/05/04 00:16:31, EhsanK wrote:
> > > > I think the test belongs to somewhere else but I leave that to creis@ to
> > > decide
> > > > (the scenario you are testing does not include any OOPIFs). 
> > > 
> > > I agree with that this file is probably not the best place for this test.
> > > However, I couldn't find any better. This seems to be the only one which
> holds
> > > "complex" tests based on TextInputManager.
> > > 
> > > I really need help or advice where to place this test.
> > 
> > Why not extend the test to cover actual OOPIFs? That way not only we verify
> the
> > change itself, but we verify it works across OOPIFs too.
> 
> Because I'm not sure how to test OOPIF. Any hint/suggestion/doc how to test or
> what to test?
Some suggestions:
You could start by reusing some other examples. For starters, set the string to
something like "a(b())" which corresponds to a page in domain A and a frame in
domain B, i.e,:
 -A----------
|            |
| --B------- |
||          ||
||          ||
| ---------- |
 ------------
 There will be two RFHs for this example. On is the main frame and the other for
the <iframe>. In this file, I use GetFrame() to get a specific frame, e.g.
GetFrame(IndexVector{}) returns the main frame and GetFrame(IndexVector{0})
returns the first child of it which is the <iframe> in domain B. Feel free to
reuse something in other SPP tests such as
site_per_process_interactive_browsertest.cc and
chrome_site_per_process_browsertest.cc.

After you get your frames then the test (I believe) can be repeated for both.
Just remember to focus the right frame by passing input or "window.focus()" or
w/e way you find fit.

[Peter Varga, 2017-05-09 06:42:29]

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
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 16:33:45, Peter Varga wrote:
> > On 2017/05/04 16:10:24, nasko wrote:
> > > On 2017/05/04 07:30:39, Peter Varga wrote:
> > > > On 2017/05/04 00:16:31, EhsanK wrote:
> > > > > I think the test belongs to somewhere else but I leave that to creis@
to
> > > > decide
> > > > > (the scenario you are testing does not include any OOPIFs). 
> > > > 
> > > > I agree with that this file is probably not the best place for this
test.
> > > > However, I couldn't find any better. This seems to be the only one which
> > holds
> > > > "complex" tests based on TextInputManager.
> > > > 
> > > > I really need help or advice where to place this test.
> > > 
> > > Why not extend the test to cover actual OOPIFs? That way not only we
verify
> > the
> > > change itself, but we verify it works across OOPIFs too.
> > 
> > Because I'm not sure how to test OOPIF. Any hint/suggestion/doc how to test
or
> > what to test?
> Some suggestions:
> You could start by reusing some other examples. For starters, set the string
to
> something like "a(b())" which corresponds to a page in domain A and a frame in
> domain B, i.e,:
>  -A----------
> |            |
> | --B------- |
> ||          ||
> ||          ||
> | ---------- |
>  ------------
>  There will be two RFHs for this example. On is the main frame and the other
for
> the <iframe>. In this file, I use GetFrame() to get a specific frame, e.g.
> GetFrame(IndexVector{}) returns the main frame and GetFrame(IndexVector{0})
> returns the first child of it which is the <iframe> in domain B. Feel free to
> reuse something in other SPP tests such as
> site_per_process_interactive_browsertest.cc and
> chrome_site_per_process_browsertest.cc.
> 
> After you get your frames then the test (I believe) can be repeated for both.
> Just remember to focus the right frame by passing input or "window.focus()" or
> w/e way you find fit.

Thank you for the suggestions. I did something similar in patch set 2.

[EhsanK, 2017-05-09 15:16:16]

Thanks. LGTM (you still need owner approval)

I also had one question: If a page has an event handler for input and the
handler changes text selection, does that cause a user initiated updated (and is
it correct if it does)?

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2856093003/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:1041:
CreateIframePage("a()");
On 2017/05/09 06:42:29, Peter Varga wrote:
> On 2017/05/08 16:56:25, EhsanK wrote:
> > On 2017/05/04 16:33:45, Peter Varga wrote:
> > > On 2017/05/04 16:10:24, nasko wrote:
> > > > On 2017/05/04 07:30:39, Peter Varga wrote:
> > > > > On 2017/05/04 00:16:31, EhsanK wrote:
> > > > > > I think the test belongs to somewhere else but I leave that to
creis@
> to
> > > > > decide
> > > > > > (the scenario you are testing does not include any OOPIFs). 
> > > > > 
> > > > > I agree with that this file is probably not the best place for this
> test.
> > > > > However, I couldn't find any better. This seems to be the only one
which
> > > holds
> > > > > "complex" tests based on TextInputManager.
> > > > > 
> > > > > I really need help or advice where to place this test.
> > > > 
> > > > Why not extend the test to cover actual OOPIFs? That way not only we
> verify
> > > the
> > > > change itself, but we verify it works across OOPIFs too.
> > > 
> > > Because I'm not sure how to test OOPIF. Any hint/suggestion/doc how to
test
> or
> > > what to test?
> > Some suggestions:
> > You could start by reusing some other examples. For starters, set the string
> to
> > something like "a(b())" which corresponds to a page in domain A and a frame
in
> > domain B, i.e,:
> >  -A----------
> > |            |
> > | --B------- |
> > ||          ||
> > ||          ||
> > | ---------- |
> >  ------------
> >  There will be two RFHs for this example. On is the main frame and the other
> for
> > the <iframe>. In this file, I use GetFrame() to get a specific frame, e.g.
> > GetFrame(IndexVector{}) returns the main frame and GetFrame(IndexVector{0})
> > returns the first child of it which is the <iframe> in domain B. Feel free
to
> > reuse something in other SPP tests such as
> > site_per_process_interactive_browsertest.cc and
> > chrome_site_per_process_browsertest.cc.
> > 
> > After you get your frames then the test (I believe) can be repeated for
both.
> > Just remember to focus the right frame by passing input or "window.focus()"
or
> > w/e way you find fit.
> 
> Thank you for the suggestions. I did something similar in patch set 2.

Acknowledged.

[nasko, 2017-05-09 22:34:53]

Deferring to ekaramad@ on this review. LGTM and welcome to the Chromium
community!

[Peter Varga, 2017-05-10 06:35:07]

On 2017/05/09 15:16:16, EhsanK wrote:
> Thanks. LGTM (you still need owner approval)
> 
> I also had one question: If a page has an event handler for input and the
> handler changes text selection, does that cause a user initiated updated (and
is
> it correct if it does)?

Yes, this case will be user initiated. For example, when clicking on a button
and its onClick() handler triggers the JavaScript select(), it is user initiated
(see https://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_text_select ).
I think this is the correct behavior since user_initiated is intended to
separate selection events triggered by user interaction. Non-user initiated
selections are triggered by events that don't need user interaction, for example
the <body> element's onload() event handler.

[Peter Varga, 2017-05-10 06:36:55]

On 2017/05/09 22:34:53, nasko wrote:
> Deferring to ekaramad@ on this review. LGTM and welcome to the Chromium
> community!

Thank you for the review and all the help to make this work! :)

[Peter Varga, 2017-05-10 06:37:21]

The CQ bit was checked by pvarga@inf.u-szeged.hu

[commit-bot: I haz the power, 2017-05-10 06:37:54]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 06:46:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 06:46:55]

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

[Peter Varga, 2017-05-10 06:52:21]

The CQ bit was unchecked by pvarga@inf.u-szeged.hu

[nasko, 2017-05-10 17:04:28]

nasko@chromium.org changed reviewers:
+ thestig@chromium.org

[nasko, 2017-05-10 17:04:29]

thestig@, can you do an OWNERS review of chrome/?

[Lei Zhang, 2017-05-10 21:06:03]

lgtm, but there are a bunch of red bots.

https://codereview.chromium.org/2856093003/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2856093003/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:1066:
EXPECT_TRUE(!!clipboard);
ASSERT_TRUE() because if this fails, the test executable crashes, or if you
don't mind that, just omit it?

[Peter Varga, 2017-05-10 21:12:12]

On 2017/05/10 21:06:03, Lei Zhang wrote:
> lgtm, but there are a bunch of red bots.
> 
>
https://codereview.chromium.org/2856093003/diff/20001/chrome/browser/renderer...
> File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/2856093003/diff/20001/chrome/browser/renderer...
> chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:1066:
> EXPECT_TRUE(!!clipboard);
> ASSERT_TRUE() because if this fails, the test executable crashes, or if you
> don't mind that, just omit it?

If I'm not mistaken the bots are red because I put it to CQ with a missing owner
review.

[Peter Varga, 2017-05-10 21:12:36]

https://codereview.chromium.org/2856093003/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2856093003/diff/20001/chrome/browser/renderer...
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() because if this fails, the test executable crashes, or if you
> don't mind that, just omit it?

I copied this from another clipboard test. I will change this.

[Lei Zhang, 2017-05-10 21:14:16]

On 2017/05/10 21:12:12, Peter Varga wrote:
> If I'm not mistaken the bots are red because I put it to CQ with a missing
owner
> review.

That should only turn the chromium_presubmit bot red. Many other bots are red,
e.g. win_chromium_compile_dbg_ng in the compile step:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_com...

[Peter Varga, 2017-05-11 06:36:32]

The CQ bit was checked by pvarga@inf.u-szeged.hu to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 06:37:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 07:36:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 07:36:23]

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

[Peter Varga, 2017-05-16 16:13:00]

The CQ bit was checked by pvarga@inf.u-szeged.hu to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 16:13:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Varga, 2017-05-16 16:16:12]

I have investigated the test failures on macOS and Windows. I've found that
there are issues with the "fake" mouse events on platforms other than Linux. I
tried to modify the test to use keyboard events for user-initiated text
selection. Moreover, I split the test into 2 (user and non-user initiated) to
make it clear whether the failure triggered by "fake" input event or JavaScript.

[commit-bot: I haz the power, 2017-05-16 17:46:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 17:46:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Lei Zhang, 2017-05-16 18:40:48]

Looks like the mac bots are still failing.

[Peter Varga, 2017-05-17 14:27:42]

The CQ bit was checked by pvarga@inf.u-szeged.hu to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 14:28:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Varga, 2017-05-17 14:29:25]

I have debugged and workarounded the mac failure. See the comment in the test
for details. The other trybot failures doesn't seem to me reletad. Can someone
confirm this?

[commit-bot: I haz the power, 2017-05-17 15:26:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 15:26:54]

Dry run: This issue passed the CQ dry run.

[Peter Varga, 2017-05-17 15:31:15]

Tests seem to be passing with the mentioned changes. Is it ok to land this way?

[Lei Zhang, 2017-05-17 19:19:41]

On 2017/05/17 15:31:15, Peter Varga wrote:
> Tests seem to be passing with the mentioned changes. Is it ok to land this
way?

Perhaps Ehsan should take another look.

[Lei Zhang, 2017-05-17 19:23:05]

https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:296:
bool last_user_initiated_;
Should these bools be initialized in the ctor?

https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:998:
ASSERT_TRUE(!!clipboard);
I think ASSERT_TRUE() works with the "!!"

[Peter Varga, 2017-05-18 08:27:20]

https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer...
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:
> Should these bools be initialized in the ctor?

They are initialized to false this way and I don't see any reason to initialize
them with a particular value in the constructor.

|expected_user_initiated_| is initialized in the WaitForSelectedText function
and I don't see any other use case of this property.

|last_user_initiated_| is initialized when the TextSelectionChangedCallback is
called. If the callback is not called this property is not used thus it doesn't
matter whether it is true or false. This is guaranteed by the |callback_called_|
property what is initialized in the constructor.

https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:998:
ASSERT_TRUE(!!clipboard);
On 2017/05/17 19:23:05, Lei Zhang wrote:
> I think ASSERT_TRUE() works with the "!!"

I don't really understand what is wrong with this line, but yes it works with
the "!!" :)

If you mean it works without the "!!" then I don't see any reason to remove it.
I copied this from another test and based on the doc in gtest.h, ASSERT_TRUE
expects boolean expression.

[Lei Zhang, 2017-05-18 18:13:46]

https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer...
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:
> On 2017/05/17 19:23:05, Lei Zhang wrote:
> > Should these bools be initialized in the ctor?
> 
> They are initialized to false this way and I don't see any reason to
initialize
> them with a particular value in the constructor.
> 
> |expected_user_initiated_| is initialized in the WaitForSelectedText function
> and I don't see any other use case of this property.

Can VerifyChange() get called before the first WaitForSelectedText() call? If
yes, and |last_selected_text_| ends up as an empty string, then the code will
try to read an uninitialized |expected_user_initiated_|. Can this scenario
occur?

> |last_user_initiated_| is initialized when the TextSelectionChangedCallback is
> called. If the callback is not called this property is not used thus it
doesn't
> matter whether it is true or false. This is guaranteed by the
|callback_called_|
> property what is initialized in the constructor.

Maybe combine |callback_called_| and |last_user_initiated_| a
base::Optional<bool> since it's really a tri-state? [not called, called/false,
called/true]

https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:998:
ASSERT_TRUE(!!clipboard);
On 2017/05/18 08:27:20, Peter Varga wrote:
> On 2017/05/17 19:23:05, Lei Zhang wrote:
> > I think ASSERT_TRUE() works with the "!!"
> 
> I don't really understand what is wrong with this line, but yes it works with
> the "!!" :)
> 
> If you mean it works without the "!!" then I don't see any reason to remove
it.
> I copied this from another test and based on the doc in gtest.h, ASSERT_TRUE
> expects boolean expression.

I just wanted to point out that you can omit the "!!" and accomplish the same
thing. Just because some other piece of code does something does not always mean
it's always the best idea. Let's take another situation where a statement
expects a boolean expression:

void* ptr = MayReturnNullptrFunc();
if (...) {
  DoSomething();
}

Do you write if (ptr), or if (!!ptr) ?

[Peter Varga, 2017-05-19 08:50:02]

https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:296:
bool last_user_initiated_;
> Can VerifyChange() get called before the first WaitForSelectedText() call? If
> yes, and |last_selected_text_| ends up as an empty string, then the code will
> try to read an uninitialized |expected_user_initiated_|. Can this scenario
> occur?
hmm, yes it can.
 
> Maybe combine |callback_called_| and |last_user_initiated_| a
> base::Optional<bool> since it's really a tri-state? [not called, called/false,
> called/true]

I wasn't familiar with this base::Optional solution but sounds better than use
the |callback_called_|. However, I would rather make the |last_selected_text_|
property optional due to simpler value assignment in VerifyChange(). Thank you
for pointing this out.

https://codereview.chromium.org/2856093003/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:998:
ASSERT_TRUE(!!clipboard);
On 2017/05/18 18:13:46, Lei Zhang wrote:
> On 2017/05/18 08:27:20, Peter Varga wrote:
> > On 2017/05/17 19:23:05, Lei Zhang wrote:
> > > I think ASSERT_TRUE() works with the "!!"
> > 
> > I don't really understand what is wrong with this line, but yes it works
with
> > the "!!" :)
> > 
> > If you mean it works without the "!!" then I don't see any reason to remove
> it.
> > I copied this from another test and based on the doc in gtest.h, ASSERT_TRUE
> > expects boolean expression.
> 
> I just wanted to point out that you can omit the "!!" and accomplish the same
> thing. Just because some other piece of code does something does not always
mean
> it's always the best idea. Let's take another situation where a statement
> expects a boolean expression:
> 
> void* ptr = MayReturnNullptrFunc();
> if (...) {
>   DoSomething();
> }
> 
> Do you write if (ptr), or if (!!ptr) ?

I would rather write if (ptr) :) Since I haven't found any suggestion about
coding conventions in this case I tried to work from other tests, hoping that
the same solution doesn't break any style/code convention. Removing "!!" doesn't
seem to me risky either, so let it be removed.

[Lei Zhang, 2017-05-19 23:35:06]

Thanks for addressing my remaining concerns.

ekaramad: Want to give this CL another look?

[EhsanK, 2017-05-22 15:15:24]

LGTM (with a nit) for the test file. Thanks!

https://codereview.chromium.org/2856093003/diff/100001/chrome/browser/rendere...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2856093003/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:286: if
(!expected_text_.has_value())
Is this needed given line 267? Don't we always have |expected_text_.has_value()|
== true?

https://codereview.chromium.org/2856093003/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:990: //
about text selection change event, the corresponding |user_initiated|
nit: Delete 'about' ("about a about" => "about a").

[Peter Varga, 2017-05-23 06:59:41]

https://codereview.chromium.org/2856093003/diff/100001/chrome/browser/rendere...
File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
(right):

https://codereview.chromium.org/2856093003/diff/100001/chrome/browser/rendere...
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 this needed given line 267? Don't we always have
|expected_text_.has_value()|
> == true?

The VerifyChange might be called before the WaitForSelectedText call. This was
discussed in a previous patch set.

https://codereview.chromium.org/2856093003/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:990: //
about text selection change event, the corresponding |user_initiated|
On 2017/05/22 15:15:23, EhsanK wrote:
> nit: Delete 'about' ("about a about" => "about a").

Acknowledged.

[Peter Varga, 2017-05-23 07:02:58]

The CQ bit was checked by pvarga@inf.u-szeged.hu

[Peter Varga, 2017-05-23 07:02:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, nasko@chromium.org, ekaramad@chromium.org
Link to the patchset: https://codereview.chromium.org/2856093003/#ps120001
(title: "Update TextSelection for non-user initiated events")

[commit-bot: I haz the power, 2017-05-23 07:03:09]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 08:05:23]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495522978046280,
"parent_rev": "f6fd56037a18d025817064d21222bc078b081428", "commit_rev":
"d48794b9728276c1183b0c56f8cb7afe71b7bca4"}

[commit-bot: I haz the power, 2017-05-23 08:05:36]

Description was changed from

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

to

==========
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/+/d48794b9728276c1183b0c56f8cb...
==========

[commit-bot: I haz the power, 2017-05-23 08:05:38]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/d48794b9728276c1183b0c56f8cb...

[Changwan Ryu, 2017-05-23 19:13:13]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2901093003/ by 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..

[Changwan Ryu, 2017-05-23 23:13:37]

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.