Issue 8501002: Use XFixes to update the clipboard sequence number.

Issue 8501002: Use XFixes to update the clipboard sequence number. (Closed)

Created:
9 years, 1 month ago by dcheng

Modified:
9 years, 1 month ago

Reviewers:
Daniel Erat, Ben Goodger (Google)

CC:
chromium-reviews

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

Description

Reland with a virtual destructor. Use XFixes to update the clipboard sequence number. BUG=73478 TEST=manual testing Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109528 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109537

Patch Set 4 : int64 -> uint64 #

Total comments: 12

Patch Set 5 : . #

Created: 9 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+107 lines, -5 lines)			Patch
M	build/linux/system.gyp	View	1	1 chunk	+21 lines, -0 lines	0 comments	Download
M	ui/base/clipboard/clipboard_gtk.cc	View	1 2 3 4	4 chunks	+85 lines, -5 lines	0 comments	Download
M	ui/ui.gyp	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

Daniel Erat

http://codereview.chromium.org/8501002/diff/2002/ui/base/clipboard/clipboard_gtk.cc File ui/base/clipboard/clipboard_gtk.cc (right): http://codereview.chromium.org/8501002/diff/2002/ui/base/clipboard/clipboard_gtk.cc#newcode24 ui/base/clipboard/clipboard_gtk.cc:24: #include "ui/base/x/x11_util.h" nit: this should appear immediately after ui/base/gtk/gtk_signal.h ...

9 years, 1 month ago (2011-11-10 19:03:48 UTC) #2

dcheng

9 years, 1 month ago (2011-11-10 20:13:05 UTC) #3

http://codereview.chromium.org/8501002/diff/2002/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_gtk.cc (right):

http://codereview.chromium.org/8501002/diff/2002/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_gtk.cc:24: #include "ui/base/x/x11_util.h"
On 2011/11/10 19:03:49, Daniel Erat wrote:
> nit: this should appear immediately after ui/base/gtk/gtk_signal.h

Done.

http://codereview.chromium.org/8501002/diff/2002/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_gtk.cc:34: static SelectionChangeObserver*
GetInstance();
On 2011/11/10 19:03:49, Daniel Erat wrote:
> nit: move this first?

Done.

http://codereview.chromium.org/8501002/diff/2002/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_gtk.cc:52: : sequence_number_(0), event_base_(-1) {
On 2011/11/10 19:03:49, Daniel Erat wrote:
> nit: one per line unless they fit on the same line as the c'tor definition

Done.

http://codereview.chromium.org/8501002/diff/2002/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_gtk.cc:55: Atom clipboard_atom =
XInternAtom(GetXDisplay(), "CLIPBOARD", false);
On 2011/11/10 19:03:49, Daniel Erat wrote:
> cache this so you can use it in OnXEvent(); it's guaranteed to not change

I didn't cache it because it was originally only used in a debugging line after
this.

http://codereview.chromium.org/8501002/diff/2002/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_gtk.cc:65: XA_PRIMARY,
On 2011/11/10 19:03:49, Daniel Erat wrote:
> do you really need XA_PRIMARY here?  it doesn't look like you do anything with
> it later.

See below.

http://codereview.chromium.org/8501002/diff/2002/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_gtk.cc:90: << "Unexpected clipboard buffer in
selection notify.";
On 2011/11/10 19:03:49, Daniel Erat wrote:
> does it make sense to update the sequence_number_ even if you get an event
about
> a selection other than CLIPBOARD?  i'd expect something more like this:
> 
>   if (ev->selection == clipboard_atom_)
>     sequence_number_++;
>   else
>     DLOG(ERROR) << "Unexpected selection " << ev->selection;

I have a separate patch to update GetSequenceNumber to take a Buffer argument. I
just landed it so I updated this class to use that distinction.

lgtm

Expand Messages | Collapse Messages