[Desktop Linux]: Fix copy and pasting long selections

[Desktop Linux]: Fix copy and pasting long selections

When a client requests the clipboard selection and Chrome has a long selection
Chrome sends the data incrementally. Chrome transfers the clipboard data to the
client via a property on the client X Window. The client deletes the property to
indicate to Chrome that Chrome can send the next bit of data. Chrome listens for
the deletion XPropertyEvent from the client.

This CL fixes a bug where Chrome would stop listening for XPropertyEvents on the
client X Window after finishing servicing the client's first request instead of
after servicing all of the client's requests.

BUG=686499
Review-Url: https://codereview.chromium.org/2700043002
Cr-Commit-Position: refs/heads/master@{#451932}
Committed: https://chromium.googlesource.com/chromium/src/+/c8d772b0617ab7f2d15fc2e0bf02ada35dd21cfc

[pkotwicz, 2017-02-17 02:00:28]

pkotwicz@chromium.org changed reviewers:
+ dcheng@chromium.org

[pkotwicz, 2017-02-17 02:00:29]

dcheng@ can you please take a look?

[pkotwicz, 2017-02-17 02:00:59]

Description was changed from

==========
[Desktop Linux]: Fix copy and pasting long selections

When a client requests the clipboard selection and Chrome has a long selection
Chrome sends the data incrementally. Chrome transfers the clipboard data to the
client via a property on the client X Window. The client deletes the property to
indicate to Chrome that Chrome can send the next bit of data. Chrome listens for
the deletion XPropertyEvent from the client.

This CL fixes a bug where Chrome would stop listening fir XPropertyEvents on the
client X Window after finishing servicing the client's first request instead of
after servicing all of the client's requests.

BUG=686499
==========

to

==========
[Desktop Linux]: Fix copy and pasting long selections

When a client requests the clipboard selection and Chrome has a long selection
Chrome sends the data incrementally. Chrome transfers the clipboard data to the
client via a property on the client X Window. The client deletes the property to
indicate to Chrome that Chrome can send the next bit of data. Chrome listens for
the deletion XPropertyEvent from the client.

This CL fixes a bug where Chrome would stop listening for XPropertyEvents on the
client X Window after finishing servicing the client's first request instead of
after servicing all of the client's requests.

BUG=686499
==========

[dcheng, 2017-02-17 07:42:13]

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.cc
File ui/base/x/selection_owner.cc (left):

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.c...
ui/base/x/selection_owner.cc:341: requestor_events_.reset();
Is it possible to just only reset this when transfers is empty()?

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.cc
File ui/base/x/selection_owner.cc (right):

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.c...
ui/base/x/selection_owner.cc:209: IncrementalTransfer* incremental_transfer =
(*it).get();
Does it->get() work?

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.c...
ui/base/x/selection_owner.cc:275: base::WrapUnique(new XScopedEventSelector(
Nit: base::MakeUnique

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.c...
ui/base/x/selection_owner.cc:353: IncrementalTransfer* incremental_transfer =
(*it).get();
Nit: Does it->get() work?

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.h
File ui/base/x/selection_owner.h (right):

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.h...
ui/base/x/selection_owner.h:97: typedef typename
std::vector<std::unique_ptr<IncrementalTransfer>>::iterator
Nit: prefer using A = B; rather than typename B A;

Also, is "typename" required here? I think it can probably be omitted, since we
don't have any types dependent on unknown template params here.

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.h...
ui/base/x/selection_owner.h:134:
std::vector<std::unique_ptr<IncrementalTransfer>> incremental_transfers_;
Another possibility (to make the typename shorter) is to make
IncrementalTransfer move-only and just store it directly. I don't have a strong
preference... this is a bit of extra heap traffic, but moving a unique_ptr will
be cheaper than moving IncrementalTransfer.

(I don't really have a preference here, so whatever you think is best)

[pkotwicz, 2017-02-17 19:19:07]

dcheng@ can you please take another look?

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.cc
File ui/base/x/selection_owner.cc (left):

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.c...
ui/base/x/selection_owner.cc:341: requestor_events_.reset();
Unfortunately, there may be multiple requesting apps doing incremental transfers
from the clipboard.

XScopedEventSelector() start listening for PropertyNotify events on the
requesting app's window.

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.cc
File ui/base/x/selection_owner.cc (right):

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.c...
ui/base/x/selection_owner.cc:209: IncrementalTransfer* incremental_transfer =
(*it).get();
Yes it does :) Thank you!

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.c...
ui/base/x/selection_owner.cc:275: base::WrapUnique(new XScopedEventSelector(
On 2017/02/17 07:42:13, dcheng wrote:
> Nit: base::MakeUnique

Done.

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.c...
ui/base/x/selection_owner.cc:353: IncrementalTransfer* incremental_transfer =
(*it).get();
On 2017/02/17 07:42:13, dcheng wrote:
> Nit: Does it->get() work?

Done.

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.h
File ui/base/x/selection_owner.h (right):

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.h...
ui/base/x/selection_owner.h:97: typedef typename
std::vector<std::unique_ptr<IncrementalTransfer>>::iterator
On 2017/02/17 07:42:13, dcheng wrote:
> Nit: prefer using A = B; rather than typename B A;
> 
> Also, is "typename" required here? I think it can probably be omitted, since
we
> don't have any types dependent on unknown template params here.

Done.

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.h...
ui/base/x/selection_owner.h:134:
std::vector<std::unique_ptr<IncrementalTransfer>> incremental_transfers_;
I'd rather not implement a custom move assignment operator. I think that it is
easy to add a member to the struct and to forget to update the move assignment
operator.

[dcheng, 2017-02-17 19:21:16]

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.h
File ui/base/x/selection_owner.h (right):

https://codereview.chromium.org/2700043002/diff/1/ui/base/x/selection_owner.h...
ui/base/x/selection_owner.h:134:
std::vector<std::unique_ptr<IncrementalTransfer>> incremental_transfers_;
On 2017/02/17 19:19:07, pkotwicz wrote:
> I'd rather not implement a custom move assignment operator. I think that it is
> easy to add a member to the struct and to forget to update the move assignment
> operator.

That's what = default is for though =)

IncrementalTransfer::IncrementalTransfer(IncrementalTransfer&&) = default;

[pkotwicz, 2017-02-17 21:44:44]

Patchset #3 (id:40001) has been deleted

[pkotwicz, 2017-02-17 21:52:22]

dcheng@ can you please take another look? Switched to using =default

[dcheng, 2017-02-17 23:55:24]

lgtm

[pkotwicz, 2017-02-21 02:00:12]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2017-02-21 02:00:26]

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-02-21 02:06:33]

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

[commit-bot: I haz the power, 2017-02-21 02:06:34]

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

[pkotwicz, 2017-02-21 17:57:47]

pkotwicz@chromium.org changed reviewers:
+ derat@chromium.org

[pkotwicz, 2017-02-21 17:57:47]

derat@ for OWNERS

[Daniel Erat, 2017-02-21 18:09:28]

lgtm

https://codereview.chromium.org/2700043002/diff/60001/ui/base/x/selection_own...
File ui/base/x/selection_owner.cc (right):

https://codereview.chromium.org/2700043002/diff/60001/ui/base/x/selection_own...
ui/base/x/selection_owner.cc:275:
base::MakeUnique<XScopedEventSelector>(requestor, PropertyChangeMask),
nit, just to check: is this what clang-format suggests?

[pkotwicz, 2017-02-21 22:24:22]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-02-21 22:24:25]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2017-02-22 03:06:30]

https://codereview.chromium.org/2700043002/diff/60001/ui/base/x/selection_own...
File ui/base/x/selection_owner.cc (right):

https://codereview.chromium.org/2700043002/diff/60001/ui/base/x/selection_own...
ui/base/x/selection_owner.cc:275:
base::MakeUnique<XScopedEventSelector>(requestor, PropertyChangeMask),
Run "git cl format" on this CL does not result in any diffs

[pkotwicz, 2017-02-22 03:06:34]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2017-02-22 03:07:38]

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-02-22 08:35:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487732794199370,
"parent_rev": "6031878959db66b36eedb04639f7124e1ab78d7c", "commit_rev":
"c8d772b0617ab7f2d15fc2e0bf02ada35dd21cfc"}

