[Chromoting] Handle CR-LF correctly when transferring text items to and from the clipboard on a Win…

[Chromoting] Handle CR-LF correctly when transferring text items to and from the clipboard on a Windows host.

When reading from the clipboard, CR-LF pairs are replaced by LF.
When writing to the clipboard, each LF is replaced by CR-LF.

BUG=117473
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140206

[simonmorris, 2012-05-31 22:09:23]

ptal

[dcheng, 2012-05-31 22:21:03]

Drive by...
Maybe consider using ReplaceSubstringsAfterOffset from string_util.h?

[Wez, 2012-05-31 22:36:41]

http://codereview.chromium.org/10441131/diff/1/remoting/host/clipboard_util.cc
File remoting/host/clipboard_util.cc (right):

http://codereview.chromium.org/10441131/diff/1/remoting/host/clipboard_util.c...
remoting/host/clipboard_util.cc:9: std::string
ClipboardUtil::ReplaceLfByCrLf(const std::string& in) {
This and the reverse can be implemented with ReplaceSubStringsAfterOffset() from
base/string_util.h, I think?

[simonmorris, 2012-05-31 23:10:15]

On 2012/05/31 22:21:03, dcheng wrote:
> Drive by...
> Maybe consider using ReplaceSubstringsAfterOffset from string_util.h?

That's an O(n^2) algorithm. Using it slows the speed test from about 40ms to
about 60s. That speed test uses a 10M string, which is large, but users can
select a lot of text with CTRL-A.

[Wez, 2012-05-31 23:32:50]

On 2012/05/31 23:10:15, simonmorris wrote:
> On 2012/05/31 22:21:03, dcheng wrote:
> > Drive by...
> > Maybe consider using ReplaceSubstringsAfterOffset from string_util.h?
> 
> That's an O(n^2) algorithm. Using it slows the speed test from about 40ms to
> about 60s. That speed test uses a 10M string, which is large, but users can
> select a lot of text with CTRL-A.

Ouch!  Would be preferable to optimize the existing impl rather than add a new
one.

[simonmorris, 2012-05-31 23:42:31]

On 2012/05/31 23:32:50, Wez wrote:
> On 2012/05/31 23:10:15, simonmorris wrote:
> > On 2012/05/31 22:21:03, dcheng wrote:
> > > Drive by...
> > > Maybe consider using ReplaceSubstringsAfterOffset from string_util.h?
> > 
> > That's an O(n^2) algorithm. Using it slows the speed test from about 40ms to
> > about 60s. That speed test uses a 10M string, which is large, but users can
> > select a lot of text with CTRL-A.
> 
> Ouch!  Would be preferable to optimize the existing impl rather than add a new
> one.

This CL is not a new implementation of the ReplaceSubstringsAfterOffset
interface: it's tailored to this specific use-case.

[Wez, 2012-05-31 23:47:32]

On 2012/05/31 23:42:31, simonmorris wrote:
> On 2012/05/31 23:32:50, Wez wrote:
> > On 2012/05/31 23:10:15, simonmorris wrote:
> > > On 2012/05/31 22:21:03, dcheng wrote:
> > > > Drive by...
> > > > Maybe consider using ReplaceSubstringsAfterOffset from string_util.h?
> > > 
> > > That's an O(n^2) algorithm. Using it slows the speed test from about 40ms
to
> > > about 60s. That speed test uses a 10M string, which is large, but users
can
> > > select a lot of text with CTRL-A.
> > 
> > Ouch!  Would be preferable to optimize the existing impl rather than add a
new
> > one.
> 
> This CL is not a new implementation of the ReplaceSubstringsAfterOffset
> interface: it's tailored to this specific use-case.

What I meant was can ReplaceSubstringsAfterOffset be optimized enough for our
needs? It looks to use std::string::replace() to do in-place replacement, rather
than constructing a new string as it goes, which would explain the O(n^2)...

For this CL, I'm fine with the specialized implementations, but preferably in
remoting/base/util rather than adding a clipboard_util.*, in expectation that
we'll be able to replace them in future.

[simonmorris, 2012-06-01 16:27:37]

On 2012/05/31 23:47:32, Wez wrote:
> On 2012/05/31 23:42:31, simonmorris wrote:
> > On 2012/05/31 23:32:50, Wez wrote:
> > > On 2012/05/31 23:10:15, simonmorris wrote:
> > > > On 2012/05/31 22:21:03, dcheng wrote:
> > > > > Drive by...
> > > > > Maybe consider using ReplaceSubstringsAfterOffset from string_util.h?
> > > > 
> > > > That's an O(n^2) algorithm. Using it slows the speed test from about
40ms
> to
> > > > about 60s. That speed test uses a 10M string, which is large, but users
> can
> > > > select a lot of text with CTRL-A.
> > > 
> > > Ouch!  Would be preferable to optimize the existing impl rather than add a
> new
> > > one.
> > 
> > This CL is not a new implementation of the ReplaceSubstringsAfterOffset
> > interface: it's tailored to this specific use-case.
> 
> What I meant was can ReplaceSubstringsAfterOffset be optimized enough for our
> needs? It looks to use std::string::replace() to do in-place replacement,
rather
> than constructing a new string as it goes, which would explain the O(n^2)...
> 
> For this CL, I'm fine with the specialized implementations, but preferably in
> remoting/base/util rather than adding a clipboard_util.*, in expectation that
> we'll be able to replace them in future.

Done.

[Wez, 2012-06-01 17:59:34]

lgtm

[commit-bot: I haz the power, 2012-06-01 18:27:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10441131/4002

[commit-bot: I haz the power, 2012-06-01 18:41:17]

Try job failure for 10441131-4002 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-01 20:23:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10441131/11003

[commit-bot: I haz the power, 2012-06-01 22:38:55]

Try job failure for 10441131-11003 (previous was lost) (retry) on win_rel for
step "compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-01 22:57:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10441131/11003

[commit-bot: I haz the power, 2012-06-02 02:21:05]

Try job failure for 10441131-11003 (retry) on win_rel for step "browser_tests"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-02 18:05:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10441131/11003

[commit-bot: I haz the power, 2012-06-02 18:07:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10441131/11003

[commit-bot: I haz the power, 2012-06-02 22:17:06]

Change committed as 140206