Separate out Touch Selection Orientation enum

Separate out Touch Selection Orientation enum

Separate out Touch Selection Orientation so that it can
be used both on Java side and native side easily.

BUG=NONE

Committed: https://crrev.com/083c38c02fad2d8537eb46b8cfd92f353f05801f
Cr-Commit-Position: refs/heads/master@{#318657}

[AviD, 2014-11-04 11:31:14]

avi.nitk@samsung.com changed reviewers:
+ jdduke@chromium.org

[AviD, 2014-11-04 11:31:15]

Done so that there is a unified enum for both java and native side.
PTAL.

[jdduke (slow), 2014-11-04 16:21:50]

Looks like you forgot to add the new header. Also, I may be wrong, but there
might be some additional WebView hooks that need to be modified to pick up the
enum?

https://codereview.chromium.org/701823002/diff/1/content/browser/android/popu...
File content/browser/android/popup_touch_handle_drawable.cc (right):

https://codereview.chromium.org/701823002/diff/1/content/browser/android/popu...
content/browser/android/popup_touch_handle_drawable.cc:34: case
TOUCH_HANDLE_LEFT:
No need for the switch here, let's just remove it and have:

Java_PopupTouchHandleDrawable_setOrientation(env, drawable_.obj(), orientation);

[AviD, 2014-11-04 18:24:19]

On 2014/11/04 16:21:50, jdduke wrote:
> Looks like you forgot to add the new header. Also, I may be wrong, but there
> might be some additional WebView hooks that need to be modified to pick up the
> enum?

I checked Webview build by building android_webview_apk. It was building without
any errors.

Are you referring to addition similar to
'../content/content.gyp:selection_event_type_java', in
android_webview/android_webview.gyp ?

[AviD, 2014-11-04 18:24:32]

https://codereview.chromium.org/701823002/diff/1/content/browser/android/popu...
File content/browser/android/popup_touch_handle_drawable.cc (right):

https://codereview.chromium.org/701823002/diff/1/content/browser/android/popu...
content/browser/android/popup_touch_handle_drawable.cc:34: case
TOUCH_HANDLE_LEFT:
On 2014/11/04 16:21:50, jdduke wrote:
> No need for the switch here, let's just remove it and have:
> 
> Java_PopupTouchHandleDrawable_setOrientation(env, drawable_.obj(),
orientation);

Done.

[jdduke (slow), 2014-11-06 16:41:59]

https://codereview.chromium.org/701823002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/input/touch_handle_orientation.h (right):

https://codereview.chromium.org/701823002/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/touch_handle_orientation.h:10: // This file
contains a list of Orientation types for Touch handles, used
Let's remove the text "This file contains a list of ", reformatting the comment
accordingly.

https://codereview.chromium.org/701823002/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/touch_handle_orientation.h:15: 
Nit: Remove line break between comment and enum.

Also, can you add a comment comment line below // GENERATED_JAVA_ENUM_PACKAGE...
that goes:

// GENERATED_JAVA_PREFIX_TO_STRIP: TOUCH_HANDLE_

Then you can update the Java side to use a shorter name :)

[AviD, 2014-11-10 12:13:49]

Thanks :)
PTAL.

https://codereview.chromium.org/701823002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/input/touch_handle_orientation.h (right):

https://codereview.chromium.org/701823002/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/touch_handle_orientation.h:10: // This file
contains a list of Orientation types for Touch handles, used
On 2014/11/06 16:41:59, jdduke wrote:
> Let's remove the text "This file contains a list of ", reformatting the
comment
> accordingly.

Done.

https://codereview.chromium.org/701823002/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/touch_handle_orientation.h:15: 
On 2014/11/06 16:41:59, jdduke wrote:
> Nit: Remove line break between comment and enum.
> 
> Also, can you add a comment comment line below //
GENERATED_JAVA_ENUM_PACKAGE...
> that goes:
> 
> // GENERATED_JAVA_PREFIX_TO_STRIP: TOUCH_HANDLE_
> 
> Then you can update the Java side to use a shorter name :)

Done.

[jdduke (slow), 2014-11-11 21:04:30]

Sorry for the delay, missed your last update.

https://codereview.chromium.org/701823002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle_orientation.h (right):

https://codereview.chromium.org/701823002/diff/100001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle_orientation.h:17:
TOUCH_HANDLE_LEFT,
Can you make this "enum class TouchHandleOrientation"? Might as well start using
the new C++11 fanciness.

