Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1723763002:
Add WebDragData to blink::WebView::dragtargetDrop (Closed)
|
Created:
4 years, 10 months ago by hush (inactive) Modified:
4 years, 6 months ago Reviewers:
Bernhard Bauer, aelias_OOO_until_Jul13, Avi (use Gerrit), dcheng, dmazzoni, lazyboy, no sievers, Vladislav Kaznacheev, lfg CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, newt (away), Paweł Hajdan Jr., sgurun-gerrit only, Ted C Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd WebDragData to blink::WebView::dragtargetDrop.
Different from Windows/Linux, the drag and drop data on Android is not available
when the drag starts. It is only available when the drop is performed (finger
releases from touch screen).
During DragEnter, the meta data of the drop data will be sent to renderer
to construct a WebDragData that contains only this meta data and it will be used by
blink.
This CL adds a WebDragData parameter to blink WebView::dragTargetDrop. When the
dragged data is dropped, the full drop data will be sent to renderer and then to
blink.
This CL also changes the logic around drop data filtering and permisson granting.
Different platforms (Aura, Mac, BrowserPlugin) will filter the drop data before
DragEnter, but not Android, because it is not necessary. The filtered drop data
will be cached and used at Drop for these platforms except Android.
Android logic for drop data will be in a follow up CL.
BUG=584789
Committed: https://crrev.com/8915b509e11743c64a473db748181c62ded64258
Cr-Commit-Position: refs/heads/master@{#399989}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Change the existing API instead #
Total comments: 4
Patch Set 4 : add is_valid to dropdata #
Total comments: 1
Patch Set 5 : comments #
Total comments: 10
Patch Set 6 : address some comments #Patch Set 7 : Send "meta data" of dropData in DragEnter #
Total comments: 4
Patch Set 8 : rebase #
Total comments: 2
Patch Set 9 : rebase #Patch Set 10 : #
Total comments: 1
Patch Set 11 : fix bug #
Total comments: 8
Patch Set 12 : comments #Patch Set 13 : rebase #
Total comments: 13
Patch Set 14 : comments #
Total comments: 18
Patch Set 15 : rebase #Patch Set 16 : rebase #
Total comments: 1
Patch Set 17 : rebase #Patch Set 18 : address comments #
Total comments: 8
Patch Set 19 : comments #
Total comments: 7
Patch Set 20 : Minimum Filtering At DragEnter time #Patch Set 21 : Refactor FilterDropData #Patch Set 22 : fix compile on aura #
Total comments: 20
Patch Set 23 : comments #Patch Set 24 : rebase #Patch Set 25 : use static factory methods #
Total comments: 6
Patch Set 26 : comments #Patch Set 27 : rebase #Patch Set 28 : fix compilation and tests #Patch Set 29 : compile #Patch Set 30 : compile #Patch Set 31 : add has_url #Patch Set 32 : fix chromeos compile + interactive test #
Total comments: 4
Patch Set 33 : fix browser plugin guest logic #
Total comments: 2
Patch Set 34 : always reset ignore_dragged_url_ #
Total comments: 1
Patch Set 35 : {} #Patch Set 36 : dcheng's comment #Patch Set 37 : rebase #
Total comments: 2
Patch Set 38 : review #Messages
Total messages: 176 (65 generated)
I will upload a follow up CL that makes use of this new blink API in a bit.
On 2016/02/23 at 01:41:18, hush wrote: > I will upload a follow up CL that makes use of this new blink API in a bit. Please make sure I'm CCed on any followup CLs. I don't understand though... if DropData isn't available until the actual drop event, how is Blink supposed to know the data types in the drop?
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
On 2016/02/23 03:44:02, dcheng wrote: > On 2016/02/23 at 01:41:18, hush wrote: > > I will upload a follow up CL that makes use of this new blink API in a bit. > > Please make sure I'm CCed on any followup CLs. > > I don't understand though... if DropData isn't available until the actual drop > event, how is Blink supposed to know the data types in the drop? Data type is known when the drag starts. http://developer.android.com/reference/android/view/DragEvent.html DragEvent.getClipDescription() contains the mime types.
hush@chromium.org changed reviewers: + sievers@chromium.org
Hello, dcheng@: PTAL content/common/drag_messages.h and third_party/webkit sievers@: PTAL content/renderer
Patchset #3 (id:40001) has been deleted
Description was changed from
==========
A new blink WebView API to support dropping with DropData.
Different from Windows/Linux, the drag and drop data on Android is not available
when the drag starts. It is only available when the drop is performed (finger
releases from touch screen).
This CL adds a blink WebView API to drop with DropData.
BUG=584789
==========
to
==========
Add WebDragData to blink::WebView::dragtargetDrop.
Different from Windows/Linux, the drag and drop data on Android is not available
when the drag starts. It is only available when the drop is performed (finger
releases from touch screen).
This CL adds WebDragData to blink WebView::dragTargetDrop.
BUG=584789
==========
hush@chromium.org changed reviewers: + aelias@chromium.org
Hi Alex, PTAL!
On 2016/03/01 23:55:56, hush wrote: > Hi Alex, > PTAL! I changed the existing DragTargetDrop IPC according to our discussion in https://codereview.chromium.org/1728193002/
https://codereview.chromium.org/1723763002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/1723763002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.h:124: bool is_drop_data_valid, How about instead adding a boolean member to DropData class equivalent to WebDropData::m_valid? The classes may as well be symmetrical since they're supposed to represent the same information. https://codereview.chromium.org/1723763002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1723763002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:3644: if (!webDragData.isNull()) { nit: no braces for one-line clauses in Blink style
https://codereview.chromium.org/1723763002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/1723763002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.h:124: bool is_drop_data_valid, On 2016/03/02 00:03:19, aelias wrote: > How about instead adding a boolean member to DropData class equivalent to > WebDropData::m_valid? The classes may as well be symmetrical since they're > supposed to represent the same information. Of course, I will do that. https://codereview.chromium.org/1723763002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1723763002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:3644: if (!webDragData.isNull()) { On 2016/03/02 00:03:19, aelias wrote: > nit: no braces for one-line clauses in Blink style Done.
lgtm https://codereview.chromium.org/1723763002/diff/80001/content/public/common/d... File content/public/common/drop_data.h (right): https://codereview.chromium.org/1723763002/diff/80001/content/public/common/d... content/public/common/drop_data.h:41: // Wether this DropData is valid. "Whether this DropData has been initialized" might be more informational.
Description was changed from
==========
Add WebDragData to blink::WebView::dragtargetDrop.
Different from Windows/Linux, the drag and drop data on Android is not available
when the drag starts. It is only available when the drop is performed (finger
releases from touch screen).
This CL adds WebDragData to blink WebView::dragTargetDrop.
BUG=584789
==========
to
==========
Add WebDragData to blink::WebView::dragtargetDrop.
Different from Windows/Linux, the drag and drop data on Android is not available
when the drag starts. It is only available when the drop is performed (finger
releases from touch screen).
This CL adds WebDragData to blink WebView::dragTargetDrop and a new boolean
state is_valid to content::DropData. During drop, the WebDragData will only be
used when is_valid is true.
CL=584789
==========
hush@chromium.org changed reviewers: + avi@chromium.org, lfg@chromium.org
PTAL! lfg: content/browser/browser_plugin avi: content/browser/web_contents/web_drag_dest_mac.mm dcheng: content/common/ sievers: content/public/, content/renderer/
On 2016/03/07 20:35:53, hush wrote: > PTAL! > lfg: content/browser/browser_plugin > avi: content/browser/web_contents/web_drag_dest_mac.mm > dcheng: content/common/ > sievers: content/public/, content/renderer/ content/browser/web_contents/web_drag_dest_mac.mm LGTM
https://codereview.chromium.org/1723763002/diff/100001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1723763002/diff/100001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:823: host->DragTargetDrop(DropData(), location, location, 0); Sanity check, why shouldn't we pass drop_data here?
https://codereview.chromium.org/1723763002/diff/100001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1723763002/diff/100001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:823: host->DragTargetDrop(DropData(), location, location, 0); On 2016/03/07 21:07:10, lfg wrote: > Sanity check, why shouldn't we pass drop_data here? We could, but we don't have to (waste of bytes in IPC?). The drop_data during "WebDragStatusDrop" is not different from the drop_data in "WebDragStatusEnter". So passing an invalid dropdata here will make sure we reuse the DropData in blink::WebViewImpl::dragTargetDrop. We only pass a valid drop data during dragTargetDrop on Android platform.
https://codereview.chromium.org/1723763002/diff/100001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1723763002/diff/100001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:823: host->DragTargetDrop(DropData(), location, location, 0); On 2016/03/07 21:21:31, hush wrote: > On 2016/03/07 21:07:10, lfg wrote: > > Sanity check, why shouldn't we pass drop_data here? > > We could, but we don't have to (waste of bytes in IPC?). The drop_data during > "WebDragStatusDrop" is not different from the drop_data in "WebDragStatusEnter". > > So passing an invalid dropdata here will make sure we reuse the DropData in > blink::WebViewImpl::dragTargetDrop. > We only pass a valid drop data during dragTargetDrop on Android platform. lgtm
Description was changed from ========== Add WebDragData to blink::WebView::dragtargetDrop. Different from Windows/Linux, the drag and drop data on Android is not available when the drag starts. It is only available when the drop is performed (finger releases from touch screen). This CL adds WebDragData to blink WebView::dragTargetDrop and a new boolean state is_valid to content::DropData. During drop, the WebDragData will only be used when is_valid is true. CL=584789 ========== to ========== Add WebDragData to blink::WebView::dragtargetDrop. Different from Windows/Linux, the drag and drop data on Android is not available when the drag starts. It is only available when the drop is performed (finger releases from touch screen). This CL adds WebDragData to blink WebView::dragTargetDrop and a new boolean state is_valid to content::DropData. During drop, the WebDragData will only be used when is_valid is true. BUG=584789 ==========
lgtm https://codereview.chromium.org/1723763002/diff/100001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/100001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:713: // TODO(hush): filter the drop_data if drop data is valid. nit: This comment's meaning is not evident. https://codereview.chromium.org/1723763002/diff/100001/content/public/browser... File content/public/browser/render_view_host.h (right): https://codereview.chromium.org/1723763002/diff/100001/content/public/browser... content/public/browser/render_view_host.h:145: virtual void DragTargetDrop(const DropData& drop_data, nit: Can you also document here that depending on the platform the drop_data is either passed in Enter() or Drop()? https://codereview.chromium.org/1723763002/diff/100001/content/renderer/rende... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/1723763002/diff/100001/content/renderer/rende... content/renderer/render_view_impl.h:634: void OnDragTargetDrop(const content::DropData& drop_data, nit: drop 'content::'
Looking at the drag and drop plumbing, it's going to be awkward to handle the WebDragData -> DataObject conversion in drag enter on Android when we only have type information. Instead, can we change the public API to only pass a WebDragData in WebView::dragTargetDrop and pass some lightweight version of that in dragTargetEnter? We're going to need to make the plumbing to make this work for Android anyway, and it'll make the Blink public API look nicer (since WebDragData will only get passed in one place). https://codereview.chromium.org/1723763002/diff/100001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/100001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:713: // TODO(hush): filter the drop_data if drop data is valid. On 2016/03/08 at 01:22:15, sievers wrote: > nit: This comment's meaning is not evident. Can we just extract the logic from DragTargetEnter into a helper and call it here as well? That'll save the TODO, and it'll prevent us from missing this later when Android plumbs it through here.
On 2016/03/08 01:44:08, dcheng wrote: > Looking at the drag and drop plumbing, it's going to be awkward to handle the > WebDragData -> DataObject conversion in drag enter on Android when we only have > type information. > > Instead, can we change the public API to only pass a WebDragData in > WebView::dragTargetDrop and pass some lightweight version of that in > dragTargetEnter? We're going to need to make the plumbing to make this work for > Android anyway, and it'll make the Blink public API look nicer (since > WebDragData will only get passed in one place). > That was my original plan and it didn't work out. Please see the discussion here (a different CL that builds on top of this one) https://codereview.chromium.org/1728193002/diff/120001/content/public/browser... We *have to* pass the full-fledged drag data during DragStart because JS API needs it. On Android, we are breaking the drag and drop JS API inevitably.
https://codereview.chromium.org/1723763002/diff/100001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/100001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:713: // TODO(hush): filter the drop_data if drop data is valid. On 2016/03/08 01:44:08, dcheng wrote: > On 2016/03/08 at 01:22:15, sievers wrote: > > nit: This comment's meaning is not evident. > > Can we just extract the logic from DragTargetEnter into a helper and call it > here as well? That'll save the TODO, and it'll prevent us from missing this > later when Android plumbs it through here. I've done it in a follow up CL that builds on top of this one. https://codereview.chromium.org/1728193002/ I try to make this CL related to the API plumbing only (while maintaining correctness)
https://codereview.chromium.org/1723763002/diff/100001/content/public/browser... File content/public/browser/render_view_host.h (right): https://codereview.chromium.org/1723763002/diff/100001/content/public/browser... content/public/browser/render_view_host.h:145: virtual void DragTargetDrop(const DropData& drop_data, On 2016/03/08 01:22:15, sievers wrote: > nit: Can you also document here that depending on the platform the drop_data is > either passed in Enter() or Drop()? Done. https://codereview.chromium.org/1723763002/diff/100001/content/renderer/rende... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/1723763002/diff/100001/content/renderer/rende... content/renderer/render_view_impl.h:634: void OnDragTargetDrop(const content::DropData& drop_data, On 2016/03/08 01:22:16, sievers wrote: > nit: drop 'content::' Done.
On 2016/03/08 at 18:21:04, hush wrote: > On 2016/03/08 01:44:08, dcheng wrote: > > Looking at the drag and drop plumbing, it's going to be awkward to handle the > > WebDragData -> DataObject conversion in drag enter on Android when we only have > > type information. > > > > Instead, can we change the public API to only pass a WebDragData in > > WebView::dragTargetDrop and pass some lightweight version of that in > > dragTargetEnter? We're going to need to make the plumbing to make this work for > > Android anyway, and it'll make the Blink public API look nicer (since > > WebDragData will only get passed in one place). > > > > That was my original plan and it didn't work out. Please see the discussion here (a different CL that builds on top of this one) > https://codereview.chromium.org/1728193002/diff/120001/content/public/browser... > > We *have to* pass the full-fledged drag data during DragStart because JS API needs it. On Android, we are breaking the drag and drop JS API inevitably. The drag and drop web API is specced so that a web page can only read the types from Event.dataTransfer during drag/dragenter/dragover events: actual reading of the data is only allowed in the drop event. Previously, the Blink API just sent the entire WebDragData over in dragenter, since that was the simplest thing to do. Since we want to support Android and Android (matching how the web API works) only has type information ta the time, we should just have a "lite" version of WebDragData/DataTransfer that only contains types. When we're firing the dragenter/dragover events, we can just use the lite version of this drag data.
On 2016/03/08 18:45:56, dcheng wrote: > On 2016/03/08 at 18:21:04, hush wrote: > > On 2016/03/08 01:44:08, dcheng wrote: > > > Looking at the drag and drop plumbing, it's going to be awkward to handle > the > > > WebDragData -> DataObject conversion in drag enter on Android when we only > have > > > type information. > > > > > > Instead, can we change the public API to only pass a WebDragData in > > > WebView::dragTargetDrop and pass some lightweight version of that in > > > dragTargetEnter? We're going to need to make the plumbing to make this work > for > > > Android anyway, and it'll make the Blink public API look nicer (since > > > WebDragData will only get passed in one place). > > > > > > > That was my original plan and it didn't work out. Please see the discussion > here (a different CL that builds on top of this one) > > > https://codereview.chromium.org/1728193002/diff/120001/content/public/browser... > > > > We *have to* pass the full-fledged drag data during DragStart because JS API > needs it. On Android, we are breaking the drag and drop JS API inevitably. > > The drag and drop web API is specced so that a web page can only read the types > from Event.dataTransfer during drag/dragenter/dragover events: actual reading of > the data is only allowed in the drop event. Previously, the Blink API just sent > the entire WebDragData over in dragenter, since that was the simplest thing to > do. Since we want to support Android and Android (matching how the web API > works) only has type information ta the time, we should just have a "lite" > version of WebDragData/DataTransfer that only contains types. When we're firing > the dragenter/dragover events, we can just use the lite version of this drag > data. I read the spec again and yes you are right. The spec does not allow reading the drop data during dragEnter, dragOver events, even though there is a dataTransfer object. So Android drag and drop API is not breaking the web after all... I'm going to refactor blink dragstart API to accept a "lite" version of drop data.
Hi dcheng@ and aelias@: I looked into a bit more. Creating a separate data type for "DragDataLite" seems overkill to me, and it will make the blink code unnecessarily complicated. Firstly, the new DragDataLite class would have mostly identical APIs to blink::DragData, minus "asPlainText", "asUrl", "asFilePaths", "asFragment". If I go this route, I will probably make DragData be composited of DragDataLite and the real platformDragData. Secondly, this DragDataLite/DragData duality is actually inconsistent with JS API. Throughout the drag drop operation, you would have access to "dataTransfer" object. You will get an empty string if you attempt to call dataTransfer.getData(mimetype) in "protected mode". Just that. There is no "dataTransferLite" notion in JS API in "protected mode". https://html.spec.whatwg.org/multipage/interaction.html#dom-datatransfer-getdata Anyway, I chatted with aelias@ about this too. What we actually need is not to be confusing on the content <--> blink API, and be consistent with Android dragEvent and blink dragEvent.dataTransfer API. Can we do the following instead: Add a state in blink::DragData and content::DropData indicating if its can be read, essentially if it is in "protected mode". (https://html.spec.whatwg.org/multipage/interaction.html#concept-dnd-p) You can still enumerate the contents by the way, like counting how many files are dragged. Let's call this state "can_read_contents". On all platforms: The DragData during "DragEnter" event will have can_read_contents == false, and will not contain the real content (whether it is string, url list, or file) The DragData during "drop" event will have can_read_contents == true and will contain the real content.
Patchset #7 (id:140001) has been deleted
Hello Daniel (dcheng), please take a look at patch set 7. Basically what I did was that I stripped the real data from blink::WebDragData in RenderViewImpl during DragEnter event. So blink will always get a "meta data" DragData until Drop event. I plumbed a boolean state "canReadContent" in WebDragData, DataObject and DragData. This is used by DragData::asFilePaths so that it does not discard empty filenames. DragData::asFilePaths is used by DragController to determine the "numberOfItemsToBeAccepted", the real file names are not used. I also added a "has_url" boolean into content::DropData. The other data types in DropData can differentiate between null value and empty value, except for url data.
ping..
Apologies for the delay. Overall, I wonder if we can do this without adding things like has_url. I'm thinking something like this: 1. On the browser side, we can still build DropData from the platform drag data as appropriate (e.g. everywhere but Android) in the drag enter handler. 2. However, for the IPC, all we need to send to the renderer is a vector of pairs of MIME type + "kind" [1]. We should be able to build this pretty easily while populating DropData (or just derive it from DropData). 3. In the renderer side for handling DragTargetDragEnter, we can create a bunch of dummy WebDragData::Items with no actual data but with types set. That should be good enough for Blink. 4. Then for the drop event, we can send the DropData we built earlier over (if non-Android: Android will have to build it here). Then everything proceeds according to the remainder of this CL. [1] https://html.spec.whatwg.org/multipage/interaction.html#dom-datatransferitem-... https://codereview.chromium.org/1723763002/diff/160001/content/public/browser... File content/public/browser/render_view_host.h (right): https://codereview.chromium.org/1723763002/diff/160001/content/public/browser... content/public/browser/render_view_host.h:135: // available and passed in here. I think there's no need to mention Android vs non-Android. https://codereview.chromium.org/1723763002/diff/160001/content/public/browser... content/public/browser/render_view_host.h:153: // data from DragEnter. Is this comment still accurate? https://codereview.chromium.org/1723763002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/clipboard/DataObject.h (right): https://codereview.chromium.org/1723763002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/clipboard/DataObject.h:118: bool m_canReadContent; This shouldn't be necessary, DataTransfer/DataTransferItemList/DataTransferItem already have an access policy enum: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... which enforces that only types can be read in dragenter / dragover, etc.
Patchset #10 (id:220001) has been deleted
Just updated patch set. Testing in process.. https://codereview.chromium.org/1723763002/diff/160001/content/public/browser... File content/public/browser/render_view_host.h (right): https://codereview.chromium.org/1723763002/diff/160001/content/public/browser... content/public/browser/render_view_host.h:135: // available and passed in here. On 2016/03/30 08:40:02, dcheng wrote: > I think there's no need to mention Android vs non-Android. I will remove this comment. https://codereview.chromium.org/1723763002/diff/160001/content/public/browser... content/public/browser/render_view_host.h:153: // data from DragEnter. On 2016/03/30 08:40:02, dcheng wrote: > Is this comment still accurate? Nope. Removed. https://codereview.chromium.org/1723763002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/clipboard/DataObject.h (right): https://codereview.chromium.org/1723763002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/clipboard/DataObject.h:118: bool m_canReadContent; On 2016/03/30 08:40:02, dcheng wrote: > This shouldn't be necessary, DataTransfer/DataTransferItemList/DataTransferItem > already have an access policy enum: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > which enforces that only types can be read in dragenter / dragover, etc. Yeah. Removed. https://codereview.chromium.org/1723763002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/DragData.cpp (right): https://codereview.chromium.org/1723763002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/DragData.cpp:91: unsigned DragData::numberOfFiles() const I need explain why I expose this function here: DragController wants to know how many files are dragged during DragEnter by calling DragData::asFilePaths, which ignores empty filenames. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... With the current code, the filenames are all empty at DragEnter time. So I expose this method for DragController's use. At the same time, I make sure empty filenames are ignored in content/ layer. See DropDataToMetaData in RenderViewHostImpl.
Description was changed from ========== Add WebDragData to blink::WebView::dragtargetDrop. Different from Windows/Linux, the drag and drop data on Android is not available when the drag starts. It is only available when the drop is performed (finger releases from touch screen). This CL adds WebDragData to blink WebView::dragTargetDrop and a new boolean state is_valid to content::DropData. During drop, the WebDragData will only be used when is_valid is true. BUG=584789 ========== to ========== Add WebDragData to blink::WebView::dragtargetDrop. Different from Windows/Linux, the drag and drop data on Android is not available when the drag starts. It is only available when the drop is performed (finger releases from touch screen). During DragEnter, a vector of (mime type, kind) pairs will be sent to renderer to construct a WebDragData that contains only this meta data and it will be used by blink. This CL adds a WebDragData parameter to blink WebView::dragTargetDrop. When the dragged data is dropped, the full drop data will be sent to renderer and then to blink. CL=584789 ==========
Patchset #11 (id:260001) has been deleted
Patchset #12 (id:300001) has been deleted
I tested on Linux and on Android (with https://codereview.chromium.org/1728193002/) and it worked. PTAL
not lgtm https://codereview.chromium.org/1723763002/diff/280001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/280001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:215: } For these three loops, you easily could do |for (const auto& item : items)| for clarity. https://codereview.chromium.org/1723763002/diff/280001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:1359: iter != filtered_data.filenames.end(); ++iter) { Prefer |for (:)| syntax; it's cleaner and easier to read and write. https://codereview.chromium.org/1723763002/diff/280001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/1723763002/diff/280001/content/browser/render... content/browser/renderer_host/render_view_host_impl.h:355: void FilterDropData(DropData& filtered_data); Blank line after. https://codereview.chromium.org/1723763002/diff/280001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1723763002/diff/280001/content/renderer/rende... content/renderer/render_view_impl.cc:448: if (pair.second == base::ASCIIToUTF16("filesystemfile")) { I very much dislike the idea of matching up strings on either end of a pipe. Either use an enum, or have three separate arrays, each with its own type. As an aside, re-declaring your constants on both sides is a very very very bad idea. Even if you switch to an enum, you *must* have them share a common declaration. If you have a list of constants in foo_host.cc and a list of the same constants in foo.cc, you are *guaranteeing* that somewhere down the line they will get out of sync.
Patchset #12 (id:320001) has been deleted
https://codereview.chromium.org/1723763002/diff/280001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/280001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:215: } On 2016/04/08 21:24:08, Avi wrote: > For these three loops, you easily could do |for (const auto& item : items)| for > clarity. Done. This is definitely better with the new syntax https://codereview.chromium.org/1723763002/diff/280001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:1359: iter != filtered_data.filenames.end(); ++iter) { On 2016/04/08 21:24:08, Avi wrote: > Prefer |for (:)| syntax; it's cleaner and easier to read and write. done https://codereview.chromium.org/1723763002/diff/280001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/1723763002/diff/280001/content/browser/render... content/browser/renderer_host/render_view_host_impl.h:355: void FilterDropData(DropData& filtered_data); On 2016/04/08 21:24:08, Avi wrote: > Blank line after. Done. https://codereview.chromium.org/1723763002/diff/280001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1723763002/diff/280001/content/renderer/rende... content/renderer/render_view_impl.cc:448: if (pair.second == base::ASCIIToUTF16("filesystemfile")) { On 2016/04/08 21:24:08, Avi wrote: > I very much dislike the idea of matching up strings on either end of a pipe. > Either use an enum, or have three separate arrays, each with its own type. > > As an aside, re-declaring your constants on both sides is a very very very bad > idea. Even if you switch to an enum, you *must* have them share a common > declaration. If you have a list of constants in foo_host.cc and a list of the > same constants in foo.cc, you are *guaranteeing* that somewhere down the line > they will get out of sync. I will use an enum type, defined in drop_data.h, which is used by both end of the IPC.
Description was changed from ========== Add WebDragData to blink::WebView::dragtargetDrop. Different from Windows/Linux, the drag and drop data on Android is not available when the drag starts. It is only available when the drop is performed (finger releases from touch screen). During DragEnter, a vector of (mime type, kind) pairs will be sent to renderer to construct a WebDragData that contains only this meta data and it will be used by blink. This CL adds a WebDragData parameter to blink WebView::dragTargetDrop. When the dragged data is dropped, the full drop data will be sent to renderer and then to blink. CL=584789 ========== to ========== Add WebDragData to blink::WebView::dragtargetDrop. Different from Windows/Linux, the drag and drop data on Android is not available when the drag starts. It is only available when the drop is performed (finger releases from touch screen). During DragEnter, a vector of (mime type, kind) pairs will be sent to renderer to construct a WebDragData that contains only this meta data and it will be used by blink. This CL adds a WebDragData parameter to blink WebView::dragTargetDrop. When the dragged data is dropped, the full drop data will be sent to renderer and then to blink. BUG=584789 ==========
https://codereview.chromium.org/1723763002/diff/350001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/350001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:169: std::vector<MimeTypeKindPair>& meta_data) { Reference parameters aren't allowed for out parameters (as per the style guide, https://google.github.io/styleguide/cppguide.html#Reference_Arguments ). Make this be: std::vector<MimeTypeKindPair> DropDataToMetaData(const DropData& drop_data) { ... } https://codereview.chromium.org/1723763002/diff/350001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:1391: for (auto file_system_file : filtered_data.file_system_files) { Did you mean for (auto& file_system_file : ...) ? On line 1403 (below) you modify the ".url" of file_system_file, but if you use a plain "for (auto file_system_file" here, then file_system_file is a *copy*, and you're modifying that copy below. https://codereview.chromium.org/1723763002/diff/350001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:1403: file_system_file.url = As noted above, you are modifying a local copy here unless you do auto& in the loop. https://codereview.chromium.org/1723763002/diff/350001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/1723763002/diff/350001/content/browser/render... content/browser/renderer_host/render_view_host_impl.h:349: void FilterDropData(DropData& filtered_data); This is not allowed by the style guide; reference arguments must be const. This modifies the argument, so pass it in as a pointer. https://google.github.io/styleguide/cppguide.html#Reference_Arguments https://codereview.chromium.org/1723763002/diff/350001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1723763002/diff/350001/content/renderer/rende... content/renderer/render_view_impl.cc:431: const std::vector<MimeTypeKindPair> drop_meta_data) { const vector? Did you mean const vector&? https://codereview.chromium.org/1723763002/diff/350001/content/renderer/rende... content/renderer/render_view_impl.cc:2354: const std::vector<MimeTypeKindPair> drop_meta_data, const vector&? https://codereview.chromium.org/1723763002/diff/350001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/1723763002/diff/350001/third_party/WebKit/pub... third_party/WebKit/public/web/WebView.h:352: = 0; Why the linebreak here?
Patchset #14 (id:370001) has been deleted
https://codereview.chromium.org/1723763002/diff/350001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/350001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:169: std::vector<MimeTypeKindPair>& meta_data) { On 2016/04/12 02:51:51, Avi wrote: > Reference parameters aren't allowed for out parameters (as per the style guide, > https://google.github.io/styleguide/cppguide.html#Reference_Arguments ). Make > this be: > > std::vector<MimeTypeKindPair> DropDataToMetaData(const DropData& drop_data) { > ... } Done. https://codereview.chromium.org/1723763002/diff/350001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:1391: for (auto file_system_file : filtered_data.file_system_files) { On 2016/04/12 02:51:51, Avi wrote: > Did you mean > > for (auto& file_system_file : ...) ? > > On line 1403 (below) you modify the ".url" of file_system_file, but if you use a > plain "for (auto file_system_file" here, then file_system_file is a *copy*, and > you're modifying that copy below. Yeah. Sorry I missed it. Added &. https://codereview.chromium.org/1723763002/diff/350001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/1723763002/diff/350001/content/browser/render... content/browser/renderer_host/render_view_host_impl.h:349: void FilterDropData(DropData& filtered_data); On 2016/04/12 02:51:51, Avi wrote: > This is not allowed by the style guide; reference arguments must be const. > > This modifies the argument, so pass it in as a pointer. > > https://google.github.io/styleguide/cppguide.html#Reference_Arguments Done. https://codereview.chromium.org/1723763002/diff/350001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1723763002/diff/350001/content/renderer/rende... content/renderer/render_view_impl.cc:431: const std::vector<MimeTypeKindPair> drop_meta_data) { On 2016/04/12 02:51:51, Avi wrote: > const vector? > > Did you mean const vector&? Yes, done https://codereview.chromium.org/1723763002/diff/350001/content/renderer/rende... content/renderer/render_view_impl.cc:2354: const std::vector<MimeTypeKindPair> drop_meta_data, On 2016/04/12 02:51:51, Avi wrote: > const vector&? Done. https://codereview.chromium.org/1723763002/diff/350001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/1723763002/diff/350001/third_party/WebKit/pub... third_party/WebKit/public/web/WebView.h:352: = 0; On 2016/04/12 02:51:51, Avi wrote: > Why the linebreak here? git cl format did it for me. Don't know why..
lgtm!
ping dcheng@ ..
https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:173: meta_data.push_back(pair); I think it would be appropriate to use emplace_back here. meta_data.emplace_back(base::ASCIIToUTF16(ui::Clipboard::kMimeTypeText), STRING_KIND)); etc https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:198: std::make_pair(base::string16(), FILESYSTEMFILE_KIND); Unfortunately, making things complicated, I think we might actually want to plumb through all the information for drop_data.filenames and drop_data.file_system_files. Otherwise, the MIME type plumbing for DataTransferItem won't work: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit..., etc. https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:638: #if defined(OS_CHROMEOS) This block should also be in FilterDropData. https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:650: // and can't be interpreted as a capability. This comment should be moved into FilterDropData https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:652: FilterDropData(&filtered_data); Filtering grants privileges, etc, and it seems like most of these permission grants should only happen in the drop. 1) Is it possible to refactor this interface to take ownership of the drop data (e.g. pass by unique_ptr) so we only have to filter it once? Or is it possible for the caller pre-filter before calling this? Would this work with the model you have in mind for Android? 2) Let's split the permission grants out of filtering if possible. https://codereview.chromium.org/1723763002/diff/390001/content/browser/web_co... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/1723763002/diff/390001/content/browser/web_co... content/browser/web_contents/web_contents_view_aura.cc:1125: *current_drop_data_.get(), event.location(), No .get() is necessary here. https://codereview.chromium.org/1723763002/diff/390001/content/public/common/... File content/public/common/drop_data.h (right): https://codereview.chromium.org/1723763002/diff/390001/content/public/common/... content/public/common/drop_data.h:27: enum DropDataKind { enum class DropDataKind { STRING, FILENAME, FILESYSTEMFILE, LAST = FILESYSTEMFILE }; https://codereview.chromium.org/1723763002/diff/390001/content/public/common/... content/public/common/drop_data.h:34: typedef std::pair<base::string16, DropDataKind> MimeTypeKindPair; Nit: using MimeTypeKindPair = ...
sgurun@chromium.org changed reviewers: + kaznacheev@chromium.org
https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:198: std::make_pair(base::string16(), FILESYSTEMFILE_KIND); On 2016/04/19 06:58:47, dcheng wrote: > Unfortunately, making things complicated, I think we might actually want to > plumb through all the information for drop_data.filenames and > drop_data.file_system_files. Otherwise, the MIME type plumbing for > DataTransferItem won't work: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit..., > etc. Right... This also means that we just can't do files correctly on Android platform. The reason is we don't know the exact mime type of the file. I just shared a public doc with you about this to elaborate. (Android N multi window is already public in preview) Our best guess on the mime type during DragEnter, is the mime type from ClipDescription. Well, this is usually "text/uri-list" which means a Uri to a file is being dragged, or something random set by the drag source app.
https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:198: std::make_pair(base::string16(), FILESYSTEMFILE_KIND); On 2016/04/28 at 22:13:26, hush wrote: > On 2016/04/19 06:58:47, dcheng wrote: > > Unfortunately, making things complicated, I think we might actually want to > > plumb through all the information for drop_data.filenames and > > drop_data.file_system_files. Otherwise, the MIME type plumbing for > > DataTransferItem won't work: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit..., > > etc. > > Right... This also means that we just can't do files correctly on Android platform. The reason is we don't know the exact mime type of the file. I just shared a public doc with you about this to elaborate. (Android N multi window is already public in preview) > > Our best guess on the mime type during DragEnter, is the mime type from ClipDescription. Well, this is usually "text/uri-list" which means a Uri to a file is being dragged, or something random set by the drag source app. I think it's OK if it doesn't work perfectly on Android, but we should try to keep the existing bits that already work functional. https://codereview.chromium.org/1723763002/diff/430001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1723763002/diff/430001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3681: void WebViewImpl::dragTargetDrop(const WebDragData& webDragData, const WebPoint& clientPoint, Nit: const WebDragData& dragData
Patchset #18 (id:470001) has been deleted
Patchset #18 (id:490001) has been deleted
https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:173: meta_data.push_back(pair); On 2016/04/19 06:58:47, dcheng wrote: > I think it would be appropriate to use emplace_back here. > > meta_data.emplace_back(base::ASCIIToUTF16(ui::Clipboard::kMimeTypeText), > STRING_KIND)); > > etc Done. And I changed "MimeTypeKindPair" into a "MetaData" struct, because it is not simply a string to string pair any more. https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:198: std::make_pair(base::string16(), FILESYSTEMFILE_KIND); On 2016/05/04 05:21:34, dcheng wrote: > On 2016/04/28 at 22:13:26, hush wrote: > > On 2016/04/19 06:58:47, dcheng wrote: > > > Unfortunately, making things complicated, I think we might actually want to > > > plumb through all the information for drop_data.filenames and > > > drop_data.file_system_files. Otherwise, the MIME type plumbing for > > > DataTransferItem won't work: > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit..., > > > etc. > > > > Right... This also means that we just can't do files correctly on Android > platform. The reason is we don't know the exact mime type of the file. I just > shared a public doc with you about this to elaborate. (Android N multi window is > already public in preview) > > > > Our best guess on the mime type during DragEnter, is the mime type from > ClipDescription. Well, this is usually "text/uri-list" which means a Uri to a > file is being dragged, or something random set by the drag source app. > > I think it's OK if it doesn't work perfectly on Android, but we should try to > keep the existing bits that already work functional. Done. https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:638: #if defined(OS_CHROMEOS) On 2016/04/19 06:58:47, dcheng wrote: > This block should also be in FilterDropData. Done. https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:650: // and can't be interpreted as a capability. On 2016/04/19 06:58:47, dcheng wrote: > This comment should be moved into FilterDropData Done. https://codereview.chromium.org/1723763002/diff/390001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:652: FilterDropData(&filtered_data); On 2016/04/19 06:58:47, dcheng wrote: > Filtering grants privileges, etc, and it seems like most of these permission > grants should only happen in the drop. > > 1) Is it possible to refactor this interface to take ownership of the drop data > (e.g. pass by unique_ptr) so we only have to filter it once? Or is it possible > for the caller pre-filter before calling this? Would this work with the model > you have in mind for Android? > > 2) Let's split the permission grants out of filtering if possible. I refactored it by splitting into 2 parts: A. register in isolated file systems and crack the urls B. grant permissions Part A is done in DragEnter and B is done in Drop. Android situation is quite different. We only have the Uri in Drop time. So no "filtering" is needed at DragEnter. And we only need to read the Uri at Drop. No permission granting is needed either. If I understand correctly, the point of isolated context is to prevent the renderer from poking around in the OS' file system, which is already achieved by Android's own permission granting mechanisms. The permission to read the Uri is granted by the source app that the drag initiates via a flag to DnD operation. https://codereview.chromium.org/1723763002/diff/390001/content/browser/web_co... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/1723763002/diff/390001/content/browser/web_co... content/browser/web_contents/web_contents_view_aura.cc:1125: *current_drop_data_.get(), event.location(), On 2016/04/19 06:58:47, dcheng wrote: > No .get() is necessary here. Done. https://codereview.chromium.org/1723763002/diff/390001/content/public/common/... File content/public/common/drop_data.h (right): https://codereview.chromium.org/1723763002/diff/390001/content/public/common/... content/public/common/drop_data.h:27: enum DropDataKind { On 2016/04/19 06:58:47, dcheng wrote: > enum class DropDataKind { > STRING, > FILENAME, > FILESYSTEMFILE, > LAST = FILESYSTEMFILE > }; Done. And I put them under DropData class. https://codereview.chromium.org/1723763002/diff/390001/content/public/common/... content/public/common/drop_data.h:34: typedef std::pair<base::string16, DropDataKind> MimeTypeKindPair; On 2016/04/19 06:58:47, dcheng wrote: > Nit: using MimeTypeKindPair = ... I removed this because of the complication of file case on Android VS Aura. Instead, I created a MetaData class under DropData.
Thanks for updating the CL, just a few more comments. https://codereview.chromium.org/1723763002/diff/510001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/510001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:641: FilterDropData(&filtered_data); Since we extracted this to a separate function, I think it would look nicer to write: DropData filtered_data = FilterDropData(drop_data); https://codereview.chromium.org/1723763002/diff/510001/content/public/common/... File content/public/common/drop_data.h (right): https://codereview.chromium.org/1723763002/diff/510001/content/public/common/... content/public/common/drop_data.h:44: MetaData(DropData::Kind kind, base::string16 mime_type); const base::string16, here and below. https://codereview.chromium.org/1723763002/diff/510001/content/public/common/... content/public/common/drop_data.h:48: ~MetaData() {} I'm surprised that clang's complex destructor check isn't firing here. https://codereview.chromium.org/1723763002/diff/510001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1723763002/diff/510001/content/renderer/rende... content/renderer/render_view_impl.cc:444: item.filenameData = meta_data_item.filename.AsUTF16Unsafe(); Do we need to update this to handle the mime-type only case?
Patchset #19 (id:530001) has been deleted
https://codereview.chromium.org/1723763002/diff/510001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/510001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:641: FilterDropData(&filtered_data); On 2016/05/10 07:29:08, dcheng wrote: > Since we extracted this to a separate function, I think it would look nicer to > write: > > DropData filtered_data = FilterDropData(drop_data); Done. https://codereview.chromium.org/1723763002/diff/510001/content/public/common/... File content/public/common/drop_data.h (right): https://codereview.chromium.org/1723763002/diff/510001/content/public/common/... content/public/common/drop_data.h:44: MetaData(DropData::Kind kind, base::string16 mime_type); On 2016/05/10 07:29:09, dcheng wrote: > const base::string16, here and below. Done. https://codereview.chromium.org/1723763002/diff/510001/content/public/common/... content/public/common/drop_data.h:48: ~MetaData() {} On 2016/05/10 07:29:09, dcheng wrote: > I'm surprised that clang's complex destructor check isn't firing here. Clang may have just missed it. I moved the destructor to cc file. https://codereview.chromium.org/1723763002/diff/510001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1723763002/diff/510001/content/renderer/rende... content/renderer/render_view_impl.cc:444: item.filenameData = meta_data_item.filename.AsUTF16Unsafe(); On 2016/05/10 07:29:09, dcheng wrote: > Do we need to update this to handle the mime-type only case? We definitely have to. I haven't figured out all the details about how to handle the dragging file case yet. But it seems like a blink plumbing is necessary, to allow creating a File with just mime type. I will do this in a later CL for handling files on Android. It won't be the immediate next step either. The next step would be dragging plain texts from an app to WebView/Chrome. So... for now, I will just leave a to do here.
https://codereview.chromium.org/1723763002/diff/550001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/550001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:662: GrantPermissionsToDraggedFiles(drop_data); We probably still want to filter here, to avoid data being inconsistent. I'm still not super happy that we have to pass DropData twice now, but that seems unavoidable. Would it make sense to move the filtering into WebContentsAura / WebContentsAndroid, to avoid the double filtering here? https://codereview.chromium.org/1723763002/diff/550001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/1723763002/diff/550001/content/browser/render... content/browser/renderer_host/render_view_host_impl.h:353: std::vector<std::string> filesystem_ids_to_grant_permissions_; Can this just be a local variable? It looks like it's just clear()ed at the end. https://codereview.chromium.org/1723763002/diff/550001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1723763002/diff/550001/content/renderer/rende... content/renderer/render_view_impl.cc:443: // platform. We should probably skip adding an item if it has no filename, since Blink won't know what to do with it.
https://codereview.chromium.org/1723763002/diff/550001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/550001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:662: GrantPermissionsToDraggedFiles(drop_data); On 2016/05/11 01:19:15, dcheng wrote: > We probably still want to filter here, to avoid data being inconsistent. What happens if the drop data at Drop time is indeed inconsistent with DragEnter time? This situation does not make sense and (I think) it is the operating system's fault if this happens. If the data at Drop time can be inconsistent with DragEnter time, then we cannot reliably trust the mime types from the drop data at DragEnter time. The Javascript looking at dragged mime types can be tricked. I think we should just DCHECK that the drop data is consistent. At the moment, both Aura and Android satisfy the consistency. Also, the implementation before my CL has assumed consistency, by ignoring drop data at Drop time. > > I'm still not super happy that we have to pass DropData twice now, but that > seems unavoidable. Would it make sense to move the filtering into > WebContentsAura / WebContentsAndroid, to avoid the double filtering here? This will be too complicated and too much duplicate code, because there aren't just WebContentsAura / WebContentsAndroid that would call RVHI::DragTargetDrop/DragTargetDragEnter. There are also WebDragDestMac (seems that Mac has specific drag and drop code, instead of using WebContentsAura), BrowserPluginGuest (this seems related to drag and drop in Chrome apps/extensions)
No new patch set yet. Just responding to comments. Hi Daniel, I gave the double filtering issue a little more thought, and I propose this: At DragEnter time, we don't actually need much filtering, except for clearing all filenames if did_originate_from_renderer == ture. But other filtering done in FilterDropData does not affect existential status of a dragged mime type. So I propose we just call DropDataToMetaData at DragEnter time, (and clear filenames if necessary) Then we do filtering data, registering with isolated context to retrieve a file system id, grant permissions, cracking URLs etc all at Drop time. Note there is one assumption I am making (I didn't read the isolated context code yet, if you on the top of the head, please tell): the file name extension won't change if you register a file system url with isolated context. As you see in the code, for any file system url, we firstly crack it, then register it with isolated context and get a "register_name" out of it. In blink's File app, this register_name's extension will be used to determine the mime type.
On 2016/05/12 at 05:47:06, hush wrote: > No new patch set yet. Just responding to comments. > Hi Daniel, I gave the double filtering issue a little more thought, and I propose this: > At DragEnter time, we don't actually need much filtering, except for clearing all filenames if did_originate_from_renderer == ture. > But other filtering done in FilterDropData does not affect existential status of a dragged mime type. The URL might also be filtered out, which would result in no URL being present. > So I propose we just call DropDataToMetaData at DragEnter time, (and clear filenames if necessary) > Then we do filtering data, registering with isolated context to retrieve a file system id, grant permissions, cracking URLs etc all at Drop time. Duplicating this logic makes me nervous. I don't think it's that big a burden to make the platform-specific entry points filter the drop data before passing it to RVHI. Of course, this doesn't work well for Android... but maybe that's OK on Android? > > Note there is one assumption I am making (I didn't read the isolated context code yet, if you on the top of the head, please tell): the file name extension won't change if you register a file system url with isolated context. > As you see in the code, for any file system url, we firstly crack it, then register it with isolated context and get a "register_name" out of it. In blink's File app, this register_name's extension will be used to determine the mime type. https://codereview.chromium.org/1723763002/diff/550001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/550001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:662: GrantPermissionsToDraggedFiles(drop_data); On 2016/05/12 at 05:34:47, hush wrote: > On 2016/05/11 01:19:15, dcheng wrote: > > We probably still want to filter here, to avoid data being inconsistent. > What happens if the drop data at Drop time is indeed inconsistent with DragEnter time? This situation does not make sense and (I think) it is the operating system's fault if this happens. If the data at Drop time can be inconsistent with DragEnter time, then we cannot reliably trust the mime types from the drop data at DragEnter time. The Javascript looking at dragged mime types can be tricked. > > I think we should just DCHECK that the drop data is consistent. At the moment, both Aura and Android satisfy the consistency. Also, the implementation before my CL has assumed consistency, by ignoring drop data at Drop time. > > > > > I'm still not super happy that we have to pass DropData twice now, but that > > seems unavoidable. Would it make sense to move the filtering into > > WebContentsAura / WebContentsAndroid, to avoid the double filtering here? > > This will be too complicated and too much duplicate code, because there aren't just WebContentsAura / WebContentsAndroid that would call RVHI::DragTargetDrop/DragTargetDragEnter. There are also WebDragDestMac (seems that Mac has specific drag and drop code, instead of using WebContentsAura), BrowserPluginGuest (this seems related to drag and drop in Chrome apps/extensions) That seems OK to me, that's not a lot of callsites. We can even add a is_filtered field to help enforce this programatically.
Patchset #20 (id:570001) has been deleted
Just upload patch set 20. I think there might be some confusion about what I described what I would do (we can chat tomorrow) In patch set 20, FilterDropData will do very minimal things: clear URL and filenames when necessary. Everything else will be moved to GrantPermissionsToDropData. We only need FilterDropData at DragEnter time, and FilterDropData + GrantPermissionsToDropData at Drop time. I don't go with filtering at DragEnter on the individual platforms for several reasons: 1. duplicate logic on 3 places (Aura, Mac, BrowserPlugin) 2. we still need to filter the data at Drop time, unless we cache the result of the filtering during DragEnter time somewhere. Then we need to remember to clear this cached state at appropriate times like DragLeave, DragEnd. 3. it is going to be inconsistent with the fact that we also filter at "OnStartDragging", which is when Chromium initiates the Drag and Drop operation. (4. I don't know how to write object c, so I try not to touch the .mm file when I can)
On 2016/05/16 at 23:44:06, hush wrote: > Just upload patch set 20. > I think there might be some confusion about what I described what I would do (we can chat tomorrow) > In patch set 20, FilterDropData will do very minimal things: clear URL and filenames when necessary. > Everything else will be moved to GrantPermissionsToDropData. > We only need FilterDropData at DragEnter time, and FilterDropData + GrantPermissionsToDropData at Drop time. > > I don't go with filtering at DragEnter on the individual platforms for several reasons: > 1. duplicate logic on 3 places (Aura, Mac, BrowserPlugin) As I've mentioned, this is OK. It's OK and reasonable for each platform entry point to have to do this filtering. > 2. we still need to filter the data at Drop time, unless we cache the result of the filtering during DragEnter time somewhere. Then we need to remember to clear this cached state at appropriate times like DragLeave, DragEnd. This already happens today: - https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... - https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > 3. it is going to be inconsistent with the fact that we also filter at "OnStartDragging", which is when Chromium initiates the Drag and Drop operation. That's a different kind of filtering today though. It's a completely separate code path, and it's out of scope to try to combine the dragenter filtering with the dragstart filtering. > (4. I don't know how to write object c, so I try not to touch the .mm file when I can) I'm happy to help here. It's a bit of a pain, but you'll mostly only have to call C++ interfaces anyway, so the pain will be limited.
Patchset #21 (id:610001) has been deleted
https://codereview.chromium.org/1723763002/diff/550001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/1723763002/diff/550001/content/browser/render... content/browser/renderer_host/render_view_host_impl.h:353: std::vector<std::string> filesystem_ids_to_grant_permissions_; On 2016/05/11 01:19:15, dcheng wrote: > Can this just be a local variable? It looks like it's just clear()ed at the end. deleted it https://codereview.chromium.org/1723763002/diff/550001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1723763002/diff/550001/content/renderer/rende... content/renderer/render_view_impl.cc:443: // platform. On 2016/05/11 01:19:15, dcheng wrote: > We should probably skip adding an item if it has no filename, since Blink won't > know what to do with it. Done.
Patchset #21 (id:630001) has been deleted
PTAL. I didn't put "filterDropData" as a function on DropData class because the function needs access to RenderProcessHost to filter the url, which is a content/public/browser object, inaccessible from content/public/common.
Looking good. Just a few small suggestions. https://codereview.chromium.org/1723763002/diff/670001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/670001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:1325: DropData RenderViewHostImpl::GrantPermissionsToDropData( Just return void here. https://codereview.chromium.org/1723763002/diff/670001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:1327: DropData filtered_data(drop_data); Don't need to filter again here. However, I would consider adding a debug-only field to DCHECK that we are actually filtering, e.g. something like: struct DropData { #if DCHECK_IS_ON() // RVH where the DropData will be sent. int view_id = MSG_ROUTING_NONE; #endif // DCHECK_IS_ON() }; void RenderViewHostImpl::FilterDropData(DropData* data) { /* filter */ #if DCHECK_IS_ON() data->view_id = GetRoutingID(); #endif // DCHECK_IS_ON() } And then: void RenderViewHostImpl::GrantFileAccessFromDropData(const DropData& data) { DCHECK_EQ(GetRoutingID(), data->view_id); /* grant permissions */ } https://codereview.chromium.org/1723763002/diff/670001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/1723763002/diff/670001/content/browser/render... content/browser/renderer_host/render_view_host_impl.h:127: void FilterDropData(DropData* drop_data) override; Can you put a comment on Enter / Drop that indicates the drop data must be filtered? https://codereview.chromium.org/1723763002/diff/670001/content/browser/render... content/browser/renderer_host/render_view_host_impl.h:354: DropData GrantPermissionsToDropData(const DropData& drop_data); Nit: GrantFileAccessFromDropData, to match the naming convention of the previous helper method. https://codereview.chromium.org/1723763002/diff/670001/content/browser/web_co... File content/browser/web_contents/web_drag_dest_mac.mm (right): https://codereview.chromium.org/1723763002/diff/670001/content/browser/web_co... content/browser/web_contents/web_drag_dest_mac.mm:12: #include "content/public/browser/render_process_host.h" Nit: remove this include https://codereview.chromium.org/1723763002/diff/670001/content/common/drag_me... File content/common/drag_messages.h (right): https://codereview.chromium.org/1723763002/diff/670001/content/common/drag_me... content/common/drag_messages.h:10: #include "base/strings/string16.h" This include isn't needed anymore. https://codereview.chromium.org/1723763002/diff/670001/content/public/common/... File content/public/common/drop_data.h (right): https://codereview.chromium.org/1723763002/diff/670001/content/public/common/... content/public/common/drop_data.h:42: struct MetaData { Nit: Metadata instead of MetaData, as that seems to be more prevalent https://codereview.chromium.org/1723763002/diff/670001/content/public/common/... content/public/common/drop_data.h:44: MetaData(const DropData::Kind& kind, const base::string16& mime_type); Hmm. Rather than having constructors and having to pass the kind explicitly, I suggest something like: MetaData::CreateForMimeType(...) MetaData::CreateForFilePath(...) MetaData::CreateForFilesystemUrl(...)
https://codereview.chromium.org/1723763002/diff/670001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/670001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:1325: DropData RenderViewHostImpl::GrantPermissionsToDropData( On 2016/05/24 05:51:50, dcheng wrote: > Just return void here. We still need to change the drop_data in this function. See line 1404, where we crack the url of the file system files. So I can pass DroData* to save a few copies. https://codereview.chromium.org/1723763002/diff/670001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:1327: DropData filtered_data(drop_data); On 2016/05/24 05:51:50, dcheng wrote: > Don't need to filter again here. However, I would consider adding a debug-only > field to DCHECK that we are actually filtering, e.g. something like: > > struct DropData { > #if DCHECK_IS_ON() > // RVH where the DropData will be sent. > int view_id = MSG_ROUTING_NONE; > #endif // DCHECK_IS_ON() > }; > > void RenderViewHostImpl::FilterDropData(DropData* data) { > /* filter */ > #if DCHECK_IS_ON() > data->view_id = GetRoutingID(); > #endif // DCHECK_IS_ON() > } > > And then: > void RenderViewHostImpl::GrantFileAccessFromDropData(const DropData& data) { > DCHECK_EQ(GetRoutingID(), data->view_id); > /* grant permissions */ > } Done. https://codereview.chromium.org/1723763002/diff/670001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/1723763002/diff/670001/content/browser/render... content/browser/renderer_host/render_view_host_impl.h:127: void FilterDropData(DropData* drop_data) override; On 2016/05/24 05:51:50, dcheng wrote: > Can you put a comment on Enter / Drop that indicates the drop data must be > filtered? Done. https://codereview.chromium.org/1723763002/diff/670001/content/browser/render... content/browser/renderer_host/render_view_host_impl.h:354: DropData GrantPermissionsToDropData(const DropData& drop_data); On 2016/05/24 05:51:50, dcheng wrote: > Nit: GrantFileAccessFromDropData, to match the naming convention of the previous > helper method. Done. https://codereview.chromium.org/1723763002/diff/670001/content/browser/web_co... File content/browser/web_contents/web_drag_dest_mac.mm (right): https://codereview.chromium.org/1723763002/diff/670001/content/browser/web_co... content/browser/web_contents/web_drag_dest_mac.mm:12: #include "content/public/browser/render_process_host.h" On 2016/05/24 05:51:50, dcheng wrote: > Nit: remove this include Done. https://codereview.chromium.org/1723763002/diff/670001/content/common/drag_me... File content/common/drag_messages.h (right): https://codereview.chromium.org/1723763002/diff/670001/content/common/drag_me... content/common/drag_messages.h:10: #include "base/strings/string16.h" On 2016/05/24 05:51:50, dcheng wrote: > This include isn't needed anymore. Done. And... is there a tool that tells me which includes are not needed? https://codereview.chromium.org/1723763002/diff/670001/content/public/common/... File content/public/common/drop_data.h (right): https://codereview.chromium.org/1723763002/diff/670001/content/public/common/... content/public/common/drop_data.h:42: struct MetaData { On 2016/05/24 05:51:50, dcheng wrote: > Nit: Metadata instead of MetaData, as that seems to be more prevalent Done. https://codereview.chromium.org/1723763002/diff/670001/content/public/common/... content/public/common/drop_data.h:44: MetaData(const DropData::Kind& kind, const base::string16& mime_type); On 2016/05/24 05:51:50, dcheng wrote: > Hmm. Rather than having constructors and having to pass the kind explicitly, I > suggest something like: > > MetaData::CreateForMimeType(...) > MetaData::CreateForFilePath(...) > MetaData::CreateForFilesystemUrl(...) Sorry I don't see the benefit of these static factory methods. The kind is implicit only for CreateForFilePath and CreateForFileSystemUrl. CreateForMimeType still needs both mime type and kind, because kind could be both string and filename. These static factory methods also make "DropDataToMeta" more verbose because we have to use push_back instead of emplace_back. And we also incur more copies as a result of using static factory methods (one in the factory method itself, one in push_back) How about keeping these 3 constructors, but only pass kind when the 2nd parameter is mime type, because that's the only case where kind is unclear?
Patchset #23 (id:690001) has been deleted
https://codereview.chromium.org/1723763002/diff/670001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1723763002/diff/670001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:1325: DropData RenderViewHostImpl::GrantPermissionsToDropData( On 2016/05/24 at 20:26:08, hush wrote: > On 2016/05/24 05:51:50, dcheng wrote: > > Just return void here. > > We still need to change the drop_data in this function. See line 1404, where we crack the url of the file system files. So I can pass DroData* to save a few copies. =/ I'll followup after this CL lands with the storage team on this. https://codereview.chromium.org/1723763002/diff/670001/content/common/drag_me... File content/common/drag_messages.h (right): https://codereview.chromium.org/1723763002/diff/670001/content/common/drag_me... content/common/drag_messages.h:10: #include "base/strings/string16.h" On 2016/05/24 at 20:26:09, hush wrote: > On 2016/05/24 05:51:50, dcheng wrote: > > This include isn't needed anymore. > > Done. > And... is there a tool that tells me which includes are not needed? Sadly there is not. https://codereview.chromium.org/1723763002/diff/670001/content/public/common/... File content/public/common/drop_data.h (right): https://codereview.chromium.org/1723763002/diff/670001/content/public/common/... content/public/common/drop_data.h:44: MetaData(const DropData::Kind& kind, const base::string16& mime_type); On 2016/05/24 at 20:26:09, hush wrote: > On 2016/05/24 05:51:50, dcheng wrote: > > Hmm. Rather than having constructors and having to pass the kind explicitly, I > > suggest something like: > > > > MetaData::CreateForMimeType(...) > > MetaData::CreateForFilePath(...) > > MetaData::CreateForFilesystemUrl(...) > > Sorry I don't see the benefit of these static factory methods. > The kind is implicit only for CreateForFilePath and CreateForFileSystemUrl. > CreateForMimeType still needs both mime type and kind, because kind could be both string and filename. > These static factory methods also make "DropDataToMeta" more verbose because we have to use push_back instead of emplace_back. And we also incur more copies as a result of using static factory methods (one in the factory method itself, one in push_back) > > How about keeping these 3 constructors, but only pass kind when the 2nd parameter is mime type, because that's the only case where kind is unclear? The main reason I suggested factory functions is to help overall readability. If I see: Metadata(const GURL&); it's hard to tell it's only for filesystem URLs and not for other kinds of URLs. But if I have: Metadata::CreateForFilesystemUrl(...) Then it's unambiguous. As for the extra copy... if it really matters, we should give it a move constructor. Otherwise we shouldn't worry about it.
rebase
Description was changed from ========== Add WebDragData to blink::WebView::dragtargetDrop. Different from Windows/Linux, the drag and drop data on Android is not available when the drag starts. It is only available when the drop is performed (finger releases from touch screen). During DragEnter, a vector of (mime type, kind) pairs will be sent to renderer to construct a WebDragData that contains only this meta data and it will be used by blink. This CL adds a WebDragData parameter to blink WebView::dragTargetDrop. When the dragged data is dropped, the full drop data will be sent to renderer and then to blink. BUG=584789 ========== to ========== Add WebDragData to blink::WebView::dragtargetDrop. Different from Windows/Linux, the drag and drop data on Android is not available when the drag starts. It is only available when the drop is performed (finger releases from touch screen). During DragEnter, the metadata of the drop data will be sent to renderer to construct a WebDragData that contains only this meta data and it will be used by blink. This CL adds a WebDragData parameter to blink WebView::dragTargetDrop. When the dragged data is dropped, the full drop data will be sent to renderer and then to blink. This CL also changes the logic around drop data filtering and permisson granting. Different platforms (Aura, Mac, BrowserPlugin) will filter the drop data before DragEnter, but not Android, because it is not necessary. The filtered drop data will be cached and used at Drop for these platforms except Android. Android logic for drop data will be in a follow up CL. BUG=584789 ==========
Description was changed from ========== Add WebDragData to blink::WebView::dragtargetDrop. Different from Windows/Linux, the drag and drop data on Android is not available when the drag starts. It is only available when the drop is performed (finger releases from touch screen). During DragEnter, the metadata of the drop data will be sent to renderer to construct a WebDragData that contains only this meta data and it will be used by blink. This CL adds a WebDragData parameter to blink WebView::dragTargetDrop. When the dragged data is dropped, the full drop data will be sent to renderer and then to blink. This CL also changes the logic around drop data filtering and permisson granting. Different platforms (Aura, Mac, BrowserPlugin) will filter the drop data before DragEnter, but not Android, because it is not necessary. The filtered drop data will be cached and used at Drop for these platforms except Android. Android logic for drop data will be in a follow up CL. BUG=584789 ========== to ========== Add WebDragData to blink::WebView::dragtargetDrop. Different from Windows/Linux, the drag and drop data on Android is not available when the drag starts. It is only available when the drop is performed (finger releases from touch screen). During DragEnter, the meta data of the drop data will be sent to renderer to construct a WebDragData that contains only this meta data and it will be used by blink. This CL adds a WebDragData parameter to blink WebView::dragTargetDrop. When the dragged data is dropped, the full drop data will be sent to renderer and then to blink. This CL also changes the logic around drop data filtering and permisson granting. Different platforms (Aura, Mac, BrowserPlugin) will filter the drop data before DragEnter, but not Android, because it is not necessary. The filtered drop data will be cached and used at Drop for these platforms except Android. Android logic for drop data will be in a follow up CL. BUG=584789 ==========
https://codereview.chromium.org/1723763002/diff/670001/content/public/common/... File content/public/common/drop_data.h (right): https://codereview.chromium.org/1723763002/diff/670001/content/public/common/... content/public/common/drop_data.h:44: MetaData(const DropData::Kind& kind, const base::string16& mime_type); On 2016/05/25 00:07:58, dcheng wrote: > On 2016/05/24 at 20:26:09, hush wrote: > > On 2016/05/24 05:51:50, dcheng wrote: > > > Hmm. Rather than having constructors and having to pass the kind explicitly, > I > > > suggest something like: > > > > > > MetaData::CreateForMimeType(...) > > > MetaData::CreateForFilePath(...) > > > MetaData::CreateForFilesystemUrl(...) > > > > Sorry I don't see the benefit of these static factory methods. > > The kind is implicit only for CreateForFilePath and CreateForFileSystemUrl. > > CreateForMimeType still needs both mime type and kind, because kind could be > both string and filename. > > These static factory methods also make "DropDataToMeta" more verbose because > we have to use push_back instead of emplace_back. And we also incur more copies > as a result of using static factory methods (one in the factory method itself, > one in push_back) > > > > How about keeping these 3 constructors, but only pass kind when the 2nd > parameter is mime type, because that's the only case where kind is unclear? > > The main reason I suggested factory functions is to help overall readability. If > I see: > > Metadata(const GURL&); > > it's hard to tell it's only for filesystem URLs and not for other kinds of URLs. > > But if I have: > > Metadata::CreateForFilesystemUrl(...) > > Then it's unambiguous. > > As for the extra copy... if it really matters, we should give it a move > constructor. Otherwise we shouldn't worry about it. OK. I did the static factory methods.
use static factory methods
lgtm with nits. thanks for tackling this and helping to clean this up! https://codereview.chromium.org/1723763002/diff/750001/content/public/common/... File content/public/common/drop_data.cc (right): https://codereview.chromium.org/1723763002/diff/750001/content/public/common/... content/public/common/drop_data.cc:23: const DropData::Kind& kind, You shouldn't need the DropData:: specifier here. https://codereview.chromium.org/1723763002/diff/750001/content/public/common/... content/public/common/drop_data.cc:25: DropData::Metadata metadata; Ditto, DropData:: shouldn't be needed here (and below). https://codereview.chromium.org/1723763002/diff/750001/content/public/common/... File content/public/common/drop_data.h (right): https://codereview.chromium.org/1723763002/diff/750001/content/public/common/... content/public/common/drop_data.h:48: static Metadata createForMimeType(const DropData::Kind& kind, Nit: these should be capitalized. Also, DropData:: here should be unnecessary.
comments
Patchset #26 (id:770001) has been deleted
https://codereview.chromium.org/1723763002/diff/750001/content/public/common/... File content/public/common/drop_data.cc (right): https://codereview.chromium.org/1723763002/diff/750001/content/public/common/... content/public/common/drop_data.cc:23: const DropData::Kind& kind, On 2016/05/25 03:25:37, dcheng wrote: > You shouldn't need the DropData:: specifier here. Done. https://codereview.chromium.org/1723763002/diff/750001/content/public/common/... content/public/common/drop_data.cc:25: DropData::Metadata metadata; On 2016/05/25 03:25:37, dcheng wrote: > Ditto, DropData:: shouldn't be needed here (and below). Done. https://codereview.chromium.org/1723763002/diff/750001/content/public/common/... File content/public/common/drop_data.h (right): https://codereview.chromium.org/1723763002/diff/750001/content/public/common/... content/public/common/drop_data.h:48: static Metadata createForMimeType(const DropData::Kind& kind, On 2016/05/25 03:25:37, dcheng wrote: > Nit: these should be capitalized. Also, DropData:: here should be unnecessary. Done.
The CQ bit was checked by hush@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, sievers@chromium.org, lfg@chromium.org, avi@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1723763002/#ps790001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1723763002/790001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1723763002/790001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by hush@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, avi@chromium.org, dcheng@chromium.org, sievers@chromium.org, lfg@chromium.org Link to the patchset: https://codereview.chromium.org/1723763002/#ps810001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1723763002/810001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1723763002/810001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
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/patch-status/1723763002/830001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1723763002/830001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/patch-status/1723763002/850001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1723763002/850001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by hush@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, avi@chromium.org, dcheng@chromium.org, sievers@chromium.org, lfg@chromium.org Link to the patchset: https://codereview.chromium.org/1723763002/#ps870001 (title: "compile")
The CQ bit was unchecked by hush@chromium.org
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/patch-status/1723763002/870001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1723763002/870001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
hello Paweł, Why is compilation failing on them while other try bots have passed? This view_id field is defined in drop_data.h, guarded by "DCHECK_IS_ON()" and used in render_view_host_impl.cc with DCHECK_EQ.
hush@chromium.org changed reviewers: + bauerb@chromium.org, dmazzoni@chromium.org
Hello dmazzoni and bauerb, PTAL. chrome/browser/ui/webui/webui_webview_browsertest.cc bauerb components/test_runner/event_sender.cc dmazzoni The blink API changed, so these tests need to change too. They need to pass drop data at DragTargetDrop. Also they need to call RVH::FilterDropData before DragTargetDragEnter
Web UI LGTM.
lgtm
Hello dcheng@, I am investigating the failure of WebUIWebViewBrowserTest.DragAndDropToInput. And after much debugging, I think there is indeed a bug somewhere. First of all, there are 2 blink::WebViewImpl instances in this setup (I don't know how exactly this <webview> tag works though. Maybe <webview> needs its own blink::webviewimpl instance?). Step 1. The test sends DropData with DragEnter event to blink::WebViewImpl instance #1. Step 2. blink::WebViewImpl instance #1 notifies WebPluginContainerImpl of the dragEnter event. WebPluginContainerImpl tells BrowserPlugin of this DragEnter event, passing WebDragData, which only contains the metadata. Step 3. BrowserPlugin converts this WebDragData into DropData, and sends it to BrowserPluginGuest via IPC. Then BrowserPluginGuest sends this DropData with DragEnter event to Blink::WebViewImpl instance #2. When Blink::WebViewImpl instance #2 receives DragEnter event, the test proceeds. Now, the problem is, in this DropData-->DropData::Metadata->WebDragData->DropData dance in step 2-3, the end DropData will contain just an empty URL, while the original DropData contains a URL about::blank. As a result, Blink::WebViewImpl instance #2 thinks that the WebDragData contains nothing, and sets m_dragOperation to none after DragEnter, which blocks any further Drop events on this Blink::WebViewImpl instance. This causes the test to time out, because it waits on the 'drop' event of Blink::WebViewImpl instance #2.
so I propose this solution: add "has_url" boolean state to DropData, in this way, DropData can tell the difference between not having URL at all and having an empty URL. dcheng@ what do you think?
Sorry I missed this reply. It feels kind of hacky but I guess that's the best we can do while we're still using the legacy <webview> architecture. (If you're feeling brave, you could try using base:: Optional for this) On Thu, May 26, 2016, 16:40 <hush@chromium.org> wrote: > so I propose this solution: > add "has_url" boolean state to DropData, in this way, DropData can tell the > difference between not having URL at all and having an empty URL. > > dcheng@ what do you think? > > https://codereview.chromium.org/1723763002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Sorry I missed this reply. It feels kind of hacky but I guess that's the best we can do while we're still using the legacy <webview> architecture. (If you're feeling brave, you could try using base:: Optional for this) On Thu, May 26, 2016, 16:40 <hush@chromium.org> wrote: > so I propose this solution: > add "has_url" boolean state to DropData, in this way, DropData can tell the > difference between not having URL at all and having an empty URL. > > dcheng@ what do you think? > > https://codereview.chromium.org/1723763002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #31 (id:890001) has been deleted
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/patch-status/1723763002/910001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1723763002/910001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
On 2016/05/27 08:33:35, dcheng wrote: > Sorry I missed this reply. It feels kind of hacky but I guess that's the > best we can do while we're still using the legacy <webview> architecture. Why is this be specific to <webview>? Wouldn't it happen when dragging about:blank somewhere else too?
I guess it might affect other plugins too. Maybe we need to make it so WebDragData has a distinct metadata form too so that the DropData::Metadata <-> WebDragData conversion can round trip. On Fri, May 27, 2016, 14:45 <lfg@chromium.org> wrote: > On 2016/05/27 08:33:35, dcheng wrote: > > Sorry I missed this reply. It feels kind of hacky but I guess that's the > > best we can do while we're still using the legacy <webview> architecture. > > Why is this be specific to <webview>? Wouldn't it happen when dragging > about:blank somewhere else too? > > > https://codereview.chromium.org/1723763002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
I guess it might affect other plugins too. Maybe we need to make it so WebDragData has a distinct metadata form too so that the DropData::Metadata <-> WebDragData conversion can round trip. On Fri, May 27, 2016, 14:45 <lfg@chromium.org> wrote: > On 2016/05/27 08:33:35, dcheng wrote: > > Sorry I missed this reply. It feels kind of hacky but I guess that's the > > best we can do while we're still using the legacy <webview> architecture. > > Why is this be specific to <webview>? Wouldn't it happen when dragging > about:blank somewhere else too? > > > https://codereview.chromium.org/1723763002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/05/27 21:51:46, dcheng wrote: > I guess it might affect other plugins too. Maybe we need to make it so > WebDragData has a distinct metadata form too so that the DropData::Metadata > <-> WebDragData conversion can round trip. > > On Fri, May 27, 2016, 14:45 <mailto:lfg@chromium.org> wrote: > > > On 2016/05/27 08:33:35, dcheng wrote: > > > Sorry I missed this reply. It feels kind of hacky but I guess that's the > > > best we can do while we're still using the legacy <webview> architecture. > > > > Why is this be specific to <webview>? Wouldn't it happen when dragging > > about:blank somewhere else too? > > > > > > https://codereview.chromium.org/1723763002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I agree this would be a non-hacky way to do this right.
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/patch-status/1723763002/930001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1723763002/930001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hello dcheng@, PTAL patch set 32. It changed somewhat significantly after your lgtm.
lfg@chromium.org changed reviewers: + lazyboy@chromium.org
This will also change the behavior of BrowserPluginGuest::EndSystemDragIfApplicable, I've looked through the history and this was fixing a lot of corner cases, +lazyboy to double check that this is ok. https://codereview.chromium.org/1723763002/diff/930001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1723763002/diff/930001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:822: dragged_url_ = drop_data.url; Do we still need dragged_url_ as a member?
https://codereview.chromium.org/1723763002/diff/930001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1723763002/diff/930001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:821: if (!embedder->DragEnteredGuest(this)) Hmm, this doesn't seem quite right, at this point DragEnteredGuest should always return true (i.e. the condition of this if won't be true). We want to call DidDropLink below if this drag came from anywhere other than us/this. So instead of remembering the dragged_url_, we want to remember a bool if WebDragStatusEnter was initiated by us or not. This one can be tested by looking at crbug 460757 and checking whether link dropping in chrome://chrome-signin works correctly.
hello lazyboy@ and lfg@, PTAL https://codereview.chromium.org/1723763002/diff/930001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1723763002/diff/930001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:821: if (!embedder->DragEnteredGuest(this)) On 2016/05/31 22:21:22, lazyboy wrote: > Hmm, this doesn't seem quite right, at this point DragEnteredGuest should always > return true (i.e. the condition of this if won't be true). > > We want to call DidDropLink below if this drag came from anywhere other than > us/this. > So instead of remembering the dragged_url_, we want to remember a bool if > WebDragStatusEnter was initiated by us or not. > > This one can be tested by looking at crbug 460757 and checking whether link > dropping in chrome://chrome-signin works correctly. Right. I found link dropping in chrome://chrome-signin navigates with my patch here. I fixed in the last version. Thanks! https://codereview.chromium.org/1723763002/diff/930001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:822: dragged_url_ = drop_data.url; On 2016/05/31 21:30:42, lfg wrote: > Do we still need dragged_url_ as a member? No. I will remove it in the next patch set.
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/patch-status/1723763002/950001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
One more q, otherwise looks good. https://codereview.chromium.org/1723763002/diff/950001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1723763002/diff/950001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:832: ignore_dragged_url_ = false; Should setting this to false be outside of/after the if block, i.e. regardless of url being is_valid()? Otherwise there's a chance that BPGuest's state would go inconsistent.
Patchset #34 (id:970001) has been deleted
https://codereview.chromium.org/1723763002/diff/950001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1723763002/diff/950001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:832: ignore_dragged_url_ = false; On 2016/06/04 04:52:38, lazyboy wrote: > Should setting this to false be outside of/after the if block, i.e. regardless > of url being is_valid()? > Otherwise there's a chance that BPGuest's state would go inconsistent. yeah, that's right. Done
browser_plugin_guest.cc lgtm, thanks.
lgtm https://codereview.chromium.org/1723763002/diff/990001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1723763002/diff/990001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:812: if (!embedder->DragEnteredGuest(this)) { nit: no need for {}
lgtm https://codereview.chromium.org/1723763002/diff/990001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1723763002/diff/990001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:812: if (!embedder->DragEnteredGuest(this)) { nit: no need for {}
On 2016/06/06 18:00:41, lfg wrote: > lgtm > > https://codereview.chromium.org/1723763002/diff/990001/content/browser/browse... > File content/browser/browser_plugin/browser_plugin_guest.cc (right): > > https://codereview.chromium.org/1723763002/diff/990001/content/browser/browse... > content/browser/browser_plugin/browser_plugin_guest.cc:812: if > (!embedder->DragEnteredGuest(this)) { > nit: no need for {} Done
ping? dcheng@
Sorry about the delayed reply. I'm a bit confused about the direction this CL is taking: based on the previous replies, I thought we were going to make WebDragData support a metadata format so we can roundtrip through <webview>. Failing that, I think we should just put a dummy URL into WebDragData for the metadata case: it's a bit of a hack, but having to keep has_url and url in sync is also no good.
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/patch-status/1723763002/1030001
The CQ bit was unchecked by hush@chromium.org
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/patch-status/1723763002/1050001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #36 (id:1030001) has been deleted
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/patch-status/1723763002/1070001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) ios-device-gn on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
hello dcheng@, PTAL patch set 36
the gist of the change happens in content/renderer/drop_data_builder.cc, which is used by browser plugins, to fix the DropData with empty URL issue caused by DropData-->WebDragData-->DropData round trip, which is unique to browser plugins.
LGTM with comment addressed. https://codereview.chromium.org/1723763002/diff/1070001/content/renderer/drop... File content/renderer/drop_data_builder.cc (right): https://codereview.chromium.org/1723763002/diff/1070001/content/renderer/drop... content/renderer/drop_data_builder.cc:40: // The url in WebDragData received could be empty at DragEnter time, Let's do this this when we convert DropData::Metadata -> WebDragData so we don't have to do an isEmpty() check here (since it's possible, though odd, for JS to place an empty URL in the clipboard).
The CQ bit was checked by hush@chromium.org to run a CQ dry run
https://codereview.chromium.org/1723763002/diff/1070001/content/renderer/drop... File content/renderer/drop_data_builder.cc (right): https://codereview.chromium.org/1723763002/diff/1070001/content/renderer/drop... content/renderer/drop_data_builder.cc:40: // The url in WebDragData received could be empty at DragEnter time, On 2016/06/15 12:00:39, dcheng wrote: > Let's do this this when we convert DropData::Metadata -> WebDragData so we don't > have to do an isEmpty() check here (since it's possible, though odd, for JS to > place an empty URL in the clipboard). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1723763002/1090001
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 hush@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, aelias@chromium.org, avi@chromium.org, dmazzoni@chromium.org, sievers@chromium.org, lazyboy@chromium.org, lfg@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1723763002/#ps1090001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1723763002/1090001
Message was sent while issue was closed.
Description was changed from ========== Add WebDragData to blink::WebView::dragtargetDrop. Different from Windows/Linux, the drag and drop data on Android is not available when the drag starts. It is only available when the drop is performed (finger releases from touch screen). During DragEnter, the meta data of the drop data will be sent to renderer to construct a WebDragData that contains only this meta data and it will be used by blink. This CL adds a WebDragData parameter to blink WebView::dragTargetDrop. When the dragged data is dropped, the full drop data will be sent to renderer and then to blink. This CL also changes the logic around drop data filtering and permisson granting. Different platforms (Aura, Mac, BrowserPlugin) will filter the drop data before DragEnter, but not Android, because it is not necessary. The filtered drop data will be cached and used at Drop for these platforms except Android. Android logic for drop data will be in a follow up CL. BUG=584789 ========== to ========== Add WebDragData to blink::WebView::dragtargetDrop. Different from Windows/Linux, the drag and drop data on Android is not available when the drag starts. It is only available when the drop is performed (finger releases from touch screen). During DragEnter, the meta data of the drop data will be sent to renderer to construct a WebDragData that contains only this meta data and it will be used by blink. This CL adds a WebDragData parameter to blink WebView::dragTargetDrop. When the dragged data is dropped, the full drop data will be sent to renderer and then to blink. This CL also changes the logic around drop data filtering and permisson granting. Different platforms (Aura, Mac, BrowserPlugin) will filter the drop data before DragEnter, but not Android, because it is not necessary. The filtered drop data will be cached and used at Drop for these platforms except Android. Android logic for drop data will be in a follow up CL. BUG=584789 ==========
Message was sent while issue was closed.
Committed patchset #38 (id:1090001)
Message was sent while issue was closed.
Description was changed from ========== Add WebDragData to blink::WebView::dragtargetDrop. Different from Windows/Linux, the drag and drop data on Android is not available when the drag starts. It is only available when the drop is performed (finger releases from touch screen). During DragEnter, the meta data of the drop data will be sent to renderer to construct a WebDragData that contains only this meta data and it will be used by blink. This CL adds a WebDragData parameter to blink WebView::dragTargetDrop. When the dragged data is dropped, the full drop data will be sent to renderer and then to blink. This CL also changes the logic around drop data filtering and permisson granting. Different platforms (Aura, Mac, BrowserPlugin) will filter the drop data before DragEnter, but not Android, because it is not necessary. The filtered drop data will be cached and used at Drop for these platforms except Android. Android logic for drop data will be in a follow up CL. BUG=584789 ========== to ========== Add WebDragData to blink::WebView::dragtargetDrop. Different from Windows/Linux, the drag and drop data on Android is not available when the drag starts. It is only available when the drop is performed (finger releases from touch screen). During DragEnter, the meta data of the drop data will be sent to renderer to construct a WebDragData that contains only this meta data and it will be used by blink. This CL adds a WebDragData parameter to blink WebView::dragTargetDrop. When the dragged data is dropped, the full drop data will be sent to renderer and then to blink. This CL also changes the logic around drop data filtering and permisson granting. Different platforms (Aura, Mac, BrowserPlugin) will filter the drop data before DragEnter, but not Android, because it is not necessary. The filtered drop data will be cached and used at Drop for these platforms except Android. Android logic for drop data will be in a follow up CL. BUG=584789 Committed: https://crrev.com/8915b509e11743c64a473db748181c62ded64258 Cr-Commit-Position: refs/heads/master@{#399989} ==========
Message was sent while issue was closed.
Patchset 38 (id:??) landed as https://crrev.com/8915b509e11743c64a473db748181c62ded64258 Cr-Commit-Position: refs/heads/master@{#399989} |
