Web Search is happening for whitespace selection

Web Search is happening for whitespace  selection.

Searching for a whitespace in the web
has no meaning because it's not going
to return any relevant result.So,I
have taken care to remove the leading
and trailing whitespaces to make the
search more optimized.

BUG=397794

[Cyan, 2014-07-26 11:18:22]

@jdduke:
PTAL

[jdduke (slow), 2014-07-28 15:21:23]

How often does this happen in practice?

https://codereview.chromium.org/417373003/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/417373003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2049:
if (TextUtils.isEmpty(query)) return;
What if we instead do:

if (TextUtils.getTrimmedLength(query) <= 0) return;

Then we don't have to reallocate a string (although that's probably not a big
deal).

[aurimas (slooooooooow), 2014-07-28 22:38:04]

On 2014/07/28 15:21:23, jdduke wrote:
> How often does this happen in practice?
> 
>
https://codereview.chromium.org/417373003/diff/20001/content/public/android/j...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/417373003/diff/20001/content/public/android/j...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2049:
> if (TextUtils.isEmpty(query)) return;
> What if we instead do:
> 
> if (TextUtils.getTrimmedLength(query) <= 0) return;
> 
> Then we don't have to reallocate a string (although that's probably not a big
> deal).

Not LGTM. This is not really a real bug, I don't think it's worth adding code to
fix this issue.

[Cyan, 2014-07-30 14:31:02]

On 2014/07/28 22:38:04, aurimas wrote:
> On 2014/07/28 15:21:23, jdduke wrote:
> > How often does this happen in practice?
> > 
> >
>
https://codereview.chromium.org/417373003/diff/20001/content/public/android/j...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/417373003/diff/20001/content/public/android/j...
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2049:
> > if (TextUtils.isEmpty(query)) return;
> > What if we instead do:
> > 
> > if (TextUtils.getTrimmedLength(query) <= 0) return;
> > 
> > Then we don't have to reallocate a string (although that's probably not a
big
> > deal).
> 
> Not LGTM. This is not really a real bug, I don't think it's worth adding code
to
> fix this issue.

@jdduke:
 if (TextUtils.getTrimmedLength(query) <= 0) return; condition,as suggested by
you
is fixing the issue as well.Please suggest me whether I can upload the modified
patch.
@aurimas:
 Although I raised the bug with only space selection but the real purpose behind
the
 fix is to send the proper string( with unnecessary whitespaces removed from
front and
 back of the srting) being queried. Please suggest your opinion regarding
this.thanks.

[jdduke (slow), 2014-07-30 15:06:53]

On 2014/07/30 14:31:02, Cyan wrote:
> @jdduke:
>  if (TextUtils.getTrimmedLength(query) <= 0) return; condition,as suggested by
> you
> is fixing the issue as well.Please suggest me whether I can upload the
modified
> patch.
> @aurimas:
>  Although I raised the bug with only space selection but the real purpose
behind
> the
>  fix is to send the proper string( with unnecessary whitespaces removed from
> front and
>  back of the srting) being queried. Please suggest your opinion regarding
> this.thanks.

This sounds like a premature optimization. A few extra characters in the search
query are not going to meaningfully impact end-to-end memory/CPU usage, and the
marginal cost of their inclusion is minimal.  Creating a new string by trimming
on every query however seems more wasteful (more garbage).

[aurimas (slooooooooow), 2014-07-30 18:31:30]

On 2014/07/30 15:06:53, jdduke wrote:
> On 2014/07/30 14:31:02, Cyan wrote:
> > @jdduke:
> >  if (TextUtils.getTrimmedLength(query) <= 0) return; condition,as suggested
by
> > you
> > is fixing the issue as well.Please suggest me whether I can upload the
> modified
> > patch.
> > @aurimas:
> >  Although I raised the bug with only space selection but the real purpose
> behind
> > the
> >  fix is to send the proper string( with unnecessary whitespaces removed from
> > front and
> >  back of the srting) being queried. Please suggest your opinion regarding
> > this.thanks.
> 
> This sounds like a premature optimization. A few extra characters in the
search
> query are not going to meaningfully impact end-to-end memory/CPU usage, and
the
> marginal cost of their inclusion is minimal.  Creating a new string by
trimming
> on every query however seems more wasteful (more garbage).

Agreed. Let's not proceed with this.

[Cyan, 2014-07-30 18:59:48]

On 2014/07/30 18:31:30, aurimas wrote:
> On 2014/07/30 15:06:53, jdduke wrote:
> > On 2014/07/30 14:31:02, Cyan wrote:
> > > @jdduke:
> > >  if (TextUtils.getTrimmedLength(query) <= 0) return; condition,as
suggested
> by
> > > you
> > > is fixing the issue as well.Please suggest me whether I can upload the
> > modified
> > > patch.
> > > @aurimas:
> > >  Although I raised the bug with only space selection but the real purpose
> > behind
> > > the
> > >  fix is to send the proper string( with unnecessary whitespaces removed
from
> > > front and
> > >  back of the srting) being queried. Please suggest your opinion regarding
> > > this.thanks.
> > 
> > This sounds like a premature optimization. A few extra characters in the
> search
> > query are not going to meaningfully impact end-to-end memory/CPU usage, and
> the
> > marginal cost of their inclusion is minimal.  Creating a new string by
> trimming
> > on every query however seems more wasteful (more garbage).
> 
> Agreed. Let's not proceed with this.

As per discussion I'll close the review. Thanks for the review comments.