Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1947153003:
Modify Pasteboard handling in OSExchangeData (Closed)
DescriptionModify Pasteboard handling in OSExchangeData
BUG=599585
Committed: https://crrev.com/6bdd9b835d5f6ef1ef209e301d02af2f0b12485e
Cr-Commit-Position: refs/heads/master@{#391894}
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 1
Patch Set 6 : used firstObject #Messages
Total messages: 19 (10 generated)
Description was changed from ========== Modify Pasteboard Writing in OSXExchangeData of OSX BUG= ========== to ========== Modify Pasteboard handling in OSXExchangeData BUG=599585 ==========
spqchan@chromium.org changed reviewers: + erikchen@chromium.org
Description was changed from ========== Modify Pasteboard handling in OSXExchangeData BUG=599585 ========== to ========== Modify Pasteboard handling in OSExchangeData BUG=599585 ==========
PTAL
https://codereview.chromium.org/1947153003/diff/20001/ui/base/dragdrop/os_exc... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/1947153003/diff/20001/ui/base/dragdrop/os_exc... ui/base/dragdrop/os_exchange_data_provider_mac.mm:75: NSString* item = [pasteboard_->get() stringForType:NSPasteboardTypeString]; Can you confirm that this doesn't break anything? Your new code is "cleaner" than the old code, but also changes the behavior https://codereview.chromium.org/1947153003/diff/20001/ui/base/dragdrop/os_exc... ui/base/dragdrop/os_exchange_data_provider_mac.mm:78: if (!item) { minor nit: Why a negative conditional? All else being held equal, prefer positive conditions. e.g.: """ if (item) { *data = base::SysNSStringToUTF16(item); return true; } // There was no NSString, check for an NSURL. """ https://codereview.chromium.org/1947153003/diff/20001/ui/base/dragdrop/os_exc... ui/base/dragdrop/os_exchange_data_provider_mac.mm:98: return PopulateURLAndTitleFromPasteboard( Is it possible to use methods from ui/base/clipboard/clipboard_util_mac.mm instead? https://codereview.chromium.org/1947153003/diff/20001/ui/base/dragdrop/os_exc... ui/base/dragdrop/os_exchange_data_provider_mac.mm:132: bool OSExchangeDataProviderMac::HasString() const { Hm. I expect HasString() to mirror GetString(). Any possibility we can remove the URL processing part of GetString() and still have everything work?
PTAL https://codereview.chromium.org/1947153003/diff/20001/ui/base/dragdrop/os_exc... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/1947153003/diff/20001/ui/base/dragdrop/os_exc... ui/base/dragdrop/os_exchange_data_provider_mac.mm:75: NSString* item = [pasteboard_->get() stringForType:NSPasteboardTypeString]; On 2016/05/04 18:56:21, erikchen wrote: > Can you confirm that this doesn't break anything? Your new code is "cleaner" > than the old code, but also changes the behavior Sure thing, currently Cocoa doesn't actually use OSExchangeData. The existing implementation was implemented to just pass the unit tests. I made sure that my changes will still pass the OSExchangeDataTest https://codereview.chromium.org/1947153003/diff/20001/ui/base/dragdrop/os_exc... ui/base/dragdrop/os_exchange_data_provider_mac.mm:78: if (!item) { On 2016/05/04 18:56:21, erikchen wrote: > minor nit: Why a negative conditional? All else being held equal, prefer > positive conditions. e.g.: > > > """ > if (item) { > *data = base::SysNSStringToUTF16(item); > return true; > } > > // There was no NSString, check for an NSURL. > """ Done. I was trying to follow the format of the existing code. https://codereview.chromium.org/1947153003/diff/20001/ui/base/dragdrop/os_exc... ui/base/dragdrop/os_exchange_data_provider_mac.mm:98: return PopulateURLAndTitleFromPasteboard( On 2016/05/04 18:56:21, erikchen wrote: > Is it possible to use methods from ui/base/clipboard/clipboard_util_mac.mm > instead? Done https://codereview.chromium.org/1947153003/diff/20001/ui/base/dragdrop/os_exc... ui/base/dragdrop/os_exchange_data_provider_mac.mm:132: bool OSExchangeDataProviderMac::HasString() const { On 2016/05/04 18:56:21, erikchen wrote: > Hm. I expect HasString() to mirror GetString(). Any possibility we can remove > the URL processing part of GetString() and still have everything work? Discussed offline: it needs to be there. Made a change so that HasString() will take the URL processing part in account.
lgtm
spqchan@chromium.org changed reviewers: + avi@chromium.org
+avi for OWNERS
lgtm https://codereview.chromium.org/1947153003/diff/80001/ui/base/dragdrop/os_exc... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/1947153003/diff/80001/ui/base/dragdrop/os_exc... ui/base/dragdrop/os_exchange_data_provider_mac.mm:103: *title = base::SysNSStringToUTF16([titleArray objectAtIndex:0]); Does -firstObject work yet in the SDK that we use?
The CQ bit was checked by spqchan@chromium.org
The CQ bit was unchecked by spqchan@chromium.org
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/1947153003/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1947153003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1947153003/100001
Message was sent while issue was closed.
Description was changed from ========== Modify Pasteboard handling in OSExchangeData BUG=599585 ========== to ========== Modify Pasteboard handling in OSExchangeData BUG=599585 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Modify Pasteboard handling in OSExchangeData BUG=599585 ========== to ========== Modify Pasteboard handling in OSExchangeData BUG=599585 Committed: https://crrev.com/6bdd9b835d5f6ef1ef209e301d02af2f0b12485e Cr-Commit-Position: refs/heads/master@{#391894} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6bdd9b835d5f6ef1ef209e301d02af2f0b12485e Cr-Commit-Position: refs/heads/master@{#391894} |
