Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1964283002:
MacViews: Implemented Drag & Drop (Closed)
DescriptionMacViews: Implemented Drag & Drop
Created a Drag & Drop client on Mac that bridges between the Views
and native OSX's drag and drop. This approach is based on the Aura
approach.
BUG=599585
TEST= Run the view unit test: DragDropClientMacTest
Committed: https://crrev.com/cf5eeb5c035be5a986fa1a6488f2189e7aebe198
Cr-Commit-Position: refs/heads/master@{#397801}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Renamed #Patch Set 3 : Added test #Patch Set 4 : #Patch Set 5 : comments #Patch Set 6 : Clean up #Patch Set 7 : Forgot to add the test #
Total comments: 78
Patch Set 8 : Fix for tapted #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Total comments: 17
Patch Set 12 : Added missing commit #Patch Set 13 : fixes for tapted #Patch Set 14 : PickledData format now supported #Patch Set 15 : Added a runloop #Patch Set 16 : Nit #
Total comments: 54
Patch Set 17 : #Patch Set 18 : #
Total comments: 19
Patch Set 19 : more fixes for tapted #Patch Set 20 : #
Total comments: 24
Patch Set 21 : #Patch Set 22 : #Patch Set 23 : Add missing file #
Total comments: 4
Patch Set 24 : fix for sky #
Total comments: 4
Patch Set 25 : fix for dcheng #
Total comments: 2
Messages
Total messages: 49 (17 generated)
Description was changed from ========== MacViews: Implemented Drag&Drop BUG= ========== to ========== MacViews: Implemented Drag & Drop BUG= ==========
spqchan@chromium.org changed reviewers: + tapted@chromium.org
Hey Trent, Can you have a look at what I have right now for drag and drop for MacViews? The code is a little messy right now but I want to make sure that I'm on the right direction before I spend time refactoring and cleaning it. I basically implemented bridges between the native OSX's drag and drop and View's existing one. I created a CocoaDragDropManager class which handles drag and drop. The manager is owned by NativeWidgetMac and the BridgedContentView will propagate the dragging events into it. CocoaDragDropManager will then handle the events and feed it into View's DropHelper. One nice side effect to implementing this is that it's not limited to TextViews, pretty much almost anything that already implements Drag&Drop in Views should work (unless I miss a format). Feel free to play with the bookmark bar or drag random stuff from the WebContent. Anyway, let me know if you need any clarifications Thanks!
This looks awesome! I think your approach is spot-on. Just some high-level comments (held back the nits :). We should try to add some tests too - there might be some good examples to follow in desktop_drag_drop_client_aurax11_unittest.cc Haven't tried patching it in yet to have a play - might get a chance tomorrow (got buried in sheriff duties today..) https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/drag_drop_... File ui/base/dragdrop/drag_drop_types.h (right): https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/drag_drop_... ui/base/dragdrop/drag_drop_types.h:37: static uint32_t DragOperationToNSDragOperation(int drag_operation); (nit NSUInteger is 64-bits on 64-bit platforms, so I guess this should return a uint64_t) https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/drag_drop_... File ui/base/dragdrop/drag_drop_types_mac.mm (right): https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/drag_drop_... ui/base/dragdrop/drag_drop_types_mac.mm:12: uint32_t ns_drag_operation = NSDragOperationNone; and now it's safe to #include <Cocoa> this can probably be NSUInteger https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/os_exchang... File ui/base/dragdrop/os_exchange_data.h (right): https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/os_exchang... ui/base/dragdrop/os_exchange_data.h:143: virtual void SetDragImage(const gfx::ImageSkia& image) = 0; it's probably nicer to expose the existing Aura methods on Mac (e.g. replace USE_AURA -> TOOLKIT_VIEWS), and just ignore cursor_offset if we don't use it (yet) https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/os_exchang... File ui/base/dragdrop/os_exchange_data_provider_mac.h (right): https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/os_exchang... ui/base/dragdrop/os_exchange_data_provider_mac.h:62: static Provider* CreateProviderFromPasteboard(NSPasteboard*); return std::unique_ptr maybe https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/os_exchang... ui/base/dragdrop/os_exchange_data_provider_mac.h:71: base::NativeEvent event_; this might need to be retained -> scoped_nsobject<NSEvent>? https://codereview.chromium.org/1964283002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/1964283002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.h:34: views::CocoaDragDropManager* drag_drop_manager_; It might be nicer to have a helper function that does BridgedNativeWidget* bridge = NativeWidgetMac::GetBridgeForNativeWindow([self window]); return bridge ? bridge->drag_drop_manager() : nullptr; ObjC objects holding on to C++ objects tends to lead to tricky lifetime problems due to reference counting. https://codereview.chromium.org/1964283002/diff/1/ui/views/cocoa/cocoa_drag_d... File ui/views/cocoa/cocoa_drag_drop_manager.h (right): https://codereview.chromium.org/1964283002/diff/1/ui/views/cocoa/cocoa_drag_d... ui/views/cocoa/cocoa_drag_drop_manager.h:35: class VIEWS_EXPORT CocoaDragDropManager { This class looks a lot like an aura::DragDropClient - we can't inherit from that just yet, but down the line we will probably need a "DesktopDragDropClientMac" similar to the one in ui/views/widget/desktop_aura/desktop_drag_drop_client_win.h So, it would be nice to keep the same nomenclature - e.g. call this a DragDropClientMac and use similar method names. https://codereview.chromium.org/1964283002/diff/1/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/1964283002/diff/1/ui/views/view.cc#newcode2416 ui/views/view.cc:2416: data.provider().SetNativeEvent(event.native_event()); This looks funny, but might be justified. It would be nice to isolate this in a mac-specific file, or remove the #ifdef. Another way to do this might be to pass the event in to a (maybe overloaded) constructor for OSExchangeData and just ignore the argument on other platforms. Or perhaps pass the event into Widget::RunShellDrag. However, I think there are other callers of RunShellDrag that concoct their own OSExchangeData, and those other call sites might need to work on Mac, but we can't rely on callers remembering to do SetNativeEvent on Mac. Yet another cheaty way might be to simply use [NSApp currentEvent] inside RunShellDrag. But that could be tricky to use in tests. If it justified like this, we will need a good comment here to appease OWNERS :). (along the lines of "On Mac, we Mac need to retain the native event here so it can later be passed to .. etc.)
I cleaned up the code and wrote DragDropTestNativeWidgetMac A good chunk of the set up is copied from BridgedNativeWidgetUnitTest. I tried simulating actual drag and drop events, but I didn't have much luck. One issue I’m having with the test is getting the correct root view. In the test, I set view_ as the root view and add the target_ to it. However, for some reason, target_->GetWidget()->GetRootView() returns a different root view. This is an issue because DropHelper retrieves and uses the root view via GetWidget()->GetRootView() and in OnDrop(). The change I made DropHelper gives me the correct root_view. Is there something I might be missing in the test set up? https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/drag_drop_... File ui/base/dragdrop/drag_drop_types.h (right): https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/drag_drop_... ui/base/dragdrop/drag_drop_types.h:37: static uint32_t DragOperationToNSDragOperation(int drag_operation); On 2016/05/11 12:36:15, tapted wrote: > (nit NSUInteger is 64-bits on 64-bit platforms, so I guess this should return a > uint64_t) Done. https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/drag_drop_... File ui/base/dragdrop/drag_drop_types_mac.mm (right): https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/drag_drop_... ui/base/dragdrop/drag_drop_types_mac.mm:12: uint32_t ns_drag_operation = NSDragOperationNone; On 2016/05/11 12:36:15, tapted wrote: > and now it's safe to #include <Cocoa> this can probably be NSUInteger Done. https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/os_exchang... File ui/base/dragdrop/os_exchange_data.h (right): https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/os_exchang... ui/base/dragdrop/os_exchange_data.h:143: virtual void SetDragImage(const gfx::ImageSkia& image) = 0; On 2016/05/11 12:36:15, tapted wrote: > it's probably nicer to expose the existing Aura methods on Mac (e.g. replace > USE_AURA -> TOOLKIT_VIEWS), and just ignore cursor_offset if we don't use it > (yet) I added defined(OS_MACOSX), I ran into some trouble when I simply replaced USE_AURA with TOOLKIT_VIEWS https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/os_exchang... File ui/base/dragdrop/os_exchange_data_provider_mac.h (right): https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/os_exchang... ui/base/dragdrop/os_exchange_data_provider_mac.h:62: static Provider* CreateProviderFromPasteboard(NSPasteboard*); On 2016/05/11 12:36:15, tapted wrote: > return std::unique_ptr maybe Done. https://codereview.chromium.org/1964283002/diff/1/ui/base/dragdrop/os_exchang... ui/base/dragdrop/os_exchange_data_provider_mac.h:71: base::NativeEvent event_; On 2016/05/11 12:36:15, tapted wrote: > this might need to be retained -> scoped_nsobject<NSEvent>? Done. https://codereview.chromium.org/1964283002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/1964283002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.h:34: views::CocoaDragDropManager* drag_drop_manager_; On 2016/05/11 12:36:15, tapted wrote: > It might be nicer to have a helper function that does > > BridgedNativeWidget* bridge = > NativeWidgetMac::GetBridgeForNativeWindow([self window]); > return bridge ? bridge->drag_drop_manager() : nullptr; > > ObjC objects holding on to C++ objects tends to lead to tricky lifetime problems > due to reference counting. Done. https://codereview.chromium.org/1964283002/diff/1/ui/views/cocoa/cocoa_drag_d... File ui/views/cocoa/cocoa_drag_drop_manager.h (right): https://codereview.chromium.org/1964283002/diff/1/ui/views/cocoa/cocoa_drag_d... ui/views/cocoa/cocoa_drag_drop_manager.h:35: class VIEWS_EXPORT CocoaDragDropManager { On 2016/05/11 12:36:15, tapted wrote: > This class looks a lot like an aura::DragDropClient - we can't inherit from that > just yet, but down the line we will probably need a "DesktopDragDropClientMac" > similar to the one in > ui/views/widget/desktop_aura/desktop_drag_drop_client_win.h > > So, it would be nice to keep the same nomenclature - e.g. call this a > DragDropClientMac and use similar method names. Sounds good. I renamed most of my methods according to DragDropClient. https://codereview.chromium.org/1964283002/diff/1/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/1964283002/diff/1/ui/views/view.cc#newcode2416 ui/views/view.cc:2416: data.provider().SetNativeEvent(event.native_event()); On 2016/05/11 12:36:15, tapted wrote: > This looks funny, but might be justified. It would be nice to isolate this in a > mac-specific file, or remove the #ifdef. Another way to do this might be to pass > the event in to a (maybe overloaded) constructor for OSExchangeData and just > ignore the argument on other platforms. Or perhaps pass the event into > Widget::RunShellDrag. However, I think there are other callers of RunShellDrag > that concoct their own OSExchangeData, and those other call sites might need to > work on Mac, but we can't rely on callers remembering to do SetNativeEvent on > Mac. > > Yet another cheaty way might be to simply use [NSApp currentEvent] inside > RunShellDrag. But that could be tricky to use in tests. > > If it justified like this, we will need a good comment here to appease OWNERS > :). (along the lines of "On Mac, we Mac need to retain the native event here so > it can later be passed to .. etc.) I tried out the ideas you suggested but I think the simplest and cleanest way is to just set it here. I added a comment to explain it. Let me know though if you have other suggestions!
Description was changed from ========== MacViews: Implemented Drag & Drop BUG= ========== to ========== MacViews: Implemented Drag & Drop BUG=599585 ==========
I played around a bit - the main thing I noticed: dragging text from a views textfield always "copies", but it should move if dragging within the same textfield. Maybe implementing the NSDraggingSource protocol (see below) is the way to fix that. Let's flesh out the CL description too. E.g., to butter up views OWNERS it's worth mentioning there that it's modelled of the aura approach. https://codereview.chromium.org/1964283002/diff/120001/ui/base/dragdrop/drag_... File ui/base/dragdrop/drag_utils_mac.mm (right): https://codereview.chromium.org/1964283002/diff/120001/ui/base/dragdrop/drag_... ui/base/dragdrop/drag_utils_mac.mm:24: data_object->provider().SetDragImage(image_skia, cursor_offset); hum - these are the same implementations provided by drag_utils.cc and drag_utils_aura.cc I think we should: - move drag_utils_aura.cc into drag_utils.cc and guard it with #if !defined(OS_WIN). delete drag_utils_aura.cc - compile and link drag_utils.cc on Mac - delete drag_utils_mac.mm https://codereview.chromium.org/1964283002/diff/120001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_mac.h (right): https://codereview.chromium.org/1964283002/diff/120001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.h:42: void SetDragImage(const gfx::ImageSkia& image, nit: move down after HasCustomFormat() -- generally the order of overrides should match the order things were declared in the parent class. GetDragImage() and the others should be moved down as well (make sure to move them in the .mm as well) https://codereview.chromium.org/1964283002/diff/120001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.h:56: NSData* GetNSDataForType(NSString* type) const; non-overrides should be separate (move below, with a comment) https://codereview.chromium.org/1964283002/diff/120001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/1964283002/diff/120001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.mm:92: // There was no NSString, check for an NSURL. nit: put back blank line above? https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:284: return [self dragDropClient]->DragUpdate(sender); [self dragDropClient] needs to return null under some weird lifetime scenarios (see below) -- we will have to check on all of these methods https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:392: return views::NativeWidgetMac::GetBridgeForNativeWindow([self window]) GetBridgeForNativeWindow can return null. Annoying things like when AppKit has an NSEvent sitting in the event queue, which might also have a retained NSWindow or NSView referenced from it. So that needs to propagate through. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:340: drag_drop_client_.reset(new DragDropClientMac(this)); can this be done at the end of BridgedNativeWidget::Init()? Just after tooltip_manager_ might make sense, since they're declared that way https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:532: drag_drop_client_->SetRootView(view); hopefully we don't need this if we can avoid some of the weirdness of BridgedNativeWidgetTest -- we just need more of the setup from views::Widget https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... File ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:11: #include "ui/views/cocoa/drag_drop_client_mac.h" nit: This should be at the top of the file (and #import ) https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:28: @property NSDraggingFormation draggingFormation; looks like the 10.11 SDK "requires" this to implement @property(readonly) NSSpringLoadingHighlight springLoadingHighlight bj/ui/views/cocoa/views_unittests.desktop_drag_drop_client_mac_unittest.o ../../ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:32:17: error: auto property synthesis will not synthesize property 'springLoadingHighlight' declared in protocol 'NSDraggingInfo' [-Werror,-Wobjc-protocol-property-synthesis] @implementation MockDraggingInfo ^ /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSDragging.h:101:47: note: property declared here @property (readonly) NSSpringLoadingHighlight springLoadingHighlight NS_AVAILABLE_MAC(10_11); ^ 1 error generated. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:48: let's squish all these together by removing these lines between https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:103: class DragDropTestNativeWidgetMac : public NativeWidgetMac { I think we won't need this https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:127: bool GetDropFormats( // View: (i.e. to indicate overrides) https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:135: nit: these overrides can be squished together too https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:144: void SetDropFormats(int formats) { formats_ = formats; } nit: move this above the overrides, and call it set_formats(..) https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:148: int formats_; = 0; (needs to be initialized) https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:149: }; nit: DISALLOW_COPY_AND_ASSIGN(..) https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:151: class CocoaDragDropTest : public ui::CocoaTest { hopefuly inheriting from WidgetTest will get us better mileage. We miss out on some cool stuff from CocoaTest, but it should all be stuff that BridgedNativeWidget is responsible for, and has its own tests for. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:231: std::unique_ptr<views::View> view_; normally views are owned by the Widget* (via its RootView), so this will probably need to be a raw pointer https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:232: BridgedContentView* ns_view_; This can be retrieved from widget_->GetNativeView() https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:233: std::unique_ptr<Widget> widget_; It's more common for NativeWidgets to own themselves in regular usage (i.e. remove the `WIDGET_OWNS_NATIVE_WIDGET` bit), this becomes a Widget* and TearDown needs to call widget_->CloseNow(). I think that will make the test harness simpler, and more flexible. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:239: }; nit: private: DISALLOW_COPY_AND_ASSIGN(..) https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.h (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:5: #ifndef UI_VIEWS_COCOA_COCOA_DRAG_DROP_MANAGER_H_ update guard https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:21: @interface CocoaDragDropDataProvider : NSObject<NSPasteboardItemDataProvider> This needs to be VIEWS_EXPORT in order to link views_unittests in a component build. It needs a comment too. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:29: } // namespace gfx nit: we don't usually bother with comments on namespace scope ends for forward declaration blocks. same below https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:42: class VIEWS_EXPORT DragDropClientMac { nit: needs a comment (it's probably worth mentioning how it mimics the aura classes) https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:80: int operation_; needs to be initialized https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:83: BridgedNativeWidget* bridge_; // Weak. Owns |this|. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.mm (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:5: #include "ui/views/cocoa/drag_drop_client_mac.h" nit: import https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:10: #include "ui/base/dragdrop/os_exchange_data_provider_mac.h" nit:import https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:46: - (void)pasteboard:(NSPasteboard*)sender // NSPasteboardItemDataProvider protocol implementation. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:47: item:(NSPasteboardItem*)item nit: align colons https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:75: const ui::OSExchangeDataProviderMac* provider = const ui::OSExchangeDataProviderMac& (i.e. cast to the reference type rather than the pointer typE) https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:77: const base::NativeEvent event = provider->GetEvent()->native_event(); NSEvent* event? (`const` and NS* objects tend not to play nice) https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:96: source:nil]; looks like source: must be non-nil to compile against the 10.11 SDK. docs say it's typically the view or the window (BridgedContentView?), but it will need to implement the NSDraggingSource protocol. https://codereview.chromium.org/1964283002/diff/120001/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/view.cc#newco... ui/views/view.cc:2418: data.provider().SetEvent(event); So I'm still unsure about this. I found one place where it will come unstuck. In a mac_views_browser build, if you go to chrome://bookmarks and dry to drag a bookmark, you get a DCHECK failure: [48403:1295:0524/171204:FATAL:os_exchange_data_provider_mac.mm(172)] Check failed: event_.get(). logging::LogMessage::~LogMessage() + 35 ui::OSExchangeDataProviderMac::GetEvent() const + 328 views::DragDropClientMac::StartDragAndDrop(ui::OSExchangeData const&, gfx::Point const&, int, ui::DragDropTypes::DragEventSource) + 196 views::NativeWidgetMac::RunShellDrag(views::View*, ui::OSExchangeData const&, gfx::Point const&, int, ui::DragDropTypes::DragEventSource) + 109 views::Widget::RunShellDrag(views::View*, ui::OSExchangeData const&, gfx::Point const&, int, ui::DragDropTypes::DragEventSource) + 156 chrome::DragBookmarks(Profile*, std::__1::vector<bookmarks::BookmarkNode const*, std::__1::allocator<bookmarks::BookmarkNode const*> > const&, NSView*, ui::DragDropTypes::DragEventSource) + 572 extensions::BookmarkManagerPrivateStartDragFunction::RunOnReady() + 1234 This RunShellDrag call is made in bookmark_drag_drop_views.cc There is actually a Cocoa implementation too, in bookmark_drag_drop_cocoa.mm -- it does // Synthesize an event for dragging, since we can't be sure that // [NSApp currentEvent] will return a valid dragging event. NSWindow* window = [view window]; NSPoint position = [window mouseLocationOutsideOfEventStream]; NSTimeInterval event_time = [[NSApp currentEvent] timestamp]; NSEvent* drag_event = [NSEvent mouseEventWithType:NSLeftMouseDragged location:position modifierFlags:NSLeftMouseDraggedMask timestamp:event_time windowNumber:[window windowNumber] context:nil eventNumber:0 clickCount:1 pressure:1.0]; Can we do something like that lower down in the mac-specific plumbing? If we can, I think I'd prefer we do this "always" for now until it's a problem, rather than changing the code here. i.e. always use a synthetic event for view::DoDrag on Mac. Otherwise, we should modify the interface of RunShellDrag so that it takes in an Event* (which may be null, in which case we generate a synthetic event). https://codereview.chromium.org/1964283002/diff/120001/ui/views/widget/drop_h... File ui/views/widget/drop_helper.cc (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/widget/drop_h... ui/views/widget/drop_helper.cc:66: root_view = root_view_; Ah, BridgedNativeWidgetTestBase does some weird stuff with the root view. It's really for testing some low-level interactions, and it's tricky to plumb together. Modelling the tests off NativeWidgetMacTest from native_widget_mac_unittest.mm instead should get more of the Widget stuff plumbed through correctly. https://codereview.chromium.org/1964283002/diff/120001/ui/views/widget/native... File ui/views/widget/native_widget_mac.h (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/widget/native... ui/views/widget/native_widget_mac.h:22: class DragDropTestNativeWidgetMac; nit: sort https://codereview.chromium.org/1964283002/diff/120001/ui/views/widget/native... ui/views/widget/native_widget_mac.h:150: friend class test::DragDropTestNativeWidgetMac; nit: sort
Description was changed from ========== MacViews: Implemented Drag & Drop BUG=599585 ========== to ========== MacViews: Implemented Drag & Drop Created a Drag & Drop client on Mac that bridges between the BUG=599585 ==========
Description was changed from ========== MacViews: Implemented Drag & Drop Created a Drag & Drop client on Mac that bridges between the BUG=599585 ========== to ========== MacViews: Implemented Drag & Drop Created a Drag & Drop client on Mac that bridges between the Views and native OSX's drag and drop. Added a unit test for the client. BUG=599585 ==========
Description was changed from ========== MacViews: Implemented Drag & Drop Created a Drag & Drop client on Mac that bridges between the Views and native OSX's drag and drop. Added a unit test for the client. BUG=599585 ========== to ========== MacViews: Implemented Drag & Drop Created a Drag & Drop client on Mac that bridges between the Views and native OSX's drag and drop. This approach is based on the Aura approach. BUG=599585 TEST= Run the view unit test: CocoaDragDropTest ==========
Good catch! I dug into it and found out that the reason why that was happening was because OSX’s native drag&drop is async. This is different from the Windows, which blocks until dragging is completed. As such, OnDragDone() was called before the dragging actually finished, which messed up TextView’s logic. I changed the logic in widget.cc so that dragDropClient will callback OnDragDone once the dragging session has ended. I’m concern that doing this might be risky, but I didn’t run into any problems when I tested it out. WDYT? Also the tests are now working :) https://codereview.chromium.org/1964283002/diff/120001/ui/base/dragdrop/drag_... File ui/base/dragdrop/drag_utils_mac.mm (right): https://codereview.chromium.org/1964283002/diff/120001/ui/base/dragdrop/drag_... ui/base/dragdrop/drag_utils_mac.mm:24: data_object->provider().SetDragImage(image_skia, cursor_offset); On 2016/05/24 08:06:00, tapted wrote: > hum - these are the same implementations provided by drag_utils.cc and > drag_utils_aura.cc > > I think we should: > > - move drag_utils_aura.cc into drag_utils.cc and guard it with #if > !defined(OS_WIN). delete drag_utils_aura.cc > > - compile and link drag_utils.cc on Mac > > - delete drag_utils_mac.mm Done. https://codereview.chromium.org/1964283002/diff/120001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_mac.h (right): https://codereview.chromium.org/1964283002/diff/120001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.h:42: void SetDragImage(const gfx::ImageSkia& image, On 2016/05/24 08:06:00, tapted wrote: > nit: move down after HasCustomFormat() -- generally the order of overrides > should match the order things were declared in the parent class. GetDragImage() > and the others should be moved down as well (make sure to move them in the .mm > as well) Done. https://codereview.chromium.org/1964283002/diff/120001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.h:56: NSData* GetNSDataForType(NSString* type) const; On 2016/05/24 08:06:00, tapted wrote: > non-overrides should be separate (move below, with a comment) Done. https://codereview.chromium.org/1964283002/diff/120001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/1964283002/diff/120001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.mm:92: // There was no NSString, check for an NSURL. On 2016/05/24 08:06:00, tapted wrote: > nit: put back blank line above? Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:284: return [self dragDropClient]->DragUpdate(sender); On 2016/05/24 08:06:01, tapted wrote: > [self dragDropClient] needs to return null under some weird lifetime scenarios > (see below) -- we will have to check on all of these methods Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:392: return views::NativeWidgetMac::GetBridgeForNativeWindow([self window]) On 2016/05/24 08:06:00, tapted wrote: > GetBridgeForNativeWindow can return null. Annoying things like when AppKit has > an NSEvent sitting in the event queue, which might also have a retained NSWindow > or NSView referenced from it. So that needs to propagate through. Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:340: drag_drop_client_.reset(new DragDropClientMac(this)); On 2016/05/24 08:06:01, tapted wrote: > can this be done at the end of BridgedNativeWidget::Init()? Just after > tooltip_manager_ might make sense, since they're declared that way Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... File ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:11: #include "ui/views/cocoa/drag_drop_client_mac.h" On 2016/05/24 08:06:01, tapted wrote: > nit: This should be at the top of the file (and #import ) Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:28: @property NSDraggingFormation draggingFormation; On 2016/05/24 08:06:01, tapted wrote: > looks like the 10.11 SDK "requires" this to implement > > @property(readonly) NSSpringLoadingHighlight springLoadingHighlight > > bj/ui/views/cocoa/views_unittests.desktop_drag_drop_client_mac_unittest.o > ../../ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:32:17: error: auto > property synthesis will not synthesize property 'springLoadingHighlight' > declared in protocol 'NSDraggingInfo' > [-Werror,-Wobjc-protocol-property-synthesis] > @implementation MockDraggingInfo > ^ > /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSDragging.h:101:47: > note: property declared here > @property (readonly) NSSpringLoadingHighlight springLoadingHighlight > NS_AVAILABLE_MAC(10_11); > ^ > 1 error generated. Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:48: On 2016/05/24 08:06:01, tapted wrote: > let's squish all these together by removing these lines between Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:103: class DragDropTestNativeWidgetMac : public NativeWidgetMac { On 2016/05/24 08:06:01, tapted wrote: > I think we won't need this Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:127: bool GetDropFormats( On 2016/05/24 08:06:01, tapted wrote: > // View: > > (i.e. to indicate overrides) Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:135: On 2016/05/24 08:06:01, tapted wrote: > nit: these overrides can be squished together too The line will be > 80 characters if I squish them though, will that be alright? https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:148: int formats_; On 2016/05/24 08:06:01, tapted wrote: > = 0; > > (needs to be initialized) Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:149: }; On 2016/05/24 08:06:01, tapted wrote: > nit: DISALLOW_COPY_AND_ASSIGN(..) Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:151: class CocoaDragDropTest : public ui::CocoaTest { On 2016/05/24 08:06:01, tapted wrote: > hopefuly inheriting from WidgetTest will get us better mileage. We miss out on > some cool stuff from CocoaTest, but it should all be stuff that > BridgedNativeWidget is responsible for, and has its own tests for. Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:231: std::unique_ptr<views::View> view_; On 2016/05/24 08:06:01, tapted wrote: > normally views are owned by the Widget* (via its RootView), so this will > probably need to be a raw pointer Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:232: BridgedContentView* ns_view_; On 2016/05/24 08:06:01, tapted wrote: > This can be retrieved from widget_->GetNativeView() Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:233: std::unique_ptr<Widget> widget_; On 2016/05/24 08:06:01, tapted wrote: > It's more common for NativeWidgets to own themselves in regular usage (i.e. > remove the `WIDGET_OWNS_NATIVE_WIDGET` bit), this becomes a Widget* and TearDown > needs to call widget_->CloseNow(). I think that will make the test harness > simpler, and more flexible. Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:233: std::unique_ptr<Widget> widget_; On 2016/05/24 08:06:01, tapted wrote: > It's more common for NativeWidgets to own themselves in regular usage (i.e. > remove the `WIDGET_OWNS_NATIVE_WIDGET` bit), this becomes a Widget* and TearDown > needs to call widget_->CloseNow(). I think that will make the test harness > simpler, and more flexible. Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:239: }; On 2016/05/24 08:06:01, tapted wrote: > nit: private: DISALLOW_COPY_AND_ASSIGN(..) Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.h (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:5: #ifndef UI_VIEWS_COCOA_COCOA_DRAG_DROP_MANAGER_H_ On 2016/05/24 08:06:02, tapted wrote: > update guard Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:21: @interface CocoaDragDropDataProvider : NSObject<NSPasteboardItemDataProvider> On 2016/05/24 08:06:02, tapted wrote: > This needs to be VIEWS_EXPORT in order to link views_unittests in a component > build. It needs a comment too. Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:29: } // namespace gfx On 2016/05/24 08:06:02, tapted wrote: > nit: we don't usually bother with comments on namespace scope ends for forward > declaration blocks. same below Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:42: class VIEWS_EXPORT DragDropClientMac { On 2016/05/24 08:06:02, tapted wrote: > nit: needs a comment (it's probably worth mentioning how it mimics the aura > classes) Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:80: int operation_; On 2016/05/24 08:06:02, tapted wrote: > needs to be initialized Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:83: BridgedNativeWidget* bridge_; On 2016/05/24 08:06:01, tapted wrote: > // Weak. Owns |this|. Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.mm (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:5: #include "ui/views/cocoa/drag_drop_client_mac.h" On 2016/05/24 08:06:02, tapted wrote: > nit: import Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:10: #include "ui/base/dragdrop/os_exchange_data_provider_mac.h" On 2016/05/24 08:06:02, tapted wrote: > nit:import Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:46: - (void)pasteboard:(NSPasteboard*)sender On 2016/05/24 08:06:02, tapted wrote: > // NSPasteboardItemDataProvider protocol implementation. Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:47: item:(NSPasteboardItem*)item On 2016/05/24 08:06:02, tapted wrote: > nit: align colons Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:75: const ui::OSExchangeDataProviderMac* provider = On 2016/05/24 08:06:02, tapted wrote: > const ui::OSExchangeDataProviderMac& > > (i.e. cast to the reference type rather than the pointer typE) Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:77: const base::NativeEvent event = provider->GetEvent()->native_event(); On 2016/05/24 08:06:02, tapted wrote: > NSEvent* event? > > (`const` and NS* objects tend not to play nice) Removed https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:96: source:nil]; On 2016/05/24 08:06:02, tapted wrote: > looks like source: must be non-nil to compile against the 10.11 SDK. docs say > it's typically the view or the window (BridgedContentView?), but it will need to > implement the NSDraggingSource protocol. Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/view.cc#newco... ui/views/view.cc:2418: data.provider().SetEvent(event); On 2016/05/24 08:06:02, tapted wrote: > So I'm still unsure about this. I found one place where it will come unstuck. In > a mac_views_browser build, if you go to chrome://bookmarks and dry to drag a > bookmark, you get a DCHECK failure: > > [48403:1295:0524/171204:FATAL:os_exchange_data_provider_mac.mm(172)] Check > failed: event_.get(). > logging::LogMessage::~LogMessage() + 35 > ui::OSExchangeDataProviderMac::GetEvent() const + 328 > views::DragDropClientMac::StartDragAndDrop(ui::OSExchangeData const&, gfx::Point > const&, int, ui::DragDropTypes::DragEventSource) + 196 > views::NativeWidgetMac::RunShellDrag(views::View*, ui::OSExchangeData const&, > gfx::Point const&, int, ui::DragDropTypes::DragEventSource) + 109 > views::Widget::RunShellDrag(views::View*, ui::OSExchangeData const&, gfx::Point > const&, int, ui::DragDropTypes::DragEventSource) + 156 > chrome::DragBookmarks(Profile*, std::__1::vector<bookmarks::BookmarkNode const*, > std::__1::allocator<bookmarks::BookmarkNode const*> > const&, NSView*, > ui::DragDropTypes::DragEventSource) + 572 > extensions::BookmarkManagerPrivateStartDragFunction::RunOnReady() + 1234 > > This RunShellDrag call is made in bookmark_drag_drop_views.cc > > There is actually a Cocoa implementation too, in bookmark_drag_drop_cocoa.mm -- > it does > > > > // Synthesize an event for dragging, since we can't be sure that > // [NSApp currentEvent] will return a valid dragging event. > NSWindow* window = [view window]; > NSPoint position = [window mouseLocationOutsideOfEventStream]; > NSTimeInterval event_time = [[NSApp currentEvent] timestamp]; > NSEvent* drag_event = [NSEvent mouseEventWithType:NSLeftMouseDragged > location:position > modifierFlags:NSLeftMouseDraggedMask > timestamp:event_time > windowNumber:[window windowNumber] > context:nil > eventNumber:0 > clickCount:1 > pressure:1.0]; > > > Can we do something like that lower down in the mac-specific plumbing? > > If we can, I think I'd prefer we do this "always" for now until it's a problem, > rather than changing the code here. i.e. always use a synthetic event for > view::DoDrag on Mac. > > Otherwise, we should modify the interface of RunShellDrag so that it takes in an > Event* (which may be null, in which case we generate a synthetic event). Ahh, that's a really good point! I made a change so that it generates a synthetic event instead. What do you mean by "lower down in the mac-specific plumbing"? https://codereview.chromium.org/1964283002/diff/120001/ui/views/widget/drop_h... File ui/views/widget/drop_helper.cc (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/widget/drop_h... ui/views/widget/drop_helper.cc:66: root_view = root_view_; On 2016/05/24 08:06:02, tapted wrote: > Ah, BridgedNativeWidgetTestBase does some weird stuff with the root view. It's > really for testing some low-level interactions, and it's tricky to plumb > together. > > Modelling the tests off NativeWidgetMacTest from native_widget_mac_unittest.mm > instead should get more of the Widget stuff plumbed through correctly. Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/widget/native... File ui/views/widget/native_widget_mac.h (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/widget/native... ui/views/widget/native_widget_mac.h:22: class DragDropTestNativeWidgetMac; On 2016/05/24 08:06:02, tapted wrote: > nit: sort Done. https://codereview.chromium.org/1964283002/diff/120001/ui/views/widget/native... ui/views/widget/native_widget_mac.h:150: friend class test::DragDropTestNativeWidgetMac; On 2016/05/24 08:06:02, tapted wrote: > nit: sort Done.
Ooh - the async thing is interesting. I think on Windows the OS requires it to be synchronous. On Linux/X11, it looks like it is natively asynchronous, but we spin our own run loop to make it synchronous to match Windows. I might need to play with it some more (ran out of time today). If it can meet all our use cases I don't know of any deal breakers in diverging from the other platforms - but sky or sadrul might know details/traps. It's also possible that spinning a run loop on Mac to make it synchronous is a simpler prospect than it is on Linux (X11WholeScreenMoveLoop is quite complex). If dragging extension icons around on the toolbar works OK with this, and dragging bookmarks on the bookmark bar (and dragging items around on open folders/menus coming off the bookmark bar), then maybe this is all we need. But we'd need to make a case for diverging from platform parity :/ There's some overlap with tab dragging too (and in fact for X11 the git logs suggest they share[d] code). The tab dragging stuff is in review at https://codereview.chromium.org/1747803003/diff/280001/ui/views/cocoa/cocoa_w... - there might be ideas there if we _do_ need to try out the make-it-synchronous-by-spinning-a-run-loop thing. There are some other things here (e.g. we need to implement the NSDraggingSource). Then I think the next steps should be - see if there are any drag and drop interactive ui tests we can try out on a mac_views_browser_build (e.g. for the bookmark bar) - see if they pass - try manually testing dragging toolbar/bookmarks with mac_views_browser - see if it's easy to switch to synchronous with a run loop - reach out to sky/sadrul once we've gathered data on the above if we think asynchronous is the better option for Mac https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... File ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm (right): https://codereview.chromium.org/1964283002/diff/120001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:135: On 2016/05/26 01:56:54, spqchan wrote: > On 2016/05/24 08:06:01, tapted wrote: > > nit: these overrides can be squished together too > > The line will be > 80 characters if I squish them though, will that be alright? Ah, I just meant remove the blank lines between the methods. Then I'd just do a `git cl format` to ensure it's still happy https://codereview.chromium.org/1964283002/diff/200001/ui/base/BUILD.gn File ui/base/BUILD.gn (right): https://codereview.chromium.org/1964283002/diff/200001/ui/base/BUILD.gn#newco... ui/base/BUILD.gn:158: "dragdrop/drag_utils.cc", this is added in !is_ios, so doesn't need to be listed here https://codereview.chromium.org/1964283002/diff/200001/ui/base/BUILD.gn#newco... ui/base/BUILD.gn:516: "dragdrop/drag_utils.h", this should be removed too https://codereview.chromium.org/1964283002/diff/200001/ui/base/ui_base.gyp File ui/base/ui_base.gyp (right): https://codereview.chromium.org/1964283002/diff/200001/ui/base/ui_base.gyp#ne... ui/base/ui_base.gyp:571: 'dragdrop/drag_utils.h', remove https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:304: return false; nit: false -> NO https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/desktop... File ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm (right): https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:108: LOG(INFO) << "GetDropFormats"; remove stray log https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:147: widget_->GetRootView()->AddChildView(target_); the view should be added to as a child of GetContentsView (usually it's just window frames and containers that end up as children of the root view) https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:148: widget_->GetContentsView()->size(); remove (since it has no side-effects) https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.mm (right): https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:112: source:nil]; On 2016/05/26 01:56:55, spqchan wrote: > On 2016/05/24 08:06:02, tapted wrote: > > looks like source: must be non-nil to compile against the 10.11 SDK. docs say > > it's typically the view or the window (BridgedContentView?), but it will need > to > > implement the NSDraggingSource protocol. > > Done. ping? (The 10.11 SDK requires this to be non-nil) https://codereview.chromium.org/1964283002/diff/200001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1964283002/diff/200001/ui/views/widget/widget... ui/views/widget/widget.cc:785: DraggingEnded(view); Hm - x11 spins its own runloop to "force" it to be synchronous. e.g. desktop_drag_drop_client_aurax11.cc has (in StartDragAndDrop(..) // Windows has a specific method, DoDragDrop(), which performs the entire // drag. We have to emulate this, so we spin off a nested runloop which will // track all cursor movement and reroute events to a specific handler. move_loop_->RunMoveLoop( source_window, cursor_manager_->GetInitializedCursor(ui::kCursorGrabbing)); RunMoveLoop calls X11WholeScreenMoveLoop which (eventually) does base::MessageLoopForUI* loop = base::MessageLoopForUI::current(); base::MessageLoop::ScopedNestableTaskAllower allow_nested(loop); base::RunLoop run_loop; quit_closure_ = run_loop.QuitClosure(); run_loop.Run(); (i.e. that last Run() line will block, running a nested message loop until the drag is finished). X11WholeScreenMoveLoop seems to be massively complicated though -- its git log is huge (lots of corner cases and lifetime bugs :/ ). Maybe an asynchronous drag and drop is a much nicer way to solve this use case. But I'm out of my depth here :/. Presumably there's a good reason for it to be done on x11 the way it is.
Sounds good, I managed to get a runloop working by copying some of the code in bookmark_button.mm. It works out pretty well, so I think we can use synchronous :) I looked around and I can’t find any interactive ui tests for drag and drop. Writing one would be tricky, especially since Cocoa’s drag and drop events are different from the mouse processing ones. https://codereview.chromium.org/1964283002/diff/200001/ui/base/BUILD.gn File ui/base/BUILD.gn (right): https://codereview.chromium.org/1964283002/diff/200001/ui/base/BUILD.gn#newco... ui/base/BUILD.gn:158: "dragdrop/drag_utils.cc", On 2016/05/26 12:31:41, tapted wrote: > this is added in !is_ios, so doesn't need to be listed here Done. https://codereview.chromium.org/1964283002/diff/200001/ui/base/BUILD.gn#newco... ui/base/BUILD.gn:516: "dragdrop/drag_utils.h", On 2016/05/26 12:31:41, tapted wrote: > this should be removed too Done. https://codereview.chromium.org/1964283002/diff/200001/ui/base/ui_base.gyp File ui/base/ui_base.gyp (right): https://codereview.chromium.org/1964283002/diff/200001/ui/base/ui_base.gyp#ne... ui/base/ui_base.gyp:571: 'dragdrop/drag_utils.h', On 2016/05/26 12:31:42, tapted wrote: > remove Done. https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:304: return false; On 2016/05/26 12:31:42, tapted wrote: > nit: false -> NO Done. https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/desktop... File ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm (right): https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:108: LOG(INFO) << "GetDropFormats"; On 2016/05/26 12:31:42, tapted wrote: > remove stray log Done. https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:147: widget_->GetRootView()->AddChildView(target_); On 2016/05/26 12:31:42, tapted wrote: > the view should be added to as a child of GetContentsView (usually it's just > window frames and containers that end up as children of the root view) Done. https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:148: widget_->GetContentsView()->size(); On 2016/05/26 12:31:42, tapted wrote: > remove (since it has no side-effects) Done. https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.mm (right): https://codereview.chromium.org/1964283002/diff/200001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:112: source:nil]; On 2016/05/26 12:31:42, tapted wrote: > On 2016/05/26 01:56:55, spqchan wrote: > > On 2016/05/24 08:06:02, tapted wrote: > > > looks like source: must be non-nil to compile against the 10.11 SDK. docs > say > > > it's typically the view or the window (BridgedContentView?), but it will > need > > to > > > implement the NSDraggingSource protocol. > > > > Done. > > ping? (The 10.11 SDK requires this to be non-nil) Ah sorry, it looks like when I switched branches, I accidentally left a couple of commits behind. I just uploaded the commit with the changes
One thing I forgot to mention, I added a "x-os-exchange-data" format to OSExchangeDataProvider so that PickledData can now be drag and drop. The extension buttons are now droppable
https://codereview.chromium.org/1964283002/diff/300001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/1964283002/diff/300001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.mm:22: const std::string kClipboardFormatString = "chromium/x-os-exchange-data"; I don't really understand this bit -- what's the use case for the PickleData? Is one of the kMimeTypeFoo constants in ui/base/clipboard/clipboard.h or ui/base/clipboard/custom_data_helper.h a better fit? https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:29: #include "ui/views/cocoa/drag_drop_client_mac.h" nit: import https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:284: [self registerForDraggedTypes:ui::OSExchangeDataProviderMac:: nit: move this into the if {} https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:318: - (NSDragOperation)draggingEntered:(id<NSDraggingInfo>)sender { nit: these should move down after the other NSView overrides - probably immediately before the NSTextInputClient protocol implementation, and be proceeded by // NSDraggingDestination protocol overrides: https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:332: return NO; nit: we can do just return client && client->Drop(sender) != NSDragOperationNone; (sorry - should have noticed this before) https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1228: // NSDraggingSource protocol implementation nit: move this up, to come after NSUserInterfaceValidations https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:532: drag_drop_client_->SetRootView(view); ooh - so we should remove the reference to |view| in drag_drop_client_ when something calls SetRootView(nullptr), otherwise it might be referring to deleted data. Even better, we should just reset drag_drop_client_ I think the best way to do this is to pass |view| into the DragDropClient constructor. That means: move the `drag_drop_client_.reset(new DragDropClientMac(this));` call in Init() down to here (and also pass |view| -- remove DragDropClientMac::SetRootView Then add a call to `drag_drop_client_.reset()` up above, just before the [bridged_view_ clearView] call. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/desktop... File ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm (right): https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:10: #import "ui/gfx/test/ui_cocoa_test_helper.h" remove? unused now i think https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:169: MockDraggingInfo* info = I think these are leaked right now. To fix, maybe something like a scoped_nsobject<MockDraggingInfo> dragging_info_; DragUpdate(... pasteboard) { DCHECK(!dragging_info_); dragging_info_.reset([[MockDraggingInfo alloc] init:..]); } Then Drop() doesn't need to take a |pasteboard| argument - just use the existing dragging_info_, but it should do dragging_info_.reset() after calling client->Drop(..) https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:206: EXPECT_NE(DragUpdate(nil), NSDragOperationNone); Can we EXPECT_EQ against something for a stronger test? https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:239: EXPECT_NE(DragUpdate(pasteboard), NSDragOperationNone); EXPECT_EQ? https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.h (right): https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:11: #include <vector> remove? (seems unused) https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:17: #include "ui/views/widget/drop_helper.h" remove? (forward declare should work - see below). Edit: but if we can set the root view in the constructor, DropHelper can just be a value member rather than a unique_ptr (and <memory> can be removed). See comments in BridgedNativeWidget::SetRootView() https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:19: class DropHelper; should this be in namespace views {? https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:26: nit: remove blank line https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:28: nit: remove blank line https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:91: View* view_; This seems unused -- can we remove it? In fact neither NativeWidgetAura nor DesktopNativeWidgetAura's RunShellDrag implementations seem to pass through the |view| argument - maybe it can be purged. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.mm (right): https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:19: std::unique_ptr<ui::OSExchangeData> data_; ah, so I think this can actually go to the @implementation - that's the more common approach. However, we should still keep the private category to define the `private` API: the stuff that not on the public @interface, and not on any inherited methods, so here w should list @interface CocoaDragDropDataProvider () - (id)initWithPasteboard:(NSPasteboard*)pasteboard; - (ui::OSExchangeData*)data; @end https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:49: const ui::OSExchangeDataProviderMac* provider = nit: const ui::OSExchangeDataProviderMac& provider (i.e cast the reference type instead of the pointer type) https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:60: : operation_(0), bridge_(bridge) { add (if we keep it): view_(nullptr) https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:70: const gfx::Point& location, We're not using |location| here...We should probably prefer it over [window mouseLocationOutsideOfEventStream]; since it's possible for the event queue to lag behind. But we need to convert it into AppKit coordinates. But also that's more complicated and maybe we actually don't need it :). But we shouldn't pass in |location| if we don't use it. I think StartDragAndDrop(..) just needs to take |data|, |operation| and |source| https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:103: base::scoped_nsobject<NSDraggingItem> dragItem( nit: dragItem -> drag_item since we're in a C++ fn https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:105: NSRect draggingFrame = Maybe NSRect dragging_frame = { [event locationInWindow], [image size] }; dragging_frame.origin.y -= dragging_frame.size.height; But also it's not immediately obvious why we need to subtract the height - comment? https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:116: [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode We should use the run loop stuff in base (what you have is close to MessagePumpNSRunLoop::DoRun() but there is some other important goo that happens either side). Here, we would do: base::RunLoop run_loop; quit_closure_ = run_loop.QuitClosure(); run_loop.Run(); and at the end of EndDrag(), we do quit_closure_.Run(); (and declare quit_closure_ as a data member) https://codereview.chromium.org/1964283002/diff/300001/ui/views/widget/native... File ui/views/widget/native_widget_mac.h (right): https://codereview.chromium.org/1964283002/diff/300001/ui/views/widget/native... ui/views/widget/native_widget_mac.h:147: friend class test::HitTestNativeWidgetMac; ah, I guess we should take this out of the diff now that we no longer have changes to this file :) https://codereview.chromium.org/1964283002/diff/300001/ui/views/widget/native... File ui/views/widget/native_widget_mac.mm (right): https://codereview.chromium.org/1964283002/diff/300001/ui/views/widget/native... ui/views/widget/native_widget_mac.mm:23: #include "ui/views/cocoa/drag_drop_client_mac.h" nit: import https://codereview.chromium.org/1964283002/diff/300001/ui/views/widget/native... ui/views/widget/native_widget_mac.mm:205: // TODO(tapted): Something for drag and drop might be needed here in future. I think we need to plumb this in as well. E.g. NativeWidgetAura does DCHECK(drop_helper_.get() != NULL); drop_helper_->ResetTargetViewIfEquals(view); We need something like DCHECK(bridge_); DCHECK(bridge_->drag_drop_client()); bridge_->drag_drop_client()->drop_helper()->ResetTargetViewIfEquals(view);
PTAL https://codereview.chromium.org/1964283002/diff/300001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/1964283002/diff/300001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.mm:22: const std::string kClipboardFormatString = "chromium/x-os-exchange-data"; On 2016/05/31 11:49:57, tapted wrote: > I don't really understand this bit -- what's the use case for the PickleData? Is > one of the kMimeTypeFoo constants in ui/base/clipboard/clipboard.h or > ui/base/clipboard/custom_data_helper.h a better fit? Ah, good point. I switched to kWebCustomDataPboardType. PickleData is pretty much custom data, and is currently used by BrowserActionDragData https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:29: #include "ui/views/cocoa/drag_drop_client_mac.h" On 2016/05/31 11:49:58, tapted wrote: > nit: import Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:284: [self registerForDraggedTypes:ui::OSExchangeDataProviderMac:: On 2016/05/31 11:49:57, tapted wrote: > nit: move this into the if {} Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:318: - (NSDragOperation)draggingEntered:(id<NSDraggingInfo>)sender { On 2016/05/31 11:49:57, tapted wrote: > nit: these should move down after the other NSView overrides - probably > immediately before the NSTextInputClient protocol implementation, and be > proceeded by > > // NSDraggingDestination protocol overrides: Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:332: return NO; On 2016/05/31 11:49:57, tapted wrote: > nit: we can do just > > return client && client->Drop(sender) != NSDragOperationNone; > > (sorry - should have noticed this before) Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1228: // NSDraggingSource protocol implementation On 2016/05/31 11:49:57, tapted wrote: > nit: move this up, to come after NSUserInterfaceValidations Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:532: drag_drop_client_->SetRootView(view); On 2016/05/31 11:49:58, tapted wrote: > ooh - so we should remove the reference to |view| in drag_drop_client_ when > something calls SetRootView(nullptr), otherwise it might be referring to deleted > data. Even better, we should just reset drag_drop_client_ > > I think the best way to do this is to pass |view| into the DragDropClient > constructor. That means: > > move the `drag_drop_client_.reset(new DragDropClientMac(this));` call in Init() > down to here (and also pass |view| -- remove DragDropClientMac::SetRootView > > Then add a call to `drag_drop_client_.reset()` up above, just before the > [bridged_view_ clearView] call. Done. Note, I didn't add drag_drop_client_.reset() because it actually conflicts with the implementation for ViewRemoved(). https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/desktop... File ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm (right): https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:10: #import "ui/gfx/test/ui_cocoa_test_helper.h" On 2016/05/31 11:49:58, tapted wrote: > remove? unused now i think Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:169: MockDraggingInfo* info = On 2016/05/31 11:49:58, tapted wrote: > I think these are leaked right now. To fix, maybe something like a > scoped_nsobject<MockDraggingInfo> dragging_info_; > > DragUpdate(... pasteboard) { > DCHECK(!dragging_info_); > dragging_info_.reset([[MockDraggingInfo alloc] init:..]); > } > > Then Drop() doesn't need to take a |pasteboard| argument - just use the existing > dragging_info_, but it should do dragging_info_.reset() after calling > client->Drop(..) Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:206: EXPECT_NE(DragUpdate(nil), NSDragOperationNone); On 2016/05/31 11:49:58, tapted wrote: > Can we EXPECT_EQ against something for a stronger test? Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:239: EXPECT_NE(DragUpdate(pasteboard), NSDragOperationNone); On 2016/05/31 11:49:58, tapted wrote: > EXPECT_EQ? Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.h (right): https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:11: #include <vector> On 2016/05/31 11:49:58, tapted wrote: > remove? (seems unused) Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:17: #include "ui/views/widget/drop_helper.h" On 2016/05/31 11:49:58, tapted wrote: > remove? (forward declare should work - see below). Edit: but if we can set the > root view in the constructor, DropHelper can just be a value member rather than > a unique_ptr (and <memory> can be removed). See comments in > BridgedNativeWidget::SetRootView() Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:19: class DropHelper; On 2016/05/31 11:49:58, tapted wrote: > should this be in namespace views {? Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:26: On 2016/05/31 11:49:58, tapted wrote: > nit: remove blank line Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:28: On 2016/05/31 11:49:58, tapted wrote: > nit: remove blank line Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:91: View* view_; On 2016/05/31 11:49:58, tapted wrote: > This seems unused -- can we remove it? In fact neither NativeWidgetAura nor > DesktopNativeWidgetAura's RunShellDrag implementations seem to pass through the > |view| argument - maybe it can be purged. Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.mm (right): https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:19: std::unique_ptr<ui::OSExchangeData> data_; On 2016/05/31 11:49:58, tapted wrote: > ah, so I think this can actually go to the @implementation - that's the more > common approach. However, we should still keep the private category to define > the `private` API: the stuff that not on the public @interface, and not on any > inherited methods, so here w should list > > @interface CocoaDragDropDataProvider () > - (id)initWithPasteboard:(NSPasteboard*)pasteboard; > - (ui::OSExchangeData*)data; > @end > Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:49: const ui::OSExchangeDataProviderMac* provider = On 2016/05/31 11:49:58, tapted wrote: > nit: > > const ui::OSExchangeDataProviderMac& provider (i.e cast the reference type > instead of the pointer type) Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:60: : operation_(0), bridge_(bridge) { On 2016/05/31 11:49:58, tapted wrote: > add (if we keep it): view_(nullptr) Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:70: const gfx::Point& location, On 2016/05/31 11:49:58, tapted wrote: > We're not using |location| here...We should probably prefer it over [window > mouseLocationOutsideOfEventStream]; since it's possible for the event queue to > lag behind. But we need to convert it into AppKit coordinates. But also that's > more complicated and maybe we actually don't need it :). > > But we shouldn't pass in |location| if we don't use it. I think > StartDragAndDrop(..) just needs to take |data|, |operation| and |source| Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:103: base::scoped_nsobject<NSDraggingItem> dragItem( On 2016/05/31 11:49:58, tapted wrote: > nit: dragItem -> drag_item since we're in a C++ fn Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:105: NSRect draggingFrame = On 2016/05/31 11:49:58, tapted wrote: > Maybe > NSRect dragging_frame = { [event locationInWindow], [image size] }; > dragging_frame.origin.y -= dragging_frame.size.height; > > But also it's not immediately obvious why we need to subtract the height - > comment? Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:116: [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode On 2016/05/31 11:49:58, tapted wrote: > We should use the run loop stuff in base (what you have is close to > MessagePumpNSRunLoop::DoRun() but there is some other important goo that happens > either side). > > Here, we would do: > > base::RunLoop run_loop; > quit_closure_ = run_loop.QuitClosure(); > run_loop.Run(); > > and at the end of EndDrag(), we do > > quit_closure_.Run(); > > > (and declare quit_closure_ as a data member) Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/widget/native... File ui/views/widget/native_widget_mac.h (right): https://codereview.chromium.org/1964283002/diff/300001/ui/views/widget/native... ui/views/widget/native_widget_mac.h:147: friend class test::HitTestNativeWidgetMac; On 2016/05/31 11:49:58, tapted wrote: > ah, I guess we should take this out of the diff now that we no longer have > changes to this file :) Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/widget/native... File ui/views/widget/native_widget_mac.mm (right): https://codereview.chromium.org/1964283002/diff/300001/ui/views/widget/native... ui/views/widget/native_widget_mac.mm:23: #include "ui/views/cocoa/drag_drop_client_mac.h" On 2016/05/31 11:49:59, tapted wrote: > nit: import Done. https://codereview.chromium.org/1964283002/diff/300001/ui/views/widget/native... ui/views/widget/native_widget_mac.mm:205: // TODO(tapted): Something for drag and drop might be needed here in future. On 2016/05/31 11:49:58, tapted wrote: > I think we need to plumb this in as well. > > E.g. NativeWidgetAura does > > DCHECK(drop_helper_.get() != NULL); > drop_helper_->ResetTargetViewIfEquals(view); > > We need something like > > DCHECK(bridge_); > DCHECK(bridge_->drag_drop_client()); > bridge_->drag_drop_client()->drop_helper()->ResetTargetViewIfEquals(view); Done.
https://codereview.chromium.org/1964283002/diff/340001/base/mac/sdk_forward_d... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/1964283002/diff/340001/base/mac/sdk_forward_d... base/mac/sdk_forward_declarations.h:251: typedef NSUInteger NSSpringLoadingHighlight; hm https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Applica... lists this as `NSUInteger`, but MacOSX10.11.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSDragging.h lists this as `NSInteger` hopefully that won't matter - this is fine. https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:603: // NSDraggingDestination protocol overrides: nit: full stop instead of colon https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1211: // NSDraggingSource protocol implementation nit: full stop at end https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:522: hm - I think we do still need the drag_drop_client_.reset() here, so that accesses from the NSView* get nerfed. We probably just have to change a DCHECK in ViewRemoved into an `if` https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/desktop... File ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm (right): https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:174: NSDragOperation Drop(NSPasteboard* pasteboard) { remove |pasteboard| argument https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:192: DISALLOW_COPY_AND_ASSIGN(CocoaDragDropTest); nit: blank line before, but also this needs to be in private: to work properly. i.e. ... base::scoped_nsobject<MockDraggingInfo> dragging_info_; private: DISALLOW_COPY_AND_ASSIGN(CocoaDragDropTest); }; https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.h (right): https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:77: std::unique_ptr<DropHelper> drop_helper_; Can this just be DropHelper drop_helper_; we can initialize it in the initializer list. https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.mm (right): https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:151: if (is_run_loop_running_) Does this work: // Allow a test to invoke EndDrag() without spinning the nested run loop. if (!quit_closure_.is_null()) { quit_closure_.Run(); quit_closure_.Reset(); } then we can get rid of is_run_loop_running_ https://codereview.chromium.org/1964283002/diff/340001/ui/views/widget/native... File ui/views/widget/native_widget_mac.mm (right): https://codereview.chromium.org/1964283002/diff/340001/ui/views/widget/native... ui/views/widget/native_widget_mac.mm:205: if (bridge_) { Maybe something like DragDropClientMac* client = bridge_ ? bridge_->drag_drop_client() : nullptr; if (client) client->drop_helper()->ResetTargetViewIfEquals(view);
PTAL https://codereview.chromium.org/1964283002/diff/340001/base/mac/sdk_forward_d... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/1964283002/diff/340001/base/mac/sdk_forward_d... base/mac/sdk_forward_declarations.h:251: typedef NSUInteger NSSpringLoadingHighlight; On 2016/06/01 01:57:00, tapted wrote: > hm > https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Applica... > lists this as `NSUInteger`, but > MacOSX10.11.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSDragging.h > lists this as `NSInteger` > > hopefully that won't matter - this is fine. Acknowledged. https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:603: // NSDraggingDestination protocol overrides: On 2016/06/01 01:57:01, tapted wrote: > nit: full stop instead of colon Done. https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1211: // NSDraggingSource protocol implementation On 2016/06/01 01:57:00, tapted wrote: > nit: full stop at end Done. https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:522: On 2016/06/01 01:57:01, tapted wrote: > hm - I think we do still need the drag_drop_client_.reset() here, so that > accesses from the NSView* get nerfed. We probably just have to change a DCHECK > in ViewRemoved into an `if` Done. https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/desktop... File ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm (right): https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:174: NSDragOperation Drop(NSPasteboard* pasteboard) { On 2016/06/01 01:57:01, tapted wrote: > remove |pasteboard| argument Done. https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:192: DISALLOW_COPY_AND_ASSIGN(CocoaDragDropTest); On 2016/06/01 01:57:01, tapted wrote: > nit: blank line before, but also this needs to be in private: to work properly. > i.e. > > ... > base::scoped_nsobject<MockDraggingInfo> dragging_info_; > > private: > DISALLOW_COPY_AND_ASSIGN(CocoaDragDropTest); > }; Done. https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.h (right): https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:77: std::unique_ptr<DropHelper> drop_helper_; On 2016/06/01 01:57:01, tapted wrote: > Can this just be > > DropHelper drop_helper_; > > we can initialize it in the initializer list. I don't think it'll work, since DropHelper is DISALLOW_COPY_AND_ASSIGN, unless I'm missing something here. https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.mm (right): https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:151: if (is_run_loop_running_) On 2016/06/01 01:57:01, tapted wrote: > Does this work: > > // Allow a test to invoke EndDrag() without spinning the nested run loop. > if (!quit_closure_.is_null()) { > quit_closure_.Run(); > quit_closure_.Reset(); > } > > then we can get rid of is_run_loop_running_ Nice! Yep, it works https://codereview.chromium.org/1964283002/diff/340001/ui/views/widget/native... File ui/views/widget/native_widget_mac.mm (right): https://codereview.chromium.org/1964283002/diff/340001/ui/views/widget/native... ui/views/widget/native_widget_mac.mm:205: if (bridge_) { On 2016/06/01 01:57:01, tapted wrote: > Maybe something like > > DragDropClientMac* client = bridge_ ? bridge_->drag_drop_client() : nullptr; > if (client) > client->drop_helper()->ResetTargetViewIfEquals(view); Done.
lgtm - the rest is just mechanical stuff. When it's ready, we should rope in sky for OWNERS, since I think he knows the most about how OSExchangeData should work, and compatibility with how views:: likes its drag and drop to operate https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.h (right): https://codereview.chromium.org/1964283002/diff/340001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:77: std::unique_ptr<DropHelper> drop_helper_; On 2016/06/01 19:09:29, spqchan wrote: > On 2016/06/01 01:57:01, tapted wrote: > > Can this just be > > > > DropHelper drop_helper_; > > > > we can initialize it in the initializer list. > > I don't think it'll work, since DropHelper is DISALLOW_COPY_AND_ASSIGN, unless > I'm missing something here. It should still work. DragDropClientMac is also DISALLOW_COPY_AND_ASSIGN so there shouldn't be any code generated that tries to copy or assign its members. And this is sorta called out in the chromium style guide, so we should probably try to get it to work - i.e. https://www.chromium.org/developers/coding-style#TOC-Forward-declarations-vs.... says "if it would otherwise make sense to use a type as a member by-value, don't convert it to a pointer just to be able to forward-declare the type" Generally, if there's a unique_ptr that's assigned in the constructor and then never changes for the life of the object, then it should be a member by-value to avoid the separate allocation https://codereview.chromium.org/1964283002/diff/380001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_mac.h (right): https://codereview.chromium.org/1964283002/diff/380001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.h:16: @class NSArray; nit: sort https://codereview.chromium.org/1964283002/diff/380001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/1964283002/diff/380001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.mm:16: #import "ui/base/clipboard/custom_data_helper.h" nit: include https://codereview.chromium.org/1964283002/diff/380001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.mm:194: std::unique_ptr<OSExchangeData> nit: // static https://codereview.chromium.org/1964283002/diff/380001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.mm:204: NSArray* OSExchangeDataProviderMac::SupportedPasteboardTypes() { nit: // static https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:233: // Returns the native widget's drag drop client. nit: widget -> Widget Also we should add. "Possibly null." at the end https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/desktop... File ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm (right): https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:136: class CocoaDragDropTest : public WidgetTest { oops - we should name this class to be consistent with the file it's testing for. i.e. DragDropClientMacTest . The filename should be updated too -- drag_drop_client_mac_unittest.mm (i.e. remove `desktop_` prefix) https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.h (right): https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:12: #include "base/run_loop.h" nit: move to .mm? You might need #include "base/callback.h" instead https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:39: class DropHelper; nit: forward-declare class View; so we're not relying on the one in drop_helper.h https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.mm (right): https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:15: #include "ui/views/widget/drop_helper.h" nit: not needed since it's in the header https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:64: drop_helper_.reset(new DropHelper(root_view)); moving this up to the initializer list on line 62 and just having drop_helper_(root_view), in the initializer list should make the `DropHelper drop_helper_` member work. (and then git cl-format :) https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:117: base::RunLoop run_loop; nit: we should comment here, something like // Drag and drop is asynchronous on Mac, but we spin a nested run loop for // consistency with other platforms. https://codereview.chromium.org/1964283002/diff/380001/ui/views/views.gyp File ui/views/views.gyp (right): https://codereview.chromium.org/1964283002/diff/380001/ui/views/views.gyp#new... ui/views/views.gyp:587: 'cocoa/desktop_drag_drop_client_mac_unittest.mm', (remove `desktop_`)
PTAL https://codereview.chromium.org/1964283002/diff/380001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_mac.h (right): https://codereview.chromium.org/1964283002/diff/380001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.h:16: @class NSArray; On 2016/06/02 00:24:50, tapted wrote: > nit: sort Done. https://codereview.chromium.org/1964283002/diff/380001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/1964283002/diff/380001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.mm:16: #import "ui/base/clipboard/custom_data_helper.h" On 2016/06/02 00:24:50, tapted wrote: > nit: include Done. https://codereview.chromium.org/1964283002/diff/380001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.mm:194: std::unique_ptr<OSExchangeData> On 2016/06/02 00:24:50, tapted wrote: > nit: // static Done. https://codereview.chromium.org/1964283002/diff/380001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.mm:204: NSArray* OSExchangeDataProviderMac::SupportedPasteboardTypes() { On 2016/06/02 00:24:50, tapted wrote: > nit: // static Done. https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:233: // Returns the native widget's drag drop client. On 2016/06/02 00:24:51, tapted wrote: > nit: widget -> Widget > > Also we should add. "Possibly null." at the end Done. https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/desktop... File ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm (right): https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/desktop... ui/views/cocoa/desktop_drag_drop_client_mac_unittest.mm:136: class CocoaDragDropTest : public WidgetTest { On 2016/06/02 00:24:51, tapted wrote: > oops - we should name this class to be consistent with the file it's testing > for. i.e. DragDropClientMacTest . The filename should be updated too -- > drag_drop_client_mac_unittest.mm (i.e. remove `desktop_` prefix) Done. https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.h (right): https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:12: #include "base/run_loop.h" On 2016/06/02 00:24:51, tapted wrote: > nit: move to .mm? You might need #include "base/callback.h" instead Done. https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.h:39: class DropHelper; On 2016/06/02 00:24:51, tapted wrote: > nit: forward-declare > > class View; > > so we're not relying on the one in drop_helper.h Done. https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.mm (right): https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:15: #include "ui/views/widget/drop_helper.h" On 2016/06/02 00:24:51, tapted wrote: > nit: not needed since it's in the header Done https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:64: drop_helper_.reset(new DropHelper(root_view)); On 2016/06/02 00:24:51, tapted wrote: > moving this up to the initializer list on line 62 and just having > > drop_helper_(root_view), > > in the initializer list should make the `DropHelper drop_helper_` member work. > (and then git cl-format :) Ahh right, my bad. Thanks for letting me know! https://codereview.chromium.org/1964283002/diff/380001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:117: base::RunLoop run_loop; On 2016/06/02 00:24:51, tapted wrote: > nit: we should comment here, something like > > // Drag and drop is asynchronous on Mac, but we spin a nested run loop for > // consistency with other platforms. Done. https://codereview.chromium.org/1964283002/diff/380001/ui/views/views.gyp File ui/views/views.gyp (right): https://codereview.chromium.org/1964283002/diff/380001/ui/views/views.gyp#new... ui/views/views.gyp:587: 'cocoa/desktop_drag_drop_client_mac_unittest.mm', On 2016/06/02 00:24:51, tapted wrote: > (remove `desktop_`) Done.
I think drag_drop_client_mac_unittest.mm fell off the CL :) (git add?)
On 2016/06/02 23:18:52, tapted wrote: > I think drag_drop_client_mac_unittest.mm fell off the CL :) (git add?) Aw snap. Just added it, sorry about that!
awesome lgtm
spqchan@chromium.org changed reviewers: + sky@chromium.org
Awesome, thanks! +sky for OWNERS
LGTM https://codereview.chromium.org/1964283002/diff/440001/base/mac/sdk_forward_d... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/1964283002/diff/440001/base/mac/sdk_forward_d... base/mac/sdk_forward_declarations.h:251: typedef NSUInteger NSSpringLoadingHighlight; using? https://codereview.chromium.org/1964283002/diff/440001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_mac.h (right): https://codereview.chromium.org/1964283002/diff/440001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.h:62: NSPasteboard*); nit: name argument.
https://codereview.chromium.org/1964283002/diff/440001/base/mac/sdk_forward_d... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/1964283002/diff/440001/base/mac/sdk_forward_d... base/mac/sdk_forward_declarations.h:251: typedef NSUInteger NSSpringLoadingHighlight; On 2016/06/03 15:18:37, sky wrote: > using? I don't think so, everything else here is using typedef https://codereview.chromium.org/1964283002/diff/440001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_mac.h (right): https://codereview.chromium.org/1964283002/diff/440001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.h:62: NSPasteboard*); On 2016/06/03 15:18:37, sky wrote: > nit: name argument. Done.
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1964283002/#ps440002 (title: "fix for sky")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1964283002/440002
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/1964283002/diff/440002/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/1964283002/diff/440002/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.mm:202: return base::WrapUnique(new OSExchangeData(provider)); Nit: just use base::MakeUnique<OSExchangeData>(provider); https://codereview.chromium.org/1964283002/diff/440002/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.mm:209: NSURLPboardType, NSPasteboardTypeString, NSFilenamesPboardType This list is missing a few types that need to be added; otherwise, we're going to lose some functionality when we switch over to Aura. See https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
(I'm going to uncheck the CQ for now. If you feel my comments are in error, let me know and feel free to recheck)
The CQ bit was unchecked by dcheng@chromium.org
dcheng, PTAL https://codereview.chromium.org/1964283002/diff/440002/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/1964283002/diff/440002/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.mm:202: return base::WrapUnique(new OSExchangeData(provider)); On 2016/06/03 17:41:07, dcheng wrote: > Nit: just use base::MakeUnique<OSExchangeData>(provider); Done. https://codereview.chromium.org/1964283002/diff/440002/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data_provider_mac.mm:209: NSURLPboardType, NSPasteboardTypeString, NSFilenamesPboardType On 2016/06/03 17:41:07, dcheng wrote: > This list is missing a few types that need to be added; otherwise, we're going > to lose some functionality when we switch over to Aura. See > https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... Done.
Description was changed from ========== MacViews: Implemented Drag & Drop Created a Drag & Drop client on Mac that bridges between the Views and native OSX's drag and drop. This approach is based on the Aura approach. BUG=599585 TEST= Run the view unit test: CocoaDragDropTest ========== to ========== MacViews: Implemented Drag & Drop Created a Drag & Drop client on Mac that bridges between the Views and native OSX's drag and drop. This approach is based on the Aura approach. BUG=599585 TEST= Run the view unit test: DragDropClientMacTest ==========
lgtm
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1964283002/#ps470001 (title: "fix for dcheng")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1964283002/470001
Message was sent while issue was closed.
Description was changed from ========== MacViews: Implemented Drag & Drop Created a Drag & Drop client on Mac that bridges between the Views and native OSX's drag and drop. This approach is based on the Aura approach. BUG=599585 TEST= Run the view unit test: DragDropClientMacTest ========== to ========== MacViews: Implemented Drag & Drop Created a Drag & Drop client on Mac that bridges between the Views and native OSX's drag and drop. This approach is based on the Aura approach. BUG=599585 TEST= Run the view unit test: DragDropClientMacTest ==========
Message was sent while issue was closed.
Committed patchset #25 (id:470001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Implemented Drag & Drop Created a Drag & Drop client on Mac that bridges between the Views and native OSX's drag and drop. This approach is based on the Aura approach. BUG=599585 TEST= Run the view unit test: DragDropClientMacTest ========== to ========== MacViews: Implemented Drag & Drop Created a Drag & Drop client on Mac that bridges between the Views and native OSX's drag and drop. This approach is based on the Aura approach. BUG=599585 TEST= Run the view unit test: DragDropClientMacTest Committed: https://crrev.com/cf5eeb5c035be5a986fa1a6488f2189e7aebe198 Cr-Commit-Position: refs/heads/master@{#397801} ==========
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/cf5eeb5c035be5a986fa1a6488f2189e7aebe198 Cr-Commit-Position: refs/heads/master@{#397801}
Message was sent while issue was closed.
shess@chromium.org changed reviewers: + shess@chromium.org
Message was sent while issue was closed.
Of course, I could be wrong! But this is the change in the window that has OSX-specific code, and AFAICT that line will require a static initializer. https://codereview.chromium.org/1964283002/diff/470001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.mm (right): https://codereview.chromium.org/1964283002/diff/470001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:24: std::unique_ptr<ui::OSExchangeData> data_; Dollars to donuts this is responsible for the waterfall failing OSX "sizes" step starting in: https://build.chromium.org/p/chromium/builders/Mac/builds/16375
Message was sent while issue was closed.
https://codereview.chromium.org/1964283002/diff/470001/ui/views/cocoa/drag_dr... File ui/views/cocoa/drag_drop_client_mac.mm (right): https://codereview.chromium.org/1964283002/diff/470001/ui/views/cocoa/drag_dr... ui/views/cocoa/drag_drop_client_mac.mm:24: std::unique_ptr<ui::OSExchangeData> data_; On 2016/06/05 20:11:51, Scott Hess wrote: > Dollars to donuts this is responsible for the waterfall failing OSX "sizes" step > starting in: > https://build.chromium.org/p/chromium/builders/Mac/builds/16375 Ah doh - it just needs curlies around it. I'll submit a fix.
Message was sent while issue was closed.
On 2016/06/05 23:37:35, tapted wrote: > https://codereview.chromium.org/1964283002/diff/470001/ui/views/cocoa/drag_dr... > File ui/views/cocoa/drag_drop_client_mac.mm (right): > > https://codereview.chromium.org/1964283002/diff/470001/ui/views/cocoa/drag_dr... > ui/views/cocoa/drag_drop_client_mac.mm:24: std::unique_ptr<ui::OSExchangeData> > data_; > On 2016/06/05 20:11:51, Scott Hess wrote: > > Dollars to donuts this is responsible for the waterfall failing OSX "sizes" > step > > starting in: > > https://build.chromium.org/p/chromium/builders/Mac/builds/16375 > > Ah doh - it just needs curlies around it. I'll submit a fix. Cleared up in https://build.chromium.org/p/chromium/builders/Mac/builds/16418 (cl -> http://crrev.com/2039723002 ).
Message was sent while issue was closed.
On 2016/06/06 01:52:37, tapted wrote: > On 2016/06/05 23:37:35, tapted wrote: > > > https://codereview.chromium.org/1964283002/diff/470001/ui/views/cocoa/drag_dr... > > File ui/views/cocoa/drag_drop_client_mac.mm (right): > > > > > https://codereview.chromium.org/1964283002/diff/470001/ui/views/cocoa/drag_dr... > > ui/views/cocoa/drag_drop_client_mac.mm:24: std::unique_ptr<ui::OSExchangeData> > > data_; > > On 2016/06/05 20:11:51, Scott Hess wrote: > > > Dollars to donuts this is responsible for the waterfall failing OSX "sizes" > > step > > > starting in: > > > https://build.chromium.org/p/chromium/builders/Mac/builds/16375 > > > > Ah doh - it just needs curlies around it. I'll submit a fix. > > Cleared up in https://build.chromium.org/p/chromium/builders/Mac/builds/16418 > (cl -> http://crrev.com/2039723002 ). Thanks! |
