[refactor] Removing the dead method RenderWidgetHostView::GetSelectedText()

[refactor] Removing the dead method RenderWidgetHostView::GetSelectedText()

The method along with all its overrides have been removed.

BUG=578168
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

[EhsanK, 2016-07-13 17:00:54]

Description was changed from

==========
[refactor] Removing the dead method RenderWidgetHostView::GetSelectedText()

The method along with all its overrides have been removed.

BUG=NONE
==========

to

==========
[refactor] Removing the dead method RenderWidgetHostView::GetSelectedText()

The method along with all its overrides have been removed.

BUG=NONE
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[EhsanK, 2016-07-13 17:00:58]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 17:01:27]

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, 2016-07-13 17:32:49]

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

[commit-bot: I haz the power, 2016-07-13 17:32:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[EhsanK, 2016-07-13 17:44:57]

ekaramad@chromium.org changed reviewers:
+ creis@chromium.org, kenrb@chromium.org, lazyboy@chromium.org

[EhsanK, 2016-07-13 17:44:58]

PTAL.

Also adding lazyboy@ for input on the test I removed.

https://codereview.chromium.org/2149493004/diff/20001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (left):

https://codereview.chromium.org/2149493004/diff/20001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1289:
IN_PROC_BROWSER_TEST_P(WebViewInteractiveTest, TextSelection) {
This is just testing that RWHVGuest::GetSelectedText works which as we saw is
not used at all. I guess it is OK to remove this test.

[Charlie Reis, 2016-07-13 18:04:34]

I'm happy if this code is actually dead.  Looking at source history, it looks
like this was added for ActionBoxButtonController, which no longer exists:
https://codereview.chromium.org/12262050/

Strange that we have a Mac-only test for it, though.  lazyboy@, you added the
test to fix a regression in https://codereview.chromium.org/149803008.  Was
GetSelectedText used at the time, or do we still depend on it in some way? 
Ehsan, can you confirm that we're not regressing https://crbug.com/331722 with
this CL?  (Also unfortunate that we disabled the only place the test ran in
https://codereview.chromium.org/1651463003.)

Hopefully we no longer need it?

[EhsanK, 2016-07-13 18:11:21]

On 2016/07/13 18:04:34, Charlie Reis wrote:
> I'm happy if this code is actually dead.  Looking at source history, it looks
> like this was added for ActionBoxButtonController, which no longer exists:
> https://codereview.chromium.org/12262050/
> 
> Strange that we have a Mac-only test for it, though.  lazyboy@, you added the
> test to fix a regression in https://codereview.chromium.org/149803008.  Was
> GetSelectedText used at the time, or do we still depend on it in some way? 
> Ehsan, can you confirm that we're not regressing https://crbug.com/331722 with
> this CL?  (Also unfortunate that we disabled the only place the test ran in
> https://codereview.chromium.org/1651463003.)
> 
> Hopefully we no longer need it?

I think in Mac when right clicking some text (not <input> necessarily) it 
automatically selects the whole thing.

Maybe this test is verifying that behavior. 

The issue in the bug above and the CL as I understand is tracking
SelectionChanged()
for guests. A by product of that is the GetSelectedText() returns the new
selection.
But for testing the tracking, we do not need to use GetSelectedText(). I think
it just
happens to be the easier way to test it rather than add methods for testing.

With this I suspect we should modify the test rather than removing it and expose
a test
method to return something like |selection_text_| from RenderWidgetHostViewBase.

[EhsanK, 2016-07-13 18:12:54]

On 2016/07/13 18:11:21, EhsanK wrote:
> On 2016/07/13 18:04:34, Charlie Reis wrote:
> > I'm happy if this code is actually dead.  Looking at source history, it
looks
> > like this was added for ActionBoxButtonController, which no longer exists:
> > https://codereview.chromium.org/12262050/
> > 
> > Strange that we have a Mac-only test for it, though.  lazyboy@, you added
the
> > test to fix a regression in https://codereview.chromium.org/149803008.  Was
> > GetSelectedText used at the time, or do we still depend on it in some way? 
> > Ehsan, can you confirm that we're not regressing https://crbug.com/331722
with
> > this CL?  (Also unfortunate that we disabled the only place the test ran in
> > https://codereview.chromium.org/1651463003.)
> > 
> > Hopefully we no longer need it?
> 
> I think in Mac when right clicking some text (not <input> necessarily) it 
> automatically selects the whole thing.
> 
> Maybe this test is verifying that behavior. 
> 
> The issue in the bug above and the CL as I understand is tracking
> SelectionChanged()
> for guests. A by product of that is the GetSelectedText() returns the new
> selection.
> But for testing the tracking, we do not need to use GetSelectedText(). I think
> it just
> happens to be the easier way to test it rather than add methods for testing.
> 
> With this I suspect we should modify the test rather than removing it and
expose
> a test
> method to return something like |selection_text_| from
RenderWidgetHostViewBase.

Also it should not regress that issue.

[lazyboy, 2016-07-13 20:26:22]

On 2016/07/13 18:12:54, EhsanK wrote:
> On 2016/07/13 18:11:21, EhsanK wrote:
> > On 2016/07/13 18:04:34, Charlie Reis wrote:
> > > I'm happy if this code is actually dead.  Looking at source history, it
> looks
> > > like this was added for ActionBoxButtonController, which no longer exists:
> > > https://codereview.chromium.org/12262050/
> > > 
> > > Strange that we have a Mac-only test for it, though.  lazyboy@, you added
> the
> > > test to fix a regression in https://codereview.chromium.org/149803008. 
Was
> > > GetSelectedText used at the time, or do we still depend on it in some way?

> > > Ehsan, can you confirm that we're not regressing https://crbug.com/331722
> with
> > > this CL?  (Also unfortunate that we disabled the only place the test ran
in
> > > https://codereview.chromium.org/1651463003.)
> > > 
> > > Hopefully we no longer need it?
> > 
> > I think in Mac when right clicking some text (not <input> necessarily) it 
> > automatically selects the whole thing.
> > 
> > Maybe this test is verifying that behavior. 
> > 
> > The issue in the bug above and the CL as I understand is tracking
> > SelectionChanged()
> > for guests. A by product of that is the GetSelectedText() returns the new
> > selection.
> > But for testing the tracking, we do not need to use GetSelectedText(). I
think
> > it just
> > happens to be the easier way to test it rather than add methods for testing.
> > 
> > With this I suspect we should modify the test rather than removing it and
> expose
> > a test
> > method to return something like |selection_text_| from
> RenderWidgetHostViewBase.
> 
> Also it should not regress that issue.

Some replies:
 Strange that we have a Mac-only test for it, though.  lazyboy@, you added the
> test to fix a regression in https://codereview.chromium.org/149803008.  Was
> GetSelectedText used at the time, or do we still depend on it in some way? 
Yes, it must have been using GetSelectedText at that time, because it
fixed the bug.
It sounds like we stopped depending on GetSelectedText afterwards at some
point.

I think
> > it just
> > happens to be the easier way to test it rather than add methods for testing.
> > 
Yes, I think so.

> With this I suspect we should modify the test rather than removing it and
expose
> a test
> method to return something like |selection_text_| from
RenderWidgetHostViewBase.
That would be ideal/easier fix to retain the test.
I suspect there are better ways to test if selection commands
(e.g. start/stop speaking) works OK on mac. But at the time I wrote the test
I couldn't find a quick way to do so.

[EhsanK, 2016-07-13 20:53:21]

Thanks for the feedback.

Charlie: I think our options are either:
 1- removing the method from public API but keeping it locally for
RenderWidgetHostViewBase for testing (this CL)
 2- Not removing it (closing this CL) and going back to the on going text
selection CL and make everything const. It is not absolutely impossible to do
that either.

I am OK with both but I think if we really don't need the GetSelectedText(), we
should just remove it.

Istiaque: Does the new test look OK? I am cheating by directly calling the
method on the base class, but this is technically what was happening before.

[lazyboy, 2016-07-13 21:01:39]

Test look fine to me.

https://codereview.chromium.org/2149493004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2149493004/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:398: base::string16
GetSelectedTextForTesting();
const;

[EhsanK, 2016-07-13 21:04:55]

Thanks! PTAL.

https://codereview.chromium.org/2149493004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2149493004/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:398: base::string16
GetSelectedTextForTesting();
On 2016/07/13 21:01:39, lazyboy wrote:
> const;

Done. Ironically, the whole motivation behind this removal was to avoid this
constness. But it is fine now since this is for testing :).

[Charlie Reis, 2016-07-13 21:16:52]

https://codereview.chromium.org/2149493004/diff/80001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right):

https://codereview.chromium.org/2149493004/diff/80001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1314:
content::GetSelectedTextForRenderWidgetHostView(embedder_rwhv));
I don't suppose you can use GetTextFromRange (and GetSelectionRange) here rather
than adding a new test method?  I remember you mentioning that method on chat as
an alternative to GetSelectedText.

Looks like RWHV has a GetTextInputClient that would make that possible.

[EhsanK, 2016-07-13 21:31:41]

PTAL.

https://codereview.chromium.org/2149493004/diff/80001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right):

https://codereview.chromium.org/2149493004/diff/80001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1314:
content::GetSelectedTextForRenderWidgetHostView(embedder_rwhv));
On 2016/07/13 21:16:52, Charlie Reis wrote:
> I don't suppose you can use GetTextFromRange (and GetSelectionRange) here
rather
> than adding a new test method?  I remember you mentioning that method on chat
as
> an alternative to GetSelectedText.
> 
> Looks like RWHV has a GetTextInputClient that would make that possible.

You are right, but I am afraid it is not possible now. GetTextFromRange() is
NOTREACHED() except for Aura. Mac and Android do not use this. The test is for
Mac.

Eventually, after the ongoing text selection patch lands, text selection
information is stored in TextInputManager. But we have not rolled that out for
Mac yet. So for now, this seems to be the only way it works on Mac. I think
after finalizing IME this is something we can get back to and remove.

[EhsanK, 2016-07-13 21:32:09]

On 2016/07/13 21:31:41, EhsanK wrote:
> PTAL.
> 
>
https://codereview.chromium.org/2149493004/diff/80001/chrome/browser/apps/gue...
> File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
(right):
> 
>
https://codereview.chromium.org/2149493004/diff/80001/chrome/browser/apps/gue...
> chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1314:
> content::GetSelectedTextForRenderWidgetHostView(embedder_rwhv));
> On 2016/07/13 21:16:52, Charlie Reis wrote:
> > I don't suppose you can use GetTextFromRange (and GetSelectionRange) here
> rather
> > than adding a new test method?  I remember you mentioning that method on
chat
> as
> > an alternative to GetSelectedText.
> > 
> > Looks like RWHV has a GetTextInputClient that would make that possible.
> 
> You are right, but I am afraid it is not possible now. GetTextFromRange() is
> NOTREACHED() except for Aura. Mac and Android do not use this. The test is for
> Mac.
> 
> Eventually, after the ongoing text selection patch lands, text selection
> information is stored in TextInputManager. But we have not rolled that out for
> Mac yet. So for now, this seems to be the only way it works on Mac. I think
> after finalizing IME this is something we can get back to and remove.

Sorry. I meant GetTextInputClient() is NOTREACHED().

[Charlie Reis, 2016-07-14 18:28:38]

Let's list 578168 as the bug number in the CL description, just to keep these
CLs linked together.

Also, let's update the CL description to mention keeping the test-only version
for the Mac test.

https://codereview.chromium.org/2149493004/diff/80001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right):

https://codereview.chromium.org/2149493004/diff/80001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1314:
content::GetSelectedTextForRenderWidgetHostView(embedder_rwhv));
On 2016/07/13 21:31:41, EhsanK wrote:
> On 2016/07/13 21:16:52, Charlie Reis wrote:
> > I don't suppose you can use GetTextFromRange (and GetSelectionRange) here
> rather
> > than adding a new test method?  I remember you mentioning that method on
chat
> as
> > an alternative to GetSelectedText.
> > 
> > Looks like RWHV has a GetTextInputClient that would make that possible.
> 
> You are right, but I am afraid it is not possible now. GetTextFromRange() is
> NOTREACHED() except for Aura. Mac and Android do not use this. The test is for
> Mac.
> 
> Eventually, after the ongoing text selection patch lands, text selection
> information is stored in TextInputManager. But we have not rolled that out for
> Mac yet. So for now, this seems to be the only way it works on Mac. I think
> after finalizing IME this is something we can get back to and remove.

Ah, that's unfortunate.

Let's put a TODO on the declaration of GetSelectedTextForRenderWidgetHostView to
remove it in favor of the other approach once it works on all platforms.

https://codereview.chromium.org/2149493004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2149493004/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:398: base::string16
GetSelectedTextForTesting() const;
Won't this have the same problem when we get to
https://codereview.chromium.org/2130133004/?  We won't be able to implement a
const version on Aura.

Let's remove the const now if we know we can't support it.

[EhsanK, 2016-07-14 19:08:48]

Description was changed from

==========
[refactor] Removing the dead method RenderWidgetHostView::GetSelectedText()

The method along with all its overrides have been removed.

BUG=NONE
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[refactor] Removing the dead method RenderWidgetHostView::GetSelectedText()

The method along with all its overrides have been removed.

BUG=578168
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[EhsanK, 2016-07-14 19:09:17]

PTAL.

I also added the generic IME bug for tracking this change.

https://codereview.chromium.org/2149493004/diff/80001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right):

https://codereview.chromium.org/2149493004/diff/80001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1314:
content::GetSelectedTextForRenderWidgetHostView(embedder_rwhv));
On 2016/07/14 18:28:39, Charlie Reis wrote:
> On 2016/07/13 21:31:41, EhsanK wrote:
> > On 2016/07/13 21:16:52, Charlie Reis wrote:
> > > I don't suppose you can use GetTextFromRange (and GetSelectionRange) here
> > rather
> > > than adding a new test method?  I remember you mentioning that method on
> chat
> > as
> > > an alternative to GetSelectedText.
> > > 
> > > Looks like RWHV has a GetTextInputClient that would make that possible.
> > 
> > You are right, but I am afraid it is not possible now. GetTextFromRange() is
> > NOTREACHED() except for Aura. Mac and Android do not use this. The test is
for
> > Mac.
> > 
> > Eventually, after the ongoing text selection patch lands, text selection
> > information is stored in TextInputManager. But we have not rolled that out
for
> > Mac yet. So for now, this seems to be the only way it works on Mac. I think
> > after finalizing IME this is something we can get back to and remove.
> 
> Ah, that's unfortunate.
> 

