Desktop aura: Use the X11 clipboard

Desktop aura: Use the X11 clipboard for text.

This is a sketch of a clipboard implementation for linux_aura. Right now, it can only copy/paste plain text types. Dealing with bitmaps is out of scope for this patch and the two unit tests dealing with bitmaps have temporarily been disabled on linux_aura.

Moving parts:
- This moves the current clipboard_{aurax11 => chromeos}.cc so that chromeos isn't broken between now and when I'm done. I intend to remove clipboard_chromeos once I have all the features working.
- This makes what was DispatcherLinux a part of libui.so, renamed to DispatcherAuraX11 and turned into a singleton. It has no aura dependencies, though is only used in aura code. Every usage in aura code previously grabbed the singleton instance from Env and then did an ugly static_cast<> to the other type.
- Moves X11AtomCache from aura to ui. Needs to be used in aurax11 code in ui/base/; has no X11 dependencies.

BUG=130805
TEST=none

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

[Elliot Glaysher, 2012-08-15 17:54:30]

varunjain: approval as clipboard_aurax11 author
sky: owners stamp
derat: fyi since you suggested this

[varunjain, 2012-08-15 18:04:39]

On 2012/08/15 17:54:30, Elliot Glaysher wrote:
> varunjain: approval as clipboard_aurax11 author
> sky: owners stamp
> derat: fyi since you suggested this

Can we not have a clipboard that works both on chromeos and linux_aura? What is
the problem in simply adding X11 server calls in clipboard_aurax11.cc. I dont
know enough about X11 to think of disadvantages (if any).
The advantage would be that clipboard would work nicely even on a linux_chromeos
build, so that one can copy/paste URLs from other non-chrome windows (which
would be a pretty big plus for manual testing) and also simplify the code.

[Daniel Erat, 2012-08-15 18:12:26]

On 2012/08/15 18:04:39, varunjain wrote:
> On 2012/08/15 17:54:30, Elliot Glaysher wrote:
> > varunjain: approval as clipboard_aurax11 author
> > sky: owners stamp
> > derat: fyi since you suggested this
> 
> Can we not have a clipboard that works both on chromeos and linux_aura? What
is
> the problem in simply adding X11 server calls in clipboard_aurax11.cc. I dont
> know enough about X11 to think of disadvantages (if any).
> The advantage would be that clipboard would work nicely even on a
linux_chromeos
> build, so that one can copy/paste URLs from other non-chrome windows (which
> would be a pretty big plus for manual testing) and also simplify the code.

You're suggesting making Chrome OS just use the CLIPBOARD selection instead of
keeping the clipboard data within Chrome, right?  One wrinkle may be the PRIMARY
vs. CLIPBOARD selection silliness in X, which we probably don't need to deal
with in Chrome OS; see http://www.jwz.org/doc/x-cut-and-paste.html or
http://en.wikipedia.org/wiki/X_Window_selection#Clipboard.

[Elliot Glaysher, 2012-08-15 18:23:14]

On 2012/08/15 18:12:26, Daniel Erat wrote:
> On 2012/08/15 18:04:39, varunjain wrote:
> > On 2012/08/15 17:54:30, Elliot Glaysher wrote:
> > > varunjain: approval as clipboard_aurax11 author
> > > sky: owners stamp
> > > derat: fyi since you suggested this
> > 
> > Can we not have a clipboard that works both on chromeos and linux_aura? What
> is
> > the problem in simply adding X11 server calls in clipboard_aurax11.cc. I
dont
> > know enough about X11 to think of disadvantages (if any).
> > The advantage would be that clipboard would work nicely even on a
> linux_chromeos
> > build, so that one can copy/paste URLs from other non-chrome windows (which
> > would be a pretty big plus for manual testing) and also simplify the code.
> 
> You're suggesting making Chrome OS just use the CLIPBOARD selection instead of
> keeping the clipboard data within Chrome, right?  One wrinkle may be the
PRIMARY
> vs. CLIPBOARD selection silliness in X, which we probably don't need to deal
> with in Chrome OS; see http://www.jwz.org/doc/x-cut-and-paste.html or
> http://en.wikipedia.org/wiki/X_Window_selection#Clipboard.

derat: What sort of overhead does roundtripping to the X server for clipboard
handling have? I would guess that it's negligible, but this is X we're talking
about here.

[varunjain, 2012-08-15 18:29:04]

