Clean up the mess that is our Clipboard/Pasteboard code |
|||||||||||||||||
Issue descriptionOur clipboard/pasteboard code is a mess. Cross-platform issue: - We have both /ui/base/clipboard and /ui/base/dragdrop. At least on the Mac there is a ton of overlap in that code, as both are based on the same Pasteboard API. Mac issues: - Bug 861775: We use ancient non-UTI type strings in pasteboard API calls all over the place. This was probably triggered by a move to the new SDK. - Bug 155588: We should update to the "new" NSPasteboard APIs which might solve some of our type string issues. ("New" as in 10.6.) - Bug 596128: An attempt to move to the new APIs, which added even *more* pasteboard code and didn't remove any. - NSPasteboard+Utils.mm is full of ancient code. It used to use ancient APIs until fixed with bug 599264 . - Mac WebContents code uses non-UTI strings everywhere. See content/browser/web_contents/web_contents_view_mac.mm's -registerDragTypes. - Bug 754054: ClipboardMac::WriteBitmap() does a TIFF conversion when it could just ask the NSImage to write itself to the clipboard. - Bug 856987: We should center the drag image when dragging URLs from the omnibox. What needs to be done here is a revamp of all the Mac code here. 1. To whatever extent /ui/base/clipboard and /ui/base/dragdrop can be merged, they should be merged. If they cannot, one should rely on the other so that the Mac code within them can be shared and not duplicated. 2. We need to move to the new APIs and remove all the old use of the APIs. 3. We should remove NSPasteboard+Utils.mm. The one thing it does that no other code does is parse .url and .webloc files. We should re-implement that. (The new NSPasteboard APIs do not handle that special-case.) 4. We should rewrite all the WebContents code to call into the ui:: code rather than doing it itself. List of specific points: - -[NSView dragImage:at:offset:event:pasteboard:source:slideBack:] is obsoleted as of 10.7. It should be replaced with -[NSView beginDraggingSessionWithItems:event:source:]. - components/bookmarks/browser/bookmark_pasteboard_helper_mac.mm has an interesting collection of pasteboard code that should be evaluated. - ui/base/dragdrop/cocoa_dnd_util.h should be merged with ui/base/clipboard. ⛆ |
|
|
,
Jul 25
Note that os_exchange_data_unittest tests the code by round-trip converting. This is not an adequate test that shows if Chrome can actually handle drags coming from the outside world.
,
Jul 26
It makes sense to wait on this until the Cocoa code is removed, to ease the job.
,
Jul 26
,
Aug 1
,
Aug 7
Re: OP [1] (Merging //ui/base/clipboard and //ui/base/dragdrop) The case for Mac seems stronger than it is for Windows. Windows has a different set of APIs to handle both the clipboard and drag-drop mechanisms. On Windows, Win32 calls drives the clipboard and COM drives the drag-drop. So on handling the action of clipboard or drag-drop, they should be separated. However, on the data consuming side (os exchange data provider), there is a strong argument towards reorganizing some of the Windows Clipboard utilities (//ui/base/clipboard/clipboard_util_win.[h|cc]) to the drag-drop mechanism as most of those utilities are utilized by drag-drop and not the clipboard. Short answer: clipboard and dragdrop still have their individual purposes in life, but code can be reorganized into where it is actually used. No objection to having dragdrop depend on clipboard. We already have such a dependency today.
,
Aug 13
,
Aug 27
,
Aug 27
,
Aug 27
,
Aug 28
[A comment in https://chromium-review.googlesource.com/c/chromium/src/+/1191742/4/chrome/browser/download/drag_download_item_mac.mm refers to this attachment. It's not directly relevant to the bug.]
,
Aug 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/92e7ac5dbd9dc79bae6b1be7770cccb0490e823b commit 92e7ac5dbd9dc79bae6b1be7770cccb0490e823b Author: Avi Drissman <avi@chromium.org> Date: Tue Aug 28 22:53:21 2018 Upgrade drag code in the download code. BUG=867553 Change-Id: I5fc5e400c1e4d9010c09eb7f035f2ecb92010a7c Reviewed-on: https://chromium-review.googlesource.com/1191742 Commit-Queue: Avi Drissman <avi@chromium.org> Reviewed-by: Sidney San Martín <sdy@chromium.org> Cr-Commit-Position: refs/heads/master@{#586903} [modify] https://crrev.com/92e7ac5dbd9dc79bae6b1be7770cccb0490e823b/chrome/browser/BUILD.gn [rename] https://crrev.com/92e7ac5dbd9dc79bae6b1be7770cccb0490e823b/chrome/browser/download/drag_download_item_aura.cc [add] https://crrev.com/92e7ac5dbd9dc79bae6b1be7770cccb0490e823b/chrome/browser/download/drag_download_item_mac.mm [modify] https://crrev.com/92e7ac5dbd9dc79bae6b1be7770cccb0490e823b/chrome/browser/ui/BUILD.gn [delete] https://crrev.com/07a159d001c0535ea940e278cca255608ad64ff5/chrome/browser/ui/cocoa/download/download_item_drag_mac.mm [delete] https://crrev.com/07a159d001c0535ea940e278cca255608ad64ff5/chrome/browser/ui/cocoa/download/download_util_mac.h [delete] https://crrev.com/07a159d001c0535ea940e278cca255608ad64ff5/chrome/browser/ui/cocoa/download/download_util_mac.mm [delete] https://crrev.com/07a159d001c0535ea940e278cca255608ad64ff5/chrome/browser/ui/cocoa/download/download_util_mac_unittest.mm [modify] https://crrev.com/92e7ac5dbd9dc79bae6b1be7770cccb0490e823b/chrome/test/BUILD.gn
,
Aug 31
,
Sep 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a0820227901f7cea7c37f95d2b684e7befa9e14b commit a0820227901f7cea7c37f95d2b684e7befa9e14b Author: Avi Drissman <avi@chromium.org> Date: Sat Sep 01 18:02:28 2018 Upgrade services callbacks to use new pasteboard APIs. BUG=867553 Change-Id: I96804621e5ed33b991eb9870480f142e87e50050 Reviewed-on: https://chromium-review.googlesource.com/1199816 Commit-Queue: Avi Drissman <avi@chromium.org> Reviewed-by: Sidney San Martín <sdy@chromium.org> Cr-Commit-Position: refs/heads/master@{#588288} [modify] https://crrev.com/a0820227901f7cea7c37f95d2b684e7befa9e14b/chrome/browser/app_controller_mac.mm [modify] https://crrev.com/a0820227901f7cea7c37f95d2b684e7befa9e14b/content/browser/renderer_host/render_widget_host_view_cocoa.mm [modify] https://crrev.com/a0820227901f7cea7c37f95d2b684e7befa9e14b/ui/views/cocoa/bridged_content_view.h [modify] https://crrev.com/a0820227901f7cea7c37f95d2b684e7befa9e14b/ui/views/cocoa/bridged_content_view.mm
,
Sep 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a93ecd9cdfd49020aaa857b2af90e00498cbb82e commit a93ecd9cdfd49020aaa857b2af90e00498cbb82e Author: Avi Drissman <avi@chromium.org> Date: Sat Sep 01 21:53:26 2018 Upgrade remoting clipboard code to use new pasteboard APIs. BUG=867553 Change-Id: Id49631322c7c5ecbf05a26760989d53cfa82a346 Reviewed-on: https://chromium-review.googlesource.com/1199574 Commit-Queue: Sidney San Martín <sdy@chromium.org> Reviewed-by: Jamie Walch <jamiewalch@chromium.org> Reviewed-by: Sidney San Martín <sdy@chromium.org> Cr-Commit-Position: refs/heads/master@{#588295} [modify] https://crrev.com/a93ecd9cdfd49020aaa857b2af90e00498cbb82e/remoting/host/clipboard_mac.mm
,
Sep 4
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a3148766f9f2ed9e7e7f8c6fd4ca68a632991993 commit a3148766f9f2ed9e7e7f8c6fd4ca68a632991993 Author: Avi Drissman <avi@chromium.org> Date: Tue Sep 04 19:50:15 2018 Upgrade PDF dictionary lookup to use new pasteboard APIs. BUG=867553 Change-Id: I428acce1909a19cf521c249866d3bb7d0c67cf0c Reviewed-on: https://chromium-review.googlesource.com/1201403 Commit-Queue: Sidney San Martín <sdy@chromium.org> Reviewed-by: Sidney San Martín <sdy@chromium.org> Cr-Commit-Position: refs/heads/master@{#588622} [modify] https://crrev.com/a3148766f9f2ed9e7e7f8c6fd4ca68a632991993/content/browser/renderer_host/render_widget_host_view_mac.mm
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d0537e78f038940693781b36390db3bcffc51b3c commit d0537e78f038940693781b36390db3bcffc51b3c Author: Avi Drissman <avi@chromium.org> Date: Tue Sep 04 21:40:14 2018 Give 'urln' a utility constant. Also, "public.url" is defined in the SDK; use it. BUG=867553 Change-Id: I431b94520997c3a39ca1e1e921239ed9ed07c8c1 Reviewed-on: https://chromium-review.googlesource.com/1204932 Commit-Queue: Sidney San Martín <sdy@chromium.org> Reviewed-by: Sidney San Martín <sdy@chromium.org> Cr-Commit-Position: refs/heads/master@{#588646} [modify] https://crrev.com/d0537e78f038940693781b36390db3bcffc51b3c/content/browser/web_contents/web_drag_source_mac.mm [modify] https://crrev.com/d0537e78f038940693781b36390db3bcffc51b3c/ui/base/clipboard/clipboard_util_mac.h [modify] https://crrev.com/d0537e78f038940693781b36390db3bcffc51b3c/ui/base/clipboard/clipboard_util_mac.mm
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e1a5c7beb5645b7a484d7fe52050acc9fa22e120 commit e1a5c7beb5645b7a484d7fe52050acc9fa22e120 Author: Avi Drissman <avi@chromium.org> Date: Wed Sep 05 00:36:31 2018 Properly name pasteboard types so they reflect what they are. They should not be named "PboardType" if they are UTIs. This also does modern Objective-C cleanup and fixes style issues. BUG=867553 Change-Id: I12da3249339a91282894098fb4c8dc17d9041507 Reviewed-on: https://chromium-review.googlesource.com/1199581 Commit-Queue: Avi Drissman <avi@chromium.org> Commit-Queue: Sidney San Martín <sdy@chromium.org> Reviewed-by: Sidney San Martín <sdy@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#588716} [modify] https://crrev.com/e1a5c7beb5645b7a484d7fe52050acc9fa22e120/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_view.mm [modify] https://crrev.com/e1a5c7beb5645b7a484d7fe52050acc9fa22e120/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.mm [modify] https://crrev.com/e1a5c7beb5645b7a484d7fe52050acc9fa22e120/chrome/browser/ui/cocoa/bookmarks/bookmark_button.h [modify] https://crrev.com/e1a5c7beb5645b7a484d7fe52050acc9fa22e120/components/bookmarks/browser/bookmark_node_data_mac.cc [modify] https://crrev.com/e1a5c7beb5645b7a484d7fe52050acc9fa22e120/components/bookmarks/browser/bookmark_pasteboard_helper_mac.h [modify] https://crrev.com/e1a5c7beb5645b7a484d7fe52050acc9fa22e120/components/bookmarks/browser/bookmark_pasteboard_helper_mac.mm
,
Oct 1
,
Oct 1
,
Oct 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/34fe6ebdd3544516a9d0c5b15f31eede61c91637 commit 34fe6ebdd3544516a9d0c5b15f31eede61c91637 Author: Avi Drissman <avi@chromium.org> Date: Thu Oct 04 20:48:14 2018 Make the Views bookmark code use the correct Mac code. Do assorted other bits of cleanup as well. BUG= 891194 , 867553 Change-Id: I9f74c14c468819d882b38a959035ec8c1c4a2c1a Reviewed-on: https://chromium-review.googlesource.com/c/1258183 Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Sidney San Martín <sdy@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#596840} [modify] https://crrev.com/34fe6ebdd3544516a9d0c5b15f31eede61c91637/components/bookmarks/browser/BUILD.gn [modify] https://crrev.com/34fe6ebdd3544516a9d0c5b15f31eede61c91637/components/bookmarks/browser/bookmark_node_data.cc [modify] https://crrev.com/34fe6ebdd3544516a9d0c5b15f31eede61c91637/components/bookmarks/browser/bookmark_node_data.h [delete] https://crrev.com/97f45221020b6ea5ae2989831a8835d4046c3a62/components/bookmarks/browser/bookmark_node_data_mac.cc [add] https://crrev.com/34fe6ebdd3544516a9d0c5b15f31eede61c91637/components/bookmarks/browser/bookmark_node_data_mac.mm [modify] https://crrev.com/34fe6ebdd3544516a9d0c5b15f31eede61c91637/components/bookmarks/browser/bookmark_node_data_views.cc [modify] https://crrev.com/34fe6ebdd3544516a9d0c5b15f31eede61c91637/components/bookmarks/browser/bookmark_pasteboard_helper_mac.h [modify] https://crrev.com/34fe6ebdd3544516a9d0c5b15f31eede61c91637/components/bookmarks/browser/bookmark_pasteboard_helper_mac.mm [modify] https://crrev.com/34fe6ebdd3544516a9d0c5b15f31eede61c91637/ui/base/clipboard/clipboard_util_mac.h [modify] https://crrev.com/34fe6ebdd3544516a9d0c5b15f31eede61c91637/ui/base/clipboard/clipboard_util_mac.mm [modify] https://crrev.com/34fe6ebdd3544516a9d0c5b15f31eede61c91637/ui/base/dragdrop/os_exchange_data_provider_mac.h [modify] https://crrev.com/34fe6ebdd3544516a9d0c5b15f31eede61c91637/ui/base/dragdrop/os_exchange_data_provider_mac.mm
,
Oct 9
,
Oct 9
,
Nov 20
***UI Mass Triage*** Since work is in progress, adding appropriate labels. |
||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||
Comment 1 by a...@chromium.org
, Jul 25