Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10909133:
Implement clipboard for Chromoting Linux hosts. (Closed)
|
Created:
8 years, 3 months ago by Lambros Modified:
8 years, 2 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement clipboard for Chromoting Linux hosts.
BUG=132454
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=158944
Patch Set 1 #
Total comments: 4
Patch Set 2 : Initialize display_ #Patch Set 3 : Use WatchFileDescriptor instead of timer for X connection #Patch Set 4 : Remove logging and fix style nits #
Total comments: 2
Patch Set 5 : Add initial PumpEvents() #
Total comments: 10
Patch Set 6 : Rebase and address review comments #Patch Set 7 : Use desktop thread for clipboard #
Total comments: 25
Patch Set 8 : rebase #Patch Set 9 : Move x_server_clipboard to remoting/host/linux #Patch Set 10 : Fix Sergey's nits #Patch Set 11 : Stomped another MessagePumpLibevent #
Total comments: 100
Patch Set 12 : Address Wez's comments #
Total comments: 32
Patch Set 13 : Remove timestamp processing, and address comments. #
Total comments: 32
Patch Set 14 : Address comments. #Patch Set 15 : I'm an idiot! (Fix BadWindow crash) #
Total comments: 4
Patch Set 16 : Fix nits. #
Messages
Total messages: 28 (0 generated)
wez: x_server_clipboard.* sergeyu: clipboard_linux.cc (other files are pretty trivial) This is a functional first cut, there's still work to do, but I wanted to put this up so people can look at it. Still TODO: Add unit tests Implement a better message-pump than simply polling for X events on a 1-second timer. Feel free to insist these are done before committing :) And I'm sure the x_server_clipboard code could be cleaned up a bit.
Out of curiosity, if ui::Clipboard had AddObserver()/RemoveObserver() methods for listening for clipboard changes, would remoting be able to do what it needed from that? It makes me a little sad that we have to implement the unfortunate logic of dealing with the X clipboard/XFixes in multiple places. There are also good ideas that would be nice to incorporate into the ui::Clipboard implementation as well, such as the timeout when querying for selections. http://codereview.chromium.org/10909133/diff/1/remoting/host/x_server_clipboa... File remoting/host/x_server_clipboard.cc (right): http://codereview.chromium.org/10909133/diff/1/remoting/host/x_server_clipboa... remoting/host/x_server_clipboard.cc:56: void XServerClipboard::Init(Display* display, Nit: indent. http://codereview.chromium.org/10909133/diff/1/remoting/host/x_server_clipboa... remoting/host/x_server_clipboard.cc:69: clipboard_atom_ = XInternAtom(display_, "CLIPBOARD", False); It might be worth it to use X11AtomCache here. erg@ is moving it in http://codereview.chromium.org/10829341/ but I'm not sure when that will land.
On 2012/09/07 23:11:37, dcheng wrote: > Out of curiosity, if ui::Clipboard had AddObserver()/RemoveObserver() methods > for listening for clipboard changes, would remoting be able to do what it needed > from that? It makes me a little sad that we have to implement the unfortunate > logic of dealing with the X clipboard/XFixes in multiple places. > > There are also good ideas that would be nice to incorporate into the > ui::Clipboard implementation as well, such as the timeout when querying for > selections. dcheng, many thanks for taking a look at this CL :) This new code will not be compiled into chromium at all (so at least we're not adding any bloat to the browser :) It gets compiled into a standalone executable (remoting_me2me_host) and an NPAPI plugin. The main problem with ui::Clipboard is that it uses either GTK or Aura (depending on Chrome build settings), and I'm not sure if either of those would work from the plugin side of the NPAPI interface (for example, imagine a plugin compiled with Aura trying to load against a Chrome compiled with GTK, or vice-versa). Also, Chrome's MessageLoopForUI class doesn't seem to work when we try to use it inside an NPAPI plugin. I think it's because it uses some globals that the browser instantiates (and globals from GTK or glib), which don't get instantiated from the point of view of the NPAPI plugin itself. Or something like that - I can't remember the exact details of the failure. So that's why we've implemented a low-level Xlib-based clipboard class, to try to avoid these kinds of issues with NPAPI. Also, I don't know if we're allowed to add a dependency on ui:: code. In particular, we'd like to avoid pulling in any of WebKit.
Removed the 1-second timer hack in favor of watching the X11 file-descriptor instead. This feels safer than trying to re-use the Chromium MessagePump[AuraX11/GTK] code in the plugin.
On 2012/09/08 02:08:09, Lambros wrote: > On 2012/09/07 23:11:37, dcheng wrote: > > Out of curiosity, if ui::Clipboard had AddObserver()/RemoveObserver() methods > > for listening for clipboard changes, would remoting be able to do what it > needed > > from that? It makes me a little sad that we have to implement the unfortunate > > logic of dealing with the X clipboard/XFixes in multiple places. > > > > There are also good ideas that would be nice to incorporate into the > > ui::Clipboard implementation as well, such as the timeout when querying for > > selections. > dcheng, many thanks for taking a look at this CL :) > > This new code will not be compiled into chromium at all (so at least we're not > adding any bloat to the browser :) It gets compiled into a standalone > executable (remoting_me2me_host) and an NPAPI plugin. The main problem with > ui::Clipboard is that it uses either GTK or Aura (depending on Chrome build > settings), and I'm not sure if either of those would work from the plugin side > of the NPAPI interface (for example, imagine a plugin compiled with Aura trying > to load against a Chrome compiled with GTK, or vice-versa). erg@ is currently implementing an aurax11 version which only uses X11 calls. > > Also, Chrome's MessageLoopForUI class doesn't seem to work when we try to use it > inside an NPAPI plugin. I think it's because it uses some globals that the > browser instantiates (and globals from GTK or glib), which don't get > instantiated from the point of view of the NPAPI plugin itself. Or something > like that - I can't remember the exact details of the failure. > > So that's why we've implemented a low-level Xlib-based clipboard class, to try > to avoid these kinds of issues with NPAPI. > > Also, I don't know if we're allowed to add a dependency on ui:: code. In > particular, we'd like to avoid pulling in any of WebKit. ... but if it's not being built into Chromium at all, then I guess it's OK. I thought it'd be nice to try to share code eventually since there are always weird things that both implementations will have to handle, but it seems like it'd be more trouble than it's worth here.
http://codereview.chromium.org/10909133/diff/6002/remoting/host/x_server_clip... File remoting/host/x_server_clipboard.cc (right): http://codereview.chromium.org/10909133/diff/6002/remoting/host/x_server_clip... remoting/host/x_server_clipboard.cc:68: clipboard_atom_ = XInternAtom(display_, "CLIPBOARD", False); Probably want to replace this with a single XInternAtoms calls instead of doing a back and forth with the server.
http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... File remoting/host/clipboard_linux.cc (right): http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:23: ~ClipboardLinux(); virtual http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:39: virtual void OnFileCanWriteWithoutBlocking(int fd) OVERRIDE {} Better not to inline implementation of this method if you don't inline all other methods. And add NOTREACHED in it. http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:45: base::Thread x11_thread_; Do we really need to create a new thread just for clipboard capturing? If you just need to watch the display's FD then why do we need a separate thread for it? just pass the IO thread to this class and use it here. Even easier solution is to make DesktopThread an IO thread - then you will be able to watch the FD on that thread. Also can we share Display with EventExecutor? It should be easy to do given that the clipboard object is owned by the event executor.
http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... File remoting/host/clipboard_linux.cc (right): http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:39: virtual void OnFileCanWriteWithoutBlocking(int fd) OVERRIDE {} On 2012/09/12 01:00:08, sergeyu wrote: > Better not to inline implementation of this method if you don't inline all other > methods. And add NOTREACHED in it. NOTREACHED seems overkill for a notification handler method.
http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... File remoting/host/clipboard_linux.cc (right): http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:39: virtual void OnFileCanWriteWithoutBlocking(int fd) OVERRIDE {} On 2012/09/12 04:52:53, Wez wrote: > On 2012/09/12 01:00:08, sergeyu wrote: > > Better not to inline implementation of this method if you don't inline all > other > > methods. And add NOTREACHED in it. > > NOTREACHED seems overkill for a notification handler method. heh, really? http://codereview.chromium.org/10907041/diff/1/remoting/host/audio_capturer_l... :)
On 2012/09/12 18:39:12, sergeyu wrote: > http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... > File remoting/host/clipboard_linux.cc (right): > > http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... > remoting/host/clipboard_linux.cc:39: virtual void > OnFileCanWriteWithoutBlocking(int fd) OVERRIDE {} > On 2012/09/12 04:52:53, Wez wrote: > > On 2012/09/12 01:00:08, sergeyu wrote: > > > Better not to inline implementation of this method if you don't inline all > > other > > > methods. And add NOTREACHED in it. > > > > NOTREACHED seems overkill for a notification handler method. > > heh, really? > http://codereview.chromium.org/10907041/diff/1/remoting/host/audio_capturer_l... > :) Ha! Yes, I think I was wrong to ask for NOTREACHED() there. Sorry! :D
http://codereview.chromium.org/10909133/diff/1/remoting/host/x_server_clipboa... File remoting/host/x_server_clipboard.cc (right): http://codereview.chromium.org/10909133/diff/1/remoting/host/x_server_clipboa... remoting/host/x_server_clipboard.cc:56: void XServerClipboard::Init(Display* display, On 2012/09/07 23:11:37, dcheng wrote: > Nit: indent. Done. http://codereview.chromium.org/10909133/diff/1/remoting/host/x_server_clipboa... remoting/host/x_server_clipboard.cc:69: clipboard_atom_ = XInternAtom(display_, "CLIPBOARD", False); On 2012/09/07 23:11:37, dcheng wrote: > It might be worth it to use X11AtomCache here. erg@ is moving it in > http://codereview.chromium.org/10829341/ but I'm not sure when that will land. That's in ui/, which the Remoting Host currently doesn't depend on. Added TODO. http://codereview.chromium.org/10909133/diff/6002/remoting/host/x_server_clip... File remoting/host/x_server_clipboard.cc (right): http://codereview.chromium.org/10909133/diff/6002/remoting/host/x_server_clip... remoting/host/x_server_clipboard.cc:68: clipboard_atom_ = XInternAtom(display_, "CLIPBOARD", False); On 2012/09/10 22:44:21, Elliot Glaysher wrote: > Probably want to replace this with a single XInternAtoms calls instead of doing > a back and forth with the server. Done. http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... File remoting/host/clipboard_linux.cc (right): http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:23: ~ClipboardLinux(); On 2012/09/12 01:00:08, sergeyu wrote: > virtual Done. http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:39: virtual void OnFileCanWriteWithoutBlocking(int fd) OVERRIDE {} On 2012/09/12 18:39:13, sergeyu wrote: > On 2012/09/12 04:52:53, Wez wrote: > > On 2012/09/12 01:00:08, sergeyu wrote: > > > Better not to inline implementation of this method if you don't inline all > > other > > > methods. And add NOTREACHED in it. > > > > NOTREACHED seems overkill for a notification handler method. > > heh, really? > http://codereview.chromium.org/10907041/diff/1/remoting/host/audio_capturer_l... > :) Done. Chrome has many examples both with and without NOTREACHED. I'm inclined to not add it, since it's a notification handler and we don't care enough to warrant adding extra logging code. http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:45: base::Thread x11_thread_; On 2012/09/12 01:00:08, sergeyu wrote: > Do we really need to create a new thread just for clipboard capturing? If you > just need to watch the display's FD then why do we need a separate thread for > it? just pass the IO thread to this class and use it here. Even easier solution > is to make DesktopThread an IO thread - then you will be able to watch the FD on > that thread. > > Also can we share Display with EventExecutor? It should be easy to do given that > the clipboard object is owned by the event executor. Sharing the X connection and thread with EventExecutor is quite a lot of work to do at this stage. The problem is that ClipboardLinux wants a MessageLoopForIO, but EventExecutor makes some XLib calls on the plugin thread via our TaskRunner wrapper around NPAPI's interface, and that doesn't expose a MessageLoopForIO to us. So EventExecutor would have to take yet another parameter, MessageLoopForIO, to use for all its X11 tasks. I'd rather save this work as part of overall cleanup of our usage of X11 within Chromoting host. Added TODO.
http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... File remoting/host/clipboard_linux.cc (right): http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:45: base::Thread x11_thread_; On 2012/09/12 23:42:32, Lambros wrote: > On 2012/09/12 01:00:08, sergeyu wrote: > > Do we really need to create a new thread just for clipboard capturing? If you > > just need to watch the display's FD then why do we need a separate thread for > > it? just pass the IO thread to this class and use it here. Even easier > solution > > is to make DesktopThread an IO thread - then you will be able to watch the FD > on > > that thread. > > > > Also can we share Display with EventExecutor? It should be easy to do given > that > > the clipboard object is owned by the event executor. > Sharing the X connection and thread with EventExecutor is quite a lot of work to > do at this stage. The problem is that ClipboardLinux wants a MessageLoopForIO, > but EventExecutor makes some XLib calls on the plugin thread via our TaskRunner > wrapper around NPAPI's interface, and that doesn't expose a MessageLoopForIO to > us. So EventExecutor would have to take yet another parameter, > MessageLoopForIO, to use for all its X11 tasks. main_task_runner that is passed to EventExecutorLinux is actually the desktop_task_runner() from ChromotingHostContext, which is different from NPAPI thread. I don't think EventExecutorLinux is doing anything on the plugin thread. Making the desktop thread thread an IO thread would be a trivial change in ChromotingHostContext and then you can access it via MessageLoopForIO::current(). It's Ok for now if you create separate X11 connection, but I still see no reason a separate thread. > > I'd rather save this work as part of overall cleanup of our usage of X11 within > Chromoting host. Added TODO.
ptal http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... File remoting/host/clipboard_linux.cc (right): http://codereview.chromium.org/10909133/diff/2002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:45: base::Thread x11_thread_; On 2012/09/14 00:29:56, sergeyu wrote: > On 2012/09/12 23:42:32, Lambros wrote: > > On 2012/09/12 01:00:08, sergeyu wrote: > > > Do we really need to create a new thread just for clipboard capturing? If > you > > > just need to watch the display's FD then why do we need a separate thread > for > > > it? just pass the IO thread to this class and use it here. Even easier > > solution > > > is to make DesktopThread an IO thread - then you will be able to watch the > FD > > on > > > that thread. > > > > > > Also can we share Display with EventExecutor? It should be easy to do given > > that > > > the clipboard object is owned by the event executor. > > Sharing the X connection and thread with EventExecutor is quite a lot of work > to > > do at this stage. The problem is that ClipboardLinux wants a > MessageLoopForIO, > > but EventExecutor makes some XLib calls on the plugin thread via our > TaskRunner > > wrapper around NPAPI's interface, and that doesn't expose a MessageLoopForIO > to > > us. So EventExecutor would have to take yet another parameter, > > MessageLoopForIO, to use for all its X11 tasks. > > main_task_runner that is passed to EventExecutorLinux is actually the > desktop_task_runner() from ChromotingHostContext, which is different from NPAPI > thread. I don't think EventExecutorLinux is doing anything on the plugin thread. > Making the desktop thread thread an IO thread would be a trivial change in > ChromotingHostContext and then you can access it via > MessageLoopForIO::current(). > It's Ok for now if you create separate X11 connection, but I still see no reason > a separate thread. > > > > > > I'd rather save this work as part of overall cleanup of our usage of X11 > within > > Chromoting host. Added TODO. Done.
LGTM when my nits are addressed. I didn't review x_server_clipboard.cc and will leave it to Wez. http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... File remoting/host/clipboard_linux.cc (right): http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:12: #include "base/message_pump_libevent.h" don't need this include. See my comment below. http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:19: class ClipboardLinux : public Clipboard, base::MessagePumpLibevent::Watcher { It's better to use MessageLoopForIO::Watcher here. It's just a typedef for base::MessagePumpLibevent::Watcher, but looks less awkward. Also you forgot to specify public inheritance for the second parent. http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:19: class ClipboardLinux : public Clipboard, base::MessagePumpLibevent::Watcher { Add comment to exmplain that we expect this code to be used on the desktop thread only. http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:24: virtual void Start( // Clipboard interface. http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:35: // base::MessagePumpLibevent::Watcher interface nit: period at the end of the comment. MessageLoopForIO http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:36: virtual void OnFileCanReadWithoutBlocking(int fd) OVERRIDE; Per style guide interface implementation must always be public. http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:49: ClipboardLinux::ClipboardLinux() : display_(NULL) { nit: better to place initializer on a separate line http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:58: display_ = XOpenDisplay(NULL); Please add TODO to share X connection with the event executor. http://codereview.chromium.org/10909133/diff/8008/remoting/host/event_executo... File remoting/host/event_executor_linux.cc (right): http://codereview.chromium.org/10909133/diff/8008/remoting/host/event_executo... remoting/host/event_executor_linux.cc:208: clipboard_ = Clipboard::Create(); thread DCHECK please. http://codereview.chromium.org/10909133/diff/8008/remoting/host/event_executo... remoting/host/event_executor_linux.cc:292: base::Passed(client_clipboard.Pass()))); FYI: alternative, slightly shorter syntax is base::Passed(&client_clipboard). http://codereview.chromium.org/10909133/diff/8008/remoting/host/x_server_clip... File remoting/host/x_server_clipboard.h (right): http://codereview.chromium.org/10909133/diff/8008/remoting/host/x_server_clip... remoting/host/x_server_clipboard.h:7: #ifndef REMOTING_HOST_X_SERVER_CLIPBOARD_H_ Please put this file in remoting/host/linux . Ideally all linux-only code should go there. http://codereview.chromium.org/10909133/diff/8008/remoting/remoting.gyp File remoting/remoting.gyp (right): http://codereview.chromium.org/10909133/diff/8008/remoting/remoting.gyp#newco... remoting/remoting.gyp:1418: 'host/x_server_clipboard.cc', you wouldn't need to specify this explicitly if you put this file in host/linux directory. Eventually we should move host/x_server_pixel_buffer.cc there too.
http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... File remoting/host/clipboard_linux.cc (right): http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:12: #include "base/message_pump_libevent.h" On 2012/09/14 19:35:01, sergeyu wrote: > don't need this include. See my comment below. Done. http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:19: class ClipboardLinux : public Clipboard, base::MessagePumpLibevent::Watcher { On 2012/09/14 19:35:01, sergeyu wrote: > It's better to use MessageLoopForIO::Watcher here. It's just a typedef for > base::MessagePumpLibevent::Watcher, but looks less awkward. > > Also you forgot to specify public inheritance for the second parent. Done. http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:19: class ClipboardLinux : public Clipboard, base::MessagePumpLibevent::Watcher { On 2012/09/14 19:35:01, sergeyu wrote: > Add comment to exmplain that we expect this code to be used on the desktop > thread only. Done. http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:24: virtual void Start( On 2012/09/14 19:35:01, sergeyu wrote: > // Clipboard interface. Done. http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:35: // base::MessagePumpLibevent::Watcher interface On 2012/09/14 19:35:01, sergeyu wrote: > nit: period at the end of the comment. MessageLoopForIO Done. http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:36: virtual void OnFileCanReadWithoutBlocking(int fd) OVERRIDE; On 2012/09/14 19:35:01, sergeyu wrote: > Per style guide interface implementation must always be public. Done. http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:36: virtual void OnFileCanReadWithoutBlocking(int fd) OVERRIDE; On 2012/09/14 19:35:01, sergeyu wrote: > Per style guide interface implementation must always be public. Done (and moved the implementations so they match the ordering in the class declaration). http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:49: ClipboardLinux::ClipboardLinux() : display_(NULL) { On 2012/09/14 19:35:01, sergeyu wrote: > nit: better to place initializer on a separate line Done. http://codereview.chromium.org/10909133/diff/8008/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:58: display_ = XOpenDisplay(NULL); On 2012/09/14 19:35:01, sergeyu wrote: > Please add TODO to share X connection with the event executor. Done. http://codereview.chromium.org/10909133/diff/8008/remoting/host/event_executo... File remoting/host/event_executor_linux.cc (right): http://codereview.chromium.org/10909133/diff/8008/remoting/host/event_executo... remoting/host/event_executor_linux.cc:208: clipboard_ = Clipboard::Create(); On 2012/09/14 19:35:01, sergeyu wrote: > thread DCHECK please. Done. http://codereview.chromium.org/10909133/diff/8008/remoting/host/event_executo... remoting/host/event_executor_linux.cc:292: base::Passed(client_clipboard.Pass()))); On 2012/09/14 19:35:01, sergeyu wrote: > FYI: alternative, slightly shorter syntax is base::Passed(&client_clipboard). This got changed to the shorter syntax when I rebased to ToT. http://codereview.chromium.org/10909133/diff/8008/remoting/host/x_server_clip... File remoting/host/x_server_clipboard.h (right): http://codereview.chromium.org/10909133/diff/8008/remoting/host/x_server_clip... remoting/host/x_server_clipboard.h:7: #ifndef REMOTING_HOST_X_SERVER_CLIPBOARD_H_ On 2012/09/14 19:35:01, sergeyu wrote: > Please put this file in remoting/host/linux . Ideally all linux-only code should > go there. Done. http://codereview.chromium.org/10909133/diff/8008/remoting/remoting.gyp File remoting/remoting.gyp (right): http://codereview.chromium.org/10909133/diff/8008/remoting/remoting.gyp#newco... remoting/remoting.gyp:1418: 'host/x_server_clipboard.cc', On 2012/09/14 19:35:01, sergeyu wrote: > you wouldn't need to specify this explicitly if you put this file in host/linux > directory. Eventually we should move host/x_server_pixel_buffer.cc there too. Done.
We currently target Debian/Ubuntu, and any modern Linux distro will be using Xorg, which has Xfixes, so why don't we simplify this code by using XFixes if present, and assuming that a SetSelectionOwner notification indicates that we need to grab content from that selection, so we don't need to workaround bogus timestamping?
Some initial comments; unless we have a compelling reason to continue to support X servers that don't have XFIXES, I'd recommend simplifying this code by relying on XFIXES and just ignoring clipboard activity if it's not present. http://codereview.chromium.org/10909133/diff/3002/remoting/host/chromoting_ho... File remoting/host/chromoting_host_context.cc (right): http://codereview.chromium.org/10909133/diff/3002/remoting/host/chromoting_ho... remoting/host/chromoting_host_context.cc:49: MessageLoop::TYPE_IO, 0)) && nit: Since all of these threads are IO threads, create a single io_thread_options to pass to StartWithOptions() http://codereview.chromium.org/10909133/diff/3002/remoting/host/clipboard_lin... File remoting/host/clipboard_linux.cc (right): http://codereview.chromium.org/10909133/diff/3002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:18: // This code is expected to be used on the desktop thread only. nit: "used" is vague; do we expect to be _called_ from the desktop thread? What's special about the desktop thread from this class' perspective? http://codereview.chromium.org/10909133/diff/3002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:27: scoped_ptr<protocol::ClipboardStub> client_clipboard) OVERRIDE; nit: I think Start & Stop should be StartClipboard/StopClipboard to avoid clashes if a Clipboard implementation needs to implement some other interface as well. http://codereview.chromium.org/10909133/diff/3002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:39: void PumpEvents(); nit: PumpXEvents? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... File remoting/host/linux/x_server_clipboard.cc (right): http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:17: // to be 0). As you comment, 0 is normally CurrentTime; isn't it fragile to add a different meaning here? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:18: bool IsTimeLater(Time a, Time b) { nit: IsTimeLaterThan But... this function is just a > b, isn't it? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:22: if (b == 0) { nit: You don't need this clause, since at this point |a > b == true|, unless |a| is somehow -ve? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:63: // trap such errors here. Jamie's XRANDR CL adds ScopedXErrorHandler; use that here to avoid crashes? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:69: // TODO(lambroslambrou): Use ui::X11AtomCache for this. nit: Why are you not already using it in this CL? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:107: if (!display_) { Under what circumstances is it ever valid to call SetClipboard() with a NULL |display_|? Should this be DCHECK? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:118: OwnSelection(XA_PRIMARY, CurrentTime); You're calling XSetSelectionOwner() with CurrentTime; isn't that exactly the bug that causes us to see lots of X selections with zero timestamps from other apps? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:120: current_selection_time_ = CurrentTime; CurrentTime == 0, so this doesn't do what it looks like it does! http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:146: GetSelections(notify_event->selection, notify_event->selection_timestamp); |selection_timestamp| is the timestamp used to generate the event; since lots of X apps incorrectly use CurrentTime that will be zero a lot of the time, won't it? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:153: // the timestamp, the target list and the string value of the selection. I don't understand this sentence; are you saying that you as for the timestamp, target list and string value for several targets, or something else? How come targets themselves have a list of targets? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:158: // state between the various selectionNotify() callbacks we will get. We set Where are we storing this problem state? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:162: // will allow a retry after the timer expires. How does this resolve the issue of leaving stale state if our process crashes or is restarted? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:165: // decide which one to retrieve. If a selection and timestamp are passed in A timestamp is always passed it; is there some special value that means "no timestamp"? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:166: // then we always use that selection and treat it as new. Otherwise we You haven't introduced a concept of "new" up to this point; are you saying that if a selection & timestamp are supplied then we assume that the content of that selection is changed? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:168: // timestamp and one of the selections is new, then we use that selection. What does it mean for a selection to be "new"? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:180: // use that, otherwise we fall back to using STRING. As with the other parts of this big block of comments, move this description to sit immediately before the relevant bit of code. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:184: if (!get_selections_time_.is_null() && Add a comment, separate from this big block comment, explaining this early-exit test. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:198: if (IsSelectionOwner(selection)) { Add a comment to this to indicate why we ignore selections when we're the owner; simply moving the text from the big block above should be fine. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:231: XFree(data); Should you actually be doing something with this data, rather than simply consuming & freeing it? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:263: DoSelectionNotify(&event->xselection, 0, 0, 0, 0); Are you missing a closing } after this? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:263: DoSelectionNotify(&event->xselection, 0, 0, 0, 0); What does this DoSelectionNotify() call mean? Should you be doing it in the Standard Selection case, since you arlready have a call, above? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:336: if (event->property == timestamp_atom_) { What does it mean for property not to be TIMESTAMP when we reach here? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:372: bool XServerClipboard::GotSelectionTargets(XSelectionEvent* event, nit: HandleSelectionTargetsEvent? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:381: // 8 bytes, the size of a |long|. nit: This is an Xlib-ism; the protocol uses CARD32, IIRC, but Xlib was written assuming longs were 32-bit. :( http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:395: bool XServerClipboard::GotSelectionString(XSelectionEvent* event, nit: HandleSelectionStringEvent? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:411: void XServerClipboard::GotCutTextUtf8(const std::string& text) { nit: NotifyClipboardText()? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:423: void XServerClipboard::GetSelectionTimestamp(Atom selection, Time time) { nit: RequestSelectionTimestamp, since it's async? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:428: void XServerClipboard::GetSelectionTargets(Atom selection) { As above http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:433: void XServerClipboard::GetSelectionString(Atom selection, Atom target) { As above http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:438: void XServerClipboard::OwnSelection(Atom selection, Time time) { nit: AssertSelectionOwnership() http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... File remoting/host/linux/x_server_clipboard.h (right): http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:5: // Don't include this file in any .h files because it pulls in some X headers. nit: in -> from? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:23: // A class to allow manipulation of the X clipboard, using low-level X nit: low-level -> only http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:34: // The caller should ensure |display| remains valid as long as any other nit: should -> must? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:35: // methods are called on this object. nit: Suggest reword: ... |display| is still valid whenever any of this object's APIs are invoked. Or alternatively just require that Display* out-live this object? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:38: void SetClipboard(const std::string& mime_type, const std::string& data); nit: Add a sentence to describe this API. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:43: void ProcessXEvent(XEvent* event); nit: Reword: Invoked by the caller for each X11 event. I don't understand the "and all methods..." bit; do you mean that the call needs to be made on the same thread the event was received on? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:46: // This checks the state of the X selections. If there is new text selected nit: Suggest reword: "Checks the state of the X PRIMARY and CLIPBOARD selections and queues a notification to the registered ClipboardChangedCallback if the content has changed since the previous call." I don't understand why PRIMARY is tried first, if SECONDARY is used if it's newer; doesn't that just mean you use whichever is newer? Why filter out duplicates? We already have echo filtering elsewhere in the clipboard pipeline, and filtering here will lead to un-intuitive behaviour. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:55: // no callback will be triggered in this case. nit: I don't think you need to document this here; mention it at the call-site instead. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:58: // are ignored. nit: Why ignore other values rather than treating them as invalid? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:58: // are ignored. nit: Suggest reword "|selection| specifies the selection to be checked, or None to check both the CLIPBOARD and PRIMARY selections." http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:60: // and is treated as new. nit: This doesn't really explain what |timestamp| means; is it used to indicate when we most recently checked the clipboard, for example? Will it ever become zero after having been non-zero? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:61: void GetSelections(Atom selection, Time timestamp); Looks like this is the handler for XFixes' SetSelectionOwner events, so name it OnSetSelectionOwner? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:63: void FinishGetSelections(); nit: Add a comment explaining what this is for. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:70: // information about a selection. nit: Reword: "Called when the ..." http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:95: // Request that the window owns the given selection from the given time (the nit: Which window? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:96: // time should be taken from an X event). nit: Suggest reword: "Assert ownership of the specified |selection| at the specified |time|." Why should time be taken from an X event? What X event would it be taken from? Shouldn't time be "now", or thereabouts?
http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... File remoting/host/linux/x_server_clipboard.cc (right): http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:17: // to be 0). On 2012/09/17 22:13:29, Wez wrote: > As you comment, 0 is normally CurrentTime; isn't it fragile to add a different > meaning here? This is only called from one place, where 0 means uninitialized. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:18: bool IsTimeLater(Time a, Time b) { On 2012/09/17 22:13:29, Wez wrote: > nit: IsTimeLaterThan Done http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:22: if (b == 0) { On 2012/09/17 22:13:29, Wez wrote: > nit: You don't need this clause, since at this point |a > b == true|, unless |a| > is somehow -ve? I meant to test (a - b) > 0, to deal with wrap-around. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:63: // trap such errors here. On 2012/09/17 22:13:29, Wez wrote: > Jamie's XRANDR CL adds ScopedXErrorHandler; use that here to avoid crashes? Added TODO. X Error handlers are tricky, since (IIUC) they are global to all X connections, and not per-display or per-thread. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:69: // TODO(lambroslambrou): Use ui::X11AtomCache for this. On 2012/09/17 22:13:29, Wez wrote: > nit: Why are you not already using it in this CL? Clarified reason for not using it right now. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:107: if (!display_) { On 2012/09/17 22:13:29, Wez wrote: > Under what circumstances is it ever valid to call SetClipboard() with a NULL > |display_|? Should this be DCHECK? Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:118: OwnSelection(XA_PRIMARY, CurrentTime); On 2012/09/17 22:13:29, Wez wrote: > You're calling XSetSelectionOwner() with CurrentTime; isn't that exactly the bug > that causes us to see lots of X selections with zero timestamps from other apps? I don't think so - the man-page and this web-site says that passing CurrentTime is allowed: http://tronche.com/gui/x/xlib/window-information/XSetSelectionOwner.html http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:120: current_selection_time_ = CurrentTime; On 2012/09/17 22:13:29, Wez wrote: > CurrentTime == 0, so this doesn't do what it looks like it does! You're right, this will cause any other timestamp to "win". Unfortunately, there doesn't seem to be any way of querying what the X Server thinks the current time is. Maybe it doesn't matter, and we could get away with not setting this variable at all, so it just holds whatever value was most recently set? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:146: GetSelections(notify_event->selection, notify_event->selection_timestamp); On 2012/09/17 22:13:29, Wez wrote: > |selection_timestamp| is the timestamp used to generate the event; since lots of > X apps incorrectly use CurrentTime that will be zero a lot of the time, won't > it? I think the X Server is supposed to substitute the correct time, if we believe the web-site I linked above. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:153: // the timestamp, the target list and the string value of the selection. On 2012/09/17 22:13:29, Wez wrote: > I don't understand this sentence; are you saying that you as for the timestamp, > target list and string value for several targets, or something else? How come > targets themselves have a list of targets? Clarified. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:162: // will allow a retry after the timer expires. On 2012/09/17 22:13:29, Wez wrote: > How does this resolve the issue of leaving stale state if our process crashes or > is restarted? I've rephrased this, ptal. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:165: // decide which one to retrieve. If a selection and timestamp are passed in On 2012/09/17 22:13:29, Wez wrote: > A timestamp is always passed it; is there some special value that means "no > timestamp"? Clarified. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:168: // timestamp and one of the selections is new, then we use that selection. On 2012/09/17 22:13:29, Wez wrote: > What does it mean for a selection to be "new"? Newer than |current_selection_time_| http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:180: // use that, otherwise we fall back to using STRING. On 2012/09/17 22:13:29, Wez wrote: > As with the other parts of this big block of comments, move this description to > sit immediately before the relevant bit of code. Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:180: // use that, otherwise we fall back to using STRING. On 2012/09/17 22:13:29, Wez wrote: > As with the other parts of this big block of comments, move this description to > sit immediately before the relevant bit of code. Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:184: if (!get_selections_time_.is_null() && On 2012/09/17 22:13:29, Wez wrote: > Add a comment, separate from this big block comment, explaining this early-exit > test. Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:198: if (IsSelectionOwner(selection)) { On 2012/09/17 22:13:29, Wez wrote: > Add a comment to this to indicate why we ignore selections when we're the owner; > simply moving the text from the big block above should be fine. Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:231: XFree(data); On 2012/09/17 22:13:29, Wez wrote: > Should you actually be doing something with this data, rather than simply > consuming & freeing it? Added TODO for large transfers. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:263: DoSelectionNotify(&event->xselection, 0, 0, 0, 0); On 2012/09/17 22:13:29, Wez wrote: > Are you missing a closing } after this? No, this is an indentation fail! Fixed. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:263: DoSelectionNotify(&event->xselection, 0, 0, 0, 0); On 2012/09/17 22:13:29, Wez wrote: > What does this DoSelectionNotify() call mean? Should you be doing it in the > Standard Selection case, since you arlready have a call, above? Good catch! Added missing return. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:336: if (event->property == timestamp_atom_) { On 2012/09/17 22:13:29, Wez wrote: > What does it mean for property not to be TIMESTAMP when we reach here? Added comments (some copied from elsewhere as you suggested :) http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:372: bool XServerClipboard::GotSelectionTargets(XSelectionEvent* event, On 2012/09/17 22:13:29, Wez wrote: > nit: HandleSelectionTargetsEvent? Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:381: // 8 bytes, the size of a |long|. On 2012/09/17 22:13:29, Wez wrote: > nit: This is an Xlib-ism; the protocol uses CARD32, IIRC, but Xlib was written > assuming longs were 32-bit. :( Clarified that it's Xlib that does the padding. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:395: bool XServerClipboard::GotSelectionString(XSelectionEvent* event, On 2012/09/17 22:13:29, Wez wrote: > nit: HandleSelectionStringEvent? Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:411: void XServerClipboard::GotCutTextUtf8(const std::string& text) { On 2012/09/17 22:13:29, Wez wrote: > nit: NotifyClipboardText()? Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:423: void XServerClipboard::GetSelectionTimestamp(Atom selection, Time time) { On 2012/09/17 22:13:29, Wez wrote: > nit: RequestSelectionTimestamp, since it's async? Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:428: void XServerClipboard::GetSelectionTargets(Atom selection) { On 2012/09/17 22:13:29, Wez wrote: > As above Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:433: void XServerClipboard::GetSelectionString(Atom selection, Atom target) { On 2012/09/17 22:13:29, Wez wrote: > As above Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:438: void XServerClipboard::OwnSelection(Atom selection, Time time) { On 2012/09/17 22:13:29, Wez wrote: > nit: AssertSelectionOwnership() Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... File remoting/host/linux/x_server_clipboard.h (right): http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:5: // Don't include this file in any .h files because it pulls in some X headers. On 2012/09/17 22:13:29, Wez wrote: > nit: in -> from? Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:23: // A class to allow manipulation of the X clipboard, using low-level X On 2012/09/17 22:13:29, Wez wrote: > nit: low-level -> only Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:34: // The caller should ensure |display| remains valid as long as any other On 2012/09/17 22:13:29, Wez wrote: > nit: should -> must? Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:35: // methods are called on this object. On 2012/09/17 22:13:29, Wez wrote: > nit: Suggest reword: ... |display| is still valid whenever any of this object's > APIs are invoked. > > Or alternatively just require that Display* out-live this object? Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:38: void SetClipboard(const std::string& mime_type, const std::string& data); On 2012/09/17 22:13:29, Wez wrote: > nit: Add a sentence to describe this API. Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:43: void ProcessXEvent(XEvent* event); On 2012/09/17 22:13:29, Wez wrote: > nit: Reword: Invoked by the caller for each X11 event. > > I don't understand the "and all methods..." bit; do you mean that the call needs > to be made on the same thread the event was received on? Not necessarily. But then the caller needs to do synchronization if it's going to call XPending/XNextEvent on a different thread from the thread that calls methods of this class, which also call into Xlib. I've changed the wording, ptal. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:58: // are ignored. On 2012/09/17 22:13:29, Wez wrote: > nit: Why ignore other values rather than treating them as invalid? We ignore them because we might conceivably get them from XFixes, if an app were to claim ownership of a new X clipboard type. I suppose we could remote the new clipboard, but probably best not to, for now at least :) http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:60: // and is treated as new. On 2012/09/17 22:13:29, Wez wrote: > nit: This doesn't really explain what |timestamp| means; is it used to indicate > when we most recently checked the clipboard, for example? Will it ever become > zero after having been non-zero? I've expanded the wording a bit. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:61: void GetSelections(Atom selection, Time timestamp); On 2012/09/17 22:13:29, Wez wrote: > Looks like this is the handler for XFixes' SetSelectionOwner events, so name it > OnSetSelectionOwner? It's also called during init (although arguably it shouldn't be, as we shouldn't be getting the initial clipboard selection for Chromoting - I'll take a look at this). http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:63: void FinishGetSelections(); On 2012/09/17 22:13:29, Wez wrote: > nit: Add a comment explaining what this is for. Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:70: // information about a selection. On 2012/09/17 22:13:29, Wez wrote: > nit: Reword: "Called when the ..." Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:95: // Request that the window owns the given selection from the given time (the On 2012/09/17 22:13:29, Wez wrote: > nit: Which window? Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:96: // time should be taken from an X event). On 2012/09/17 22:13:29, Wez wrote: > nit: Suggest reword: "Assert ownership of the specified |selection| at the > specified |time|." > > Why should time be taken from an X event? What X event would it be taken from? > Shouldn't time be "now", or thereabouts? Given that we only ever pass CurrentTime in, I've removed this parameter.
http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... File remoting/host/linux/x_server_clipboard.cc (right): http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:17: // to be 0). On 2012/09/19 23:58:19, Lambros wrote: > On 2012/09/17 22:13:29, Wez wrote: > > As you comment, 0 is normally CurrentTime; isn't it fragile to add a different > > meaning here? > This is only called from one place, where 0 means uninitialized. And are X timestamps guaranteed never to wrap through zero? http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:63: // trap such errors here. On 2012/09/19 23:58:19, Lambros wrote: > On 2012/09/17 22:13:29, Wez wrote: > > Jamie's XRANDR CL adds ScopedXErrorHandler; use that here to avoid crashes? > > Added TODO. X Error handlers are tricky, since (IIUC) they are global to all X > connections, and not per-display or per-thread. That's decidedly unhelpful! Would we be better off using Xcb instead of Xlib? We'll need to bear that in mind in the resize-to-fit CL. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.cc:120: current_selection_time_ = CurrentTime; On 2012/09/19 23:58:19, Lambros wrote: > On 2012/09/17 22:13:29, Wez wrote: > > CurrentTime == 0, so this doesn't do what it looks like it does! > You're right, this will cause any other timestamp to "win". Unfortunately, > there doesn't seem to be any way of querying what the X Server thinks the > current time is. Maybe it doesn't matter, and we could get away with not > setting this variable at all, so it just holds whatever value was most recently > set? Leaving the most-recently-seen time is probably fine; we can spot our own selections being set and ignore them based on their ownership, instead. So I'd rename the member to latest_selection_event_time_ or similar. http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... File remoting/host/linux/x_server_clipboard.h (right): http://codereview.chromium.org/10909133/diff/3002/remoting/host/linux/x_serve... remoting/host/linux/x_server_clipboard.h:61: void GetSelections(Atom selection, Time timestamp); On 2012/09/19 23:58:19, Lambros wrote: > On 2012/09/17 22:13:29, Wez wrote: > > Looks like this is the handler for XFixes' SetSelectionOwner events, so name > it > > OnSetSelectionOwner? > It's also called during init (although arguably it shouldn't be, as we shouldn't > be getting the initial clipboard selection for Chromoting - I'll take a look at > this). OK, but can we re-name it to OnSetSelectionOwner, or similar, since it's really the XFixes event handler, or ProcessSelection, say, since it processes the specified selection? http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... File remoting/host/linux/x_server_clipboard.cc (right): http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:28: // though |a| is smaller numerically. nit: Suggest: 32-bit X timestamps will wrap around after 49 days. Our comparison assumes that we won't need to compare timestamps more than 24 days apart. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:160: // during initialization (with |selection| and |timestamp| equal to zero), If it's called during initialization with |selection| set to zero, why not set |selection| to XA_PRIMARY in the initialization call, and avoid the |selection != None| test below? http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:170: // selections is newer than |current_selection_time_|, then we use that nit for discussion off-CL: Do we really need these timestamp comparisons? Are there circumstances under which we can we see these notifications out-of-order? http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:192: // details for it. nit: Suggest: If we own the selection, don't request details for it. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:203: // targets supported for that selection (currently only UTF8_STRING and nit: ... the list of target formats it supports. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:204: // STRING are supported). nit: No need to clarify the targets we support here. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:230: // TODO(lambroslambrou): Properly support large transfers. nit: Add a bug for that and refer to it here. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:371: // have to get the actual value of the selection each time. But this will only be triggered by an XFixes notification? http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... File remoting/host/linux/x_server_clipboard.h (right): http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:38: // PRIMARY and CLIPBOARD selection. nit: selections http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:43: // is not designed to be thread-safe, so all its methods should ideally be Surely you mean "must", not "should ideally"? http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:47: // on one thread (per display). This comment seems to imply that the caller might opt to split calls across different threads but it's not clear to me what synchronization the caller would need to do to make this safe, and since we don't intend to use it multi-threaded - let's just state that this class isn't designed for multi-threaded use? http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:51: // Check the state of the X PRIMARY and CLIPBOARD selections and queues a nit: queues -> queue http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:55: // CLIPBOARD), or None to check both the CLIPBOARD and PRIMARY selections. nit: It looked to me like the implementation actually just checks PRIMARY if None is supplied? http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:60: // SelectionNotify events). I still don't understand this; if you're treating the event as "new" then you don't need a timestamp to compare other events to, surely? http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:64: // is complete. There's no mention of a timer in the comment for GetSelections, though? I'd suggest adding a line to the GetSelections() comment stating: We ignore new GetSelections() requests while we're processing a previous request, unless the earlier request appears to have stalled, e.g. the peer app is not responding. Then change this comment to something like: Called when the current GetSelections request completes, either having processed new text, or having ignored the new content, e.g. because it's text we set ourselves, or it's not text at all. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:79: bool HandleSelectionTimestampEvent(XSelectionEvent* event, nit: Add a comment before this block of methods indicating that they return true if the event completes the current GetSelections() request, false otherwise. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:96: void RequestSelectionTimestamp(Atom selection, Time time); nit: Consider adding a comment to this block of methods indicating that they request details about a selection, triggering the events handled by the methods above? http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:123: base::TimeTicks get_selections_time_; nit: get_selections_start_time_?
PTAL at this simplified version. There are still a couple of tiny comment nits I haven't addressed yet. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... File remoting/host/linux/x_server_clipboard.cc (right): http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:28: // though |a| is smaller numerically. On 2012/09/20 23:37:46, Wez wrote: > nit: Suggest: 32-bit X timestamps will wrap around after 49 days. Our comparison > assumes that we won't need to compare timestamps more than 24 days apart. Removed code. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:160: // during initialization (with |selection| and |timestamp| equal to zero), On 2012/09/20 23:37:46, Wez wrote: > If it's called during initialization with |selection| set to zero, why not set > |selection| to XA_PRIMARY in the initialization call, and avoid the |selection > != None| test below? No loner called during init. This is now just an event-handler, so I've renamed as you suggested. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:170: // selections is newer than |current_selection_time_|, then we use that On 2012/09/20 23:37:46, Wez wrote: > nit for discussion off-CL: Do we really need these timestamp comparisons? Are > there circumstances under which we can we see these notifications out-of-order? Done. PTAL at the simplified version. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:192: // details for it. On 2012/09/20 23:37:46, Wez wrote: > nit: Suggest: > If we own the selection, don't request details for it. Done. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:203: // targets supported for that selection (currently only UTF8_STRING and On 2012/09/20 23:37:46, Wez wrote: > nit: ... the list of target formats it supports. Done. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:204: // STRING are supported). On 2012/09/20 23:37:46, Wez wrote: > nit: No need to clarify the targets we support here. Done. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:230: // TODO(lambroslambrou): Properly support large transfers. On 2012/09/20 23:37:46, Wez wrote: > nit: Add a bug for that and refer to it here. Done. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:371: // have to get the actual value of the selection each time. On 2012/09/20 23:37:46, Wez wrote: > But this will only be triggered by an XFixes notification? Removed code. We no longer request the timestamp of the selection, so we no longer expect to get a timestamp response. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... File remoting/host/linux/x_server_clipboard.h (right): http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:38: // PRIMARY and CLIPBOARD selection. On 2012/09/20 23:37:46, Wez wrote: > nit: selections Done. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:43: // is not designed to be thread-safe, so all its methods should ideally be On 2012/09/20 23:37:46, Wez wrote: > Surely you mean "must", not "should ideally"? Done. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:47: // on one thread (per display). On 2012/09/20 23:37:46, Wez wrote: > This comment seems to imply that the caller might opt to split calls across > different threads but it's not clear to me what synchronization the caller would > need to do to make this safe, and since we don't intend to use it multi-threaded > - let's just state that this class isn't designed for multi-threaded use? You're right, no point in being long-winded about this :) Reworded. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:51: // Check the state of the X PRIMARY and CLIPBOARD selections and queues a On 2012/09/20 23:37:46, Wez wrote: > nit: queues -> queue Done. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:55: // CLIPBOARD), or None to check both the CLIPBOARD and PRIMARY selections. On 2012/09/20 23:37:46, Wez wrote: > nit: It looked to me like the implementation actually just checks PRIMARY if > None is supplied? Removed comment. This is just an event-handler now. http://codereview.chromium.org/10909133/diff/15001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:64: // is complete. On 2012/09/20 23:37:46, Wez wrote: > There's no mention of a timer in the comment for GetSelections, though? > > I'd suggest adding a line to the GetSelections() comment stating: > We ignore new GetSelections() requests while we're processing a previous > request, unless the earlier request appears to have stalled, e.g. the peer app > is not responding. > > Then change this comment to something like: > Called when the current GetSelections request completes, either having processed > new text, or having ignored the new content, e.g. because it's text we set > ourselves, or it's not text at all. Removed this method - it's now just one line of code, used in only two places. No point in having a method for this.
Can you deal with the comments on the other files that I left against patch-set #11 as well, please? http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... File remoting/host/linux/x_server_clipboard.cc (right): http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:120: if (have_xfixes_) { The monitoring code doesn't work if we don't have XFixes, since we'll never be notified of selection changes. How about we check for XFixes as the first thing we do in Init(), and early-exit if it's not available - then we simply check |clipboard_window_| in SetClipboard() and early-exit if it's not set, and we can get rid of the |have_fixes_| bool. , so why not check for XFixes first in Init() and if it's not present then http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:135: // quickly to our requests. We agreed it makes more sense to cancel any pending request operations, and ignore the resulting events, and dispatch new requests, if we see an event while we're already processing one - do you intend to do that in a separate CL? http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:142: if (selection != clipboard_atom_ && selection != XA_PRIMARY) { nit: Add comment "Only process PRIMARY and CLIPBOARD selections." http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:223: if (selection_event.target == targets_atom_) { nit: Add a comment to this block e.g. "Respond advertising XA_STRING, UTF8_STRING and timestamp data for the selection." http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:232: Time time = selection_own_time_[selection_event.selection]; nit: Comment "Respond with the timestamp of our selection; we always return CurrentTime since our selections are set by remote clients, so there is no associated local X event." http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:239: if (!data_.empty()) { nit: Comment "Return the actual string data. We always return UTF8, regardless of the configured locale." http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:253: void XServerClipboard::DoSelectionNotify(XSelectionEvent* event, nit: Shouldn't this be HandleSelectionNotify, since you're handling a SelectionNotify event, not "doing" one? http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:339: selection_own_time_[selection] = CurrentTime; This sets |selection_own_time_[selection]| to zero, so why bother keeping the map? Just return CurrentTime when responding to the timestamp request. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... File remoting/host/linux/x_server_clipboard.h (right): http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:34: // methods are called on this object. nit: Start this comment with an explanation of what Init does, e.g. "Start monitoring |display|'s selections and invoke |callback| whenever their content changes." http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:42: // loop. It should be invoked by the caller for each X11 event. This class nit: Rephrase the comment to describe what this call does, e.g. "Process |event| if it is an X selection notification. The caller must invoke this for every event it receives from |display|". http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:44: // main event-processing thread. nit: Move the comment on the class' threading to the class comment rather than this method. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:51: void OnSelectionRequest(XEvent* event); Add a comment to this block of methods, e.g. "Handlers for X selection events." http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:68: void* data); Add a comment before these methods indicating that they return true if selection processing is complete, false otherwise. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:73: bool IsSelectionOwner(Atom selection); nit: Move this near to AssertSelectionOwnership. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:74: void RequestSelectionTimestamp(Atom selection, Time time); Add a comment before the Request* methods indicating that they trigger the X server or selection owner to send us an event containing the requested information. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:74: void RequestSelectionTimestamp(Atom selection, Time time); nit: Remove RequestSelectionTimestamp?
http://codereview.chromium.org/10909133/diff/3002/remoting/host/chromoting_ho... File remoting/host/chromoting_host_context.cc (right): http://codereview.chromium.org/10909133/diff/3002/remoting/host/chromoting_ho... remoting/host/chromoting_host_context.cc:49: MessageLoop::TYPE_IO, 0)) && On 2012/09/17 22:13:29, Wez wrote: > nit: Since all of these threads are IO threads, create a single > io_thread_options to pass to StartWithOptions() Done. http://codereview.chromium.org/10909133/diff/3002/remoting/host/clipboard_lin... File remoting/host/clipboard_linux.cc (right): http://codereview.chromium.org/10909133/diff/3002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:18: // This code is expected to be used on the desktop thread only. On 2012/09/17 22:13:29, Wez wrote: > nit: "used" is vague; do we expect to be _called_ from the desktop thread? > What's special about the desktop thread from this class' perspective? Changed to "called". This comment was specifically added for Sergey's request in Patch #7. I don't think there's anything special about the desktop thread, except that it's the thread for EventExecutorLinux::task_runner_, where all injection-related tasks are posted. http://codereview.chromium.org/10909133/diff/3002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:27: scoped_ptr<protocol::ClipboardStub> client_clipboard) OVERRIDE; On 2012/09/17 22:13:29, Wez wrote: > nit: I think Start & Stop should be StartClipboard/StopClipboard to avoid > clashes if a Clipboard implementation needs to implement some other interface as > well. Agreed, but that should be a separate CL as it affects all platforms. http://codereview.chromium.org/10909133/diff/3002/remoting/host/clipboard_lin... remoting/host/clipboard_linux.cc:39: void PumpEvents(); On 2012/09/17 22:13:29, Wez wrote: > nit: PumpXEvents? Done. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... File remoting/host/linux/x_server_clipboard.cc (right): http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:120: if (have_xfixes_) { On 2012/09/25 20:29:29, Wez wrote: > The monitoring code doesn't work if we don't have XFixes, since we'll never be > notified of selection changes. How about we check for XFixes as the first thing > we do in Init(), and early-exit if it's not available - then we simply check > |clipboard_window_| in SetClipboard() and early-exit if it's not set, and we can > get rid of the |have_fixes_| bool. > > , so why not check for XFixes first in Init() and if it's not present then Done. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:135: // quickly to our requests. On 2012/09/25 20:29:29, Wez wrote: > We agreed it makes more sense to cancel any pending request operations, and > ignore the resulting events, and dispatch new requests, if we see an event while > we're already processing one - do you intend to do that in a separate CL? Added TODO. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:142: if (selection != clipboard_atom_ && selection != XA_PRIMARY) { On 2012/09/25 20:29:29, Wez wrote: > nit: Add comment "Only process PRIMARY and CLIPBOARD selections." Done. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:223: if (selection_event.target == targets_atom_) { On 2012/09/25 20:29:29, Wez wrote: > nit: Add a comment to this block e.g. "Respond advertising XA_STRING, > UTF8_STRING and timestamp data for the selection." Done. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:232: Time time = selection_own_time_[selection_event.selection]; On 2012/09/25 20:29:29, Wez wrote: > nit: Comment "Respond with the timestamp of our selection; we always return > CurrentTime since our selections are set by remote clients, so there is no > associated local X event." Done. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:239: if (!data_.empty()) { On 2012/09/25 20:29:29, Wez wrote: > nit: Comment "Return the actual string data. We always return UTF8, regardless > of the configured locale." Done. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:253: void XServerClipboard::DoSelectionNotify(XSelectionEvent* event, On 2012/09/25 20:29:29, Wez wrote: > nit: Shouldn't this be HandleSelectionNotify, since you're handling a > SelectionNotify event, not "doing" one? Done. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:339: selection_own_time_[selection] = CurrentTime; On 2012/09/25 20:29:29, Wez wrote: > This sets |selection_own_time_[selection]| to zero, so why bother keeping the > map? Just return CurrentTime when responding to the timestamp request. Done, and moved TODO to where we actually use CurrentTime. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... File remoting/host/linux/x_server_clipboard.h (right): http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:34: // methods are called on this object. On 2012/09/25 20:29:29, Wez wrote: > nit: Start this comment with an explanation of what Init does, e.g. "Start > monitoring |display|'s selections and invoke |callback| whenever their content > changes." Done. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:42: // loop. It should be invoked by the caller for each X11 event. This class On 2012/09/25 20:29:29, Wez wrote: > nit: Rephrase the comment to describe what this call does, e.g. "Process |event| > if it is an X selection notification. The caller must invoke this for every > event it receives from |display|". Done. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:44: // main event-processing thread. On 2012/09/25 20:29:29, Wez wrote: > nit: Move the comment on the class' threading to the class comment rather than > this method. Done. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:51: void OnSelectionRequest(XEvent* event); On 2012/09/25 20:29:29, Wez wrote: > Add a comment to this block of methods, e.g. "Handlers for X selection events." Done. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:68: void* data); On 2012/09/25 20:29:29, Wez wrote: > Add a comment before these methods indicating that they return true if selection > processing is complete, false otherwise. Done. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:73: bool IsSelectionOwner(Atom selection); On 2012/09/25 20:29:29, Wez wrote: > nit: Move this near to AssertSelectionOwnership. Done. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:74: void RequestSelectionTimestamp(Atom selection, Time time); On 2012/09/25 20:29:29, Wez wrote: > nit: Remove RequestSelectionTimestamp? Done. http://codereview.chromium.org/10909133/diff/22002/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.h:74: void RequestSelectionTimestamp(Atom selection, Time time); On 2012/09/25 20:29:29, Wez wrote: > Add a comment before the Request* methods indicating that they trigger the X > server or selection owner to send us an event containing the requested > information. Done.
lgtm http://codereview.chromium.org/10909133/diff/36001/remoting/host/linux/x_serv... File remoting/host/linux/x_server_clipboard.cc (right): http://codereview.chromium.org/10909133/diff/36001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:86: const std::string& data) { You need to check |clipboard_window_| here as well, I think? http://codereview.chromium.org/10909133/diff/36001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:101: if (clipboard_window_ == BadValue) { Is it possible for event->xany.window to be BadValue? If not then you don't need this. If you do need it, merge it into the if() below?
http://codereview.chromium.org/10909133/diff/36001/remoting/host/linux/x_serv... File remoting/host/linux/x_server_clipboard.cc (right): http://codereview.chromium.org/10909133/diff/36001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:86: const std::string& data) { On 2012/09/26 00:30:39, Wez wrote: > You need to check |clipboard_window_| here as well, I think? Done. http://codereview.chromium.org/10909133/diff/36001/remoting/host/linux/x_serv... remoting/host/linux/x_server_clipboard.cc:101: if (clipboard_window_ == BadValue) { On 2012/09/26 00:30:39, Wez wrote: > Is it possible for event->xany.window to be BadValue? If not then you don't > need this. If you do need it, merge it into the if() below? I don't know if it's possible, so I've merged the tests.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/10909133/3...
Retried try job too often for step(s) build
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/10909133/3...
Retried try job too often for step(s) build |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