On 2012/08/15 18:12:26, Daniel Erat wrote:
> On 2012/08/15 18:04:39, varunjain wrote:
> > On 2012/08/15 17:54:30, Elliot Glaysher wrote:
> > > varunjain: approval as clipboard_aurax11 author
> > > sky: owners stamp
> > > derat: fyi since you suggested this
> > 
> > Can we not have a clipboard that works both on chromeos and linux_aura? What
> is
> > the problem in simply adding X11 server calls in clipboard_aurax11.cc. I
dont
> > know enough about X11 to think of disadvantages (if any).
> > The advantage would be that clipboard would work nicely even on a
> linux_chromeos
> > build, so that one can copy/paste URLs from other non-chrome windows (which
> > would be a pretty big plus for manual testing) and also simplify the code.
> 
> You're suggesting making Chrome OS just use the CLIPBOARD selection instead of
> keeping the clipboard data within Chrome, right?  One wrinkle may be the
PRIMARY
> vs. CLIPBOARD selection silliness in X, which we probably don't need to deal
> with in Chrome OS; see http://www.jwz.org/doc/x-cut-and-paste.html or
> http://en.wikipedia.org/wiki/X_Window_selection#Clipboard.

We need to deal with that silliness for linux_aura port anyways. I dont think
there would be any extra work required for chromeos.
Although, from a UX perspective, having primary and clipboard selection in
chromeos may not be what we want.

[Daniel Erat, 2012-08-15 18:34:32]

On 2012/08/15 18:23:14, Elliot Glaysher wrote:
> On 2012/08/15 18:12:26, Daniel Erat wrote:
> > On 2012/08/15 18:04:39, varunjain wrote:
> > > On 2012/08/15 17:54:30, Elliot Glaysher wrote:
> > > > varunjain: approval as clipboard_aurax11 author
> > > > sky: owners stamp
> > > > derat: fyi since you suggested this
> > > 
> > > Can we not have a clipboard that works both on chromeos and linux_aura?
What
> > is
> > > the problem in simply adding X11 server calls in clipboard_aurax11.cc. I
> dont
> > > know enough about X11 to think of disadvantages (if any).
> > > The advantage would be that clipboard would work nicely even on a
> > linux_chromeos
> > > build, so that one can copy/paste URLs from other non-chrome windows
(which
> > > would be a pretty big plus for manual testing) and also simplify the code.
> > 
> > You're suggesting making Chrome OS just use the CLIPBOARD selection instead
of
> > keeping the clipboard data within Chrome, right?  One wrinkle may be the
> PRIMARY
> > vs. CLIPBOARD selection silliness in X, which we probably don't need to deal
> > with in Chrome OS; see http://www.jwz.org/doc/x-cut-and-paste.html or
> > http://en.wikipedia.org/wiki/X_Window_selection#Clipboard.
> 
> derat: What sort of overhead does roundtripping to the X server for clipboard
> handling have? I would guess that it's negligible, but this is X we're talking
> about here.

Here's the spec: http://tronche.com/gui/x/icccm/sec-2.html#s-2

I haven't written code to implement this before, but my understanding from ICCCM
is that asserting ownership of the clipboard (i.e. when a cut or copy operation
is performed) is cheap.  Handling the SelectionRequest event when a paste is
attempted is more costly -- the clipboard data may need to be converted before
being transferred to the X server, and then the client that requested the paste
needs to copy the data back out of the X server after that.  It's possible that
we can just avoid the copies when we're copy-and-pasting within Chrome, which
should always be the case on Chrome OS.

And yes, I'm pretty sure that we don't want to implement PRIMARY for Chrome OS:
it's an unknown concept for pretty much all potential users, and I don't believe
there's even any way to middle-click without an external mouse.

[Elliot Glaysher, 2012-08-27 21:53:49]

derat, PTAL. Specifically, if there's something architecturally better that I
could be doing, since I wasn't totally familiar w/ X selection before I started
this.

This patch has totally been redone. Chromeos will still get their own clipboard
for the interim until I can get all the details around X selection working
perfectly, but now aurax11 has a real implementation for text.

(In addition, the BookmarkContextMenuTest seems to be a legitimate failure, but
I don't know how to address it. The problem is that the AuraX11 implementation
of clipboard ties itself to the dispatcher, but that the MessageLoopForUI is
created by the test and is torn down before all the browser process stuff.)

[Daniel Erat, 2012-08-27 22:23:31]

Here are some nits.  I'll try to take not too long to go through
ui/base/clipboard/clipboard_aurax11.cc. :-/

