Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9264014:
Upstream the clipboard implementation for Android (Closed)
|
Created:
8 years, 11 months ago by Peter Beverloo Modified:
8 years, 4 months ago CC:
chromium-reviews, brettw-cc_chromium.org, John Grabowski, acw, tony, Yaron Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpstream the clipboard implementation for Android
BUG=
TEST=
Patch Set 1 #Patch Set 2 : Minor cleanup #
Total comments: 15
Patch Set 3 : Another clean-up #
Total comments: 37
Patch Set 4 : Partially addressed comments #Patch Set 5 : JNI comments #Patch Set 6 : All comments addressed. #
Total comments: 6
Patch Set 7 : Addressed dcheng's comments #
Total comments: 6
Patch Set 8 : Addressed comments. #Patch Set 9 : Address scoped JNI #Patch Set 10 : Fix typo in Clipboard::ReadHTML #Patch Set 11 : Fix typo in Clipboard::ReadHTML #Patch Set 12 : content_unit_tests already being helpful #Patch Set 13 : Change locks to use AutoLock (one exception) #
Total comments: 16
Patch Set 14 : Addressed comments. #
Messages
Total messages: 23 (0 generated)
lgtm for jni
+brettw for base/ OWNERS +sky for ui/base/ changes. Thank you, Steve! Brett and Scott, could you have a peek?
https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... File ui/base/clipboard/clipboard.h (right): https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... ui/base/clipboard/clipboard.h:79: int compare(const std::string& str) const { return data_.compare(str); } I think your cleanup ate a few lines. https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... File ui/base/clipboard/clipboard_android.cc (right): https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:82: if (context == NULL) { Nit: !context https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:245: delete [] ptr; Maybe just do: std::string data(text_data, text_len); jstring str = env->NewStringUTF(data.c_str()); Which avoids the manual allocation? https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:257: "content=\"text/html; charset=utf-8\">"; Since this is only used within the process anyway, you shouldn't need to prefix it with <meta> since you presumably know what encoding you used. https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:273: // how some code would consume this. This is probably called as a result of a right-click -> "Copy Image". It's not clear to me if this needs to be implemented on Android. https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:356: if (buffer != BUFFER_STANDARD) The standard style is to DCHECK_EQ() on platforms that can only have the standard buffer in all the Clipboard functions that take a buffer parameter (it seems like I forgot to do that myself in several cases). https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:375: // This is unimplemented on the other platforms (e.g. win, linux). This comment is no longer accurate. https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:470: // If ReadData is called for some text, just use the basic call I don't think you need to handle this case. This is only called with arbitrary formats.
https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... File ui/base/clipboard/clipboard.h (right): https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... ui/base/clipboard/clipboard.h:79: int compare(const std::string& str) const { return data_.compare(str); } On 2012/01/19 18:10:17, dcheng wrote: > I think your cleanup ate a few lines. Verified this doesn't compile; we only hit this conditional when USE_AURA is defined, which isn't true on OS_ANDROID.
https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... File ui/base/clipboard/clipboard.h (right): https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard.h:25: #include <jni.h> include jni.h first, then newline, then lock.h https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard.h:334: #if defined(OS_ANDROID) Shouldn't this be #elif? https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard.h:335: // Java class and methods for the Android ClipboardManager methods first, then fields. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... File ui/base/clipboard/clipboard_android.cc (right): https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:34: base::LazyInstance<base::Lock> g_clipboard_map_lock = LAZY_INSTANCE_INITIALIZER; Are we really using Clipboard on mutliple threads that requires a lock? https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:72: Clipboard::Clipboard() : clipboard_manager_(NULL), : should be on a newline. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:75: get_text_(NULL) { YOu didn't initialize to_string_ here. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:76: JNIEnv* env = AttachCurrentThread(); Is there a corresponding Detach? https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:85: jclass activity_thread_class = env->FindClass("android/app/ActivityThread"); How about a scoped object that does the DeleteLocalRef for you? https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:90: env->GetStaticMethodID(activity_thread_class, indent 4 here. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:105: env->GetStaticMethodID(looper_class, "prepareMainLooper", "()V"); indent 4 https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:136: jclass context_class = env->FindClass("android/content/Context"); Is it worth a single method that takes the class name and method id and does the deletelocalref for you? Seems like you do that a number of times here. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:140: env->GetMethodID(context_class, "getSystemService", indent 4 https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:184: g_clipboard_map_lock.Get().Acquire(); If you really need a lock, use AutoLock here and every where else instead of Acquire/Release directly. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:207: void Clipboard::ClearInternalClipboard() const { Order of method should match that of header. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:230: if (text_len < (sizeof(buff) - 1)) I don't think this is a performance critical section of code that warrants trying to optimize memory here. Go with the simpler code; use scoped_array all the time and avoid the stack buffer. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:267: Set(kWebKitSmartPasteFormat, std::string("")); No "", just std::string() https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:278: char* tmp = new char[total_size]; scoped_array https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:418: void Clipboard::ReadHTML(Clipboard::Buffer buffer, string16* markup, each param on its own line. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:454: memcpy(bmp.getPixels(), it->second.c_str(), it->second.length()); Does this really work? How does bmp know the size of the memory you're copying? Additionally when you write kBitmapFormat you write the width/height first, so don't you know the size so that you could config the bitmap?
Thank you for the comments so far! Not everything has been addressed yet, I'll reply with more answers shortly. https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... File ui/base/clipboard/clipboard.h (right): https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... ui/base/clipboard/clipboard.h:79: int compare(const std::string& str) const { return data_.compare(str); } On 2012/01/19 18:10:17, dcheng wrote: > I think your cleanup ate a few lines. Good catch. I actually meant to add OS_ANDROID to the conditional on l74 as well, as the interface (except for the compare() method) is shared. https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... ui/base/clipboard/clipboard.h:79: int compare(const std::string& str) const { return data_.compare(str); } On 2012/01/19 20:32:56, John Grabowski wrote: > On 2012/01/19 18:10:17, dcheng wrote: > > I think your cleanup ate a few lines. > > Verified this doesn't compile; we only hit this conditional when USE_AURA is > defined, which isn't true on OS_ANDROID. As noted, cleanup fail on my side. PS4 compiles and works well (will request trybots as well). https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... File ui/base/clipboard/clipboard_android.cc (right): https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:82: if (context == NULL) { On 2012/01/19 18:10:17, dcheng wrote: > Nit: !context Done. https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:245: delete [] ptr; On 2012/01/19 18:10:17, dcheng wrote: > Maybe just do: > std::string data(text_data, text_len); > jstring str = env->NewStringUTF(data.c_str()); > > Which avoids the manual allocation? That works perfectly, great suggestion. Thanks! https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:356: if (buffer != BUFFER_STANDARD) On 2012/01/19 18:10:17, dcheng wrote: > The standard style is to DCHECK_EQ() on platforms that can only have the > standard buffer in all the Clipboard functions that take a buffer parameter (it > seems like I forgot to do that myself in several cases). Done. https://chromiumcodereview.appspot.com/9264014/diff/4001/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:470: // If ReadData is called for some text, just use the basic call On 2012/01/19 18:10:17, dcheng wrote: > I don't think you need to handle this case. This is only called with arbitrary > formats. Removed the code. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... File ui/base/clipboard/clipboard.h (right): https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard.h:25: #include <jni.h> On 2012/01/19 21:26:56, sky wrote: > include jni.h first, then newline, then lock.h Done. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard.h:334: #if defined(OS_ANDROID) On 2012/01/19 21:26:56, sky wrote: > Shouldn't this be #elif? Done. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard.h:335: // Java class and methods for the Android ClipboardManager On 2012/01/19 21:26:56, sky wrote: > methods first, then fields. Done. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... File ui/base/clipboard/clipboard_android.cc (right): https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:72: Clipboard::Clipboard() : clipboard_manager_(NULL), On 2012/01/19 21:26:56, sky wrote: > : should be on a newline. Done. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:75: get_text_(NULL) { On 2012/01/19 21:26:56, sky wrote: > YOu didn't initialize to_string_ here. Done. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:90: env->GetStaticMethodID(activity_thread_class, On 2012/01/19 21:26:56, sky wrote: > indent 4 here. Done. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:105: env->GetStaticMethodID(looper_class, "prepareMainLooper", "()V"); On 2012/01/19 21:26:56, sky wrote: > indent 4 Done. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:140: env->GetMethodID(context_class, "getSystemService", On 2012/01/19 21:26:56, sky wrote: > indent 4 Done. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:207: void Clipboard::ClearInternalClipboard() const { On 2012/01/19 21:26:56, sky wrote: > Order of method should match that of header. Done. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:230: if (text_len < (sizeof(buff) - 1)) On 2012/01/19 21:26:56, sky wrote: > I don't think this is a performance critical section of code that warrants > trying to optimize memory here. Go with the simpler code; use scoped_array all > the time and avoid the stack buffer. Replaced with dcheng's std::string() suggestion. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:267: Set(kWebKitSmartPasteFormat, std::string("")); On 2012/01/19 21:26:56, sky wrote: > No "", just std::string() Done. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:278: char* tmp = new char[total_size]; On 2012/01/19 21:26:56, sky wrote: > scoped_array Done. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:418: void Clipboard::ReadHTML(Clipboard::Buffer buffer, string16* markup, On 2012/01/19 21:26:56, sky wrote: > each param on its own line. Done.
Addressed the JNI comments. Still a few pending. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... File ui/base/clipboard/clipboard_android.cc (right): https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:76: JNIEnv* env = AttachCurrentThread(); On 2012/01/19 21:26:56, sky wrote: > Is there a corresponding Detach? Yes, but a thread only needs to call this before it terminates, which is being done elsewhere (ThreadFunc() in platform_thread_posix.cc). In this case, AttachCurrentThread() is a convenient way to get a JNI Environment. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:85: jclass activity_thread_class = env->FindClass("android/app/ActivityThread"); On 2012/01/19 21:26:56, sky wrote: > How about a scoped object that does the DeleteLocalRef for you? Done. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:136: jclass context_class = env->FindClass("android/content/Context"); On 2012/01/19 21:26:56, sky wrote: > Is it worth a single method that takes the class name and method id and does the > deletelocalref for you? Seems like you do that a number of times here. Work on such utility methods (and more robust way of preventing references to leak) is being done downstream, but not ready yet for upstreaming.
sky, could you take another look please? Cheers! https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... File ui/base/clipboard/clipboard_android.cc (right): https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:34: base::LazyInstance<base::Lock> g_clipboard_map_lock = LAZY_INSTANCE_INITIALIZER; On 2012/01/19 21:26:56, sky wrote: > Are we really using Clipboard on mutliple threads that requires a lock? Added a TODO. https://chromiumcodereview.appspot.com/9264014/diff/5006/ui/base/clipboard/cl... ui/base/clipboard/clipboard_android.cc:454: memcpy(bmp.getPixels(), it->second.c_str(), it->second.length()); On 2012/01/19 21:26:56, sky wrote: > Does this really work? How does bmp know the size of the memory you're copying? > Additionally when you write kBitmapFormat you write the width/height first, so > don't you know the size so that you could config the bitmap? Stubbed out the method and added a call to NOTIMPLEMENTED.
Just minor nits from me. http://codereview.chromium.org/9264014/diff/20001/ui/base/clipboard/clipboard.h File ui/base/clipboard/clipboard.h (right): http://codereview.chromium.org/9264014/diff/20001/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard.h:26: #include "base/synchronization/lock.h" I think this can be moved to the .cc file. http://codereview.chromium.org/9264014/diff/20001/ui/base/clipboard/clipboard... File ui/base/clipboard/clipboard_android.cc (right): http://codereview.chromium.org/9264014/diff/20001/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:42: const char* const kPlainTextFormat = "text"; I recall that in the past, there was a slight preference for const char[] over const char* const, but I don't recall why. http://codereview.chromium.org/9264014/diff/20001/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:431: memcpy(p, (unsigned char*)&n, sizeof(int)); Are these casts necessary? It may be (slightly) safer to use sizeof(n) as well in case the size of n ever changes, though I guess it doesn't really matter.
Thanks. https://chromiumcodereview.appspot.com/9264014/diff/20001/ui/base/clipboard/c... File ui/base/clipboard/clipboard.h (right): https://chromiumcodereview.appspot.com/9264014/diff/20001/ui/base/clipboard/c... ui/base/clipboard/clipboard.h:26: #include "base/synchronization/lock.h" On 2012/02/01 19:02:04, dcheng wrote: > I think this can be moved to the .cc file. Done. https://chromiumcodereview.appspot.com/9264014/diff/20001/ui/base/clipboard/c... File ui/base/clipboard/clipboard_android.cc (right): https://chromiumcodereview.appspot.com/9264014/diff/20001/ui/base/clipboard/c... ui/base/clipboard/clipboard_android.cc:42: const char* const kPlainTextFormat = "text"; On 2012/02/01 19:02:04, dcheng wrote: > I recall that in the past, there was a slight preference for const char[] over > const char* const, but I don't recall why. Done. https://chromiumcodereview.appspot.com/9264014/diff/20001/ui/base/clipboard/c... ui/base/clipboard/clipboard_android.cc:431: memcpy(p, (unsigned char*)&n, sizeof(int)); On 2012/02/01 19:02:04, dcheng wrote: > Are these casts necessary? It may be (slightly) safer to use sizeof(n) as well > in case the size of n ever changes, though I guess it doesn't really matter. Done.
https://chromiumcodereview.appspot.com/9264014/diff/25002/ui/base/clipboard/c... File ui/base/clipboard/clipboard.h (right): https://chromiumcodereview.appspot.com/9264014/diff/25002/ui/base/clipboard/c... ui/base/clipboard/clipboard.h:73: #elif defined(USE_AURA) || defined(OS_ANDROID) Remove defined(OS_ANDROID) here, and instead create a new section around 85ish for OS_ANDROID. https://chromiumcodereview.appspot.com/9264014/diff/25002/ui/base/clipboard/c... ui/base/clipboard/clipboard.h:341: // Clear the Clipboard for all types. Both for Android and internal End sentence with '.' here and everywhere. https://chromiumcodereview.appspot.com/9264014/diff/25002/ui/base/clipboard/c... File ui/base/clipboard/clipboard_android.cc (right): https://chromiumcodereview.appspot.com/9264014/diff/25002/ui/base/clipboard/c... ui/base/clipboard/clipboard_android.cc:182: env->DeleteLocalRef(clipboard_class); Create scopying objects so that all this code isn't so fragile. See scoped_ptr for an example.
Cheers. https://chromiumcodereview.appspot.com/9264014/diff/25002/ui/base/clipboard/c... File ui/base/clipboard/clipboard.h (right): https://chromiumcodereview.appspot.com/9264014/diff/25002/ui/base/clipboard/c... ui/base/clipboard/clipboard.h:73: #elif defined(USE_AURA) || defined(OS_ANDROID) On 2012/02/03 17:14:35, sky wrote: > Remove defined(OS_ANDROID) here, and instead create a new section around 85ish > for OS_ANDROID. Done. https://chromiumcodereview.appspot.com/9264014/diff/25002/ui/base/clipboard/c... ui/base/clipboard/clipboard.h:341: // Clear the Clipboard for all types. Both for Android and internal On 2012/02/03 17:14:35, sky wrote: > End sentence with '.' here and everywhere. Done. https://chromiumcodereview.appspot.com/9264014/diff/25002/ui/base/clipboard/c... File ui/base/clipboard/clipboard_android.cc (right): https://chromiumcodereview.appspot.com/9264014/diff/25002/ui/base/clipboard/c... ui/base/clipboard/clipboard_android.cc:182: env->DeleteLocalRef(clipboard_class); On 2012/02/03 17:14:35, sky wrote: > Create scopying objects so that all this code isn't so fragile. See scoped_ptr > for an example. This is under review downstream: https://gerrit-int.chromium.org/#change,10917 and will be upstreamed afterwards.
On Tue, Feb 7, 2012 at 4:25 AM, <peter@chromium.org> wrote: > Cheers. > > > > https://chromiumcodereview.appspot.com/9264014/diff/25002/ui/base/clipboard/c... > File ui/base/clipboard/clipboard.h (right): > > https://chromiumcodereview.appspot.com/9264014/diff/25002/ui/base/clipboard/c... > ui/base/clipboard/clipboard.h:73: #elif defined(USE_AURA) || > defined(OS_ANDROID) > On 2012/02/03 17:14:35, sky wrote: >> >> Remove defined(OS_ANDROID) here, and instead create a new section > > around 85ish >> >> for OS_ANDROID. > > > Done. > > > https://chromiumcodereview.appspot.com/9264014/diff/25002/ui/base/clipboard/c... > ui/base/clipboard/clipboard.h:341: // Clear the Clipboard for all types. > Both for Android and internal > On 2012/02/03 17:14:35, sky wrote: >> >> End sentence with '.' here and everywhere. > > > Done. > > > https://chromiumcodereview.appspot.com/9264014/diff/25002/ui/base/clipboard/c... > File ui/base/clipboard/clipboard_android.cc (right): > > https://chromiumcodereview.appspot.com/9264014/diff/25002/ui/base/clipboard/c... > ui/base/clipboard/clipboard_android.cc:182: > env->DeleteLocalRef(clipboard_class); > On 2012/02/03 17:14:35, sky wrote: >> >> Create scopying objects so that all this code isn't so fragile. See > > scoped_ptr >> >> for an example. > > > This is under review downstream: > https://gerrit-int.chromium.org/#change,10917 and will be upstreamed > afterwards. I'm going to hold off until this lands, otherwise it's too painful for me to make sure you properly release all references. Additionally I can't do an adequate review until you figure out the threading story. -Scott
On 2012/02/07 16:54:32, sky wrote: > I'm going to hold off until this lands, otherwise it's too painful for > me to make sure you properly release all references. Additionally I > can't do an adequate review until you figure out the threading story. > > -Scott The patch landed for Chromium in r121284, and clipboard_android.cc has been updated with the scoped objects in PS11. The threading issue remains then. I talked to dcheng@ about threading of the clipboard operations; writing will be done on the UI thread, while reading will take place on the IO thread. Exceptions to this rule have been made for Windows and X11 in ClipboardMessageFilter. Two options would be to (1) Change ClipboardMessageFilter::OverrideThreadForMessage to deliver all clipboard messages to a certain thread (probably IO) (2) Use locks to control access to the map, which is implemented now. This should be updated to use AutoLocks. John (or anyone else), do you have an opinion on this? satish@ is working on getting content_unit_tests, to work which uses a parts of this code.
On 2012/02/14 00:41:09, Peter Beverloo wrote: > On 2012/02/07 16:54:32, sky wrote: > > I'm going to hold off until this lands, otherwise it's too painful for > > me to make sure you properly release all references. Additionally I > > can't do an adequate review until you figure out the threading story. > > > > -Scott > > The patch landed for Chromium in r121284, and clipboard_android.cc has been > updated with the scoped objects in PS11. The threading issue remains then. > > I talked to dcheng@ about threading of the clipboard operations; writing will be > done on the UI thread, while reading will take place on the IO thread. > Exceptions to this rule have been made for Windows and X11 in > ClipboardMessageFilter. > > Two options would be to > (1) Change ClipboardMessageFilter::OverrideThreadForMessage to deliver all > clipboard messages to a certain thread (probably IO) You'll need to update the two call sites in clipboard_message_filter.cc that dispatch a call to WriteObjectsHelper on the UI thread as well in this case. However, searching around more, there are also several locations in the code that use ScopedClipboardWriter directly (and not ScopedClipboardWriterGlue). Some of these happen on the UI thread (such as in browser_url_util.cc) and some probably happen off the UI thread (such as the bookmark extension function implementations). Given this, it's probably much safer (and easier) to just use locks. It's kind of silly, since we really won't ever have "overlapping" clipboard operations but I think we need it to guarantee correctness. At some point in the future, someone (me?) should add some asserts to ScopedClipboardWriter to enforce thread requirements... > (2) Use locks to control access to the map, which is implemented now. This > should be updated to use AutoLocks. > > John (or anyone else), do you have an opinion on this? > > satish@ is working on getting content_unit_tests, to work which uses a parts of > this code.
Hmm. I would have leant in the other direction and switched the UI thread to post a task (uh, Closure) to the IO thread so all operations in the clipboard happen on the IO thread. Else we potentially block the UI thread on the IO thread. In practice it is unlikely to contend but the jihad against jank is righteous. Posting a task isn't hard at all. jrg On Mon, Feb 13, 2012 at 4:52 PM, <dcheng@chromium.org> wrote: > On 2012/02/14 00:41:09, Peter Beverloo wrote: > >> On 2012/02/07 16:54:32, sky wrote: >> > I'm going to hold off until this lands, otherwise it's too painful for >> > me to make sure you properly release all references. Additionally I >> > can't do an adequate review until you figure out the threading story. >> > >> > -Scott >> > > The patch landed for Chromium in r121284, and clipboard_android.cc has >> been >> updated with the scoped objects in PS11. The threading issue remains then. >> > > I talked to dcheng@ about threading of the clipboard operations; writing >> will >> > be > >> done on the UI thread, while reading will take place on the IO thread. >> Exceptions to this rule have been made for Windows and X11 in >> ClipboardMessageFilter. >> > > Two options would be to >> (1) Change ClipboardMessageFilter::**OverrideThreadForMessage to >> deliver all >> clipboard messages to a certain thread (probably IO) >> > > You'll need to update the two call sites in clipboard_message_filter.cc > that > dispatch a call to WriteObjectsHelper on the UI thread as well in this > case. > > However, searching around more, there are also several locations in the > code > that use ScopedClipboardWriter directly (and not > ScopedClipboardWriterGlue). > Some of these happen on the UI thread (such as in browser_url_util.cc) and > some > probably happen off the UI thread (such as the bookmark extension function > implementations). > > Given this, it's probably much safer (and easier) to just use locks. It's > kind > of silly, since we really won't ever have "overlapping" clipboard > operations but > I think we need it to guarantee correctness. At some point in the future, > someone (me?) should add some asserts to ScopedClipboardWriter to enforce > thread > requirements... > > > (2) Use locks to control access to the map, which is implemented now. >> This >> should be updated to use AutoLocks. >> > > John (or anyone else), do you have an opinion on this? >> > > satish@ is working on getting content_unit_tests, to work which uses a >> parts >> > of > >> this code. >> > > > https://chromiumcodereview.**appspot.com/9264014/<https://chromiumcodereview.... >
On 2012/02/14 01:33:41, John Grabowski wrote: > Hmm. I would have leant in the other direction and switched the UI thread > to post a task (uh, Closure) to the IO thread so all operations in the > clipboard happen on the IO thread. Else we potentially block the UI thread > on the IO thread. In practice it is unlikely to contend but the jihad > against jank is righteous. > > Posting a task isn't hard at all. > > jrg It might be OK to only do this for certain platforms (e.g. Android, maybe Aura X11 as well since it maintains its own view of the clipboard). However, on Windows, Mac, and GTK, we're *supposed* to only write to the clipboard from the UI thread--though this rule isn't enforced. It's possible this is a restriction we can drop for Windows/Mac, but it will require someone to test and make sure nothing breaks.
> > > It might be OK to only do this for certain platforms (e.g. Android, maybe > Aura > X11 as well since it maintains its own view of the clipboard). However, on > Windows, Mac, and GTK, we're *supposed* to only write to the clipboard > from the > UI thread--though this rule isn't enforced. It's possible this is a > restriction > we can drop for Windows/Mac, but it will require someone to test and make > sure > nothing breaks. > > That is quite insightful. In context, then, it seems like locks are better to be consistent. And we should document the restrictions in code if not already there. https://chromiumcodereview.**appspot.com/9264014/<https://chromiumcodereview....
On Mon, Feb 13, 2012 at 4:41 PM, <peter@chromium.org> wrote: > On 2012/02/07 16:54:32, sky wrote: >> >> I'm going to hold off until this lands, otherwise it's too painful for >> me to make sure you properly release all references. Additionally I >> can't do an adequate review until you figure out the threading story. > > >> -Scott > > > The patch landed for Chromium in r121284, and clipboard_android.cc has been > updated with the scoped objects in PS11. The threading issue remains then. > > I talked to dcheng@ about threading of the clipboard operations; writing > will be > done on the UI thread, while reading will take place on the IO thread. > Exceptions to this rule have been made for Windows and X11 in > ClipboardMessageFilter. If we need to do reading on a different thread can we take a snapshot of the data and send it to said thread? That way we don't have to worry about locking. -Scott > Two options would be to > (1) Change ClipboardMessageFilter::OverrideThreadForMessage to deliver all > clipboard messages to a certain thread (probably IO) > (2) Use locks to control access to the map, which is implemented now. This > should be updated to use AutoLocks. > > John (or anyone else), do you have an opinion on this? > > satish@ is working on getting content_unit_tests, to work which uses a parts > of > this code. > > https://chromiumcodereview.appspot.com/9264014/
On 2012/02/14 04:31:43, sky wrote: > If we need to do reading on a different thread can we take a snapshot > of the data and send it to said thread? That way we don't have to > worry about locking. > > -Scott I've updated the latest version of this patch to use AutoLocks (with one exception) which you suggested earlier. Right now the locks make sure that reading and writing don't occur simultaneously, and while it's not ideal it is a workable solution for now. Can we file an issue with a TODO (either replacing or superseding the one on line 40) to figure out the more optimal situation? This patch would allow us to build several new testing targets (also covering this code), i.e. content_unittests and gfx_unittests (and possibly media_).
https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... File ui/base/clipboard/clipboard.h (right): https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard.h:87: const std::string& ToString() const { return data_; } This should be data() https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard.h:357: jmethodID set_text_; How comes these aren't some scoping object? Don't they need to be cleaned up when the clipboard is deleted? https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... File ui/base/clipboard/clipboard_android.cc (right): https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard_android.cc:25: // Global contents map and its lock. This comment isn't applicable here. https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard_android.cc:83: JNIEnv* env = AttachCurrentThread(); Do you need to detach at some point? https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard_android.cc:393: memcpy(p, &n, sizeof(n)); use sizeof(int) to match total_size above. https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard_android.cc:420: g_clipboard_map_lock.Get().Acquire(); This is fragile. Use autolock with braces around 420 to 428. https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard_android.cc:430: if (IsTextAvailableFromAndroid()) { What happens if g_clipboard_map gets mutated after this?
Thank you for the review! https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... File ui/base/clipboard/clipboard.h (right): https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard.h:87: const std::string& ToString() const { return data_; } On 2012/02/16 17:06:44, sky wrote: > This should be data() data_ actually contains the name of the format type, not the actual data itself (see uses of the FormatType constructor), so that would be wrong, wouldn't it? https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard.h:357: jmethodID set_text_; On 2012/02/16 17:06:44, sky wrote: > How comes these aren't some scoping object? Don't they need to be cleaned up > when the clipboard is deleted? No. The jmethodID int does not have to be released after it's been used, it's more like a weak reference. https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... File ui/base/clipboard/clipboard_android.cc (right): https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard_android.cc:25: // Global contents map and its lock. On 2012/02/16 17:06:44, sky wrote: > This comment isn't applicable here. Done. https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard_android.cc:83: JNIEnv* env = AttachCurrentThread(); On 2012/02/16 17:06:44, sky wrote: > Do you need to detach at some point? Yes, just before the thread exists, which is being done in platform_thread_posix.cc. Attaching multiple times does not have any side-effects. https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard_android.cc:393: memcpy(p, &n, sizeof(n)); On 2012/02/16 17:06:44, sky wrote: > use sizeof(int) to match total_size above. Done. https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard_android.cc:420: g_clipboard_map_lock.Get().Acquire(); On 2012/02/16 17:06:44, sky wrote: > This is fragile. Use autolock with braces around 420 to 428. Done. https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard_android.cc:430: if (IsTextAvailableFromAndroid()) { On 2012/02/16 17:06:44, sky wrote: > What happens if g_clipboard_map gets mutated after this? You're right, thanks, that could result in the internal_text being freed up. An issue here is that ClearInternalClipboard() creates a new lock, while we'd ideally be in a lock already to avoid the race. I could call g_clipboard_map->clear() on line 442 and 447 instead and placing an AutoLock on the whole method, but that'd be sensitive to being overseen if CIC() ever changes (even though g_clipboard_map is already being used directly in several other places). Alternatively we could add ClearInternalClipboardNoLock. Do you have a preference here?
https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... File ui/base/clipboard/clipboard.h (right): https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard.h:87: const std::string& ToString() const { return data_; } On 2012/02/16 20:38:40, Peter Beverloo wrote: > On 2012/02/16 17:06:44, sky wrote: > > This should be data() > > data_ actually contains the name of the format type, not the actual data itself > (see uses of the FormatType constructor), so that would be wrong, wouldn't it? The style guide says if you're returning a field, the name should match the field name in unix_hacker_style. https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... File ui/base/clipboard/clipboard_android.cc (right): https://chromiumcodereview.appspot.com/9264014/diff/38003/ui/base/clipboard/c... ui/base/clipboard/clipboard_android.cc:430: if (IsTextAvailableFromAndroid()) { On 2012/02/16 20:38:40, Peter Beverloo wrote: > On 2012/02/16 17:06:44, sky wrote: > > What happens if g_clipboard_map gets mutated after this? > > You're right, thanks, that could result in the internal_text being freed up. > An issue here is that ClearInternalClipboard() creates a new lock, while we'd > ideally be in a lock already to avoid the race. > I could call g_clipboard_map->clear() on line 442 and 447 instead and placing an > AutoLock on the whole method, but that'd be sensitive to being overseen if CIC() > ever changes (even though g_clipboard_map is already being used directly in > several other places). Alternatively we could add ClearInternalClipboardNoLock. > Do you have a preference here? This is why I preferred simplifying the threading. If the other thread only needs to read, can't it be passed a copy so we don't need to worry about locking? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
