Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1728193002:
Support dragging texts into Android WebView. (Closed)
|
Created:
4 years, 10 months ago by hush (inactive) Modified:
4 years, 5 months ago CC:
android-webview-reviews_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dcheng, dglazkov+blink, jam, Vladislav Kaznacheev, nasko+codewatch_chromium.org, newt (away), nona+watch_chromium.org, shuchen+watch_chromium.org, James Su, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport dragging texts into Android WebView.
WebView gets callback 'onDragEvent' when something is dragged into the WebView.
This CL extracts the texts (if any) and location from the DragEvent and creates
DropData which is Chromium's internal representation of dragged data.
The event plumbing path is WebView#onDragEvent --> AwContents#onDragEvent -->
ContentViewCore --> WebContentsViewAndroid.
This CL also uses a new blink::WebView API that performs dropping with DropData,
because on Android, the DropData is only available when the drop is performed,
not when the drag is started.
BUG=584789
Committed: https://crrev.com/445de0ecb75960ee4060dfcfaf1a86e5632e4d0a
Cr-Commit-Position: refs/heads/master@{#403417}
Patch Set 1 : #Patch Set 2 : #
Total comments: 8
Patch Set 3 : review #
Total comments: 17
Patch Set 4 : address comments #Patch Set 5 : gyp and gn #
Total comments: 6
Patch Set 6 : rebase onto https://codereview.chromium.org/1723763002/ #Patch Set 7 : filter the data during drop #Patch Set 8 : rebase onto https://codereview.chromium.org/1723763002/ #Patch Set 9 : rebase #Patch Set 10 : rebase on top of https://codereview.chromium.org/2080703004/ #Patch Set 11 : remove dead includes #
Total comments: 8
Patch Set 12 : review #
Total comments: 6
Patch Set 13 : generate DragEvent JNI bindings and use the enums from JNI #Messages
Total messages: 58 (21 generated)
This is the first draft, built on top of https://codereview.chromium.org/1723763002/. This CL only cares about dragging texts into WebView for now. Dragging files like images will be supported later. I think Chrome on Android can easily hook up "View#onDragEvent" and forward events to ContentViewCore like what AwContents did.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
hush@chromium.org changed reviewers: + sky@chromium.org, tedchoc@chromium.org
Hello sky@: PTAL ui/ tedchoc@: PTAL content/
This is only the first step. WebView can accept drops from other views with this CL. But we can't initiate a drag with WebView yet. This will be done in a follow up CL.
LGTM https://codereview.chromium.org/1728193002/diff/100001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_android.cc (right): https://codereview.chromium.org/1728193002/diff/100001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_android.cc:100: } nit: newline between 99/100 and add // namespace ui
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/1728193002/diff/100001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1728193002/diff/100001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1424: ui::OSExchangeData data(new ui::OSExchangeDataProviderAndroid()); Will there be a followup patch to address the fact that the types are not plumbed through? https://codereview.chromium.org/1728193002/diff/100001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_android.cc (right): https://codereview.chromium.org/1728193002/diff/100001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_android.cc:7: namespace ui { Nit: newline after this
hush@chromium.org changed reviewers: + sgurun@chromium.org
https://codereview.chromium.org/1728193002/diff/100001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1728193002/diff/100001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1424: ui::OSExchangeData data(new ui::OSExchangeDataProviderAndroid()); On 2016/02/25 01:18:49, dcheng wrote: > Will there be a followup patch to address the fact that the types are not > plumbed through? Types are sort of plumbed through. We only support "string" format for now. So we will return true for OSExchangeData::HasString and false for other types. Webview won't get any drag drop related events if the drop data type is not text. (See AwContents#onDragEvent. Returning false on "ACTION_DRAG_STARTED" will ensure there won't be any subsequence DragEvents) Yes, there will be a followup to support dragging image file URIs into webview. https://codereview.chromium.org/1728193002/diff/100001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_android.cc (right): https://codereview.chromium.org/1728193002/diff/100001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_android.cc:7: namespace ui { On 2016/02/25 01:18:49, dcheng wrote: > Nit: newline after this Done. https://codereview.chromium.org/1728193002/diff/100001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_android.cc:100: } On 2016/02/25 01:06:26, sky wrote: > nit: newline between 99/100 and add > // namespace ui Done.
sgurun: PTAL a_w/
tedchoc@chromium.org changed reviewers: + aelias@chromium.org, sievers@chromium.org
+aelias (for input) and sievers (for WCVA) https://codereview.chromium.org/1728193002/diff/120001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1728193002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:2980: ClipDescription clipDescription = event.getClipDescription(); I don't see any reason why we need to increase the api surface of ContentViewCore beyond onDragEvent. In particular, why doesn't this all live within that class? https://codereview.chromium.org/1728193002/diff/120001/content/browser/androi... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/1728193002/diff/120001/content/browser/androi... content/browser/android/content_view_core_impl.h:281: void OnDragEntered(JNIEnv* env, again to the api comment, onTouchEvent has a single nativeOnTouchEvent. I'm wondering why we need to differ here. They even pass the java touch event down and things are extracted as needed from native. That might be overkill as OnPerformDrop is the only thing that needs anything else at this point. We could pass null in other other cases. Although as soon as we start doing images or anything more fancy, then I would want to make sure we don't have to pollute the CVC with that logic. https://codereview.chromium.org/1728193002/diff/120001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_android.cc (right): https://codereview.chromium.org/1728193002/diff/120001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_android.cc:21: // no-op. should all of these be marked as NOTIMPLEMENTED() instead of no op?
https://codereview.chromium.org/1728193002/diff/120001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1728193002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:2991: // TODO(hush): onPerformDrop should return false when the receiving object does I don't think this one should be left as a TODO, since it just has to do with simple text dragging. It looks like the ContentViewCore methods (and below) should plumb a boolean up to the code that decides to accept or reject. Alternatively, is there even any concept in Chromium/HTML of rejecting a drag-and-drop? If not, this TODO should just be deleted. https://codereview.chromium.org/1728193002/diff/120001/content/public/browser... File content/public/browser/render_view_host.h (right): https://codereview.chromium.org/1728193002/diff/120001/content/public/browser... content/public/browser/render_view_host.h:149: virtual void DragTargetDropWithData(const DropData& drop_data, Why do you need to add a new platform-specific method? This has close to the same arguments to DragTargetDragEnter, what is the meaning/value-added of this new method? https://codereview.chromium.org/1728193002/diff/120001/ui/base/BUILD.gn File ui/base/BUILD.gn (right): https://codereview.chromium.org/1728193002/diff/120001/ui/base/BUILD.gn#newco... ui/base/BUILD.gn:574: "dragdrop/drag_drop_types.h", Can we just remove all of the files without "_android" in the name from the "if (!toolkit_views && !use_aura) { sources -= [" block below instead? It looks like these files are being added twice and removed once. https://codereview.chromium.org/1728193002/diff/120001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_android.cc (right): https://codereview.chromium.org/1728193002/diff/120001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_android.cc:54: return string_.size() > 0; !string_.empty() (likewise in HasString())
https://codereview.chromium.org/1728193002/diff/100001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1728193002/diff/100001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1424: ui::OSExchangeData data(new ui::OSExchangeDataProviderAndroid()); On 2016/02/25 at 02:40:48, hush wrote: > On 2016/02/25 01:18:49, dcheng wrote: > > Will there be a followup patch to address the fact that the types are not > > plumbed through? > > Types are sort of plumbed through. We only support "string" format for now. So we will return true for OSExchangeData::HasString and false for other types. Webview won't get any drag drop related events if the drop data type is not text. (See AwContents#onDragEvent. Returning false on "ACTION_DRAG_STARTED" will ensure there won't be any subsequence DragEvents) > I don't think I follow: I don't see how the types are plumbed through at all atm. Since we don't know what the contents of the drop are yet, OSExchangeDataProviderAndroid will be unpopulated. So PrepareDropData will always prepare a drop data that's empty. > > Yes, there will be a followup to support dragging image file URIs into webview.
https://codereview.chromium.org/1728193002/diff/120001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1728193002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:2980: ClipDescription clipDescription = event.getClipDescription(); On 2016/02/25 18:38:53, Ted C wrote: > I don't see any reason why we need to increase the api surface of > ContentViewCore beyond onDragEvent. In particular, why doesn't this all live > within that class? No particular reason in fact. During the first draft, I was just being lazy and trying to avoid creating a native counterpart class for DragEvent (just like what we do for MotionEvent) Anyway, I will do as you asked. https://codereview.chromium.org/1728193002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:2991: // TODO(hush): onPerformDrop should return false when the receiving object does On 2016/02/25 20:58:00, aelias wrote: > I don't think this one should be left as a TODO, since it just has to do with > simple text dragging. It looks like the ContentViewCore methods (and below) > should plumb a boolean up to the code that decides to accept or reject. > > Alternatively, is there even any concept in Chromium/HTML of rejecting a > drag-and-drop? If not, this TODO should just be deleted. I will delete this TODO. There is no such concept of Chromium rejecting drag-and-drop. The function is WebViewImpl::dragTargetDrop. We always accept the drop except to deal with race condition in IPC. https://codereview.chromium.org/1728193002/diff/120001/content/browser/androi... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/1728193002/diff/120001/content/browser/androi... content/browser/android/content_view_core_impl.h:281: void OnDragEntered(JNIEnv* env, On 2016/02/25 18:38:53, Ted C wrote: > again to the api comment, onTouchEvent has a single nativeOnTouchEvent. I'm > wondering why we need to differ here. > > They even pass the java touch event down and things are extracted as needed from > native. > > That might be overkill as OnPerformDrop is the only thing that needs anything > else at this point. We could pass null in other other cases. > > Although as soon as we start doing images or anything more fancy, then I would > want to make sure we don't have to pollute the CVC with that logic. Yes, I will replace this bunch with OnDragEvent. https://codereview.chromium.org/1728193002/diff/120001/content/public/browser... File content/public/browser/render_view_host.h (right): https://codereview.chromium.org/1728193002/diff/120001/content/public/browser... content/public/browser/render_view_host.h:149: virtual void DragTargetDropWithData(const DropData& drop_data, On 2016/02/25 20:58:00, aelias wrote: > Why do you need to add a new platform-specific method? This has close to the > same arguments to DragTargetDragEnter, what is the meaning/value-added of this > new method? See CL: https://codereview.chromium.org/1723763002/ On Android, we don't have the text being dragged until ACTION_DROP. So the DropData that's passed during DragTargetDragEnter does not contain the real text being dragged. We have to pass another DropData during ACTION_DROP, and that's what this function does. On other platforms (aura), we have access to the dragged when the drag starts, so there is no need to specify the drop data again during 'drop' action. https://codereview.chromium.org/1728193002/diff/120001/ui/base/BUILD.gn File ui/base/BUILD.gn (right): https://codereview.chromium.org/1728193002/diff/120001/ui/base/BUILD.gn#newco... ui/base/BUILD.gn:574: "dragdrop/drag_drop_types.h", On 2016/02/25 20:58:00, aelias wrote: > Can we just remove all of the files without "_android" in the name from the "if > (!toolkit_views && !use_aura) { sources -= [" block below instead? It looks > like these files are being added twice and removed once. Done. https://codereview.chromium.org/1728193002/diff/120001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_android.cc (right): https://codereview.chromium.org/1728193002/diff/120001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_android.cc:21: // no-op. On 2016/02/25 18:38:53, Ted C wrote: > should all of these be marked as NOTIMPLEMENTED() instead of no op? yes. That would be good to catch any unexpected calls to these methods. (there shouldn't be any though) https://codereview.chromium.org/1728193002/diff/120001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_android.cc:54: return string_.size() > 0; On 2016/02/25 20:58:00, aelias wrote: > !string_.empty() (likewise in HasString()) I changed the logic a little. I used a format_ integer flag to track whether any strings has been set. That's because there is a meaningful difference between a DataProvider with an empty string VS a DataProvider without any string. On Android, when the drag starts, we know the MIME type of dragged object (which is string), but we don't know what the string is. So we will need to set an empty string on the DataProvider to indicate that the DataProvider contains a string type.
https://codereview.chromium.org/1728193002/diff/100001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1728193002/diff/100001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1424: ui::OSExchangeData data(new ui::OSExchangeDataProviderAndroid()); On 2016/02/25 21:13:38, dcheng wrote: > On 2016/02/25 at 02:40:48, hush wrote: > > On 2016/02/25 01:18:49, dcheng wrote: > > > Will there be a followup patch to address the fact that the types are not > > > plumbed through? > > > > Types are sort of plumbed through. We only support "string" format for now. So > we will return true for OSExchangeData::HasString and false for other types. > Webview won't get any drag drop related events if the drop data type is not > text. (See AwContents#onDragEvent. Returning false on "ACTION_DRAG_STARTED" will > ensure there won't be any subsequence DragEvents) > > > > I don't think I follow: I don't see how the types are plumbed through at all > atm. Since we don't know what the contents of the drop are yet, > OSExchangeDataProviderAndroid will be unpopulated. So PrepareDropData will > always prepare a drop data that's empty. > > > > > Yes, there will be a followup to support dragging image file URIs into > webview. Sorry yeah you're right. I just updated the logic here. I will set an empty string when on DRAG_ENTERED event to indicate the data type is string.
https://codereview.chromium.org/1728193002/diff/120001/content/public/browser... File content/public/browser/render_view_host.h (right): https://codereview.chromium.org/1728193002/diff/120001/content/public/browser... content/public/browser/render_view_host.h:149: virtual void DragTargetDropWithData(const DropData& drop_data, On 2016/02/27 at 01:46:13, hush wrote: > On 2016/02/25 20:58:00, aelias wrote: > > Why do you need to add a new platform-specific method? This has close to the > > same arguments to DragTargetDragEnter, what is the meaning/value-added of this > > new method? > > See CL: https://codereview.chromium.org/1723763002/ > On Android, we don't have the text being dragged until ACTION_DROP. So the DropData that's passed during DragTargetDragEnter does not contain the real text being dragged. We have to pass another DropData during ACTION_DROP, and that's what this function does. > On other platforms (aura), we have access to the dragged when the drag starts, so there is no need to specify the drop data again during 'drop' action. It doesn't seem great that we're passing in fake/empty data in DragTargetDragEnter, that seems misleading. Can you remove the DropData from DragTargetDragEnter and refactor all platforms to use your new DropData IPC?
https://codereview.chromium.org/1728193002/diff/120001/content/public/browser... File content/public/browser/render_view_host.h (right): https://codereview.chromium.org/1728193002/diff/120001/content/public/browser... content/public/browser/render_view_host.h:149: virtual void DragTargetDropWithData(const DropData& drop_data, On 2016/02/27 01:54:41, aelias wrote: > On 2016/02/27 at 01:46:13, hush wrote: > > On 2016/02/25 20:58:00, aelias wrote: > > > Why do you need to add a new platform-specific method? This has close to > the > > > same arguments to DragTargetDragEnter, what is the meaning/value-added of > this > > > new method? > > > > See CL: https://codereview.chromium.org/1723763002/ > > On Android, we don't have the text being dragged until ACTION_DROP. So the > DropData that's passed during DragTargetDragEnter does not contain the real text > being dragged. We have to pass another DropData during ACTION_DROP, and that's > what this function does. > > On other platforms (aura), we have access to the dragged when the drag starts, > so there is no need to specify the drop data again during 'drop' action. > > It doesn't seem great that we're passing in fake/empty data in > DragTargetDragEnter, that seems misleading. Can you remove the DropData from > DragTargetDragEnter and refactor all platforms to use your new DropData IPC? I can do that. But this would bloat this CL significantly though. I would do it in another CL that sits between https://codereview.chromium.org/1723763002/ and this CL.
https://codereview.chromium.org/1728193002/diff/120001/content/public/browser... File content/public/browser/render_view_host.h (right): https://codereview.chromium.org/1728193002/diff/120001/content/public/browser... content/public/browser/render_view_host.h:149: virtual void DragTargetDropWithData(const DropData& drop_data, On 2016/02/29 18:27:41, hush wrote: > On 2016/02/27 01:54:41, aelias wrote: > > On 2016/02/27 at 01:46:13, hush wrote: > > > On 2016/02/25 20:58:00, aelias wrote: > > > > Why do you need to add a new platform-specific method? This has close to > > the > > > > same arguments to DragTargetDragEnter, what is the meaning/value-added of > > this > > > > new method? > > > > > > See CL: https://codereview.chromium.org/1723763002/ > > > On Android, we don't have the text being dragged until ACTION_DROP. So the > > DropData that's passed during DragTargetDragEnter does not contain the real > text > > being dragged. We have to pass another DropData during ACTION_DROP, and that's > > what this function does. > > > On other platforms (aura), we have access to the dragged when the drag > starts, > > so there is no need to specify the drop data again during 'drop' action. > > > > It doesn't seem great that we're passing in fake/empty data in > > DragTargetDragEnter, that seems misleading. Can you remove the DropData from > > DragTargetDragEnter and refactor all platforms to use your new DropData IPC? > > I can do that. But this would bloat this CL significantly though. I would do it > in another CL that sits between https://codereview.chromium.org/1723763002/ and > this CL. Hi Alex, I did some investigation yesterday and it turns out I can't remove the dropdata from DragEnter on all platforms as that would break the drag and drop spec. https://www.w3.org/TR/2010/WD-html5-20101019/dnd.html#the-dragevent-and-datat... Javascript API requires that "dataTransfer" object which contains the dragged data is available when the drag starts, and when it updates its location. That is to say, Android's drag and drop is breaking the HTML spec inevitably. We might just have to live with that, passing an empty dummy data when the drag starts. At least we're indicating what the type of the data is. We could ask Android to make the data available when the drag starts (if it is possible), but that's for another Android release.
https://codereview.chromium.org/1728193002/diff/160001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1728193002/diff/160001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:611: DropData filtered_data(drop_data); There's a lot of code here to filter the DropData that you aren't running. It has to do with file:// URLs which I realize we probably don't care much about, but for consistency's sake, could you also run this same code on your later DropData? https://codereview.chromium.org/1728193002/diff/160001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:712: Send(new DragMsg_TargetDrop(GetRoutingID(), client_pt_in_viewport, screen_pt, I'd propose modifying this existing message to take a WebDropData instead. For non-Android a DropData object with "m_valid = false" can be sent down that can be ignored on the receiving end. https://codereview.chromium.org/1728193002/diff/160001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:716: #if defined(OS_ANDROID) Could you remove all "#if defined(OS_ANDROID)" from this patch and also from https://codereview.chromium.org/1723763002 ? Just because a codepath happens to currently be run on Android is not a sufficient reason to have them in my mind. They make a lot of things annoying: testing, trybots, code sharing, etc.
Patchset #6 (id:180001) has been deleted
https://codereview.chromium.org/1728193002/diff/160001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1728193002/diff/160001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:611: DropData filtered_data(drop_data); On 2016/03/01 19:57:32, aelias wrote: > There's a lot of code here to filter the DropData that you aren't running. It > has to do with file:// URLs which I realize we probably don't care much about, > but for consistency's sake, could you also run this same code on your later > DropData? Yes. I just did it with the latest patch. https://codereview.chromium.org/1728193002/diff/160001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:712: Send(new DragMsg_TargetDrop(GetRoutingID(), client_pt_in_viewport, screen_pt, On 2016/03/01 19:57:32, aelias wrote: > I'd propose modifying this existing message to take a WebDropData instead. For > non-Android a DropData object with "m_valid = false" can be sent down that can > be ignored on the receiving end. I updated the other CL to change the existing DragTargetDrop IPC to include a content::DropData and a boolean (which indicates if the dropdata is valid). The reason why I am sending content::DropData instead of blink::WebDragData is for consistency: Between browser and renderer, we always send content::DropData, in TargetDragEnter,TargetDrop and StartDragging (from renderer to browser). https://codereview.chromium.org/1728193002/diff/160001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:716: #if defined(OS_ANDROID) On 2016/03/01 19:57:32, aelias wrote: > Could you remove all "#if defined(OS_ANDROID)" from this patch and also from > https://codereview.chromium.org/1723763002 ? Just because a codepath happens to > currently be run on Android is not a sufficient reason to have them in my mind. > They make a lot of things annoying: testing, trybots, code sharing, etc. I merged the new IPC into the existing IPC
Patchset #8 (id:240001) has been deleted
Patchset #10 (id:300001) has been deleted
Hello guys, I just rebased the CL. Thanks to the recent refactors, this CL is much saner and smaller now. I removed the changes in ui/ because they are not needed any more. dcheng@ PTAL content/ sgurun@ PTAL android_webview
https://codereview.chromium.org/1728193002/diff/340001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1728193002/diff/340001/content/browser/androi... content/browser/android/content_view_core_impl.cc:97: ACTION_DRAG_STARTED = 1, Nit: enum class, and just call the members STARTED, etc (it's also not clear what DRAG_LOCATION means, maybe call it MOVED or UPDATED or something?) https://codereview.chromium.org/1728193002/diff/340001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1521: for (base::string16 mime_type : mime_types) { const base::string16, otherwise this copies each member of the collection (same thing above) https://codereview.chromium.org/1728193002/diff/340001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.h (right): https://codereview.chromium.org/1728193002/diff/340001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.h:81: std::vector<int> locations); Let's just pass gfx::Points here. It's a bit more work to convert in the Java->C++ layer, but it'll make this interface a bit clearer. Since I haven't done Java<->C++ in a while... would it be even easier to just plumb these in as individual int params from java? Then we don't need to do the vector conversion, and we can do a straightforward construction of gfx::Point from int coords. https://codereview.chromium.org/1728193002/diff/340001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.h:84: void OnPerformDrop(DropData& drop_data, std::vector<int> locations); Chromium/C++ style doesn't permit mutable reference arguments =/
Patchset #12 (id:360001) has been deleted
https://codereview.chromium.org/1728193002/diff/340001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1728193002/diff/340001/content/browser/androi... content/browser/android/content_view_core_impl.cc:97: ACTION_DRAG_STARTED = 1, On 2016/06/24 23:07:53, dcheng wrote: > Nit: enum class, and just call the members STARTED, etc (it's also not clear > what DRAG_LOCATION means, maybe call it MOVED or UPDATED or something?) Sorry I can't use enum clas here. They have to be ints. This enum is the mirror list of actions of DragEvent in android. I will put a comment here. And that's also why these events are defined in this order and STARTED = 1. https://developer.android.com/reference/android/view/DragEvent.html https://codereview.chromium.org/1728193002/diff/340001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1521: for (base::string16 mime_type : mime_types) { On 2016/06/24 23:07:53, dcheng wrote: > const base::string16, otherwise this copies each member of the collection (same > thing above) Done. https://codereview.chromium.org/1728193002/diff/340001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.h (right): https://codereview.chromium.org/1728193002/diff/340001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.h:81: std::vector<int> locations); On 2016/06/24 23:07:53, dcheng wrote: > Let's just pass gfx::Points here. It's a bit more work to convert in the > Java->C++ layer, but it'll make this interface a bit clearer. > > Since I haven't done Java<->C++ in a while... would it be even easier to just > plumb these in as individual int params from java? Then we don't need to do the > vector conversion, and we can do a straightforward construction of gfx::Point > from int coords. Done. https://codereview.chromium.org/1728193002/diff/340001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.h:84: void OnPerformDrop(DropData& drop_data, std::vector<int> locations); On 2016/06/24 23:07:53, dcheng wrote: > Chromium/C++ style doesn't permit mutable reference arguments =/ use DropData* instead
LGTM
Hi Ted, PTAL content/
On 2016/06/27 20:06:59, hush wrote: > Hi Ted, PTAL content/ aw lgtm
The CQ bit was checked by hush@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.
On 2016/06/27 22:33:48, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. ping ted?
lgtm...i think generating the enums would be nicer, but I'll leave that up to you https://codereview.chromium.org/1728193002/diff/380001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1728193002/diff/380001/content/browser/androi... content/browser/android/content_view_core_impl.cc:99: ACTION_DRAG_STARTED = 1, any reason we don't generate these the same way we do MotionEvent jni bindings? from content/public/android/BUILD.gn generate_jar_jni("jar_jni") { jni_package = "content" classes = [ "java/util/HashSet.class", "android/view/MotionEvent.class", ] } Seems like we could add DragEvent here as well. https://codereview.chromium.org/1728193002/diff/380001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1728193002/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3279: public boolean onDragEvent(DragEvent event) { add /** * @see View#onDragEvent(DragEvent) */ https://codereview.chromium.org/1728193002/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3280: if (mNativeContentViewCore == 0) { make this a one liner since it'll all fit on one
https://codereview.chromium.org/1728193002/diff/380001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1728193002/diff/380001/content/browser/androi... content/browser/android/content_view_core_impl.cc:99: ACTION_DRAG_STARTED = 1, On 2016/06/30 00:15:42, Ted C wrote: > any reason we don't generate these the same way we do MotionEvent jni bindings? > > from content/public/android/BUILD.gn > > generate_jar_jni("jar_jni") { > jni_package = "content" > classes = [ > "java/util/HashSet.class", > "android/view/MotionEvent.class", > ] > } > > Seems like we could add DragEvent here as well. Didn't know we could do that. Done https://codereview.chromium.org/1728193002/diff/380001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1728193002/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3279: public boolean onDragEvent(DragEvent event) { On 2016/06/30 00:15:42, Ted C wrote: > add > > /** > * @see View#onDragEvent(DragEvent) > */ Done. https://codereview.chromium.org/1728193002/diff/380001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3280: if (mNativeContentViewCore == 0) { On 2016/06/30 00:15:42, Ted C wrote: > make this a one liner since it'll all fit on one Done.
The CQ bit was checked by hush@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, sgurun@chromium.org, dcheng@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/1728193002/#ps400001 (title: "generate DragEvent JNI bindings and use the enums from JNI")
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
Try jobs failed on following builders: ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
The CQ bit was checked by hush@chromium.org
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
Try jobs failed on following builders: ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #13 (id:400001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Support dragging texts into Android WebView. WebView gets callback 'onDragEvent' when something is dragged into the WebView. This CL extracts the texts (if any) and location from the DragEvent and creates DropData which is Chromium's internal representation of dragged data. The event plumbing path is WebView#onDragEvent --> AwContents#onDragEvent --> ContentViewCore --> WebContentsViewAndroid. This CL also uses a new blink::WebView API that performs dropping with DropData, because on Android, the DropData is only available when the drop is performed, not when the drag is started. BUG=584789 ========== to ========== Support dragging texts into Android WebView. WebView gets callback 'onDragEvent' when something is dragged into the WebView. This CL extracts the texts (if any) and location from the DragEvent and creates DropData which is Chromium's internal representation of dragged data. The event plumbing path is WebView#onDragEvent --> AwContents#onDragEvent --> ContentViewCore --> WebContentsViewAndroid. This CL also uses a new blink::WebView API that performs dropping with DropData, because on Android, the DropData is only available when the drop is performed, not when the drag is started. BUG=584789 Committed: https://crrev.com/445de0ecb75960ee4060dfcfaf1a86e5632e4d0a Cr-Commit-Position: refs/heads/master@{#403417} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/445de0ecb75960ee4060dfcfaf1a86e5632e4d0a Cr-Commit-Position: refs/heads/master@{#403417} |
