Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2782823003:
Rewrite implementation of ClipboardRecentContent in Objective C. (Closed)
|
Created:
3 years, 8 months ago by lody Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews_chromium.org, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, dcheng, mac-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRewrite implementation of ClipboardRecentContent in Objective C.
In doing so, remove use of gurl which causes a dependency on ICU. As
ClipboardRecentContent is used in extensions it is preferable to have
as small a binary as possible and ICU is a large library. An additional
benefits of having an Obj-C implementation with a C++ wrapper is that
the Obj-C implementation can be directly used without introducing C++
in the extension.
BUG=671166, 622743
Review-Url: https://codereview.chromium.org/2782823003
Cr-Commit-Position: refs/heads/master@{#463582}
Committed: https://chromium.googlesource.com/chromium/src/+/dd288990130665b1a0e4b5d3de33f68fdb9df9fd
Patch Set 1 #
Total comments: 23
Patch Set 2 : address comments #
Total comments: 62
Patch Set 3 : sdefresne's comments #
Total comments: 31
Patch Set 4 : marq comments #Patch Set 5 : rebase #
Total comments: 6
Patch Set 6 : nits #
Total comments: 4
Patch Set 7 : comment #Patch Set 8 : build.gn fix #
Dependent Patchsets: Messages
Total messages: 43 (17 generated)
lod@chromium.org changed reviewers: + jif@chromium.org
Please review, thank you.
thanks! https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/BUILD.gn (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/BUILD.gn:20: ":open_from_clipboard_ios", s/open_from_clipboard_ios/open_from_clipboard_ios_impl/ https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/clipboard_recent_content_ios.h (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios.h:53: base::scoped_nsobject<ClipboardRecentContentIOSImpl> implementation; s/implementation/implementation_/ https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/clipboard_recent_content_ios.mm (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios.mm:30: @interface ClipboardRecentContentMetricsDelegateBridge s/ClipboardRecentContentMetricsDelegateBridge/ClipboardRecentContentMetricsDelegateImpl/ https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios.mm:54: initWithDelegate:[[ClipboardRecentContentMetricsDelegateBridge alloc] nit: can you use an intermediate variable for the ClipboardRecentContentMetricsDelegateBridge instance? https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios.mm:64: *url = net::GURLWithNSURL(nsurl); lets keep using GURL's |is_valid()| (just want to make sure we don't change the logic). so: GURL tempUrl = net::GURLWithNSURL(nsurl); if (tempUrl.is_valid()) { *url = std::move(tempUrl); return true; } https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/clipboard_recent_content_ios_impl.h (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:39: // Methods below are exposed for testing purposes. I don't they they are used by tests. https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:46: // Returns the uptime. Override in tests to return custom value. Not overridden by tests. And are unittests passing? https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/clipboard_recent_content_ios_impl.mm (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:111: [[NSNotificationCenter defaultCenter] removeObserver:self]; [super dealloc]; (if the compiler does not complain about that, then there's a problem somewhere). https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm:84: void SetStoredPasteboardChangeDate(NSDate* changeDate) { original author messed up 🙄 s/changeData/change_date/ also, keep the call to |copy|.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/BUILD.gn (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/BUILD.gn:20: ":open_from_clipboard_ios", On 2017/03/29 14:17:55, jif wrote: > s/open_from_clipboard_ios/open_from_clipboard_ios_impl/ Done. https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/clipboard_recent_content_ios.h (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios.h:53: base::scoped_nsobject<ClipboardRecentContentIOSImpl> implementation; On 2017/03/29 14:17:55, jif wrote: > s/implementation/implementation_/ Done. https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/clipboard_recent_content_ios.mm (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios.mm:30: @interface ClipboardRecentContentMetricsDelegateBridge On 2017/03/29 14:17:55, jif wrote: > s/ClipboardRecentContentMetricsDelegateBridge/ClipboardRecentContentMetricsDelegateImpl/ Done. https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios.mm:54: initWithDelegate:[[ClipboardRecentContentMetricsDelegateBridge alloc] On 2017/03/29 14:17:55, jif wrote: > nit: can you use an intermediate variable for the > ClipboardRecentContentMetricsDelegateBridge instance? Done. https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios.mm:64: *url = net::GURLWithNSURL(nsurl); On 2017/03/29 14:17:55, jif wrote: > lets keep using GURL's |is_valid()| (just want to make sure we don't change the > logic). > so: > GURL tempUrl = net::GURLWithNSURL(nsurl); > if (tempUrl.is_valid()) { > *url = std::move(tempUrl); > return true; > } Done. https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/clipboard_recent_content_ios_impl.h (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:39: // Methods below are exposed for testing purposes. On 2017/03/29 14:17:56, jif wrote: > I don't they they are used by tests. Line 83 in clipboard_recent_content_ios_unittest: void SetStoredPasteboardChangeDate(NSDate* changeDate) { clipboard_content_->SetLastPasteboardChangeDate(changeDate); clipboard_content_->SaveToUserDefaults(); } https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:46: // Returns the uptime. Override in tests to return custom value. On 2017/03/29 14:17:55, jif wrote: > Not overridden by tests. And are unittests passing? You were right. Changed so uptime is actually overriden. They are (and were) passing. https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/clipboard_recent_content_ios_impl.mm (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:111: [[NSNotificationCenter defaultCenter] removeObserver:self]; On 2017/03/29 14:17:56, jif wrote: > [super dealloc]; > (if the compiler does not complain about that, then there's a problem > somewhere). This file is using ARC https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm:84: void SetStoredPasteboardChangeDate(NSDate* changeDate) { On 2017/03/29 14:17:56, jif wrote: > original author messed up 🙄 > s/changeData/change_date/ > > also, keep the call to |copy|. Done.
ping?
forgot to send comments! Thanks for doing the thing with the timestamp. https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/clipboard_recent_content_ios_impl.h (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:39: // Methods below are exposed for testing purposes. On 2017/03/30 13:36:34, lody wrote: > On 2017/03/29 14:17:56, jif wrote: > > I don't they they are used by tests. > > Line 83 in clipboard_recent_content_ios_unittest: > > void SetStoredPasteboardChangeDate(NSDate* changeDate) { > clipboard_content_->SetLastPasteboardChangeDate(changeDate); > clipboard_content_->SaveToUserDefaults(); > } What I'm trying to say is that those 2 obj-c methods would need to be publicly exposed even if there were no tests. In other words, you can remove the comment. https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:46: // Returns the uptime. Override in tests to return custom value. On 2017/03/30 13:36:34, lody wrote: > On 2017/03/29 14:17:55, jif wrote: > > Not overridden by tests. And are unittests passing? > > You were right. Changed so uptime is actually overriden. They are (and were) > passing. Acknowledged. https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/clipboard_recent_content_ios_impl.mm (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:111: [[NSNotificationCenter defaultCenter] removeObserver:self]; On 2017/03/30 13:36:34, lody wrote: > On 2017/03/29 14:17:56, jif wrote: > > [super dealloc]; > > (if the compiler does not complain about that, then there's a problem > > somewhere). > > This file is using ARC Ack. The problem was in my knowledge of ARC. https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm:162: // Check that the even if the device is restarted, pasteboard content is thanks!
besides the previous nits, lgtm
Description was changed from ========== Rewrite implementation of clipboardrecentcontents in Objective C. In doing so, remove use of gurl which causes a dependency on ICU. As clipboardrecentcontents is used in extensions it is preferable to have as small a binary as possible and ICU is a large library. An additional benefits of having an Obj-C implementation with a C++ wrapper is that the Obj-C implementation can be directly used without introducing C++ in the extension. BUG=671166, 622743 ========== to ========== Rewrite implementation of ClipboardRecentContent in Objective C. In doing so, remove use of gurl which causes a dependency on ICU. As ClipboardRecentContent is used in extensions it is preferable to have as small a binary as possible and ICU is a large library. An additional benefits of having an Obj-C implementation with a C++ wrapper is that the Obj-C implementation can be directly used without introducing C++ in the extension. BUG=671166, 622743 ==========
https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... File components/open_from_clipboard/clipboard_recent_content_ios_impl.h (right): https://codereview.chromium.org/2782823003/diff/1/components/open_from_clipbo... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:39: // Methods below are exposed for testing purposes. On 2017/03/31 13:14:47, jif wrote: > On 2017/03/30 13:36:34, lody wrote: > > On 2017/03/29 14:17:56, jif wrote: > > > I don't they they are used by tests. > > > > Line 83 in clipboard_recent_content_ios_unittest: > > > > void SetStoredPasteboardChangeDate(NSDate* changeDate) { > > clipboard_content_->SetLastPasteboardChangeDate(changeDate); > > clipboard_content_->SaveToUserDefaults(); > > } > > What I'm trying to say is that those 2 obj-c methods would need to be publicly > exposed even if there were no tests. > In other words, you can remove the comment. As clarified offline, they are in fact only needed publicly for tests.
The CQ bit was checked by lod@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
I've commented this CL on the assumption that providing an internal implementation in pure Objective-C for use by the today extension is the solution we want to use. A possible alternative implementation is to see whether it is possible to use GURL without having a dependency on icudtl.dat. It appears that GURL uses icudtl.dat for decoding punnycode, so it may be worth investigating if it is possible to compile GURL without punnycode when building the extension (would involve using a separate toolchain probably, so may be too complex). https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... File components/open_from_clipboard/BUILD.gn (right): https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/BUILD.gn:20: ":open_from_clipboard_ios_impl", This should be "deps", not "public_deps" (as no code using //components/open_from_clipboard should use clipboard_recent_content_ios_impl.h directly). https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/BUILD.gn:24: static_library("open_from_clipboard_ios_impl") { This can be a source_set. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/BUILD.gn:34: configs += [ "//build/config/compiler:enable_arc" ] You'll need to put this configs behind "if (is_ios)" because it is only defined if the current host is "mac" (i.e. only for mac/ios builds). Why not put the whole target behind "if (is_ios)"? We want to avoid having target being conditionally defined as it make the rest of the code harder to read/write (you have to put conditional everywhere the target is used). So, change this to something like this. static_library("open_from_clipboard") { sources = [ "clipboard_recent_content.cc", "clipboard_recent_content.h", "clipboard_recent_content_ios.h", "clipboard_recent_content_ios.mm", ] deps = [ ":open_from_clipboard_helper", "//base", "//net", "//url", ] } # Helper classes used by "open_from_clipboard" target. Those classes have # no dependencies on "//base:i18n". source_set("open_from_clipboard_helper") { sources = [ "clipboard_recent_content_helper_ios.h" "clipboard_recent_content_helper_ios.mm" ] deps = [ "//base", ] assert_no_deps = [ "//base:i18n", ] if (is_ios) { configs += [ "//build/config/compiler:enable_arc" ] } } Note: open_from_clipboard_helper and clipboard_recent_content_helper_ios.* are placeholder names (I'm terrible at naming). They should make sense if you use the internal API. Note: //components/open_from_clipboard is used on other platforms for the tests (to have omnibox component tests be the same on all platforms, even if there is no production use/implementation of the component on other platforms). Note: the automatic filtering of file only look for "_ios" as a suffix, so "foo_ios_impl.h" will not be filtered on other platforms, but "foo_impl_ios.h" will. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_ios.h (right): https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.h:30: explicit ClipboardRecentContentIOS(const std::string& application_scheme, Remove "explicit", it is only necessary when the constructor can be used with exactly one argument (this is not the case here). To allow for easier testing, you should instead allow client code to inject the ClipboardRecentContentIOSImpl instance to use (rule-of-thumb, if there is object creation in the implementation of your code, it is going to be tough to unit test, better to use factory/injection to make code easier to test). ClipboardRecentContentIOS(const std::string& application_scheme, NSUserDefaults* group_user_defaults); ClipboardRecentContentIOS(const std::string& application_scheme, NSUserDefaults* group_user_defaults, ClipboardRecentContentIOSImpl* implementation); https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.h:45: // Set estimation of the date when the pasteboard changed. This is not necessary. Since you inject a subclass of ClipboardRecentContentIOSImpl during test, you can keep a pointer to the injected object, and call the method on it directly. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.h:49: NSArray* GetAuthorizedSchemeList(const std::string& application_scheme); This does not have to be part of the class API, make it a free function in the implementation file. For test, when creating the fake ClipboardRecentContentIOSImpl, just inject list of schemes defined in the test file and tests that the code respect that list. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.h:52: base::scoped_nsobject<ClipboardRecentContentIOSImpl> implementation_; I don't like "implementation_" as a field member name. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_ios.mm (right): https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:37: base::RecordAction(base::UserMetricsAction("MobileOmniboxClipboardChanged")); (outside of the CL) I think action that is recorded is part of the embedder, not of the component (you just proved it by having another use of the code outside of the omnibox). So this delegate should be a C++ class passed to the public ClipboardRecentContent abstract class. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:45: ClipboardRecentContentMetricsDelegateImpl* metricsDelegate = s/metricsDelegate/metrics_delegate/. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:56: NSURL* nsurl = [implementation_ getRecentURLFromClipboard]; url_from_pasteboard https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:57: if (nsurl != nil) { No need to test nsurl for nil, net::GURLWithNSURL() supports nil as part of its API. // Method for creating a valid GURL from a NSURL. This method will return an // empty GURL if the |url| is nil. NET_EXPORT GURL GURLWithNSURL(NSURL* url); https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:58: GURL tempUrl = net::GURLWithNSURL(nsurl); NSURL* url_from_pasteboard = [implementation_ getRecentURLFromClipboard]; GURL converted_url = net::GURLWithNSURL(url_from_pasteboard); if (!converted_url) return false; *url = std::move(converted_url); return true; https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:86: NSArray* ClipboardRecentContentIOS::GetAuthorizedSchemeList( This can be a free function, no need to export this method as part of the class API. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_ios_impl.h (right): https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. File need to be named ${something}_ios.{h,mm} to be automatically filtered on other platforms. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:10: #include "base/macros.h" Why this include? You do not use any of the macros defined as far as I can tell. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:13: @protocol ClipboardRecentContentMetricsDelegate<NSObject> Maybe just ClipboardRecentContentDelegate (or ${Class}Delegate if we end up using a different name for ClipboardRecentContentIOSImpl). https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:19: // Objective C iOS implementation of ClipboardRecentContent. Describe what this class does without referencing ClipboardRecentContent (since it will be called by code that does not use ClipboardRecentContent). https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:26: - (instancetype)initWithDelegate: delegate seems the least important parameter for this initializer (as it can be nil, and is optional), so I would put it last. - (instancetype)initWithAuthorizedSchemes:(NSArray<NSString*>*)authorizedSchemes userDefaults:(NSUserDefaults*)groupUserDefaults delegate: (id<ClipboardRecentContentMetricsDelegate>)delegate NS_DESIGNATED_INITIALIZER; https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:34: // ClipboardRecentContent implementation. Describe the methods without referencing ClipboardRecentContent (since it will be called by code that does not use ClipboardRecentContent). https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_ios_impl.mm (right): https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:131: NSInteger changeCount = [UIPasteboard generalPasteboard].changeCount; It looks like changeCount and changeCountChanged are only used if "notRebooted" is true, maybe move them into the corresponding if block. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:134: bool notRebooted = [self uptime] > [self getClipboardContentAge]; Avoid using negative names for you variables. Instead you can name if "deviceRebooted" by changing the test: bool deviceRebooted = [self getClipboardContentAge] >= [self uptime]; if (!deviceRebooted) return changeCountChanged; https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:139: if (!pasteboardString) { No need to check for nil here, WeakMD5FromNSString supports nil NSString* (as base::SysNSStringToUTF8 does). https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:181: NSString* pasteboardString = [UIPasteboard generalPasteboard].string; This is duplicated with line 138-142. Can you extract to a separate method. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:182: if (!pasteboardString) { ditto https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:186: self.lastPasteboardEntryMD5 = [MD5 init]; You're sending -init to an already initialised Objective-C object. I think this is incorrect. Did you want to use -copy? https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:192: if (!clipboardString) { Braces for simple blocks are optional (simple blocks are defined as both condition and block being single line). However, they should be consistent in a given file. Here you use braces while you don't at line 174. Please make all consistents. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:197: if (url) { You can use an early return here to avoid deep nesting: if (!url) return nil; for (NSString* scheme in self.authorizedSchemes) { if ([url.scheme isEqualToString:scheme]) { return url; } } BTW, it looks like self.authorizedScheme is only used to check whether url.scheme is a member of the collection. Maybe instead change this to use -indexOfObject:, or even better, change the collection from a NSArray* to a NSSet*, and use -containsObject:. @property (nonatomic, readonly) NSSet<NSString*>* authorizedSchemes; if (![self.authorizedSchemes containsObject:url.scheme]) return nil; return url; https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:212: [self.sharedUserDefaults objectForKey:kPasteboardChangeDateKey]; You can use base::ObjCCastStrict here and remove the DCHECK below: self.lastPasteboardChangeDate = base::ObjCCastStrict<NSDate>( [self.sharedUserDefaults objectForKey:kPasteboardChangeDateKey]); https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm (right): https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm:79: - (NSTimeInterval)uptime { I would change this to a public property instead (see comment line 121). https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm:111: clipboard_content_->implementation_.reset( This is exactly how you should not write test. Instead provide a constructor that takes a ClipboardRecentContentIOSImpl* and use that constructor when creating your ClipboardRecentContentIOS. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm:114: authorizedSchemes:clipboard_content_->GetAuthorizedSchemeList( Just inject a know list/set of schemes here, no need to access this private method. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm:121: clipboard_content_->SetLastPasteboardChangeDate([change_date copy]); Can't you simulate this by instead manipulating the fake uptime? \
https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm (right): https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm:111: clipboard_content_->implementation_.reset( On 2017/04/03 09:27:17, sdefresne wrote: > This is exactly how you should not write test. Instead provide a constructor > that takes a ClipboardRecentContentIOSImpl* and use that constructor when > creating your ClipboardRecentContentIOS. Sorry, this comment tone is too negative. I should have remove the first sentence. Please read this as: I think injecting the ClipboardRecentContentIOSImpl via the constructor would make the test easier to write as it would not require accessing private member fields of ClipboardRecentContentIOS.
lod@chromium.org changed reviewers: + marq@chromium.org
+ marq at his own urging. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... File components/open_from_clipboard/BUILD.gn (right): https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/BUILD.gn:20: ":open_from_clipboard_ios_impl", On 2017/04/03 09:27:16, sdefresne wrote: > This should be "deps", not "public_deps" (as no code using > //components/open_from_clipboard should use clipboard_recent_content_ios_impl.h > directly). Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/BUILD.gn:24: static_library("open_from_clipboard_ios_impl") { On 2017/04/03 09:27:16, sdefresne wrote: > This can be a source_set. Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/BUILD.gn:34: configs += [ "//build/config/compiler:enable_arc" ] On 2017/04/03 09:27:16, sdefresne wrote: > You'll need to put this configs behind "if (is_ios)" because it is only defined > if the current host is "mac" (i.e. only for mac/ios builds). > > Why not put the whole target behind "if (is_ios)"? We want to avoid having > target being conditionally defined as it make the rest of the code harder to > read/write (you have to put conditional everywhere the target is used). > > So, change this to something like this. > > static_library("open_from_clipboard") { > sources = [ > "clipboard_recent_content.cc", > "clipboard_recent_content.h", > "clipboard_recent_content_ios.h", > "clipboard_recent_content_ios.mm", > ] > deps = [ > ":open_from_clipboard_helper", > "//base", > "//net", > "//url", > ] > } > > # Helper classes used by "open_from_clipboard" target. Those classes have > # no dependencies on "//base:i18n". > source_set("open_from_clipboard_helper") { > sources = [ > "clipboard_recent_content_helper_ios.h" > "clipboard_recent_content_helper_ios.mm" > ] > deps = [ > "//base", > ] > assert_no_deps = [ > "//base:i18n", > ] > if (is_ios) { > configs += [ "//build/config/compiler:enable_arc" ] > } > } > > Note: open_from_clipboard_helper and clipboard_recent_content_helper_ios.* are > placeholder names (I'm terrible at naming). They should make sense if you use > the internal API. > > Note: //components/open_from_clipboard is used on other platforms for the tests > (to have omnibox component tests be the same on all platforms, even if there is > no production use/implementation of the component on other platforms). > > Note: the automatic filtering of file only look for "_ios" as a suffix, so > "foo_ios_impl.h" will not be filtered on other platforms, but "foo_impl_ios.h" > will. Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_ios.h (right): https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.h:30: explicit ClipboardRecentContentIOS(const std::string& application_scheme, On 2017/04/03 09:27:16, sdefresne wrote: > Remove "explicit", it is only necessary when the constructor can be used with > exactly one argument (this is not the case here). > > To allow for easier testing, you should instead allow client code to inject the > ClipboardRecentContentIOSImpl instance to use (rule-of-thumb, if there is object > creation in the implementation of your code, it is going to be tough to unit > test, better to use factory/injection to make code easier to test). > > ClipboardRecentContentIOS(const std::string& application_scheme, > NSUserDefaults* group_user_defaults); > ClipboardRecentContentIOS(const std::string& application_scheme, > NSUserDefaults* group_user_defaults, > ClipboardRecentContentIOSImpl* implementation); Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.h:45: // Set estimation of the date when the pasteboard changed. On 2017/04/03 09:27:16, sdefresne wrote: > This is not necessary. Since you inject a subclass of > ClipboardRecentContentIOSImpl during test, you can keep a pointer to the > injected object, and call the method on it directly. Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.h:49: NSArray* GetAuthorizedSchemeList(const std::string& application_scheme); On 2017/04/03 09:27:16, sdefresne wrote: > This does not have to be part of the class API, make it a free function in the > implementation file. > > For test, when creating the fake ClipboardRecentContentIOSImpl, just inject list > of schemes defined in the test file and tests that the code respect that list. Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_ios.mm (right): https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:45: ClipboardRecentContentMetricsDelegateImpl* metricsDelegate = On 2017/04/03 09:27:16, sdefresne wrote: > s/metricsDelegate/metrics_delegate/. Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:56: NSURL* nsurl = [implementation_ getRecentURLFromClipboard]; On 2017/04/03 09:27:16, sdefresne wrote: > url_from_pasteboard Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:57: if (nsurl != nil) { On 2017/04/03 09:27:16, sdefresne wrote: > No need to test nsurl for nil, net::GURLWithNSURL() supports nil as part of its > API. > > // Method for creating a valid GURL from a NSURL. This method will return an > // empty GURL if the |url| is nil. > NET_EXPORT GURL GURLWithNSURL(NSURL* url); Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:58: GURL tempUrl = net::GURLWithNSURL(nsurl); On 2017/04/03 09:27:16, sdefresne wrote: > NSURL* url_from_pasteboard = [implementation_ getRecentURLFromClipboard]; > GURL converted_url = net::GURLWithNSURL(url_from_pasteboard); > if (!converted_url) > return false; > *url = std::move(converted_url); > return true; Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:86: NSArray* ClipboardRecentContentIOS::GetAuthorizedSchemeList( On 2017/04/03 09:27:16, sdefresne wrote: > This can be a free function, no need to export this method as part of the class > API. Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_ios_impl.h (right): https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/04/03 09:27:17, sdefresne wrote: > File need to be named ${something}_ios.{h,mm} to be automatically filtered on > other platforms. Acknowledged. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:10: #include "base/macros.h" On 2017/04/03 09:27:16, sdefresne wrote: > Why this include? You do not use any of the macros defined as far as I can tell. Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:13: @protocol ClipboardRecentContentMetricsDelegate<NSObject> On 2017/04/03 09:27:17, sdefresne wrote: > Maybe just ClipboardRecentContentDelegate (or ${Class}Delegate if we end up > using a different name for ClipboardRecentContentIOSImpl). Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:19: // Objective C iOS implementation of ClipboardRecentContent. On 2017/04/03 09:27:16, sdefresne wrote: > Describe what this class does without referencing ClipboardRecentContent (since > it will be called by code that does not use ClipboardRecentContent). Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:26: - (instancetype)initWithDelegate: On 2017/04/03 09:27:16, sdefresne wrote: > delegate seems the least important parameter for this initializer (as it can be > nil, and is optional), so I would put it last. > > - (instancetype)initWithAuthorizedSchemes:(NSArray<NSString*>*)authorizedSchemes > userDefaults:(NSUserDefaults*)groupUserDefaults > delegate: > (id<ClipboardRecentContentMetricsDelegate>)delegate > NS_DESIGNATED_INITIALIZER; Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.h:34: // ClipboardRecentContent implementation. On 2017/04/03 09:27:16, sdefresne wrote: > Describe the methods without referencing ClipboardRecentContent (since > it will be called by code that does not use ClipboardRecentContent). Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_ios_impl.mm (right): https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:131: NSInteger changeCount = [UIPasteboard generalPasteboard].changeCount; On 2017/04/03 09:27:17, sdefresne wrote: > It looks like changeCount and changeCountChanged are only used if "notRebooted" > is true, maybe move them into the corresponding if block. Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:134: bool notRebooted = [self uptime] > [self getClipboardContentAge]; On 2017/04/03 09:27:17, sdefresne wrote: > Avoid using negative names for you variables. Instead you can name if > "deviceRebooted" by changing the test: > > bool deviceRebooted = [self getClipboardContentAge] >= [self uptime]; > if (!deviceRebooted) > return changeCountChanged; Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:139: if (!pasteboardString) { On 2017/04/03 09:27:17, sdefresne wrote: > No need to check for nil here, WeakMD5FromNSString supports nil NSString* (as > base::SysNSStringToUTF8 does). Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:181: NSString* pasteboardString = [UIPasteboard generalPasteboard].string; On 2017/04/03 09:27:17, sdefresne wrote: > This is duplicated with line 138-142. Can you extract to a separate method. Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:182: if (!pasteboardString) { On 2017/04/03 09:27:17, sdefresne wrote: > ditto Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:186: self.lastPasteboardEntryMD5 = [MD5 init]; On 2017/04/03 09:27:17, sdefresne wrote: > You're sending -init to an already initialised Objective-C object. I think this > is incorrect. Did you want to use -copy? Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:192: if (!clipboardString) { On 2017/04/03 09:27:17, sdefresne wrote: > Braces for simple blocks are optional (simple blocks are defined as both > condition and block being single line). However, they should be consistent in a > given file. Here you use braces while you don't at line 174. Please make all > consistents. Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:197: if (url) { On 2017/04/03 09:27:17, sdefresne wrote: > You can use an early return here to avoid deep nesting: > > if (!url) > return nil; > > for (NSString* scheme in self.authorizedSchemes) { > if ([url.scheme isEqualToString:scheme]) { > return url; > } > } > > BTW, it looks like self.authorizedScheme is only used to check whether > url.scheme is a member of the collection. Maybe instead change this to use > -indexOfObject:, or even better, change the collection from a NSArray* to a > NSSet*, and use -containsObject:. > > @property (nonatomic, readonly) NSSet<NSString*>* authorizedSchemes; > > if (![self.authorizedSchemes containsObject:url.scheme]) > return nil; > return url; Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_impl.mm:212: [self.sharedUserDefaults objectForKey:kPasteboardChangeDateKey]; On 2017/04/03 09:27:17, sdefresne wrote: > You can use base::ObjCCastStrict here and remove the DCHECK below: > > self.lastPasteboardChangeDate = > base::ObjCCastStrict<NSDate>( > [self.sharedUserDefaults objectForKey:kPasteboardChangeDateKey]); Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm (right): https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm:111: clipboard_content_->implementation_.reset( On 2017/04/03 09:37:32, sdefresne wrote: > On 2017/04/03 09:27:17, sdefresne wrote: > > This is exactly how you should not write test. Instead provide a constructor > > that takes a ClipboardRecentContentIOSImpl* and use that constructor when > > creating your ClipboardRecentContentIOS. > > Sorry, this comment tone is too negative. I should have remove the first > sentence. Please read this as: > > I think injecting the ClipboardRecentContentIOSImpl via the constructor would > make the test easier to write as it would not require accessing private member > fields of ClipboardRecentContentIOS. Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm:114: authorizedSchemes:clipboard_content_->GetAuthorizedSchemeList( On 2017/04/03 09:27:17, sdefresne wrote: > Just inject a know list/set of schemes here, no need to access this private > method. Done. https://codereview.chromium.org/2782823003/diff/40001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm:121: clipboard_content_->SetLastPasteboardChangeDate([change_date copy]); On 2017/04/03 09:27:17, sdefresne wrote: > Can't you simulate this by instead manipulating the fake uptime? \ I don't think so? There's two things, one is the uptime (if the uptime is shorter than last change date md5 is used to check how many changes have occured rather than change count) and the changedate (if the changedate is more than 3 hours then nothing is returned). I can't see how to use only uptime to simulate this.
with actual patchset.
https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_impl_ios.h (right): https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.h:26: - (instancetype)initWithAuthorizedSchemes:(NSSet*)authorizedSchemes Use lightweight generics -- NSSet<NSString*>* https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.h:34: // Returns true if the clipboard contains a recent URL that has not been This method doesn't return a boolean at all. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.h:37: - (NSURL*)getRecentURLFromClipboard; Don't use 'get' to name ObjC methods unless they are populating values passed in by reference. See https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Co... https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.h:42: // Prevent GetRecentURLFromClipboard from returning anything until the Prevents https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_impl_ios.mm (right): https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.mm:22: NSString* kPasteboardChangeCountKey = @"PasteboardChangeCount"; NSString* const kPasteboardChangeCountKey ..., here and below. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.mm:30: NSTimeInterval kMaximumAgeOfClipboard = 3 * 60 * 60; const? https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.mm:139: bool deviceRebooted = [self getClipboardContentAge] >= [self uptime]; BOOL https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.mm:158: if (urlFromPasteboard) { If this conditional fails, that means |urlFromPasteboard| is nil, and then nil is returned. So these five lines can just be collapsed into: return [self URLFromPasteboard]; https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.mm:164: - (NSTimeInterval)getClipboardContentAge { remove 'get'. Method needs a comment somewhere. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.mm:192: if (!clipboardString) { You can remove a lot of the nil checking, since [NSURL URLWithString:nil] is also nil, and [NSSet containsObect:nil] always returns NO. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.mm:208: self.lastPasteboardEntryMD5 = Why not also cast this value? https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_ios.h (right): https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.h:43: base::scoped_nsobject<ClipboardRecentContentImplIOS> implementation_; I'm fine with |implementation_| as a name here given the class names as they stand. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_ios.mm (right): https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:30: NSSet* getAuthorizedSchemeList(const std::string& application_scheme) { NSSet<NSString*>* https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:31: NSMutableSet* schemes = [NSMutableSet set]; NSMutableSet<NSString*>* https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:39: return schemes; nit: return [schemes copy] so the return value isn't mutable.
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_impl_ios.h (right): https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.h:26: - (instancetype)initWithAuthorizedSchemes:(NSSet*)authorizedSchemes On 2017/04/05 07:58:10, marq wrote: > Use lightweight generics -- NSSet<NSString*>* Done. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.h:34: // Returns true if the clipboard contains a recent URL that has not been On 2017/04/05 07:58:10, marq wrote: > This method doesn't return a boolean at all. ... sorry :( that's what I get for lazy copy pasting. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.h:37: - (NSURL*)getRecentURLFromClipboard; On 2017/04/05 07:58:10, marq wrote: > Don't use 'get' to name ObjC methods unless they are populating values passed > in by reference. See > https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Co... Done (also, that links gives a sorry, that page cannot be found) https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.h:42: // Prevent GetRecentURLFromClipboard from returning anything until the On 2017/04/05 07:58:10, marq wrote: > Prevents Done. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_impl_ios.mm (right): https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.mm:22: NSString* kPasteboardChangeCountKey = @"PasteboardChangeCount"; On 2017/04/05 07:58:10, marq wrote: > NSString* const kPasteboardChangeCountKey ..., here and below. Done. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.mm:30: NSTimeInterval kMaximumAgeOfClipboard = 3 * 60 * 60; On 2017/04/05 07:58:10, marq wrote: > const? Done. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.mm:139: bool deviceRebooted = [self getClipboardContentAge] >= [self uptime]; On 2017/04/05 07:58:10, marq wrote: > BOOL Done. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.mm:158: if (urlFromPasteboard) { On 2017/04/05 07:58:10, marq wrote: > If this conditional fails, that means |urlFromPasteboard| is nil, and then nil > is returned. > > So these five lines can just be collapsed into: > > return [self URLFromPasteboard]; Done. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.mm:164: - (NSTimeInterval)getClipboardContentAge { On 2017/04/05 07:58:10, marq wrote: > remove 'get'. > > Method needs a comment somewhere. Done It's in the .h https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.mm:192: if (!clipboardString) { On 2017/04/05 07:58:10, marq wrote: > You can remove a lot of the nil checking, since [NSURL URLWithString:nil] is > also nil, and [NSSet containsObect:nil] always returns NO. Done. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.mm:208: self.lastPasteboardEntryMD5 = On 2017/04/05 07:58:10, marq wrote: > Why not also cast this value? Done. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_ios.h (right): https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.h:43: base::scoped_nsobject<ClipboardRecentContentImplIOS> implementation_; On 2017/04/05 07:58:10, marq wrote: > I'm fine with |implementation_| as a name here given the class names as they > stand. Acknowledged. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_ios.mm (right): https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:30: NSSet* getAuthorizedSchemeList(const std::string& application_scheme) { On 2017/04/05 07:58:10, marq wrote: > NSSet<NSString*>* Done. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:31: NSMutableSet* schemes = [NSMutableSet set]; On 2017/04/05 07:58:10, marq wrote: > NSMutableSet<NSString*>* Done. https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_ios.mm:39: return schemes; On 2017/04/05 07:58:10, marq wrote: > nit: return [schemes copy] so the return value isn't mutable. Done.
Patchset #5 (id:120001) has been deleted
lgtm https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_impl_ios.h (right): https://codereview.chromium.org/2782823003/diff/60001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_impl_ios.h:37: - (NSURL*)getRecentURLFromClipboard; On 2017/04/06 11:27:21, lody wrote: > On 2017/04/05 07:58:10, marq wrote: > > Don't use 'get' to name ObjC methods unless they are populating values passed > > in by reference. See > > > https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Co... > > Done > (also, that links gives a sorry, that page cannot be found) It should be .html, not .htm : https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Co...
lgtm https://codereview.chromium.org/2782823003/diff/140001/components/open_from_c... File components/open_from_clipboard/BUILD.gn (right): https://codereview.chromium.org/2782823003/diff/140001/components/open_from_c... components/open_from_clipboard/BUILD.gn:29: # Helper classes used by "open_from_clipboard" target. These classes have nit: These classes must have no dependencies on "//base:i18n". https://codereview.chromium.org/2782823003/diff/140001/components/open_from_c... File components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm (right): https://codereview.chromium.org/2782823003/diff/140001/components/open_from_c... components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm:118: clipboard_content_.reset( Use base::MakeUnique<> if possible here. https://codereview.chromium.org/2782823003/diff/140001/components/open_from_c... components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm:129: ClipboardRecentContentImplIOSWithFakeUptime* clipboard_content_impl_; s/clipboard_content_impl_/clipboard_content_implementation_/?
https://codereview.chromium.org/2782823003/diff/140001/components/open_from_c... File components/open_from_clipboard/BUILD.gn (right): https://codereview.chromium.org/2782823003/diff/140001/components/open_from_c... components/open_from_clipboard/BUILD.gn:29: # Helper classes used by "open_from_clipboard" target. These classes have On 2017/04/06 14:51:11, sdefresne wrote: > nit: These classes must have no dependencies on "//base:i18n". Done. https://codereview.chromium.org/2782823003/diff/140001/components/open_from_c... File components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm (right): https://codereview.chromium.org/2782823003/diff/140001/components/open_from_c... components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm:118: clipboard_content_.reset( On 2017/04/06 14:51:11, sdefresne wrote: > Use base::MakeUnique<> if possible here. Done. https://codereview.chromium.org/2782823003/diff/140001/components/open_from_c... components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm:129: ClipboardRecentContentImplIOSWithFakeUptime* clipboard_content_impl_; On 2017/04/06 14:51:11, sdefresne wrote: > s/clipboard_content_impl_/clipboard_content_implementation_/? Done.
lgtm https://codereview.chromium.org/2782823003/diff/160001/components/open_from_c... File components/open_from_clipboard/clipboard_recent_content_ios.h (right): https://codereview.chromium.org/2782823003/diff/160001/components/open_from_c... components/open_from_clipboard/clipboard_recent_content_ios.h:19: // IOS implementation of ClipboardRecentContent Can you add // IOS implementation of ClipboardRecentContent. // A large part of the implementation is in clipboard_recent_content_impl_ios, // a GURL-free class also used by iOS extensions. iOS extensions can't use GURL // because GURL requires depending on ICU. or something like that. Not sure what's the correct name used for extensions. https://codereview.chromium.org/2782823003/diff/160001/components/open_from_c... File components/open_from_clipboard/clipboard_recent_content_ios.mm (right): https://codereview.chromium.org/2782823003/diff/160001/components/open_from_c... components/open_from_clipboard/clipboard_recent_content_ios.mm:42: } } // namespace
https://codereview.chromium.org/2782823003/diff/160001/components/open_from_c... File components/open_from_clipboard/clipboard_recent_content_ios.h (right): https://codereview.chromium.org/2782823003/diff/160001/components/open_from_c... components/open_from_clipboard/clipboard_recent_content_ios.h:19: // IOS implementation of ClipboardRecentContent On 2017/04/06 15:56:53, jif wrote: > Can you add > > // IOS implementation of ClipboardRecentContent. > // A large part of the implementation is in clipboard_recent_content_impl_ios, > // a GURL-free class also used by iOS extensions. iOS extensions can't use GURL > // because GURL requires depending on ICU. > > or something like that. > Not sure what's the correct name used for extensions. Done. https://codereview.chromium.org/2782823003/diff/160001/components/open_from_c... File components/open_from_clipboard/clipboard_recent_content_ios.mm (right): https://codereview.chromium.org/2782823003/diff/160001/components/open_from_c... components/open_from_clipboard/clipboard_recent_content_ios.mm:42: } On 2017/04/06 15:56:53, jif wrote: > } // namespace Done.
The CQ bit was checked by lod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, marq@chromium.org, jif@chromium.org Link to the patchset: https://codereview.chromium.org/2782823003/#ps180001 (title: "comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lod@chromium.org changed reviewers: + juliatuttle@chromium.org
+juliatuttle for /net (added to DEPS). Thanks!
On 2017/04/10 18:06:04, lody wrote: > +juliatuttle for /net (added to DEPS). Thanks! //net DEPS lgtm!
The CQ bit was checked by lod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org, sdefresne@chromium.org, marq@chromium.org Link to the patchset: https://codereview.chromium.org/2782823003/#ps200001 (title: "build.gn fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1491897500141600,
"parent_rev": "26b90c72f02e0423dc191e76ced2991f663ee2ce", "commit_rev":
"dd288990130665b1a0e4b5d3de33f68fdb9df9fd"}
Message was sent while issue was closed.
Description was changed from ========== Rewrite implementation of ClipboardRecentContent in Objective C. In doing so, remove use of gurl which causes a dependency on ICU. As ClipboardRecentContent is used in extensions it is preferable to have as small a binary as possible and ICU is a large library. An additional benefits of having an Obj-C implementation with a C++ wrapper is that the Obj-C implementation can be directly used without introducing C++ in the extension. BUG=671166, 622743 ========== to ========== Rewrite implementation of ClipboardRecentContent in Objective C. In doing so, remove use of gurl which causes a dependency on ICU. As ClipboardRecentContent is used in extensions it is preferable to have as small a binary as possible and ICU is a large library. An additional benefits of having an Obj-C implementation with a C++ wrapper is that the Obj-C implementation can be directly used without introducing C++ in the extension. BUG=671166, 622743 Review-Url: https://codereview.chromium.org/2782823003 Cr-Commit-Position: refs/heads/master@{#463582} Committed: https://chromium.googlesource.com/chromium/src/+/dd288990130665b1a0e4b5d3de33... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:200001) as https://chromium.googlesource.com/chromium/src/+/dd288990130665b1a0e4b5d3de33... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