Yes. For me this is were IME becomes complicated. We have this nice interface
InputMethod/TextInputClient but apparently just for aura.
> Let's put a TODO on the declaration of GetSelectedTextForRenderWidgetHostView
to
> remove it in favor of the other approach once it works on all platforms.
Done.

https://codereview.chromium.org/2149493004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2149493004/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:398: base::string16
GetSelectedTextForTesting() const;
On 2016/07/14 18:28:39, Charlie Reis wrote:
> Won't this have the same problem when we get to
> https://codereview.chromium.org/2130133004/  We won't be able to implement a
> const version on Aura.
> 
> Let's remove the const now if we know we can't support it.

Removed const. We will hopefully remove this method soon in favor of
TextInputManager API after we finish IME.

[EhsanK, 2016-07-14 19:13:04]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 19:13:47]

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, 2016-07-14 19:13:49]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[Charlie Reis, 2016-07-14 19:20:01]

Thanks, LGTM.

[EhsanK, 2016-07-14 19:39:56]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-07-14 19:39:56]

The CQ bit was checked by ekaramad@chromium.org

[commit-bot: I haz the power, 2016-07-14 19:40:12]

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, 2016-07-14 19:40:15]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[EhsanK, 2016-07-14 19:40:16]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-07-14 19:40:34]

