[Android] Extend smart clip to return html along with text

[Android] Extend smart clip to return html along with text

BUG=388961
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283159

[Changwan Ryu, 2014-07-09 23:02:08]

Who do you think I should add as reviewers?

[Yaron, 2014-07-09 23:08:14]

https://codereview.chromium.org/378233002/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/378233002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:214:
public void onSmartClipDataExtracted(String text, String html, Rect cliprect);
clipRect (and below)

https://codereview.chromium.org/378233002/diff/20001/content/renderer/render_...
File content/renderer/render_view_impl_android.cc (right):

https://codereview.chromium.org/378233002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl_android.cc:67: void
RenderViewImpl::OnExtractSmartClipData(const gfx::Rect& rect) {
whomever reviewed this original CL

https://codereview.chromium.org/378233002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl_android.cc:71: blink::WebString clip_html =
webview()->extractSmartClipDataAsMarkup(rect);
can this not be done in the previous call?

[Changwan Ryu, 2014-07-10 01:20:30]

https://codereview.chromium.org/378233002/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/378233002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:214:
public void onSmartClipDataExtracted(String text, String html, Rect cliprect);
On 2014/07/09 23:08:14, Yaron wrote:
> clipRect (and below)

Done.

https://codereview.chromium.org/378233002/diff/20001/content/renderer/render_...
File content/renderer/render_view_impl_android.cc (right):

https://codereview.chromium.org/378233002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl_android.cc:67: void
RenderViewImpl::OnExtractSmartClipData(const gfx::Rect& rect) {
Added jam@

On 2014/07/09 23:08:14, Yaron wrote:
> whomever reviewed this original CL

https://codereview.chromium.org/378233002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl_android.cc:71: blink::WebString clip_html =
webview()->extractSmartClipDataAsMarkup(rect);
Fixed by extending getSmartClipData. However, changing the API may break the
build at one point, as webkit and this function will see different APIs at that
point. Would it be ok?

[Changwan Ryu, 2014-07-10 02:14:29]

adding tsepez@ for approval on content/common

[aelias_OOO_until_Jul13, 2014-07-11 00:32:29]

content/browser/renderer_host/render_widget_host_view_android.* lgtm

[Changwan Ryu, 2014-07-11 00:42:42]

Adding jamesr@ for approval on content/renderer

[Yaron, 2014-07-11 18:39:25]

remaining android/java lgtm

https://codereview.chromium.org/378233002/diff/80001/chrome/android/javatests...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewTest.java
(right):

https://codereview.chromium.org/378233002/diff/80001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewTest.java:77:
String text = bundle.getString("text");
pass to notifyCalled and assert on them?

[jamesr, 2014-07-11 19:06:04]

lgtm

[Changwan Ryu, 2014-07-11 20:32:41]

I have one concern. There is a counterpart WebKit change, and submitting the two
CLs, no matter how hard I try to make it happen simultaneously, may break the
build at some point. Would it be ok? Anyone?

https://codereview.chromium.org/378233002/diff/80001/chrome/android/javatests...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewTest.java
(right):

https://codereview.chromium.org/378233002/diff/80001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewTest.java:77:
String text = bundle.getString("text");
On 2014/07/11 18:39:24, Yaron wrote:
> pass to notifyCalled and assert on them?

Done.

[aelias_OOO_until_Jul13, 2014-07-11 20:36:55]

There's usually some way to avoid breaking the build with cross-dependent Blink
and Chromium changes.  Sometimes that involves splitting your change into 3
patches instead of two.  If your patch would make the tree red it's possible to
coordinate with the Blink gardener to avoid getting reverted and force a roll
anyway, but that's rather stressful to arrange and I recommend thinking of a way
to avoid redness.

[Changwan Ryu, 2014-07-11 21:41:08]

On 2014/07/11 20:36:55, aelias wrote:
> There's usually some way to avoid breaking the build with cross-dependent
Blink
> and Chromium changes.  Sometimes that involves splitting your change into 3
> patches instead of two.  If your patch would make the tree red it's possible
to
> coordinate with the Blink gardener to avoid getting reverted and force a roll
> anyway, but that's rather stressful to arrange and I recommend thinking of a
way
> to avoid redness.

I see. Thanks for letting me know. I changed the function name to avoid build
breakage. I'll remove the original function later.

[Changwan Ryu, 2014-07-11 21:45:50]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2014-07-11 21:47:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/changwan@chromium.org/378233002/120001

[Changwan Ryu, 2014-07-11 21:52:24]

adding security engineers for approval on content/common

[Changwan Ryu, 2014-07-11 22:25:54]

The CQ bit was unchecked by changwan@chromium.org

[Changwan Ryu, 2014-07-11 23:14:18]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2014-07-11 23:16:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/changwan@chromium.org/378233002/120001

[commit-bot: I haz the power, 2014-07-12 00:50:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-12 00:53:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-12 00:53:04]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Tom Sepez, 2014-07-14 16:01:51]

Messages LGTM.

[Changwan Ryu, 2014-07-14 23:44:35]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2014-07-14 23:46:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/changwan@chromium.org/378233002/120001

[commit-bot: I haz the power, 2014-07-15 06:48:43]

Change committed as 283159