https://codereview.chromium.org/701823002/diff/100001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/701823002/diff/100001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:61:
private int mOrientation = -1;
Let's initialize this to  TouchHandleOrientation.ORIENTATION_UNDEFINED.

https://codereview.chromium.org/701823002/diff/100001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:150:
final boolean hadValidOrientation = mOrientation != -1;
Also make this reference TouchHandleOrientation.ORIENTATION_UNDEFINED.

[AviD, 2015-02-25 05:49:05]

@jdduke: Changes are done according to new design. Could you please take a look?

https://codereview.chromium.org/701823002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/input/touch_handle_orientation.h (right):

https://codereview.chromium.org/701823002/diff/100001/content/browser/rendere...
content/browser/renderer_host/input/touch_handle_orientation.h:17:
TOUCH_HANDLE_LEFT,
On 2014/11/11 21:04:30, jdduke wrote:
> Can you make this "enum class TouchHandleOrientation"? Might as well start
using
> the new C++11 fanciness.

Done.

https://codereview.chromium.org/701823002/diff/100001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/701823002/diff/100001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:61:
private int mOrientation = -1;
On 2014/11/11 21:04:30, jdduke wrote:
> Let's initialize this to  TouchHandleOrientation.ORIENTATION_UNDEFINED.

Done.

https://codereview.chromium.org/701823002/diff/100001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:150:
final boolean hadValidOrientation = mOrientation != -1;
On 2014/11/11 21:04:30, jdduke wrote:
> Also make this reference TouchHandleOrientation.ORIENTATION_UNDEFINED.

Done.

[jdduke (slow), 2015-02-25 17:36:48]

Thanks, this looks good, just one thought about the casting.

https://codereview.chromium.org/701823002/diff/120001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle.cc (right):