[commit-bot: I haz the power, 2017-02-22 08:36:02]

Description was changed from

==========
[Desktop Linux]: Fix copy and pasting long selections

When a client requests the clipboard selection and Chrome has a long selection
Chrome sends the data incrementally. Chrome transfers the clipboard data to the
client via a property on the client X Window. The client deletes the property to
indicate to Chrome that Chrome can send the next bit of data. Chrome listens for
the deletion XPropertyEvent from the client.

This CL fixes a bug where Chrome would stop listening for XPropertyEvents on the
client X Window after finishing servicing the client's first request instead of
after servicing all of the client's requests.

BUG=686499
==========

to

==========
[Desktop Linux]: Fix copy and pasting long selections

When a client requests the clipboard selection and Chrome has a long selection
Chrome sends the data incrementally. Chrome transfers the clipboard data to the
client via a property on the client X Window. The client deletes the property to
indicate to Chrome that Chrome can send the next bit of data. Chrome listens for
the deletion XPropertyEvent from the client.

This CL fixes a bug where Chrome would stop listening for XPropertyEvents on the
client X Window after finishing servicing the client's first request instead of
after servicing all of the client's requests.

BUG=686499

Review-Url: https://codereview.chromium.org/2700043002
Cr-Commit-Position: refs/heads/master@{#451932}
Committed:
https://chromium.googlesource.com/chromium/src/+/c8d772b0617ab7f2d15fc2e0bf02...
==========

[commit-bot: I haz the power, 2017-02-22 08:36:03]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c8d772b0617ab7f2d15fc2e0bf02...