Description was changed from

==========
[refactor] Removing the dead method RenderWidgetHostView::GetSelectedText()

The method along with all its overrides have been removed.

BUG=578168
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[refactor] Removing the dead method RenderWidgetHostView::GetSelectedText()

The method along with all its overrides have been removed.

BUG=578168
==========

[EhsanK, 2016-07-14 19:40:45]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 19:41:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2016-07-14 20:28:00]

web_view_interactive_browsertest.cc lgtm

[EhsanK, 2016-07-14 20:28:56]

On 2016/07/14 20:28:00, lazyboy wrote:
> web_view_interactive_browsertest.cc lgtm

Thank you for the reviews!

[EhsanK, 2016-07-14 20:29:01]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-07-14 20:29:02]

The CQ bit was checked by ekaramad@chromium.org

[commit-bot: I haz the power, 2016-07-14 20:29:14]

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

[EhsanK, 2016-07-14 21:05:02]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-07-14 21:27:25]

Description was changed from

==========
[refactor] Removing the dead method RenderWidgetHostView::GetSelectedText()

The method along with all its overrides have been removed.

BUG=578168
==========

to

==========
[refactor] Removing the dead method RenderWidgetHostView::GetSelectedText()

The method along with all its overrides have been removed.

BUG=578168
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[EhsanK, 2016-07-14 21:59:47]

Patchset #9 (id:160001) has been deleted

[EhsanK, 2016-07-14 22:31:22]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 22:32:38]

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, 2016-07-14 22:32:45]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[Charlie Reis, 2016-07-14 22:45:06]

https://codereview.chromium.org/2149493004/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/2149493004/diff/200001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_guest.cc:441:
RenderWidgetHostViewBase::SelectionChanged(text, offset, range);
I don't think we should add this, if we can avoid it.  Seems like too much of a
hack to keep the test happy.

Is there any way the test could get to platform_view_->GetSelectedText()
directly?  Maybe have a guest-specific public test method, instead of the
general GetSelectedTextForRenderWidgetHostView?

[EhsanK, 2016-07-14 23:21:09]

Given the complicated situation with removing the method and
the WebViewInteractiveTest.TextSelection, I am closing this
CL in favor of https://codereview.chromium.org/2152073002 and
also future implementation of OOPIF IME based on TextInputManager
which will, hopefully,enable us to remove this method altogether and satisfy the
test at the same time.