https://codereview.chromium.org/701823002/diff/120001/ui/touch_selection/touc...
ui/touch_selection/touch_handle.cc:119: DCHECK_NE(static_cast<int>(orientation),
Dang, I didn't realize our DCHECK code doesn't play nice with enum classes (yet,
see https://codereview.chromium.org/835783003/... hopefully we'll have a generic
solution soon).

If we only use DCHECK_EQ/NE for that enum in this file, could we define:

std::ostream& operator<<(std::ostream& os, const TouchHandlerOrientation&
orientation) {...}

at the top of this file in the anon namespace. Would that work?

[AviD, 2015-02-26 06:28:08]

https://codereview.chromium.org/701823002/diff/120001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle.cc (right):

https://codereview.chromium.org/701823002/diff/120001/ui/touch_selection/touc...
ui/touch_selection/touch_handle.cc:119: DCHECK_NE(static_cast<int>(orientation),
On 2015/02/25 17:36:48, jdduke wrote:
> Dang, I didn't realize our DCHECK code doesn't play nice with enum classes
(yet,
> see https://codereview.chromium.org/835783003/... hopefully we'll have a
generic
> solution soon).
> 
> If we only use DCHECK_EQ/NE for that enum in this file, could we define:
> 
> std::ostream& operator<<(std::ostream& os, const TouchHandlerOrientation&
> orientation) {...}
> 
> at the top of this file in the anon namespace. Would that work?

If we define this in anonymous namespace, it gives unused error.
Defining it in ui namespace is working fine.

[jdduke (slow), 2015-02-26 16:50:54]

lgtm with one nit, thanks.

https://codereview.chromium.org/701823002/diff/140001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle.cc (right):

https://codereview.chromium.org/701823002/diff/140001/ui/touch_selection/touc...
ui/touch_selection/touch_handle.cc:48: std::ostream& operator<<(std::ostream&
os,
Thanks, let's make this static. Also, there's only 4 types, so let's just
enumerate them:

switch (orientation) {
  case TouchHandlerOrientation::LEFT:
    return os << "LEFT":
 ...
  default:
    return os << "INVALID: " << static_cast<int>(orientation);
}

[AviD, 2015-02-27 10:02:28]

Thanks @jdduke.

https://codereview.chromium.org/701823002/diff/140001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle.cc (right):

https://codereview.chromium.org/701823002/diff/140001/ui/touch_selection/touc...
ui/touch_selection/touch_handle.cc:48: std::ostream& operator<<(std::ostream&
os,
On 2015/02/26 16:50:54, jdduke wrote:
> Thanks, let's make this static. Also, there's only 4 types, so let's just
> enumerate them:
> 
> switch (orientation) {
>   case TouchHandlerOrientation::LEFT:
>     return os << "LEFT":
>  ...
>   default:
>     return os << "INVALID: " << static_cast<int>(orientation);
> }

Done.

[AviD, 2015-02-27 10:05:28]

The CQ bit was checked by avi.nitk@samsung.com

[AviD, 2015-02-27 10:05:28]

The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org
Link to the patchset: https://codereview.chromium.org/701823002/#ps160001
(title: "updated as per review comments")

[commit-bot: I haz the power, 2015-02-27 10:06:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701823002/160001

[commit-bot: I haz the power, 2015-02-27 10:22:32]

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

[commit-bot: I haz the power, 2015-02-27 10:22:33]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[AviD, 2015-02-27 10:32:48]

avi.nitk@samsung.com changed reviewers:
+ boliu@chromium.org, sievers@chromium.org

[AviD, 2015-02-27 10:32:49]

Adding boliu@ sievers@ for owner review

[boliu, 2015-02-27 16:36:10]

android_webview rs lgtm once android_aosp bot is happy

[jdduke (slow), 2015-02-27 16:46:46]

https://codereview.chromium.org/701823002/diff/160001/android_webview/java_li...
File android_webview/java_library_common.mk (right):

https://codereview.chromium.org/701823002/diff/160001/android_webview/java_li...
android_webview/java_library_common.mk:65: $(call
intermediates-dir-for,GYP,shared)/enums/selection_event_type_java/org/chromium/ui/touch_selection/TouchHandleOrientation.java
\
Change selection_event_type_java to touch_handle_orientation_java

https://codereview.chromium.org/701823002/diff/160001/ui/touch_selection/BUIL...
File ui/touch_selection/BUILD.gn (right):

https://codereview.chromium.org/701823002/diff/160001/ui/touch_selection/BUIL...
ui/touch_selection/BUILD.gn:62:
java_cpp_enum("ui_touch_handle_orientation_srcjar") {
You need to add a reference to this jar in content/public/android/BUILD.gn.

[no sievers, 2015-02-27 19:39:51]

On 2015/02/27 10:32:49, AviD wrote:
> Adding boliu@ sievers@ for owner review

lgtm

[AviD, 2015-03-02 05:14:09]

https://codereview.chromium.org/701823002/diff/160001/android_webview/java_li...
File android_webview/java_library_common.mk (right):

https://codereview.chromium.org/701823002/diff/160001/android_webview/java_li...
android_webview/java_library_common.mk:65: $(call
intermediates-dir-for,GYP,shared)/enums/selection_event_type_java/org/chromium/ui/touch_selection/TouchHandleOrientation.java
\
On 2015/02/27 16:46:46, jdduke wrote:
> Change selection_event_type_java to touch_handle_orientation_java

Done.

https://codereview.chromium.org/701823002/diff/160001/ui/touch_selection/BUIL...
File ui/touch_selection/BUILD.gn (right):

https://codereview.chromium.org/701823002/diff/160001/ui/touch_selection/BUIL...
ui/touch_selection/BUILD.gn:62:
java_cpp_enum("ui_touch_handle_orientation_srcjar") {
On 2015/02/27 16:46:46, jdduke wrote:
> You need to add a reference to this jar in content/public/android/BUILD.gn.

Done.

[AviD, 2015-03-02 05:14:19]

The CQ bit was checked by avi.nitk@samsung.com

[AviD, 2015-03-02 05:14:19]

The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org,
boliu@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/701823002/#ps180001
(title: "android_aosp build")

[commit-bot: I haz the power, 2015-03-02 05:14:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701823002/180001

[commit-bot: I haz the power, 2015-03-02 05:34:28]

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

[commit-bot: I haz the power, 2015-03-02 05:34:29]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[AviD, 2015-03-02 05:42:19]

The CQ bit was unchecked by avi.nitk@samsung.com

[AviD, 2015-03-02 05:42:21]

The CQ bit was checked by avi.nitk@samsung.com

[AviD, 2015-03-02 05:42:21]

The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org,
boliu@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/701823002/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-03-02 05:42:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701823002/200001

[commit-bot: I haz the power, 2015-03-02 06:47:43]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-03-02 06:48:24]

Patchset 11 (id:??) landed as
https://crrev.com/083c38c02fad2d8537eb46b8cfd92f353f05801f
Cr-Commit-Position: refs/heads/master@{#318657}