Refactor Samsung SmartClip implementation.

Refactor Samsung SmartClip implementation.

Samsung "Smart Clip" (a.k.a. "Smart Select") is a Galaxy Note-specific
proprietary API to select a rectangle with the stylus and extract the text
contents.  This moves it from the ContentViewCore->ViewMsg->WebViewImpl
flow into the more modern WebContents->RenderFrameHost->WebLocalFrame
path.

Notes:
- One of the values we're providing in the return data is the original
rectangle that was passed into us.  It's currently being passed in back
and forth from Blink unnecessarily, and wasn't DIP-adjusted properly.
Empirically on a Galaxy Note 5, there's no evidence that the OEM code
ever looks at this rectangle, so just delete it.

- This never supported subframes (even local iframes) and still doesn't.
This patch is intended as a no-op refactoring for now; for subframe
support, we might consider generating this out of the accessibility
trees in the future.

BUG=665665
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2667283007
Cr-Commit-Position: refs/heads/master@{#448816}
Committed: https://chromium.googlesource.com/chromium/src/+/a3d72e6945823292c6fa7487268333b538b6a10e

[aelias_OOO_until_Jul13, 2017-02-04 01:56:52]

Description was changed from

==========
Refactor Samsung SmartClip implementation.

Samsung "Smart Clip" (a.k.a. "Smart Select") is a Galaxy Note-specific
proprietary API to select a rectangle with the stylus and extract the text
contents.  This moves it from the ContentViewCore->ViewMsg->WebViewImpl
flow into the more modern WebContents->RenderFrameHost->WebLocalFrame
path.

Notes:
- One of the values we're providing in the return data is the original
rectangle that was passed into us.  It's currently being passed in back
and forth from Blink unnecessarily, and wasn't DIP-adjusted properly.
Empirically on a Galaxy Note 5, there's no evidence that the OEM code
ever looks at this rectangle, so just delete it.

- This never supported subframes (even local iframes) and still doesn't.
This patch is intended as a no-op refactoring for now; for subframe
support, we might consider generating this out of the accessibility
trees in the future.

BUG=665665
==========

to

==========
Refactor Samsung SmartClip implementation.

Samsung "Smart Clip" (a.k.a. "Smart Select") is a Galaxy Note-specific
proprietary API to select a rectangle with the stylus and extract the text
contents.  This moves it from the ContentViewCore->ViewMsg->WebViewImpl
flow into the more modern WebContents->RenderFrameHost->WebLocalFrame
path.

Notes:
- One of the values we're providing in the return data is the original
rectangle that was passed into us.  It's currently being passed in back
and forth from Blink unnecessarily, and wasn't DIP-adjusted properly.
Empirically on a Galaxy Note 5, there's no evidence that the OEM code
ever looks at this rectangle, so just delete it.

- This never supported subframes (even local iframes) and still doesn't.
This patch is intended as a no-op refactoring for now; for subframe
support, we might consider generating this out of the accessibility
trees in the future.

BUG=665665
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[aelias_OOO_until_Jul13, 2017-02-04 01:56:56]

The CQ bit was checked by aelias@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-04 01:57:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aelias_OOO_until_Jul13, 2017-02-04 02:14:41]

aelias@chromium.org changed reviewers:
+ boliu@chromium.org, dtrainor@chromium.org, tsepez@chromium.org

[aelias_OOO_until_Jul13, 2017-02-04 02:52:08]

PTAL,

boliu@ for content/browser and generally
dtrainor@ for chrome/
tsepez@ for IPC

[Tom Sepez, 2017-02-04 03:22:44]

https://codereview.chromium.org/2667283007/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2667283007/diff/1/content/common/frame_messag...
content/common/frame_messages.h:1444: // Extracts the data at the given rect,
returning it through the
This one belongs in the prior section, above the divider at line 948.

[aelias_OOO_until_Jul13, 2017-02-04 03:26:38]

The CQ bit was checked by aelias@chromium.org to run a CQ dry run

[aelias_OOO_until_Jul13, 2017-02-04 03:26:53]