http://codereview.chromium.org/10829341/diff/18007/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10829341/diff/18007/ash/shell.cc#newcode281
ash/shell.cc:281: // Remove OutputConfigurator from Dispatcher.
nit: mind removing this comment?  it doesn't appear to add any value

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboard.h
File ui/base/clipboard/clipboard.h (right):

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard.h:98: public:
nit: indent two more spaces

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard.h:100: private:
ditto

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard.h:204: || defined(OS_ANDROID)
nit: is it simpler to just invert this condition now?  what isn't matched, just
linux?

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard.h:206: #else  // !defined(OS_WIN) &&
!defined(OS_MACOSX)
nit: remove this comment?  it's outdated now

http://codereview.chromium.org/10829341/diff/18007/ui/base/x/dispatcher_aurax...
File ui/base/x/dispatcher_aurax11.cc (right):

http://codereview.chromium.org/10829341/diff/18007/ui/base/x/dispatcher_aurax...
ui/base/x/dispatcher_aurax11.cc:92:
//MessageLoopForUI::current()->RemoveObserver(this);
nit: why's this commented out?

http://codereview.chromium.org/10829341/diff/18007/ui/base/x/x11_atom_cache.h
File ui/base/x/x11_atom_cache.h (right):

http://codereview.chromium.org/10829341/diff/18007/ui/base/x/x11_atom_cache.h...
ui/base/x/x11_atom_cache.h:23: // GetAtom() were passeed to the constructor, but
this behaviour can be changed
nit: s/passeed/passed/

http://codereview.chromium.org/10829341/diff/18007/ui/base/x/x11_atom_cache.h...
ui/base/x/x11_atom_cache.h:43: mutable std::map<std::string, ::Atom>
cached_atoms_;
nit: just get rid of the 'const' on GetAtom() instead of making this mutable?

[Elliot Glaysher, 2012-08-27 22:25:52]

On 2012/08/27 22:23:31, Daniel Erat wrote:
> Here are some nits.  I'll try to take not too long to go through
> ui/base/clipboard/clipboard_aurax11.cc. :-/

(I'm sorry it's so long, but that would require me to disable ALL the unittests!
instead of just two.)

[Elliot Glaysher, 2012-08-27 22:42:22]

Reacting to nits (and opening discussion on lifetime issues on
DispatcherAuraX11).

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboard.h
File ui/base/clipboard/clipboard.h (right):

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard.h:204: || defined(OS_ANDROID)
On 2012/08/27 22:23:31, Daniel Erat wrote:
> nit: is it simpler to just invert this condition now?  what isn't matched,
just
> linux?

I want to simplify this to TOOLKIT_GTK || defined(USE_AURA) after I get rid of
the clipboard_chromeos.cc file. But for now, I'm leaving this as is. (It will
match anything that's GTK (bsd/linux/solaris/etc) along with certain aura
builds.

http://codereview.chromium.org/10829341/diff/18007/ui/base/x/dispatcher_aurax...
File ui/base/x/dispatcher_aurax11.cc (right):

http://codereview.chromium.org/10829341/diff/18007/ui/base/x/dispatcher_aurax...
ui/base/x/dispatcher_aurax11.cc:92:
//MessageLoopForUI::current()->RemoveObserver(this);
On 2012/08/27 22:23:31, Daniel Erat wrote:
> nit: why's this commented out?

It should be cut. Maybe.

