Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
368973003:
MacViews: Partially implement OSExchangeDataProviderMac. (Closed)
|
Created:
6 years, 5 months ago by Andre Modified:
6 years, 5 months ago CC:
dcheng, chromium-reviews, mac-views-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionMacViews: Partially implement OSExchangeDataProviderMac.
Partially implement OSExchangeDataProviderMac, just enough to get
OSExchangeDataTest and TextFieldTest.DragAndDrop* to pass.
The implementation is backed by NSPasteboard, and relies on the category
NSPasteboard(ChimeraPasteboardURLUtils) defined in third_party/mozilla.
BUG=378134
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282277
Patch Set 1 #Patch Set 2 : #
Total comments: 21
Patch Set 3 : Fixes for dcheng and tapted. #
Total comments: 7
Patch Set 4 : Fixes for dcheng #
Total comments: 3
Patch Set 5 : Fix for tapted #Patch Set 6 : lastObject -> objectAtIndex #
Messages
Total messages: 23 (0 generated)
Trent, I took a stab at the initial implementation of OSExchangeDataProviderMac. Can you take an initial look?
cool - everything lg at a high level. I don't know much about these interfaces, so I'll poke around a bit today to make sure I understand https://codereview.chromium.org/368973003/diff/20001/ui/ui_unittests.gyp File ui/ui_unittests.gyp (right): https://codereview.chromium.org/368973003/diff/20001/ui/ui_unittests.gyp#newc... ui/ui_unittests.gyp:183: 'xcode_settings': {'OTHER_LDFLAGS': ['-Wl,-ObjC']}, mozilla.gyp has ['component=="shared_library"', { # Needed to link to Obj-C static libraries. 'xcode_settings': {'OTHER_LDFLAGS': ['-Wl,-ObjC']}, } ], is that 'shared_library' condition backward/unnecessary? I encountered this in app_list.gyp, and the condition I had to add there was 'component=="static_library"' -- not shared_ (although it's possible I got the app_ilst one backwards :o) And you might still need it here, but I'd guard it in `['component=="static_library"',` if you can get away with it. My understanding is that it skips a pruning step when linking, so developer link times could be longer with it.
https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:75: *data = base::SysNSStringToUTF16([items lastObject]); Why lastObject? https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:113: return [pasteboard_ containsURLData]; As currently written, won't there be situations where this potentially returns false even when the corresponding call to GetURLAndTitle() would return something (and vise versa)?
https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:71: options:nil]; should options be @[ ]? -- i.e. an empty array rather than nil - a lot of Apple example code does this. More below https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:72: if (!items || [items count] < 1) optional nit: I'd probably go with `if ([items count] == 0)` - although being explicit can be good too. (note: I read the documentation as returning `nil` or an array with at least one item) https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:77: } clipboard_util_win.cc seems to fallback to URL as well -- will we need to that here? https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:101: NSData* ns_data = [pasteboard_ dataForType:format.ToNSString()]; dataForType can return nil in some cases (e.g. timeout) - Should we DCHECK? Or return false for those cases?
https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:71: options:nil]; On 2014/07/03 07:01:50, tapted wrote: > should options be @[ ]? -- i.e. an empty array rather than nil - a lot of Apple > example code does this. More below Done. Passing nil as options is pretty common in Cocoa, but the documentation does not specify that nil is allowed, so @{ } is a little safer. https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:72: if (!items || [items count] < 1) On 2014/07/03 07:01:50, tapted wrote: > optional nit: I'd probably go with `if ([items count] == 0)` - although being > explicit can be good too. (note: I read the documentation as returning `nil` or > an array with at least one item) Done. https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:75: *data = base::SysNSStringToUTF16([items lastObject]); On 2014/07/03 05:44:16, dcheng (OOO) wrote: > Why lastObject? items is a 1 element array at this point, so lastObject gets us the object. https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:77: } On 2014/07/03 07:01:50, tapted wrote: > clipboard_util_win.cc seems to fallback to URL as well -- will we need to that > here? I'm not sure, OSExchangeDataProviderAura does not seem to fallback. I think we should not do it unless we later find that we need to. https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:101: NSData* ns_data = [pasteboard_ dataForType:format.ToNSString()]; On 2014/07/03 07:01:50, tapted wrote: > dataForType can return nil in some cases (e.g. timeout) - Should we DCHECK? Or > return false for those cases? Done. https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:113: return [pasteboard_ containsURLData]; On 2014/07/03 05:44:16, dcheng (OOO) wrote: > As currently written, won't there be situations where this potentially returns > false even when the corresponding call to GetURLAndTitle() would return > something (and vise versa)? Possibly, are you referring to the CONVERT_FILENAMES policy in GetURLAndTitle? I'm not sure what to do about it though, any suggestions? https://codereview.chromium.org/368973003/diff/20001/ui/ui_unittests.gyp File ui/ui_unittests.gyp (right): https://codereview.chromium.org/368973003/diff/20001/ui/ui_unittests.gyp#newc... ui/ui_unittests.gyp:183: 'xcode_settings': {'OTHER_LDFLAGS': ['-Wl,-ObjC']}, On 2014/07/02 23:36:32, tapted wrote: > mozilla.gyp has > > ['component=="shared_library"', > { > # Needed to link to Obj-C static libraries. > 'xcode_settings': {'OTHER_LDFLAGS': ['-Wl,-ObjC']}, > } > ], > > is that 'shared_library' condition backward/unnecessary? > > I encountered this in app_list.gyp, and the condition I had to add there was > 'component=="static_library"' -- not shared_ (although it's possible I got the > app_ilst one backwards :o) > > > And you might still need it here, but I'd guard it in > `['component=="static_library"',` if you can get away with it. My understanding > is that it skips a pruning step when linking, so developer link times could be > longer with it. Done.
sky, can you review as owner?
https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:75: *data = base::SysNSStringToUTF16([items lastObject]); On 2014/07/07 18:29:05, Andre wrote: > On 2014/07/03 05:44:16, dcheng (OOO) wrote: > > Why lastObject? > > items is a 1 element array at this point, so lastObject gets us the object. I guess as a reader, I would usually expect to see firstObject here (since in general, coming earlier in the array means you prefer that data type more strongly). Obviously it doesn't matter here but it feels a bit more idiomatic. https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:113: return [pasteboard_ containsURLData]; On 2014/07/07 18:29:05, Andre wrote: > On 2014/07/03 05:44:16, dcheng (OOO) wrote: > > As currently written, won't there be situations where this potentially returns > > false even when the corresponding call to GetURLAndTitle() would return > > something (and vise versa)? > > Possibly, are you referring to the CONVERT_FILENAMES policy in GetURLAndTitle? > I'm not sure what to do about it though, any suggestions? Yes. The simplest solution is to forward the arguments to GetURLAndTitle() and return true iff the returned URL is non empty. https://codereview.chromium.org/368973003/diff/40001/ui/base/dragdrop/os_exch... File ui/base/dragdrop/os_exchange_data_unittest.cc (right): https://codereview.chromium.org/368973003/diff/40001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_unittest.cc:26: TEST_F(OSExchangeDataTest, StringDataGetAndSet) { Since you're updating this anyway, is it possible to add a test to verify that SetFile() followed by GetURLAndTitle(DO_NOT_CONVERT_FILENAMES) does not return a file:/// URL, but GetURLAndTitle(CONVERT_FILENAMES) does? https://codereview.chromium.org/368973003/diff/40001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_unittest.cc:52: EXPECT_FALSE(data.HasURL(OSExchangeData::CONVERT_FILENAMES)); Can we explicitly use DO_NOT_CONVERT_FILENAMES here and below? https://codereview.chromium.org/368973003/diff/40001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_unittest.cc:61: EXPECT_TRUE(data2.HasURL(OSExchangeData::CONVERT_FILENAMES)); Ditto.
https://codereview.chromium.org/368973003/diff/40001/ui/base/dragdrop/os_exch... File ui/base/dragdrop/os_exchange_data_unittest.cc (right): https://codereview.chromium.org/368973003/diff/40001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_unittest.cc:26: TEST_F(OSExchangeDataTest, StringDataGetAndSet) { On 2014/07/07 18:42:19, dcheng (OOO) wrote: > Since you're updating this anyway, is it possible to add a test to verify that > SetFile() followed by GetURLAndTitle(DO_NOT_CONVERT_FILENAMES) does not return a > file:/// URL, but GetURLAndTitle(CONVERT_FILENAMES) does? Actually, I'll just add this test separately since your patch doesn't implement file-related functionality yet. Do I need to worry about breaking tests on MacViews at this point?
https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:75: *data = base::SysNSStringToUTF16([items lastObject]); On 2014/07/07 18:42:18, dcheng (OOO) wrote: > On 2014/07/07 18:29:05, Andre wrote: > > On 2014/07/03 05:44:16, dcheng (OOO) wrote: > > > Why lastObject? > > > > items is a 1 element array at this point, so lastObject gets us the object. > > I guess as a reader, I would usually expect to see firstObject here (since in > general, coming earlier in the array means you prefer that data type more > strongly). Obviously it doesn't matter here but it feels a bit more idiomatic. I agree, however firstObject is a relatively late addition to NSArray, and is not present in the header in 10.6sdk, even though the docs say that it is available on 10.6 or later. https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:113: return [pasteboard_ containsURLData]; On 2014/07/07 18:42:18, dcheng (OOO) wrote: > On 2014/07/07 18:29:05, Andre wrote: > > On 2014/07/03 05:44:16, dcheng (OOO) wrote: > > > As currently written, won't there be situations where this potentially > returns > > > false even when the corresponding call to GetURLAndTitle() would return > > > something (and vise versa)? > > > > Possibly, are you referring to the CONVERT_FILENAMES policy in GetURLAndTitle? > > I'm not sure what to do about it though, any suggestions? > > Yes. The simplest solution is to forward the arguments to GetURLAndTitle() and > return true iff the returned URL is non empty. Done. https://codereview.chromium.org/368973003/diff/40001/ui/base/dragdrop/os_exch... File ui/base/dragdrop/os_exchange_data_unittest.cc (right): https://codereview.chromium.org/368973003/diff/40001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_unittest.cc:26: TEST_F(OSExchangeDataTest, StringDataGetAndSet) { On 2014/07/07 18:43:03, dcheng (OOO) wrote: > On 2014/07/07 18:42:19, dcheng (OOO) wrote: > > Since you're updating this anyway, is it possible to add a test to verify that > > SetFile() followed by GetURLAndTitle(DO_NOT_CONVERT_FILENAMES) does not return > a > > file:/// URL, but GetURLAndTitle(CONVERT_FILENAMES) does? > > Actually, I'll just add this test separately since your patch doesn't implement > file-related functionality yet. Do I need to worry about breaking tests on > MacViews at this point? I think you can just add the test that fails on the Mac for now, and I can add the Mac implementation to pass it. Or you can disable them for Mac, either way. https://codereview.chromium.org/368973003/diff/40001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_unittest.cc:52: EXPECT_FALSE(data.HasURL(OSExchangeData::CONVERT_FILENAMES)); On 2014/07/07 18:42:19, dcheng (OOO) wrote: > Can we explicitly use DO_NOT_CONVERT_FILENAMES here and below? Done. https://codereview.chromium.org/368973003/diff/40001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_unittest.cc:61: EXPECT_TRUE(data2.HasURL(OSExchangeData::CONVERT_FILENAMES)); On 2014/07/07 18:42:19, dcheng (OOO) wrote: > Ditto. Done.
lgtm https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:77: } On 2014/07/07 18:29:05, Andre wrote: > On 2014/07/03 07:01:50, tapted wrote: > > clipboard_util_win.cc seems to fallback to URL as well -- will we need to that > > here? > > I'm not sure, OSExchangeDataProviderAura does not seem to fallback. > I think we should not do it unless we later find that we need to. > sg https://codereview.chromium.org/368973003/diff/60001/ui/base/dragdrop/os_exch... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/368973003/diff/60001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:104: return false; nit: linebreak after conditional ( http://www.chromium.org/developers/coding-style#Code_Formatting )
(slgtm) https://codereview.chromium.org/368973003/diff/60001/ui/base/dragdrop/os_exch... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/368973003/diff/60001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:104: return false; On 2014/07/07 23:39:13, tapted wrote: > nit: linebreak after conditional ( > http://www.chromium.org/developers/coding-style#Code_Formatting ) (actually that link is probably just to say ~"don't put the return on the same line" but a blank line after an early return is usually a nice rule to follow too and consistent with GetString above).
https://codereview.chromium.org/368973003/diff/60001/ui/base/dragdrop/os_exch... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/368973003/diff/60001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:104: return false; On 2014/07/07 23:42:11, tapted wrote: > On 2014/07/07 23:39:13, tapted wrote: > > nit: linebreak after conditional ( > > http://www.chromium.org/developers/coding-style#Code_Formatting ) > > (actually that link is probably just to say ~"don't put the return on the same > line" but a blank line after an early return is usually a nice rule to follow > too and consistent with GetString above). Done.
lgtm https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:75: *data = base::SysNSStringToUTF16([items lastObject]); On 2014/07/07 20:10:51, Andre wrote: > On 2014/07/07 18:42:18, dcheng (OOO) wrote: > > On 2014/07/07 18:29:05, Andre wrote: > > > On 2014/07/03 05:44:16, dcheng (OOO) wrote: > > > > Why lastObject? > > > > > > items is a 1 element array at this point, so lastObject gets us the object. > > > > I guess as a reader, I would usually expect to see firstObject here (since in > > general, coming earlier in the array means you prefer that data type more > > strongly). Obviously it doesn't matter here but it feels a bit more idiomatic. > > I agree, however firstObject is a relatively late addition to NSArray, and is > not > present in the header in 10.6sdk, even though the docs say that it is available > on 10.6 or later. I have a very very slight preference for objectAtIndex:0 here. If we go through and do a blanket cleanup to convert these references to firstObject in the future, then we can catch this.
LGTM
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/368973003/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by andresantoso@chromium.org
https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... File ui/base/dragdrop/os_exchange_data_provider_mac.mm (right): https://codereview.chromium.org/368973003/diff/20001/ui/base/dragdrop/os_exch... ui/base/dragdrop/os_exchange_data_provider_mac.mm:75: *data = base::SysNSStringToUTF16([items lastObject]); On 2014/07/09 00:26:35, dcheng (OOO) wrote: > On 2014/07/07 20:10:51, Andre wrote: > > On 2014/07/07 18:42:18, dcheng (OOO) wrote: > > > On 2014/07/07 18:29:05, Andre wrote: > > > > On 2014/07/03 05:44:16, dcheng (OOO) wrote: > > > > > Why lastObject? > > > > > > > > items is a 1 element array at this point, so lastObject gets us the > object. > > > > > > I guess as a reader, I would usually expect to see firstObject here (since > in > > > general, coming earlier in the array means you prefer that data type more > > > strongly). Obviously it doesn't matter here but it feels a bit more > idiomatic. > > > > I agree, however firstObject is a relatively late addition to NSArray, and is > > not > > present in the header in 10.6sdk, even though the docs say that it is > available > > on 10.6 or later. > > I have a very very slight preference for objectAtIndex:0 here. If we go through > and do a blanket cleanup to convert these references to firstObject in the > future, then we can catch this. Done.
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/368973003/10...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 282277 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
