linux_aura: Make X11 selection buffers work on text in both direction.

linux_aura: Make X11 selection buffers work on text in both direction.

BUG=130805
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157368

[Elliot Glaysher, 2012-09-14 18:22:56]

This mirrors what we're doing in RenderWidgetHostViewGtk, except using the
Clipboard api instead of gtk.

[Daniel Erat, 2012-09-14 18:33:15]

https://codereview.chromium.org/10917270/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/10917270/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:537: size_t pos =
range.GetMin() - offset;
is there any danger of negative overflow here?

https://codereview.chromium.org/10917270/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:538: size_t n =
range.length();
nit: you'll end up with the same number of lines but more-readable-code, IMHO,
if you remove this and just do:

  DCHECK(pos + range.length() <= text.length())
      << "The text cannot fully cover the range";

[Elliot Glaysher, 2012-09-14 20:16:16]

https://codereview.chromium.org/10917270/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/10917270/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:537: size_t pos =
range.GetMin() - offset;
On 2012/09/14 18:33:15, Daniel Erat wrote:
> is there any danger of negative overflow here?

I believe so. This check was added by penghuang@ in the gtk version, so I assume
this is IME fiddliness.

[Daniel Erat, 2012-09-14 20:38:16]

lgtm

https://codereview.chromium.org/10917270/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/10917270/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:537: size_t pos =
range.GetMin() - offset;
On 2012/09/14 20:16:16, Elliot Glaysher wrote:
> On 2012/09/14 18:33:15, Daniel Erat wrote:
> > is there any danger of negative overflow here?
> 
> I believe so. This check was added by penghuang@ in the gtk version, so I
assume
> this is IME fiddliness.

sorry -- i meant, should there also be:

  if (offset > range.GetMin()) {
    NOTREACHED() << "Offset " << offset << " makes range starting at "
                 << range.GetMin() << " precede beginning of text";
    return;
  }

?  pos will be huge in this case, which means we should hit the next
NOTREACHED(), so maybe it's not a big deal.

[Elliot Glaysher, 2012-09-17 18:26:18]

Looking at this some more, I don't think I should be doing the check at all. So
in the GTK version, these checks are done because they muck with substrings on
the text buffer.

However, checking locally, the incoming text buffer is correct and doesn't need
munging. Doing range modification shouldn't matter.

[jam, 2012-09-17 19:51:56]

rubberstamp lgtm

[commit-bot: I haz the power, 2012-09-17 20:05:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/10917270/1007

[commit-bot: I haz the power, 2012-09-17 20:05:55]

Presubmit check for 10917270-1007 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ui/base/clipboard

[Elliot Glaysher, 2012-09-17 20:11:20]

dcheng: clipboard stamp

[dcheng, 2012-09-17 20:41:43]

On 2012/09/17 20:11:20, Elliot Glaysher wrote:
> dcheng: clipboard stamp

lgtm

[commit-bot: I haz the power, 2012-09-17 20:42:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/10917270/1007

[commit-bot: I haz the power, 2012-09-18 01:04:17]

Retried try job too often for step(s) browser_tests