The problem here is that DispatcherAuraX11 is now a singleton. It now can
outlive MessageLoopForUI, which shouldn't be a problem, except for the chromeos
unit tests, which switch out X11 instances at runtime. (I am surprised that the
trybots aren't screaming about this.)

http://codereview.chromium.org/10829341/diff/18007/ui/base/x/x11_atom_cache.h
File ui/base/x/x11_atom_cache.h (right):

http://codereview.chromium.org/10829341/diff/18007/ui/base/x/x11_atom_cache.h...
ui/base/x/x11_atom_cache.h:43: mutable std::map<std::string, ::Atom>
cached_atoms_;
On 2012/08/27 22:23:31, Daniel Erat wrote:
> nit: just get rid of the 'const' on GetAtom() instead of making this mutable?

Removing const from GetAtom would have a fan out effect, making a bunch of
methods I'd prefer to keep const in clipboard_aurax11.cc not const.

[dcheng, 2012-08-27 23:26:58]

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:150: to_delete.insert(it->second.first);
Why not just delete here?

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:239: void SelectionData::Set(::Atom type,
char* data, size_t size, bool owned) {
Personal opinion: I think that owned would be clearer as an enum, especially at
call sites.

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:334: explicit AuraX11Details();
Explicit not needed.

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:365: // and do the asynchronous dance
with whatever application is
I think this comment was

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:515: SelectionData*
Clipboard::AuraX11Details::RequestAndWaitForTypes(
Can we return a scoped_ptr?

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:595: // hoops to do that here.
If we don't support this, things like KeePass break :-(

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:669: *out_data_bytes = sizeof(long) *
nitems;
int32. Won't long be wrong on 64 bit?

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:881: if
(target_list.ContainsFormat(GetPlainTextFormatType()))
Should we be doing target_list.ContainsAtoms(GetAtomsForText()) in this case?

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:970: NOTIMPLEMENTED();
Will this be implemented eventually?

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:1079: // do.
Isn't this used for right-click "Copy Image"?

[Elliot Glaysher, 2012-08-27 23:44:31]

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:150: to_delete.insert(it->second.first);
On 2012/08/27 23:26:58, dcheng wrote:
> Why not just delete here?

Because WriteText() inserts the same pointer multiple times for different
representations. (I asked the same question about why the GTK port deduped
pointers, didn't copy what they did, and then had to debug w/ valgrind.)

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:595: // hoops to do that here.
On 2012/08/27 23:26:58, dcheng wrote:
> If we don't support this, things like KeePass break :-(

It will be done eventually. This won't effect chromeos; I moved the original
clipboard_aurax11.cc to clipboard_chromeos.cc so that I don't impact chromeos
while I'm developing the desktop aura version.

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:669: *out_data_bytes = sizeof(long) *
nitems;
On 2012/08/27 23:26:58, dcheng wrote:
> int32. Won't long be wrong on 64 bit?

Unsure. This is what GTK does and it doesn't have problems. I run 64 bit on my
machine, etc. (I had to peek at how GTK does this when I got really confused by
the nbytes being set to 0 regardless of the data transferred.)

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:881: if
(target_list.ContainsFormat(GetPlainTextFormatType()))
On 2012/08/27 23:26:58, dcheng wrote:
> Should we be doing target_list.ContainsAtoms(GetAtomsForText()) in this case?

Unsure. Only accepting the plain text type is behavior copied from the GTK port,
though I do find it odd. If you think I should change it anyway, I'm not going
to argue since it *is* weird.

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:970: NOTIMPLEMENTED();
On 2012/08/27 23:26:58, dcheng wrote:
> Will this be implemented eventually?

Yes. I've moved the current impl to chromeos so I can implement this piecewise.
(Note how much code this is just for text.)

[dcheng, 2012-08-28 01:00:54]

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:150: to_delete.insert(it->second.first);
On 2012/08/27 23:44:31, Elliot Glaysher wrote:
> On 2012/08/27 23:26:58, dcheng wrote:
> > Why not just delete here?
> 
> Because WriteText() inserts the same pointer multiple times for different
> representations. (I asked the same question about why the GTK port deduped
> pointers, didn't copy what they did, and then had to debug w/ valgrind.)

Would be OK to add a comment to that effect here? Otherwise, it's not
necessarily obvious and someone (like me) who missed that fact might remove this
and not realize it causes explosions =)

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:881: if
(target_list.ContainsFormat(GetPlainTextFormatType()))
On 2012/08/27 23:44:31, Elliot Glaysher wrote:
> On 2012/08/27 23:26:58, dcheng wrote:
> > Should we be doing target_list.ContainsAtoms(GetAtomsForText()) in this
case?
> 
> Unsure. Only accepting the plain text type is behavior copied from the GTK
port,
> though I do find it odd. If you think I should change it anyway, I'm not going
> to argue since it *is* weird.

The GTK version does check for the other text atoms. It uses IsFormatAvailable()
which has a special case for the plain text format.

[Elliot Glaysher, 2012-08-28 17:24:18]

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:150: to_delete.insert(it->second.first);
On 2012/08/28 01:00:54, dcheng wrote:
> On 2012/08/27 23:44:31, Elliot Glaysher wrote:
> > On 2012/08/27 23:26:58, dcheng wrote:
> > > Why not just delete here?
> > 
> > Because WriteText() inserts the same pointer multiple times for different
> > representations. (I asked the same question about why the GTK port deduped
> > pointers, didn't copy what they did, and then had to debug w/ valgrind.)
> 
> Would be OK to add a comment to that effect here? Otherwise, it's not
> necessarily obvious and someone (like me) who missed that fact might remove
this
> and not realize it causes explosions =)

Done.

http://codereview.chromium.org/10829341/diff/18007/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:881: if
(target_list.ContainsFormat(GetPlainTextFormatType()))
On 2012/08/28 01:00:54, dcheng wrote:
> On 2012/08/27 23:44:31, Elliot Glaysher wrote:
> > On 2012/08/27 23:26:58, dcheng wrote:
> > > Should we be doing target_list.ContainsAtoms(GetAtomsForText()) in this
> case?
> > 
> > Unsure. Only accepting the plain text type is behavior copied from the GTK
> port,
> > though I do find it odd. If you think I should change it anyway, I'm not
going
> > to argue since it *is* weird.
> 
> The GTK version does check for the other text atoms. It uses
IsFormatAvailable()
> which has a special case for the plain text format.

OK, I see now. I'm now checking all text atom types, and I also implemented the
Java hack, too.

[Daniel Erat, 2012-08-28 23:11:10]

This is just a first pass over clipboard_aurax11.cc.  I need to spend some
quality time with ICCCM and go through it again. :-(

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:72: clipboard_atom_ =
XInternAtom(GetXDisplay(), "CLIPBOARD", false);
move "CLIPBOARD" to a constant variable so it's not repeated throughout the file

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:91:
SelectionChangeObserver::~SelectionChangeObserver() {
base::MessagePumpAuraX11::Current()->RemoveObserver(this)?

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:203: SelectionData(X11AtomCache*
atom_cache);
nit: explicit

also document whether this takes ownership of atom_cache

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:287: "TARGETS",
move this into a const variable too.  probably best to just do it for all of
them?

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:353: // Insert a mapping into
clipboard_data_.
nit: s/Insert/Inserts/

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:356: // Move the temporary
|clipboard_data_| to the long term data storage for
nit: s/Move/Moves/

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:363: // If the selection holder is us,
this call is synchronous and we pull out
nit: s/pull out/pull/

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:382: // |out_data_items| is the length it
|out_type| items.
nit: s/it/in/

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:393: std::vector< ::Atom>
GetAtomsForFormat(const Clipboard::FormatType& format);
nit: add a comment describing what this does

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:400: virtual bool Dispatch(const
base::NativeEvent& event) OVERRIDE;
nit: move overridden methods below non-overridden ones

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:417: Display* xdisplay_;
nit: add underscores after 'x' in this and xwindow_ to match x_root_window_?

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:419: ::Window xwindow_;
nit: add a comment like "Input-only window used as a selection owner."

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:454: swa.background_pixmap = None;
why do you need to set the background pixmap on an input-only window?

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:463: &swa);
nit: add a title to this window to aid in debugging.  you can probably just do
something like this:

XStoreName(xdisplay_, xwindow_, "Chromium clipboard");

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:465: XMapWindow(xdisplay_, xwindow_);
does this actually need to be mapped?

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:615: size_t* out_data_items,   // Length
in items.
nit: these comments should be indented the same amount

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:618: ::Atom property_to_set =
atom_cache_.GetAtom("CHROME_SELECTION");
move "CHROME_SELECTION" to const var

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:666: // So even though we should
theoretically have nbytes (and we can't NULL
nit: s/can't NULL/can't/

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:724: case SelectionRequest: {
nit: don't need these curly brackets, do you?

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:762: ::Atom TARGETS =
atom_cache_.GetAtom("TARGETS");
nit: s/TARGETS/targets_atom/

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:772: PropModeReplace, (unsigned
char*)&targets.front(),
nit: use c++ cast instead

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:777: // MULTIPLE case, but I haven't seen
it in the wild yet.
nit: NOTIMPLEMENTED()?

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:784: PropModeReplace, (unsigned
char*)it->second.first,
nit: c++ cast

[Elliot Glaysher, 2012-08-31 00:00:19]

I've solved the linux_aura unit test issues.

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/10829341/diff/26001/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:454: swa.background_pixmap = None;
On 2012/08/28 23:11:11, Daniel Erat wrote:
> why do you need to set the background pixmap on an input-only window?

Was copying what gtk did. NULL works too.

[dcheng, 2012-09-04 21:16:01]

http://codereview.chromium.org/10829341/diff/45018/ui/base/clipboard/clipboar...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/10829341/diff/45018/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:907: // There are multiple targets that
correspond with the text mime type.
Super minor nit: This doesn't actually affect anything at the moment, but it
might be better to do this hack in TargetList::ContainsFormat.

No one currently calls Clipboard::IsFormatAvailable() for the plaintext format; 
the code that used to do this has been deleted from WebCore for reasons I do not
recall. If it were added back though, having the hack in
TargetList::ContainsFormat would make sure that things Just Worked.

[Daniel Erat, 2012-09-05 00:21:25]

The general approach that you're taking seems correct to me.

http://codereview.chromium.org/10829341/diff/45018/ui/base/clipboard/clipboar...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/10829341/diff/45018/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:729: if (XGetSelectionOwner(x_display_,
selection_name) == x_window_)
it looks like there's a race here, but grabbing the server for this seems like
overkill -- if gtk's doing something similar to this without grabbing, seems
okay to me

http://codereview.chromium.org/10829341/diff/45018/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:753: ::Atom TARGETS =
atom_cache_.GetAtom(kTargets);
nit: s/TARGETS/targets_atom/ -- think i mentioned this earlier; were previous
nits resolved?

http://codereview.chromium.org/10829341/diff/45018/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:786: DLOG(ERROR) << "Requested on a
selection we don't support.";
nit: include the selection in the error message?

[Elliot Glaysher, 2012-09-05 20:47:56]

ptal

I'm going to try to land https://chromiumcodereview.appspot.com/10911074/ before
I land this.

http://codereview.chromium.org/10829341/diff/45018/ui/base/clipboard/clipboar...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/10829341/diff/45018/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:729: if (XGetSelectionOwner(x_display_,
selection_name) == x_window_)
On 2012/09/05 00:21:25, Daniel Erat wrote:
> it looks like there's a race here, but grabbing the server for this seems like
> overkill -- if gtk's doing something similar to this without grabbing, seems
> okay to me

This is what GTK does.

http://codereview.chromium.org/10829341/diff/45018/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:753: ::Atom TARGETS =
atom_cache_.GetAtom(kTargets);
On 2012/09/05 00:21:25, Daniel Erat wrote:
> nit: s/TARGETS/targets_atom/ -- think i mentioned this earlier; were previous
> nits resolved?

This was the result of a sed gone awry when addressing the previous nits.

http://codereview.chromium.org/10829341/diff/45018/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:907: // There are multiple targets that
correspond with the text mime type.
On 2012/09/04 21:16:01, dcheng wrote:
> Super minor nit: This doesn't actually affect anything at the moment, but it
> might be better to do this hack in TargetList::ContainsFormat.
> 
> No one currently calls Clipboard::IsFormatAvailable() for the plaintext
format; 
> the code that used to do this has been deleted from WebCore for reasons I do
not
> recall. If it were added back though, having the hack in
> TargetList::ContainsFormat would make sure that things Just Worked.

Added TargetList::ContainsText().

[Daniel Erat, 2012-09-11 01:35:45]

I have trouble saying this with any degree of certainty for a change this big,
but: LGTM

http://codereview.chromium.org/10829341/diff/48032/ui/base/clipboard/clipboar...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/10829341/diff/48032/ui/base/clipboard/clipboar...
ui/base/clipboard/clipboard_aurax11.cc:195: // Returns a list of all text atoms
that we handle.
nit: move this to the top of the file since it's called by multiple classes

http://codereview.chromium.org/10829341/diff/48032/ui/base/x/x11_atom_cache.cc
File ui/base/x/x11_atom_cache.cc (right):

http://codereview.chromium.org/10829341/diff/48032/ui/base/x/x11_atom_cache.c...
ui/base/x/x11_atom_cache.cc:39: cached_atoms_.insert(std::make_pair(name,
atom));
nit: might be a bit simpler as:

  if (uncached_atoms_allowed_ && it == cached_atoms_.end()) {
    ::Atom atom = XInternAtom(xdisplay_, name, false);
    it = cached_atoms_.insert(std::make_pair(name, atom));
  }
  CHECK(it != cached_atoms_.end());
  return it->second;

[Elliot Glaysher, 2012-09-12 20:00:07]

sky: asking for ui stamp now that I have all prerequisite patches committed and
have green linux_aura trybots for this patch.

[sky, 2012-09-12 21:33:24]

LGTM

[commit-bot: I haz the power, 2012-09-12 21:35:44]

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

[commit-bot: I haz the power, 2012-09-13 00:09:21]

Change committed as 156433