Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
59023007:
Enable pasting HTML content from the Android clipboard (Closed)
|
Created:
7 years, 1 month ago by Kristian Monsen Modified:
7 years ago CC:
chromium-reviews, dcheng Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnable pasting HTML content from the Android clipboard
BUG=316717
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238530
Patch Set 1 #
Total comments: 10
Patch Set 2 : Removed the diff due to changes upstream #Patch Set 3 : Using coerceToHtmlText #
Total comments: 3
Patch Set 4 : Don't use coerceToHtmlText #
Total comments: 1
Patch Set 5 : Don't get an empty string during androidSync #Patch Set 6 : rebase #
Total comments: 4
Patch Set 7 : clearing html entry if not there on the Java side #Patch Set 8 : Updating tests to match how Android works #
Total comments: 2
Patch Set 9 : Fixing more tests #Patch Set 10 : Fixing tests on pre jb devices #Patch Set 11 : Remove unneeded function call #Patch Set 12 : Upload *all* the files #
Messages
Total messages: 27 (0 generated)
Test are coming, but opening the code review since we need this yesterday donwstream. (I feel like I am back in the darkpod btw.)
https://codereview.chromium.org/59023007/diff/1/ui/android/java/src/org/chrom... File ui/android/java/src/org/chromium/ui/Clipboard.java (right): https://codereview.chromium.org/59023007/diff/1/ui/android/java/src/org/chrom... ui/android/java/src/org/chromium/ui/Clipboard.java:90: return clip.getItemAt(0).getHtmlText(); It looks like coerceToHtmlText handles a few additional cases: http://developer.android.com/reference/android/content/ClipData.Item.html#coe... e.g. a content: URI or intent: that this clipboard represents. should we use that instead? Might need to ask around for concrete examples of clipboard item suppliers that may use those mechanisms. https://codereview.chromium.org/59023007/diff/1/ui/base/clipboard/clipboard_a... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/59023007/diff/1/ui/base/clipboard/clipboard_a... ui/base/clipboard/clipboard_android.cc:131: // in the Android clipboard, clear the map and insert the Android text into it. this comment sounds out of date (well, incomplete) https://codereview.chromium.org/59023007/diff/1/ui/base/clipboard/clipboard_a... ui/base/clipboard/clipboard_android.cc:143: if (it == map_.end() || it->second != android_string) { the change from hasPlainText() method to the code you have here makes me nervous, mostly because this has no automated tests and I don't know what the manual test cases would be for this. Suggestions welcome on how to ease my nerves. https://codereview.chromium.org/59023007/diff/1/ui/base/clipboard/clipboard_a... ui/base/clipboard/clipboard_android.cc:160: map_[kHTMLFormat] = ConvertJavaStringToUTF8(java_string_html); the memory overhead of stashing two copies of everything in the clipboard in a global map seems a bit messy (the long term memory cost). If user copies a massive HTML doc from one app's webview into another, both processes have 2 x massive doc in RAM until they each do another clipboard operation. probably the best answer is to wire this thing up to base::MemoryPressureListener (e.g. to clear the map on the moderate level). maybe a new crbug for that? https://codereview.chromium.org/59023007/diff/1/ui/base/clipboard/clipboard_a... ui/base/clipboard/clipboard_android.cc:201: DCHECK_EQ(buffer, BUFFER_STANDARD); are all these edits from here down intended? look like M30 / master versions skew mess.
https://codereview.chromium.org/59023007/diff/1/ui/base/clipboard/clipboard_a... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/59023007/diff/1/ui/base/clipboard/clipboard_a... ui/base/clipboard/clipboard_android.cc:201: DCHECK_EQ(buffer, BUFFER_STANDARD); On 2013/11/12 01:45:39, joth wrote: > are all these edits from here down intended? look like M30 / master versions > skew mess. No, need a manual merge here. Will fix.
https://codereview.chromium.org/59023007/diff/1/ui/android/java/src/org/chrom... File ui/android/java/src/org/chromium/ui/Clipboard.java (right): https://codereview.chromium.org/59023007/diff/1/ui/android/java/src/org/chrom... ui/android/java/src/org/chromium/ui/Clipboard.java:90: return clip.getItemAt(0).getHtmlText(); On 2013/11/12 01:45:39, joth wrote: > It looks like coerceToHtmlText handles a few additional cases: > http://developer.android.com/reference/android/content/ClipData.Item.html#coe...) > > e.g. a content: URI or intent: that this clipboard represents. > > should we use that instead? > > Might need to ask around for concrete examples of clipboard item suppliers that > may use those mechanisms. Done. https://codereview.chromium.org/59023007/diff/1/ui/base/clipboard/clipboard_a... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/59023007/diff/1/ui/base/clipboard/clipboard_a... ui/base/clipboard/clipboard_android.cc:131: // in the Android clipboard, clear the map and insert the Android text into it. On 2013/11/12 01:45:39, joth wrote: > this comment sounds out of date (well, incomplete) Done. https://codereview.chromium.org/59023007/diff/1/ui/base/clipboard/clipboard_a... ui/base/clipboard/clipboard_android.cc:143: if (it == map_.end() || it->second != android_string) { On 2013/11/12 01:45:39, joth wrote: > the change from hasPlainText() method to the code you have here makes me > nervous, mostly because this has no automated tests and I don't know what the > manual test cases would be for this. > > Suggestions welcome on how to ease my nerves. I mostly did not want to quit when there was not plain text in case there was html text. That being said, hasPlainText needed to get the text and check if it was null, so I don't think it optimized anything, but did it twice instead. The if (java_string_text.obj()) should do the same. Also not that the hasPlainText was potentially not safe in case someone messes with the clipboard in between. There is not really that much code change here, this code is mostly the same as the one git thinks was deleted just below. I just mixed it up a bit since we don't want to return before adding the html text. https://codereview.chromium.org/59023007/diff/1/ui/base/clipboard/clipboard_a... ui/base/clipboard/clipboard_android.cc:160: map_[kHTMLFormat] = ConvertJavaStringToUTF8(java_string_html); On 2013/11/12 01:45:39, joth wrote: > the memory overhead of stashing two copies of everything in the clipboard in a > global map seems a bit messy (the long term memory cost). > If user copies a massive HTML doc from one app's webview into another, both > processes have 2 x massive doc in RAM until they each do another clipboard > operation. > > probably the best answer is to wire this thing up to > base::MemoryPressureListener (e.g. to clear the map on the moderate level). > maybe a new crbug for that? We don't really need the map as I understand, can just query android when needed. But that is not for this CL.
basically lgtm, would be good to have some clipboard knowledgeable persons look at it too. https://codereview.chromium.org/59023007/diff/100001/ui/base/clipboard/clipbo... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/59023007/diff/100001/ui/base/clipboard/clipbo... ui/base/clipboard/clipboard_android.cc:162: map_[kHTMLFormat] = ConvertJavaStringToUTF8(java_string_html); as the java side now uses coerce, whenever the primary clip has plain text then this will also return non-null for HTML text. Is that a problem? (e.g. Does chrome paste the kPlainTextFormat clipboard in a different way to the kHTMLFormat clip, and the former path will now be effectively unreachable with this change?)
BTW, I feel the basic clipboard.h header is ready for some refactoring. Lots of #ifdefs there makes the code more complicated than it needs to be. I don't think this code is performance critical so some platform specific subclasses would be nicer. Also to save memory, and have less code, this class could always query the Android clipboard. That would kill HTML pasting pre JB though, so maybe not time for it yet. https://codereview.chromium.org/59023007/diff/100001/ui/base/clipboard/clipbo... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/59023007/diff/100001/ui/base/clipboard/clipbo... ui/base/clipboard/clipboard_android.cc:162: map_[kHTMLFormat] = ConvertJavaStringToUTF8(java_string_html); On 2013/11/12 04:50:29, joth wrote: > as the java side now uses coerce, whenever the primary clip has plain text then > this will also return non-null for HTML text. Is that a problem? (e.g. Does > chrome paste the kPlainTextFormat clipboard in a different way to the > kHTMLFormat clip, and the former path will now be effectively unreachable with > this change?) I don't know, hopefully one of the other reviewers will :-) You are right that this will always be set now, sometimes with a plain text string.
LGTM Should there be a new test case in clipboard_unittest.cc? https://codereview.chromium.org/59023007/diff/100001/ui/base/clipboard/clipbo... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/59023007/diff/100001/ui/base/clipboard/clipbo... ui/base/clipboard/clipboard_android.cc:162: map_[kHTMLFormat] = ConvertJavaStringToUTF8(java_string_html); On 2013/11/12 06:08:06, Kristian Monsen wrote: > On 2013/11/12 04:50:29, joth wrote: > > as the java side now uses coerce, whenever the primary clip has plain text > then > > this will also return non-null for HTML text. Is that a problem? (e.g. Does > > chrome paste the kPlainTextFormat clipboard in a different way to the > > kHTMLFormat clip, and the former path will now be effectively unreachable with > > this change?) > > I don't know, hopefully one of the other reviewers will :-) > > You are right that this will always be set now, sometimes with a plain text > string. AFAIK, you won't be making some other code path unreachable, but I think it's a bit weird to put plain text into the html clipboard. I don't think it breaks anything though.
We decided to not use coerceToHtmlText downstream as that seems to be the safest fix for now. Will do the same here to have them match.
https://codereview.chromium.org/59023007/diff/170001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/Clipboard.java (right): https://codereview.chromium.org/59023007/diff/170001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/Clipboard.java:90: return clip.getItemAt(0).getHtmlText(); want to comment that coerce would be better (as it also captures content: and intent clip types) but we'd rather avoid promoting plain text promoted to html. (the bigger fix here would be remove the native map<> and use ClipboardManager.addPrimaryClipChangedListener() to implement GetSequenceNumber. SelectionChangeObserver in the gtk version gives a good example)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kristianm@chromium.org/59023007/330001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kristianm@chromium.org/59023007/330001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
bulach: need owners lgtm
Yaron: Need an owner lgtm for the java file
yfriedman->newt
https://codereview.chromium.org/59023007/diff/330001/ui/base/clipboard/clipbo... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/59023007/diff/330001/ui/base/clipboard/clipbo... ui/base/clipboard/clipboard_android.cc:163: map_[kHTMLFormat] = ConvertJavaStringToUTF8(java_string_html); what if map_ contains an HTML clipboard entry, but there's no HTML clipboard entry on the Android/Java side? Do we need to remove the HTML clipboard entry from map_ in this case?
https://codereview.chromium.org/59023007/diff/330001/ui/base/clipboard/clipbo... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/59023007/diff/330001/ui/base/clipboard/clipbo... ui/base/clipboard/clipboard_android.cc:163: map_[kHTMLFormat] = ConvertJavaStringToUTF8(java_string_html); On 2013/11/26 18:36:14, newt wrote: > what if map_ contains an HTML clipboard entry, but there's no HTML clipboard > entry on the Android/Java side? Do we need to remove the HTML clipboard entry > from map_ in this case? Updated to do that, and check if the HTML string is not empty.
Will try to debug this for a bit on the trybots as my local checkout is stuck with SDK 18 and not compiling. https://codereview.chromium.org/59023007/diff/330001/ui/base/clipboard/clipbo... File ui/base/clipboard/clipboard_android.cc (left): https://codereview.chromium.org/59023007/diff/330001/ui/base/clipboard/clipbo... ui/base/clipboard/clipboard_android.cc:156: map_.clear(); This should probably just erase the text entry? https://codereview.chromium.org/59023007/diff/330001/ui/base/clipboard/clipbo... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/59023007/diff/330001/ui/base/clipboard/clipbo... ui/base/clipboard/clipboard_android.cc:163: map_[kHTMLFormat] = ConvertJavaStringToUTF8(java_string_html); On 2013/11/26 22:47:39, Kristian Monsen wrote: > On 2013/11/26 18:36:14, newt wrote: > > what if map_ contains an HTML clipboard entry, but there's no HTML clipboard > > entry on the Android/Java side? Do we need to remove the HTML clipboard entry > > from map_ in this case? > > Updated to do that, and check if the HTML string is not empty. For slightly complicated reasons we can't do both this and pass unit tests. We don't write to the Android clipboard if there is not a plain text entry as well. When we then sync to Android before retrieving a result in the unit test the current entry will get nuked. We are also slightly dodging another bullet, see my comment below.
just a nit below.. the try bots don't look too happy though? failing ui_unittests.. https://codereview.chromium.org/59023007/diff/420001/ui/base/clipboard/clipbo... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/59023007/diff/420001/ui/base/clipboard/clipbo... ui/base/clipboard/clipboard_android.cc:165: map_[kHTMLFormat] = ConvertJavaStringToUTF8(java_string_html); nit: s/ConvertJavaStringToUTF8(java_string_html)/android_string/
Tests still fail on pre KK devices, but all pas on N5 now. Will dig up an older device and do some testing there. Testing on the bots only cover code that is running on older devices. I guess that is OK for now.
All tests passing, and nit fixed :-) PTAL, still need one owners lgtm. https://codereview.chromium.org/59023007/diff/420001/ui/base/clipboard/clipbo... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/59023007/diff/420001/ui/base/clipboard/clipbo... ui/base/clipboard/clipboard_android.cc:165: map_[kHTMLFormat] = ConvertJavaStringToUTF8(java_string_html); On 2013/11/27 16:26:38, bulach wrote: > nit: s/ConvertJavaStringToUTF8(java_string_html)/android_string/ Done.
lgtm, thanks!!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kristianm@chromium.org/59023007/500001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kristianm@chromium.org/59023007/500001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kristianm@chromium.org/59023007/500001
Message was sent while issue was closed.
Change committed as 238530 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
