Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10695051:
Upstream the Android clipboard implementation (Closed)
|
Created:
8 years, 5 months ago by cjhopman Modified:
8 years, 5 months ago CC:
chromium-reviews, dcheng, John Grabowski, Peter Beverloo Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionUpstream the Android clipboard implementation
Previous discussion on upstreaming Android clipboard is at:
http://codereview.chromium.org/9264014/
The main difference here is that synchronization has been pulled into a
separate class to make it simpler/clearer (and more correct). This also
simplifies a great deal of the logic.
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145798
Patch Set 1 #
Total comments: 18
Patch Set 2 : #Patch Set 3 : #
Total comments: 24
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Messages
Total messages: 13 (0 generated)
CC: jrg@, peter@ as you two were involved in the previous discussion
http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... File ui/base/clipboard/clipboard_android.cc (right): http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:43: public: 1-space indent for public/private http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:44: ClipboardMap(); 2 spaces, etc. http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:107: return !Get(format).empty(); Consider using ContainsKey() from base/stl_util.h http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:142: std::string internal_string = map_[kPlainTextFormat]; You could use a const ref here to avoid a copy. http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:251: *fragment_end = static_cast<uint32>(input.length()); This should be markup->length() after the conversion. http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:350: static const char* html_prefix = "<meta http-equiv=\"content-type\" " Do you actually need this? Since only Android Chrome can see this string, and we (presumably) always know the encoding, we shouldn't need to set this <meta> tag. http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:370: g_map.Get().Set(kWebKitSmartPasteFormat, std::string("")); std::string() instead of std::string("")
http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... File ui/base/clipboard/clipboard_android.cc (right): http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:43: public: On 2012/06/30 03:28:42, dcheng wrote: > 1-space indent for public/private Done. http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:44: ClipboardMap(); On 2012/06/30 03:28:42, dcheng wrote: > 2 spaces, etc. Done. http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:107: return !Get(format).empty(); On 2012/06/30 03:28:42, dcheng wrote: > Consider using ContainsKey() from base/stl_util.h Added a comment to clarify. In addition, in Get() we may insert a "" for a key that we don't have data for, and so we can't just check if the map contains the key. I'm not sure if I prefer this to using find()/!=end()/etc in Get(). http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:142: std::string internal_string = map_[kPlainTextFormat]; On 2012/06/30 03:28:42, dcheng wrote: > You could use a const ref here to avoid a copy. Done. http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:251: *fragment_end = static_cast<uint32>(input.length()); On 2012/06/30 03:28:42, dcheng wrote: > This should be markup->length() after the conversion. Done. http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:350: static const char* html_prefix = "<meta http-equiv=\"content-type\" " On 2012/06/30 03:28:42, dcheng wrote: > Do you actually need this? Since only Android Chrome can see this string, and we > (presumably) always know the encoding, we shouldn't need to set this <meta> tag. Done. http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:370: g_map.Get().Set(kWebKitSmartPasteFormat, std::string("")); On 2012/06/30 03:28:42, dcheng wrote: > std::string() instead of std::string("") Done.
http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... File ui/base/clipboard/clipboard_android.cc (right): http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:107: return !Get(format).empty(); On 2012/07/02 17:29:11, cjhopman wrote: > On 2012/06/30 03:28:42, dcheng wrote: > > Consider using ContainsKey() from base/stl_util.h > > Added a comment to clarify. In addition, in Get() we may insert a "" for a key > that we don't have data for, and so we can't just check if the map contains the > key. I'm not sure if I prefer this to using find()/!=end()/etc in Get(). We should use find(). Get() shouldn't mutate the map. Then Get() and HasFormat() can be made const, and this method can use the ContainsKey() helper.
http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... File ui/base/clipboard/clipboard_android.cc (right): http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:107: return !Get(format).empty(); On 2012/07/02 20:22:30, dcheng wrote: > On 2012/07/02 17:29:11, cjhopman wrote: > > On 2012/06/30 03:28:42, dcheng wrote: > > > Consider using ContainsKey() from base/stl_util.h > > > > Added a comment to clarify. In addition, in Get() we may insert a "" for a key > > that we don't have data for, and so we can't just check if the map contains > the > > key. I'm not sure if I prefer this to using find()/!=end()/etc in Get(). > > We should use find(). Get() shouldn't mutate the map. Then Get() and HasFormat() > can be made const, and this method can use the ContainsKey() helper. Currently, the Validate() call can also mutate the map to sync it with the state of the Android clipboard. This allows the Validate call to have the postcondition that map_ is in sync and that map_[kPlainTextFormat] == [[Android's clipboard content]]. This simplifies the logic in all the other functions (get/set/clear/has). Logically, Get and HasFormat are not actually mutating the map (the mutation logically occurs when the Android clipboard was changed). Three possible next steps: 1. Leave it as is. 2. Change Get() to use find() and Has() to use ContainsKey but both still Validate and sync the map_. (optionally make map_ mutable and the logically const functions const) 3. Make Validate not mutate map_, and then the same for Get/Has and mark these functions const. Also, I think Validate could be renamed to something more appropriate like SyncWithAndroidClipboard.
http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... File ui/base/clipboard/clipboard_android.cc (right): http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:107: return !Get(format).empty(); On 2012/07/02 21:00:49, cjhopman wrote: > On 2012/07/02 20:22:30, dcheng wrote: > > On 2012/07/02 17:29:11, cjhopman wrote: > > > On 2012/06/30 03:28:42, dcheng wrote: > > > > Consider using ContainsKey() from base/stl_util.h > > > > > > Added a comment to clarify. In addition, in Get() we may insert a "" for a > key > > > that we don't have data for, and so we can't just check if the map contains > > the > > > key. I'm not sure if I prefer this to using find()/!=end()/etc in Get(). > > > > We should use find(). Get() shouldn't mutate the map. Then Get() and > HasFormat() > > can be made const, and this method can use the ContainsKey() helper. > > Currently, the Validate() call can also mutate the map to sync it with the state > of the Android clipboard. This allows the Validate call to have the > postcondition that map_ is in sync and that map_[kPlainTextFormat] == > [[Android's clipboard content]]. This simplifies the logic in all the other > functions (get/set/clear/has). Logically, Get and HasFormat are not actually > mutating the map (the mutation logically occurs when the Android clipboard was > changed). > > Three possible next steps: > 1. Leave it as is. > 2. Change Get() to use find() and Has() to use ContainsKey but both still > Validate and sync the map_. (optionally make map_ mutable and the logically > const functions const) > 3. Make Validate not mutate map_, and then the same for Get/Has and mark these > functions const. > > Also, I think Validate could be renamed to something more appropriate like > SyncWithAndroidClipboard. Oh right, I forgot about Validate(). I would prefer option #2 and just leave the const-ness alone.
http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... File ui/base/clipboard/clipboard_android.cc (right): http://codereview.chromium.org/10695051/diff/1/ui/base/clipboard/clipboard_an... ui/base/clipboard/clipboard_android.cc:107: return !Get(format).empty(); On 2012/07/02 21:09:52, dcheng wrote: > On 2012/07/02 21:00:49, cjhopman wrote: > > On 2012/07/02 20:22:30, dcheng wrote: > > > On 2012/07/02 17:29:11, cjhopman wrote: > > > > On 2012/06/30 03:28:42, dcheng wrote: > > > > > Consider using ContainsKey() from base/stl_util.h > > > > > > > > Added a comment to clarify. In addition, in Get() we may insert a "" for a > > key > > > > that we don't have data for, and so we can't just check if the map > contains > > > the > > > > key. I'm not sure if I prefer this to using find()/!=end()/etc in Get(). > > > > > > We should use find(). Get() shouldn't mutate the map. Then Get() and > > HasFormat() > > > can be made const, and this method can use the ContainsKey() helper. > > > > Currently, the Validate() call can also mutate the map to sync it with the > state > > of the Android clipboard. This allows the Validate call to have the > > postcondition that map_ is in sync and that map_[kPlainTextFormat] == > > [[Android's clipboard content]]. This simplifies the logic in all the other > > functions (get/set/clear/has). Logically, Get and HasFormat are not actually > > mutating the map (the mutation logically occurs when the Android clipboard was > > changed). > > > > Three possible next steps: > > 1. Leave it as is. > > 2. Change Get() to use find() and Has() to use ContainsKey but both still > > Validate and sync the map_. (optionally make map_ mutable and the logically > > const functions const) > > 3. Make Validate not mutate map_, and then the same for Get/Has and mark these > > functions const. > > > > Also, I think Validate could be renamed to something more appropriate like > > SyncWithAndroidClipboard. > > Oh right, I forgot about Validate(). I would prefer option #2 and just leave the > const-ness alone. Done. Required small change to Validate/Sync since we no longer treat an empty string the same as no string.
http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... File ui/base/clipboard/clipboard_android.cc (right): http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:108: return it == map_.end() ? "" : it->second; Minor nit: "" -> std::string() http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:199: void Clipboard::WriteObjects(Buffer buffer, const ObjectMap& objects) { DCHECK_EQ(buffer, BUFFER_STANDARD); http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:240: void Clipboard::ReadText(Clipboard::Buffer buffer, string16* result) const { DCHECK_EQ(buffer, BUFFER_STANDARD); http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:257: uint32* fragment_end) const { DCHECK_EQ(buffer, BUFFER_STANDARD); http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:272: SkBitmap Clipboard::ReadImage(Buffer buffer) const { DCHECK_EQ(...); http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:278: const gfx::Size* size = reinterpret_cast<const gfx::Size*>(input.c_str()); Prefer data() instead of c_str() here because of \0's may be embedded. http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:287: memcpy(bmp.getPixels(), input.c_str() + sizeof(gfx::Size), bm_size); Same. http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:303: std::string* result) const { DCHECK_EQ(...); http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... File ui/base/clipboard/clipboard_unittest.cc (right): http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_unittest.cc:661: unsigned int fake_bitmap[] = { const unsigned int kFakeBitmap http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_unittest.cc:682: DCHECK(env); ASSERT instead of DCHECK in unit tests. http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_unittest.cc:724: EXPECT_TRUE(!contents.compare(new_value)); EXPECT_EQ() might read a bit more naturally.
http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... File ui/base/clipboard/clipboard_android.cc (right): http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:108: return it == map_.end() ? "" : it->second; On 2012/07/02 22:50:19, dcheng wrote: > Minor nit: > "" -> std::string() Done. http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:199: void Clipboard::WriteObjects(Buffer buffer, const ObjectMap& objects) { On 2012/07/02 22:50:19, dcheng wrote: > DCHECK_EQ(buffer, BUFFER_STANDARD); Done. http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:240: void Clipboard::ReadText(Clipboard::Buffer buffer, string16* result) const { On 2012/07/02 22:50:19, dcheng wrote: > DCHECK_EQ(buffer, BUFFER_STANDARD); Done. http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:257: uint32* fragment_end) const { On 2012/07/02 22:50:19, dcheng wrote: > DCHECK_EQ(buffer, BUFFER_STANDARD); Done. http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:272: SkBitmap Clipboard::ReadImage(Buffer buffer) const { On 2012/07/02 22:50:19, dcheng wrote: > DCHECK_EQ(...); Done. http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:278: const gfx::Size* size = reinterpret_cast<const gfx::Size*>(input.c_str()); On 2012/07/02 22:50:19, dcheng wrote: > Prefer data() instead of c_str() here because of \0's may be embedded. Done. http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:287: memcpy(bmp.getPixels(), input.c_str() + sizeof(gfx::Size), bm_size); On 2012/07/02 22:50:19, dcheng wrote: > Same. Done. http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:303: std::string* result) const { On 2012/07/02 22:50:19, dcheng wrote: > DCHECK_EQ(...); Neither ReadData nor ReadBookmark take a buffer argument. http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... File ui/base/clipboard/clipboard_unittest.cc (right): http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_unittest.cc:661: unsigned int fake_bitmap[] = { On 2012/07/02 22:50:19, dcheng wrote: > const unsigned int kFakeBitmap unsigned int fake_bitmap is consistent with the rest of the file, should I still change it? http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_unittest.cc:682: DCHECK(env); On 2012/07/02 22:50:19, dcheng wrote: > ASSERT instead of DCHECK in unit tests. Done. http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_unittest.cc:724: EXPECT_TRUE(!contents.compare(new_value)); On 2012/07/02 22:50:19, dcheng wrote: > EXPECT_EQ() might read a bit more naturally. Done.
LGTM with the following addressed. http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... File ui/base/clipboard/clipboard_unittest.cc (right): http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_unittest.cc:661: unsigned int fake_bitmap[] = { On 2012/07/02 23:26:51, cjhopman wrote: > On 2012/07/02 22:50:19, dcheng wrote: > > const unsigned int kFakeBitmap > > unsigned int fake_bitmap is consistent with the rest of the file, should I still > change it? Hm. Well... how about fixing this one, and I'll fix the others at a later date to follow the style? Sorry about the inconsistency. http://codereview.chromium.org/10695051/diff/1006/ui/base/clipboard/clipboard... File ui/base/clipboard/clipboard_android.cc (right): http://codereview.chromium.org/10695051/diff/1006/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:384: g_map.Get().Set(kWebKitSmartPasteFormat, ""); std::string() here as well.
http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... File ui/base/clipboard/clipboard_unittest.cc (right): http://codereview.chromium.org/10695051/diff/4005/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_unittest.cc:661: unsigned int fake_bitmap[] = { On 2012/07/02 23:40:26, dcheng wrote: > On 2012/07/02 23:26:51, cjhopman wrote: > > On 2012/07/02 22:50:19, dcheng wrote: > > > const unsigned int kFakeBitmap > > > > unsigned int fake_bitmap is consistent with the rest of the file, should I > still > > change it? > > Hm. Well... how about fixing this one, and I'll fix the others at a later date > to follow the style? Sorry about the inconsistency. Done. http://codereview.chromium.org/10695051/diff/1006/ui/base/clipboard/clipboard... File ui/base/clipboard/clipboard_android.cc (right): http://codereview.chromium.org/10695051/diff/1006/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_android.cc:384: g_map.Get().Set(kWebKitSmartPasteFormat, ""); On 2012/07/02 23:40:26, dcheng wrote: > std::string() here as well. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cjhopman@chromium.org/10695051/1008
Change committed as 145798 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