https://codereview.chromium.org/2667283007/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2667283007/diff/1/content/common/frame_messag...
content/common/frame_messages.h:1444: // Extracts the data at the given rect,
returning it through the
On 2017/02/04 at 03:22:44, Tom Sepez wrote:
> This one belongs in the prior section, above the divider at line 948.

Done.

[commit-bot: I haz the power, 2017-02-04 03:26:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-04 05:27:39]

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

[commit-bot: I haz the power, 2017-02-04 05:27:40]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_TIMED_OUT,
no build URL)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no
build URL)
  win_clang on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL)

[boliu, 2017-02-06 13:08:09]

the code in content went from android-only to cross-platform, intended?

https://codereview.chromium.org/2667283007/diff/20001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (left):

https://codereview.chromium.org/2667283007/diff/20001/content/browser/android...
content/browser/android/content_view_core_impl.cc:1416: 1 : (int)(width /
dpi_scale())),
this code used to ensure width/height at least 1, and this got dropped

I imagine it's not super important though

https://codereview.chromium.org/2667283007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2667283007/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1662: static int next_id =
1;
unsigned int, signed overflow is undefined behavior

https://codereview.chromium.org/2667283007/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1671: std::map<int,
SmartClipCallback>::iterator it =
nit: auto

https://codereview.chromium.org/2667283007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2667283007/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:480: void
OnSmartClipDataExtracted(int id,
IPC handlers are grouped together

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
(right):

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:376:
nativeRequestSmartClipExtract(
check mSmartClipCallback is not null?

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:395:
Message msg = Message.obtain(smartClipHandler, 0);
old code had a try/catch block here

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/SmartClipCallback.java
(right):

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/SmartClipCallback.java:11:
public abstract class SmartClipCallback {
only one method here, interface is better than abstract class

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:301:
void requestSmartClipExtract(int x, int y, int width, int height);
javadoc

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:303:
void setSmartClipResultHandler(final Handler smartClipHandler);
final here is kinda leaking implementation into interface, write it such that it
doesn't need to be declared final in the interface?

[Tom Sepez, 2017-02-06 17:29:29]

Messages LGTM

[David Trainor- moved to gerrit, 2017-02-07 19:28:59]

chrome/ lgtm

[aelias_OOO_until_Jul13, 2017-02-07 20:35:03]

The CQ bit was checked by aelias@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 20:35:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aelias_OOO_until_Jul13, 2017-02-07 20:47:46]

The CQ bit was checked by aelias@chromium.org to run a CQ dry run

[aelias_OOO_until_Jul13, 2017-02-07 20:48:13]

https://codereview.chromium.org/2667283007/diff/20001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (left):

https://codereview.chromium.org/2667283007/diff/20001/content/browser/android...
content/browser/android/content_view_core_impl.cc:1416: 1 : (int)(width /
dpi_scale())),
On 2017/02/06 at 13:08:09, boliu wrote:
> this code used to ensure width/height at least 1, and this got dropped
> 
> I imagine it's not super important though

Right, I dropped it intentionally, seemed like unnecessary complexity.

https://codereview.chromium.org/2667283007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2667283007/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1662: static int next_id =
1;
On 2017/02/06 at 13:08:09, boliu wrote:
> unsigned int, signed overflow is undefined behavior

Done.

https://codereview.chromium.org/2667283007/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1671: std::map<int,
SmartClipCallback>::iterator it =
On 2017/02/06 at 13:08:09, boliu wrote:
> nit: auto

Done.

https://codereview.chromium.org/2667283007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2667283007/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:480: void
OnSmartClipDataExtracted(int id,
On 2017/02/06 at 13:08:09, boliu wrote:
> IPC handlers are grouped together

Done.

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
(right):

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:376:
nativeRequestSmartClipExtract(
On 2017/02/06 at 13:08:09, boliu wrote:
> check mSmartClipCallback is not null?

Added an assertion.

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:395:
Message msg = Message.obtain(smartClipHandler, 0);
On 2017/02/06 at 13:08:09, boliu wrote:
> old code had a try/catch block here

I removed it because it doesn't happen and fine to crash if Samsung ever
introduces a bug in future version.

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/SmartClipCallback.java
(right):

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/SmartClipCallback.java:11:
public abstract class SmartClipCallback {
On 2017/02/06 at 13:08:09, boliu wrote:
> only one method here, interface is better than abstract class

Done.

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:301:
void requestSmartClipExtract(int x, int y, int width, int height);
On 2017/02/06 at 13:08:09, boliu wrote:
> javadoc

Done.

https://codereview.chromium.org/2667283007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:303:
void setSmartClipResultHandler(final Handler smartClipHandler);
On 2017/02/06 at 13:08:09, boliu wrote:
> final here is kinda leaking implementation into interface, write it such that
it doesn't need to be declared final in the interface?

The final is already baked into the """public""" API in
https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...
which I can't change, may as well keep the internal API consistent with it as
well.

https://codereview.chromium.org/2667283007/diff/40001/content/renderer/render...
File content/renderer/render_view_impl_android.cc (left):

https://codereview.chromium.org/2667283007/diff/40001/content/renderer/render...
content/renderer/render_view_impl_android.cc:74: void
RenderViewImpl::OnExtractSmartClipData(const gfx::Rect& rect) {
> the code in content went from android-only to cross-platform, intended?

Yes, the bulk of the code on the Blink side was already cross-platform and
separating out this tiny amount of shim code doesn't achieve much (IMO having
Android-only code just harms testability and refactorability in general anyway).

[commit-bot: I haz the power, 2017-02-07 20:48:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[boliu, 2017-02-07 21:52:35]

lgtm

[aelias_OOO_until_Jul13, 2017-02-07 22:03:45]

The CQ bit was unchecked by aelias@chromium.org

[aelias_OOO_until_Jul13, 2017-02-07 22:03:47]

The CQ bit was checked by aelias@chromium.org

[aelias_OOO_until_Jul13, 2017-02-07 22:03:47]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2667283007/#ps80001
(title: "Really apply code review comments")

[commit-bot: I haz the power, 2017-02-07 22:04:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 00:28:02]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486505027105410,
"parent_rev": "9aad3ae4ce41aeec08f59af3536d72ec79f5a641", "commit_rev":
"a3d72e6945823292c6fa7487268333b538b6a10e"}

[commit-bot: I haz the power, 2017-02-08 00:29:18]

Description was changed from

==========
Refactor Samsung SmartClip implementation.

Samsung "Smart Clip" (a.k.a. "Smart Select") is a Galaxy Note-specific
proprietary API to select a rectangle with the stylus and extract the text
contents.  This moves it from the ContentViewCore->ViewMsg->WebViewImpl
flow into the more modern WebContents->RenderFrameHost->WebLocalFrame
path.

Notes:
- One of the values we're providing in the return data is the original
rectangle that was passed into us.  It's currently being passed in back
and forth from Blink unnecessarily, and wasn't DIP-adjusted properly.
Empirically on a Galaxy Note 5, there's no evidence that the OEM code
ever looks at this rectangle, so just delete it.

- This never supported subframes (even local iframes) and still doesn't.
This patch is intended as a no-op refactoring for now; for subframe
support, we might consider generating this out of the accessibility
trees in the future.

BUG=665665
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Refactor Samsung SmartClip implementation.

Samsung "Smart Clip" (a.k.a. "Smart Select") is a Galaxy Note-specific
proprietary API to select a rectangle with the stylus and extract the text
contents.  This moves it from the ContentViewCore->ViewMsg->WebViewImpl
flow into the more modern WebContents->RenderFrameHost->WebLocalFrame
path.

Notes:
- One of the values we're providing in the return data is the original
rectangle that was passed into us.  It's currently being passed in back
and forth from Blink unnecessarily, and wasn't DIP-adjusted properly.
Empirically on a Galaxy Note 5, there's no evidence that the OEM code
ever looks at this rectangle, so just delete it.

- This never supported subframes (even local iframes) and still doesn't.
This patch is intended as a no-op refactoring for now; for subframe
support, we might consider generating this out of the accessibility
trees in the future.

BUG=665665
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2667283007
Cr-Commit-Position: refs/heads/master@{#448816}
Committed:
https://chromium.googlesource.com/chromium/src/+/a3d72e6945823292c6fa74872683...
==========

[commit-bot: I haz the power, 2017-02-08 00:29:19]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a3d72e6945823292c6fa74872683...