Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10381115:
[Chromoting] The Windows IT2Me host gets any new text items it finds on the clipboard. (Closed)
|
Created:
8 years, 7 months ago by simonmorris Modified:
8 years, 7 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, dcheng, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Chromoting] The Windows IT2Me host gets any new text items it finds on the clipboard.
A follow-up CL will send those items to the client.
The approach used in this CL will do nothing on OSes earlier than Vista.
BUG=117473
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137455
Patch Set 1 #
Total comments: 41
Patch Set 2 : Reviews. #
Total comments: 8
Patch Set 3 : Review. #
Total comments: 9
Patch Set 4 : Move ClipboardWin to the UI thread, and tidy up. #
Total comments: 10
Patch Set 5 : Review. #Patch Set 6 : Sync. #
Messages
Total messages: 20 (0 generated)
ptal sergeyu@ -> chromoting_host_context.[c|h] alexeypa@ -> everything else
http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc File remoting/host/clipboard_win.cc (right): http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:81: HANDLE GetData(UINT uFormat) { u_format or just format. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:95: class ScopedGlobalLock { Can you use ScopedHGlobalfrom base/win? http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:230: CREATESTRUCT* cs = reinterpret_cast<CREATESTRUCT*>(lParam); avar or a_var instead of aVar. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:296: string16 text_temp(text_lock.GetPointer()); You can just use assign instead of swapping.
http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc File remoting/host/clipboard_win.cc (right): http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:81: HANDLE GetData(UINT uFormat) { |uFormat| -> |format| http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:97: ScopedGlobalLock(HGLOBAL hGlobalMem) : hGlobalMem_(hGlobalMem) { explicit http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:97: ScopedGlobalLock(HGLOBAL hGlobalMem) : hGlobalMem_(hGlobalMem) { hGlobalMem -> global_mem. Same thing for the data member. I don't think we use Camel notation elsewhere even if we deal with Win32 APIs. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:98: pointer_ = ::GlobalLock(hGlobalMem_); Validate that the handle is valid. The check should be in the Release build too. I guess... http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:107: T GetPointer() { This is the only place where T is needed. WDYT about making this only method templated but not the whole class? http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:112: HGLOBAL hGlobalMem_; Make it non-copyable. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:138: void OnClipboardUpdate(); nit: I tend to put static members last. I'm not sure what the style guide says about it but it feels like the static members are looked at less often. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:167: ::GetProcAddress(::GetModuleHandle(L"user32.dll"), GetModuleHandle will increment the library reference counter. Not fatal in this case but generally bad. You can use GetModuleHandleEx to avoid incrementing the counter. You can also group two GetProcAddress calls and cache the pointers in the class because GetProcAddress calls are expensive. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:174: LOG(WARNING) << "AddClipboardFormatListener() couldn't be loaded."; This will pollute logs. We should complain in the logs only if the OS version is less than Vista. PS. And if this is the case - we should probably CHECK instead. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:185: (*remove)(hwnd_); This should be called only if the Add function was called before. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:236: ClipboardWin* clipboardWin = |clipboard_win|, or, maybe, simply |clipboard| http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:269: if (!hwnd_) { Make it a DCHECK. I don't think there is a way to call it with |hwnd_| being NULL. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:297: text.swap(text_temp); I don't think text.swap buys anything here. text can be assigned directly. I.e. text.assign(...) should do.
http://codereview.chromium.org/10381115/diff/1/remoting/host/chromoting_host_... File remoting/host/chromoting_host_context.cc (right): http://codereview.chromium.org/10381115/diff/1/remoting/host/chromoting_host_... remoting/host/chromoting_host_context.cc:34: base::Thread::Options(MessageLoop::TYPE_UI, 0)) && On some platforms (mac and linux) it might be unsafe to start second UI thread. AFAIK, on Mac only the main thread can be used for UI. On linux some glib/gtk API's may not be thread-safe (e.g. gconf is not threadsafe). I'm not sure we really need desktop_thread separate from ui_thread, so maybe we could use the ui_thread everywhere we currently use desktop_thread?
http://codereview.chromium.org/10381115/diff/1/remoting/host/chromoting_host_... File remoting/host/chromoting_host_context.cc (right): http://codereview.chromium.org/10381115/diff/1/remoting/host/chromoting_host_... remoting/host/chromoting_host_context.cc:34: base::Thread::Options(MessageLoop::TYPE_UI, 0)) && On 2012/05/11 21:10:11, sergeyu wrote: > On some platforms (mac and linux) it might be unsafe to start second UI thread. > AFAIK, on Mac only the main thread can be used for UI. On linux some glib/gtk > API's may not be thread-safe (e.g. gconf is not threadsafe). > > I'm not sure we really need desktop_thread separate from ui_thread, so maybe we > could use the ui_thread everywhere we currently use desktop_thread? Sounds good to me. But it'd be better to consult more widely on that question, so I'll leave this CL until Monday. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc File remoting/host/clipboard_win.cc (right): http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:81: HANDLE GetData(UINT uFormat) { On 2012/05/11 20:28:31, dcheng wrote: > u_format or just format. Done. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:81: HANDLE GetData(UINT uFormat) { On 2012/05/11 20:42:37, alexeypa wrote: > |uFormat| -> |format| Done. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:95: class ScopedGlobalLock { On 2012/05/11 20:28:31, dcheng wrote: > Can you use ScopedHGlobalfrom base/win? Done. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:97: ScopedGlobalLock(HGLOBAL hGlobalMem) : hGlobalMem_(hGlobalMem) { On 2012/05/11 20:42:37, alexeypa wrote: > explicit Removed code. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:97: ScopedGlobalLock(HGLOBAL hGlobalMem) : hGlobalMem_(hGlobalMem) { On 2012/05/11 20:42:37, alexeypa wrote: > hGlobalMem -> global_mem. Same thing for the data member. I don't think we use > Camel notation elsewhere even if we deal with Win32 APIs. Removed code. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:98: pointer_ = ::GlobalLock(hGlobalMem_); On 2012/05/11 20:42:37, alexeypa wrote: > Validate that the handle is valid. The check should be in the Release build too. > I guess... Removed code. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:107: T GetPointer() { On 2012/05/11 20:42:37, alexeypa wrote: > This is the only place where T is needed. WDYT about making this only method > templated but not the whole class? Removed code. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:112: HGLOBAL hGlobalMem_; On 2012/05/11 20:42:37, alexeypa wrote: > Make it non-copyable. Removed code. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:138: void OnClipboardUpdate(); On 2012/05/11 20:42:37, alexeypa wrote: > nit: I tend to put static members last. I'm not sure what the style guide says > about it but it feels like the static members are looked at less often. Done. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:167: ::GetProcAddress(::GetModuleHandle(L"user32.dll"), On 2012/05/11 20:42:37, alexeypa wrote: > GetModuleHandle will increment the library reference counter. Not fatal in this > case but generally bad. You can use GetModuleHandleEx to avoid incrementing the > counter. > <http://msdn.microsoft.com/en-us/library/windows/desktop/ms683199(v=vs.85).aspx> says GetModuleHandle doesn't increment the reference counter. > You can also group two GetProcAddress calls and cache the pointers in the class > because GetProcAddress calls are expensive. > Done. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:174: LOG(WARNING) << "AddClipboardFormatListener() couldn't be loaded."; On 2012/05/11 20:42:37, alexeypa wrote: > This will pollute logs. We should complain in the logs only if the OS version is > less than Vista. > Done (assuming you meant "greater than Vista".) > PS. And if this is the case - we should probably CHECK instead. The user might not have cared about clipboard functionality... http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:185: (*remove)(hwnd_); On 2012/05/11 20:42:37, alexeypa wrote: > This should be called only if the Add function was called before. Done. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:230: CREATESTRUCT* cs = reinterpret_cast<CREATESTRUCT*>(lParam); On 2012/05/11 20:28:31, dcheng wrote: > avar or a_var instead of aVar. Done. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:236: ClipboardWin* clipboardWin = On 2012/05/11 20:42:37, alexeypa wrote: > |clipboard_win|, or, maybe, simply |clipboard| Done. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:269: if (!hwnd_) { On 2012/05/11 20:42:37, alexeypa wrote: > Make it a DCHECK. I don't think there is a way to call it with |hwnd_| being > NULL. Done. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:296: string16 text_temp(text_lock.GetPointer()); On 2012/05/11 20:28:31, dcheng wrote: > You can just use assign instead of swapping. Done. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:297: text.swap(text_temp); On 2012/05/11 20:42:37, alexeypa wrote: > I don't think text.swap buys anything here. text can be assigned directly. I.e. > text.assign(...) should do. Done.
LGTM for remoting/host/clipboard_win.cc http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc File remoting/host/clipboard_win.cc (right): http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:167: ::GetProcAddress(::GetModuleHandle(L"user32.dll"), On 2012/05/11 22:12:58, simonmorris wrote: > GetModuleHandle doesn't increment the reference counter. Oops. Yes, indeed. It is actually GetModuleHandleEx who can increment the counter. http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:174: LOG(WARNING) << "AddClipboardFormatListener() couldn't be loaded."; On 2012/05/11 22:12:58, simonmorris wrote: > (assuming you meant "greater than Vista".) Yes, I meant that. > > PS. And if this is the case - we should probably CHECK instead. > The user might not have cared about clipboard functionality... It does not matter. It is the same sort of sanity check as AV-ing on a NULL pointer is. It means that something went terribly wrong and now we don't know what we are doing. http://codereview.chromium.org/10381115/diff/5/remoting/host/clipboard_win.cc File remoting/host/clipboard_win.cc (right): http://codereview.chromium.org/10381115/diff/5/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:136: HMODULE user32_module = ::GetModuleHandle(L"user32.dll"); nit: Presubmit check used to frown on L"" literals. A workaround I used else where was using the TO_L_STRING macro from base/stringize_macros.h. I don't think we should change it it the presubmit check was fixed since than. http://codereview.chromium.org/10381115/diff/5/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:136: HMODULE user32_module = ::GetModuleHandle(L"user32.dll"); nit: I think it make sense to check if GetModuleHandle returned NULL. It is highly unlikely of course and, I think, GetProcAddress should handle NULL handle just fine but there is still a chance that I'm wrong and GetProcessAdress will AV crashing the process. http://codereview.chromium.org/10381115/diff/5/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:148: if (!remove_clipboard_format_listener_) { nit: It is highly unlikely, but it is better to reset both pointers if one of them is NULL. We can also have a single log message for both functions. http://codereview.chromium.org/10381115/diff/5/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:179: if (add_clipboard_format_listener_ && remove_clipboard_format_listener_) { nit: This check will be simpler if you make sure that both pointers either set or not.
http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc File remoting/host/clipboard_win.cc (right): http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:174: LOG(WARNING) << "AddClipboardFormatListener() couldn't be loaded."; On 2012/05/11 23:49:17, alexeypa wrote: > On 2012/05/11 22:12:58, simonmorris wrote: > > > PS. And if this is the case - we should probably CHECK instead. > > The user might not have cared about clipboard functionality... > > It does not matter. It is the same sort of sanity check as AV-ing on a NULL > pointer is. It means that something went terribly wrong and now we don't know > what we are doing. > I don't agree. It means something went surprisingly wrong, and we know exactly what we are doing - not listening for items placed on the clipboard. I don't think some missing functionality is a good reason to crash the entire host. http://codereview.chromium.org/10381115/diff/5/remoting/host/clipboard_win.cc File remoting/host/clipboard_win.cc (right): http://codereview.chromium.org/10381115/diff/5/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:136: HMODULE user32_module = ::GetModuleHandle(L"user32.dll"); On 2012/05/11 23:49:17, alexeypa wrote: > nit: Presubmit check used to frown on L"" literals. A workaround I used else > where was using the TO_L_STRING macro from base/stringize_macros.h. > > I don't think we should change it it the presubmit check was fixed since than. Presubmit doesn't complain, and this seems to be the most common style in Chromium. http://codereview.chromium.org/10381115/diff/5/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:136: HMODULE user32_module = ::GetModuleHandle(L"user32.dll"); On 2012/05/11 23:49:17, alexeypa wrote: > nit: I think it make sense to check if GetModuleHandle returned NULL. It is > highly unlikely of course and, I think, GetProcAddress should handle NULL handle > just fine but there is still a chance that I'm wrong and GetProcessAdress will > AV crashing the process. Done. http://codereview.chromium.org/10381115/diff/5/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:148: if (!remove_clipboard_format_listener_) { On 2012/05/11 23:49:17, alexeypa wrote: > nit: It is highly unlikely, but it is better to reset both pointers if one of > them is NULL. We can also have a single log message for both functions. Done. http://codereview.chromium.org/10381115/diff/5/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:179: if (add_clipboard_format_listener_ && remove_clipboard_format_listener_) { On 2012/05/11 23:49:17, alexeypa wrote: > nit: This check will be simpler if you make sure that both pointers either set > or not. Done.
ping
On 2012/05/15 00:29:30, simonmorris wrote: > ping Can you please address my concern about multiple UI threads?
On 2012/05/15 00:40:59, sergeyu wrote: > On 2012/05/15 00:29:30, simonmorris wrote: > > ping > > Can you please address my concern about multiple UI threads? Yes, the ping is partly for wez@, to whom I've forwarded the suggestion that we use ui_thread_ instead of desktop_thread_, and partly for alexeypa@.
http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc File remoting/host/clipboard_win.cc (right): http://codereview.chromium.org/10381115/diff/1/remoting/host/clipboard_win.cc... remoting/host/clipboard_win.cc:174: LOG(WARNING) << "AddClipboardFormatListener() couldn't be loaded."; On 2012/05/12 00:16:56, simonmorris wrote: > I don't agree. It means something went surprisingly wrong, and we know exactly > what we are doing - not listening for items placed on the clipboard. I don't > think some missing functionality is a good reason to crash the entire host. Yes, I agree with this part. I was concerned about that while we know what we are doing we don't know what is going wrong. I can think of three possibilities: the OS installation is corrupted; some one is injected into our process, we survived the ice age and the API is being deprecated. There can be other reasons. I'm not entirely convinced that this particular "something is very wrong" is worth of killing the whole process, so if you think it is fine to keep it alive - I'm OK with it.
http://codereview.chromium.org/10381115/diff/1/remoting/host/chromoting_host_... File remoting/host/chromoting_host_context.cc (right): http://codereview.chromium.org/10381115/diff/1/remoting/host/chromoting_host_... remoting/host/chromoting_host_context.cc:34: base::Thread::Options(MessageLoop::TYPE_UI, 0)) && On 2012/05/11 22:12:58, simonmorris wrote: > On 2012/05/11 21:10:11, sergeyu wrote: > > On some platforms (mac and linux) it might be unsafe to start second UI > thread. > > AFAIK, on Mac only the main thread can be used for UI. On linux some glib/gtk > > API's may not be thread-safe (e.g. gconf is not threadsafe). > > > > I'm not sure we really need desktop_thread separate from ui_thread, so maybe > we > > could use the ui_thread everywhere we currently use desktop_thread? > > Sounds good to me. But it'd be better to consult more widely on that question, > so I'll leave this CL until Monday. As Sergey points out, we can only have multiple UI threads on Windows, so we can't make desktop_thread_ a UI thread. We run captures on a non-UI thread so that they don't impact activity on the UI thread, which is most relevant in the NPAPI plugin, where the UI thread is the plugin main thread. In practice it's probably not an issue, but we'd need to do some testing to confirm that. In principle the desktop integration threading should be separate from ChromotingHost threading, though - the integration components should be provided with the UI thread in case they need it, and otherwise create any threads they need themselves. For this CL, can the clipboard handler trap change notifications on the UI thread and hand them off to the desktop thread?
http://codereview.chromium.org/10381115/diff/5002/remoting/host/clipboard_win.cc File remoting/host/clipboard_win.cc (right): http://codereview.chromium.org/10381115/diff/5002/remoting/host/clipboard_win... remoting/host/clipboard_win.cc:136: HMODULE user32_module = ::GetModuleHandle(L"user32.dll"); Consider using base::LoadNativeLibrary/GetFunctionPointerFromNativeLibrary? http://codereview.chromium.org/10381115/diff/5002/remoting/host/clipboard_win... remoting/host/clipboard_win.cc:146: // If we can't load both functions, then store neither. Rather than NULLing these here, consider a HaveClipboardListenerApis() helper and checking that rather than testing the function ptrs directly. http://codereview.chromium.org/10381115/diff/5002/remoting/host/clipboard_win... remoting/host/clipboard_win.cc:153: if (base::win::GetVersion() >= base::win::VERSION_VISTA) { nit: I'd either not bother logging a warning at all, or log it regardless of OS version - clipboard won't work if we're pre-Vista, so the warning is equally valid. http://codereview.chromium.org/10381115/diff/5002/remoting/host/clipboard_win... remoting/host/clipboard_win.cc:234: string16 text; nit: Move this after the comment, before the {?
PTAL ClipboardWin now runs on the UI thread. http://codereview.chromium.org/10381115/diff/5002/remoting/host/clipboard_win.cc File remoting/host/clipboard_win.cc (right): http://codereview.chromium.org/10381115/diff/5002/remoting/host/clipboard_win... remoting/host/clipboard_win.cc:136: HMODULE user32_module = ::GetModuleHandle(L"user32.dll"); On 2012/05/15 18:50:07, Wez wrote: > Consider using base::LoadNativeLibrary/GetFunctionPointerFromNativeLibrary? Those try to load a library, whereas ::GetModuleHandle expects the module to be loaded already, which is more accurate, and avoids any expectation that user32.dll should be unloaded. http://codereview.chromium.org/10381115/diff/5002/remoting/host/clipboard_win... remoting/host/clipboard_win.cc:146: // If we can't load both functions, then store neither. On 2012/05/15 18:50:07, Wez wrote: > Rather than NULLing these here, consider a HaveClipboardListenerApis() helper > and checking that rather than testing the function ptrs directly. Done. http://codereview.chromium.org/10381115/diff/5002/remoting/host/clipboard_win... remoting/host/clipboard_win.cc:153: if (base::win::GetVersion() >= base::win::VERSION_VISTA) { On 2012/05/15 18:50:07, Wez wrote: > nit: I'd either not bother logging a warning at all, or log it regardless of OS > version - clipboard won't work if we're pre-Vista, so the warning is equally > valid. Done. http://codereview.chromium.org/10381115/diff/5002/remoting/host/clipboard_win... remoting/host/clipboard_win.cc:234: string16 text; On 2012/05/15 18:50:07, Wez wrote: > nit: Move this after the comment, before the {? But then the comment would be separated from the code to which it applies.
http://codereview.chromium.org/10381115/diff/5002/remoting/host/clipboard_win.cc File remoting/host/clipboard_win.cc (right): http://codereview.chromium.org/10381115/diff/5002/remoting/host/clipboard_win... remoting/host/clipboard_win.cc:136: HMODULE user32_module = ::GetModuleHandle(L"user32.dll"); On 2012/05/15 21:47:48, simonmorris wrote: > Those try to load a library, whereas ::GetModuleHandle expects the module to be > loaded already, which is more accurate, and avoids any expectation that > user32.dll should be unloaded. +1
http://codereview.chromium.org/10381115/diff/4003/remoting/host/clipboard.h File remoting/host/clipboard.h (right): http://codereview.chromium.org/10381115/diff/4003/remoting/host/clipboard.h#n... remoting/host/clipboard.h:23: // This method must be called on the UI thread. Perhaps move this comment and its friends to a single comment on the class, explaining that Clipboard implementations typically need to run on a UI thread to get change notifications? http://codereview.chromium.org/10381115/diff/4003/remoting/host/event_executor.h File remoting/host/event_executor.h (right): http://codereview.chromium.org/10381115/diff/4003/remoting/host/event_executo... remoting/host/event_executor.h:24: static scoped_ptr<EventExecutor> Create(MessageLoop* message_loop, Now that we're passing two message loops, the comment on the ctor should explain that most stuff should happen on |message_loop|, and |ui_loop| is just used when we really need a UI loop. :) http://codereview.chromium.org/10381115/diff/4003/remoting/host/event_executo... File remoting/host/event_executor_win.cc (right): http://codereview.chromium.org/10381115/diff/4003/remoting/host/event_executo... remoting/host/event_executor_win.cc:76: ui_loop_->PostTask( You could test if (!ui_loop_->BelongsToCurrentThread()) here to avoid the need for a separate HandleClipboard() member. http://codereview.chromium.org/10381115/diff/4003/remoting/host/event_executo... remoting/host/event_executor_win.cc:79: base::Unretained(this), Doesn't this mean we risk a crash if a clipboard event comes in just as we're tearing down the EventExecutor? http://codereview.chromium.org/10381115/diff/4003/remoting/host/event_executo... remoting/host/event_executor_win.cc:108: ui_loop_->PostTask( As above, with an if (BelongsToCurrentThread()) test, couldn't you get rid of the separate HandleSessionStarted()?
http://codereview.chromium.org/10381115/diff/4003/remoting/host/clipboard.h File remoting/host/clipboard.h (right): http://codereview.chromium.org/10381115/diff/4003/remoting/host/clipboard.h#n... remoting/host/clipboard.h:23: // This method must be called on the UI thread. On 2012/05/15 23:46:19, Wez wrote: > Perhaps move this comment and its friends to a single comment on the class, > explaining that Clipboard implementations typically need to run on a UI thread > to get change notifications? Done. http://codereview.chromium.org/10381115/diff/4003/remoting/host/event_executor.h File remoting/host/event_executor.h (right): http://codereview.chromium.org/10381115/diff/4003/remoting/host/event_executo... remoting/host/event_executor.h:24: static scoped_ptr<EventExecutor> Create(MessageLoop* message_loop, On 2012/05/15 23:46:19, Wez wrote: > Now that we're passing two message loops, the comment on the ctor should explain > that most stuff should happen on |message_loop|, and |ui_loop| is just used when > we really need a UI loop. :) Done. http://codereview.chromium.org/10381115/diff/4003/remoting/host/event_executo... File remoting/host/event_executor_win.cc (right): http://codereview.chromium.org/10381115/diff/4003/remoting/host/event_executo... remoting/host/event_executor_win.cc:76: ui_loop_->PostTask( On 2012/05/15 23:46:19, Wez wrote: > You could test if (!ui_loop_->BelongsToCurrentThread()) here to avoid the need > for a separate HandleClipboard() member. Done. (I liked the consistency of having a separate HandleXXX, but I'm in the minority). http://codereview.chromium.org/10381115/diff/4003/remoting/host/event_executo... remoting/host/event_executor_win.cc:79: base::Unretained(this), On 2012/05/15 23:46:19, Wez wrote: > Doesn't this mean we risk a crash if a clipboard event comes in just as we're > tearing down the EventExecutor? No more than with InjectKeyEvent() and InjectMouseEvent(). The DesktopEnvironment and EventExecutor outlive the host, so I don't think there's a problem. http://codereview.chromium.org/10381115/diff/4003/remoting/host/event_executo... remoting/host/event_executor_win.cc:108: ui_loop_->PostTask( On 2012/05/15 23:46:19, Wez wrote: > As above, with an if (BelongsToCurrentThread()) test, couldn't you get rid of > the separate HandleSessionStarted()? Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10381115/7012
Change committed as 137455 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
