Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2812933003:
Support paste with style from Spanned (Closed)
|
Created:
3 years, 8 months ago by Shimi Zhang Modified:
3 years, 8 months ago Reviewers:
Theresa CC:
chromium-reviews, agrieve+watch_chromium.org, sgurun-gerrit only Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport paste with style from Spanned
Only coerce text to HTML when needed, since replace getHtmlText with coerceToHtmlText will break two bookmark tests.
1. if text is HTML.
2. if text is plain text and has style (Spanned).
BUG=709549
Review-Url: https://codereview.chromium.org/2812933003
Cr-Commit-Position: refs/heads/master@{#464584}
Committed: https://chromium.googlesource.com/chromium/src/+/b65bc61d6aa279bc1012d33c55c0019c21abed6e
Patch Set 1 #Patch Set 2 : check if text need to convert to HTML #Patch Set 3 : text fix #
Total comments: 8
Patch Set 4 : add test #
Messages
Total messages: 33 (22 generated)
The CQ bit was checked by ctzsm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by ctzsm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [WIP] Replace getHtmlText with coerceToHtmlText BUG=709549 ========== to ========== Support paste with style from Spanned Only coerce text to HTML when needed, since replace getHtmlText with coerceToHtmlText will break two bookmarks test. 1. if text is HTML. 2. if text is plain text and has style (Spanned). BUG=709549 ==========
ctzsm@chromium.org changed reviewers: + twellington@chromium.org
On 2017/04/12 20:39:31, Shimi Zhang wrote: twellington@, could you please take a look.
On 2017/04/12 20:40:33, Shimi Zhang wrote: For hasStyleSpan method, we had a discussion in b/36853021, comment 10 says the reason to check those three classes. comment 20 we decide to move this to android later.
On 2017/04/12 20:40:33, Shimi Zhang wrote: For hasStyleSpan method, we had a discussion in b/36853021, comment 10 says the reason to check those three classes. comment 20 we decide to move this to android later.
The CQ bit was checked by ctzsm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Support paste with style from Spanned Only coerce text to HTML when needed, since replace getHtmlText with coerceToHtmlText will break two bookmarks test. 1. if text is HTML. 2. if text is plain text and has style (Spanned). BUG=709549 ========== to ========== Support paste with style from Spanned Only coerce text to HTML when needed, since replace getHtmlText with coerceToHtmlText will break two bookmark tests. 1. if text is HTML. 2. if text is plain text and has style (Spanned). BUG=709549 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It would be great to get some test added to ClipboardTest.java for this change in logic: https://cs.chromium.org/chromium/src/ui/android/junit/src/org/chromium/ui/bas... https://codereview.chromium.org/2812933003/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/Clipboard.java (right): https://codereview.chromium.org/2812933003/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/Clipboard.java:120: // TODO(ctzsm) remove this method after we move this to Android API. Is the Android change going in the support library or the framework? nit: add a colon after "(ctzsm": // TODO(ctzsm): Remove this method after Android API is updated https://codereview.chromium.org/2812933003/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/Clipboard.java:123: CharacterStyle.class, ParagraphStyle.class, UpdateAppearance.class}; Are these the only style base classes? https://codereview.chromium.org/2812933003/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/Clipboard.java:151: if (hasStyleSpan(spanned)) { Don't we still need to return something if there's no style span in the plain text?
https://codereview.chromium.org/2812933003/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/Clipboard.java (right): https://codereview.chromium.org/2812933003/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/Clipboard.java:120: // TODO(ctzsm) remove this method after we move this to Android API. On 2017/04/12 23:41:34, Theresa wrote: > Is the Android change going in the support library or the framework? > > > nit: add a colon after "(ctzsm": > > // TODO(ctzsm): Remove this method after Android API is updated > > Acknowledged. https://codereview.chromium.org/2812933003/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/Clipboard.java:120: // TODO(ctzsm) remove this method after we move this to Android API. On 2017/04/12 23:41:34, Theresa wrote: > Is the Android change going in the support library or the framework? That we don't know yet, but will talk to Android side. https://codereview.chromium.org/2812933003/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/Clipboard.java:123: CharacterStyle.class, ParagraphStyle.class, UpdateAppearance.class}; On 2017/04/12 23:41:34, Theresa wrote: > Are these the only style base classes? Yeah, I confirmed with Android team about this. See the internal bug discussion, comment 10 and 11. https://codereview.chromium.org/2812933003/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/Clipboard.java:151: if (hasStyleSpan(spanned)) { On 2017/04/12 23:41:34, Theresa wrote: > Don't we still need to return something if there's no style span in the plain > text? That is the part which will break bookmark tests. By copying a bookmark, the logic of bookmark will put a plain text and a bookmark to clipboard, but when pasting, if it gets both plain text and html text, https://cs.chromium.org/chromium/src/ui/base/clipboard/clipboard_android.cc?q... will update old map to a new map contains only text and html, then we lose bookmark info. An alternative way to resolve this issue is to check the platform (if defined(OS_ANDROID)) and use scw.WriteHtml(base::UTF8ToUTF16(title), url) here https://cs.chromium.org/chromium/src/components/bookmarks/browser/bookmark_no..., but I didn't go with that way because I want to minimize the side effect of code change in Clipboard.java, so if it is not necessary, we return null at the end of this method.
https://codereview.chromium.org/2812933003/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/Clipboard.java (right): https://codereview.chromium.org/2812933003/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/Clipboard.java:151: if (hasStyleSpan(spanned)) { On 2017/04/13 00:16:12, Shimi Zhang wrote: > On 2017/04/12 23:41:34, Theresa wrote: > > Don't we still need to return something if there's no style span in the plain > > text? > > That is the part which will break bookmark tests. > > By copying a bookmark, the logic of bookmark will put a plain text and a > bookmark to clipboard, but when pasting, if it gets both plain text and html > text, > https://cs.chromium.org/chromium/src/ui/base/clipboard/clipboard_android.cc?q... > will update old map to a new map contains only text and html, then we lose > bookmark info. > > An alternative way to resolve this issue is to check the platform (if > defined(OS_ANDROID)) and use scw.WriteHtml(base::UTF8ToUTF16(title), url) here > https://cs.chromium.org/chromium/src/components/bookmarks/browser/bookmark_no..., > but I didn't go with that way because I want to minimize the side effect of code > change in Clipboard.java, so if it is not necessary, we return null at the end > of this method. I agree that it makes sense to isolate the change to this class. If there's no HTML or styled text that needs to be represented as HTML then this method should return null -- I should have read the method return description more carefully.
The CQ bit was checked by ctzsm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
twellington@, I added a test, PTAL.
lgtm Thank you for adding the test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ctzsm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492120281718990,
"parent_rev": "71606ecc2ff8f79fd8aa0b9d6f41c8f747a6d470", "commit_rev":
"b65bc61d6aa279bc1012d33c55c0019c21abed6e"}
Message was sent while issue was closed.
Description was changed from ========== Support paste with style from Spanned Only coerce text to HTML when needed, since replace getHtmlText with coerceToHtmlText will break two bookmark tests. 1. if text is HTML. 2. if text is plain text and has style (Spanned). BUG=709549 ========== to ========== Support paste with style from Spanned Only coerce text to HTML when needed, since replace getHtmlText with coerceToHtmlText will break two bookmark tests. 1. if text is HTML. 2. if text is plain text and has style (Spanned). BUG=709549 Review-Url: https://codereview.chromium.org/2812933003 Cr-Commit-Position: refs/heads/master@{#464584} Committed: https://chromium.googlesource.com/chromium/src/+/b65bc61d6aa279bc1012d33c55c0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b65bc61d6aa279bc1012d33c55c0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
