Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2529763002:
Create FakeProviders for NativeAppMetadata and NativeAppManager (Closed)
DescriptionCreate FakeProviders for NativeAppMetadata and NativeAppManager
These 2 classes will be used instead of Mocks for all tests where a basic NativeApp* implementation is needed.
BUG=662573
Committed: https://crrev.com/361b7f0fcda579b98df8e77090ff994bebcdd4cc
Cr-Commit-Position: refs/heads/master@{#435116}
Patch Set 1 #Patch Set 2 : Memory handling #Patch Set 3 : WIP FakeProvider #Patch Set 4 : FakeMetadata Changes #
Total comments: 33
Patch Set 5 : Switches to ARC #Patch Set 6 : Add implementation for |schemeForAppId:| #
Total comments: 10
Patch Set 7 : Feedback #
Total comments: 4
Patch Set 8 : Rebase and CL Feedback #
Messages
Total messages: 39 (23 generated)
Description was changed from ========== Create fakeproviders for NativeAppMetadata BUG= ========== to ========== Create FakeProviders for NativeAppMetadata and NativeAppManager These 2 classes will be used instead of Mocks for all tests where a basic NativeApp* implementation is needed. BUG=662573 ==========
sczs@google.com changed reviewers: + rohitrao@chromium.org, sdefresne@chromium.org
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by sczs@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This might probably need small changes depending on what's needed for tests. But if you could PTAL it would be great.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Do all of the tests pass when using these fakes? https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/BUILD.gn:19: "native_app_launcher/fake_native_app_metadata.h", These should move down into the "test_support" target. That will help ensure we never accidentally ship them with the main app. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.h (right): https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.h:16: @property(nonatomic, assign) NSString* appName; This is personal preference, but I prefer to explicitly use "readwrite" on properties. (nonatomic, readwrite, assign) https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm (right): https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:49: - (void)setGoogleOwnedApp:(BOOL)googleOwnedApp { @synthesize will create this getter and setter for you, so you can omit it. You only need it for the scoped_nsobject-backed properties. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:57: - (void)setInstalled:(BOOL)installed { Can leave out this getter and setter. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:71: return; We should probably call the completion block with nil, in case the caller is expecting a response. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:74: - (BOOL)canOpenURL:(const GURL&)url { How did you come up with the implementation for this method? https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:90: return; No need to write "return;" for methods that return void. Just leave the body empty. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:114: - (BOOL)shouldAutoOpenLinks { Consider ordering these methods to match the order that's in the protocol. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.h (right): https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.h:15: // Holds the metadata object from initWithMetadata. This comment should mention what metadata is used for. Maybe something like: // The metadata returned by calls to |newNativeAppForURL:|. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.h:19: - (instancetype)initWithMetadata:(id<NativeAppMetadata>)metadata; How about removing this initializer and making the property readwrite? You could imagine a test wanting to change the returned metadata during the test. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.mm (right): https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.mm:11: base::scoped_nsprotocol<id<NativeAppMetadata, NSObject>> _metadata; Is the NSObject necessary? The NativeAppMetadata already inherits from the NSObject protocol. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.mm:18: - (instancetype)initWithMetadata:(id<NativeAppMetadata, NSObject>)metadata { Should be able to leave out the NSObject. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.mm:45: return; No need for "return;". https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.mm:49: - (id<NativeAppMetadata, NSObject>)metadata { Should be able to leave out the NSObject.
In the CL description, I would change "Create FakeProviders" to "Create fakes". These aren't really providers, they're just fake objects.
https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/BUILD.gn:19: "native_app_launcher/fake_native_app_metadata.h", On 2016/11/25 00:34:38, rohitrao wrote: > These should move down into the "test_support" target. That will help ensure we > never accidentally ship them with the main app. +1 https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.h (right): https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.h:22: @property(getter=isGoogleOwnedApp) BOOL googleOwnedApp; Is this readonly? Maybe add "readwrite" here too. You need "nonatomic" too as atomic is the default and we do not want this default. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.h:25: @property(getter=isInstalled) BOOL installed; ditto "nonatomic". https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm (right): https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:11: @interface FakeNativeAppMetadata () { Move to @implementation https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:49: - (void)setGoogleOwnedApp:(BOOL)googleOwnedApp { On 2016/11/25 00:34:38, rohitrao wrote: > @synthesize will create this getter and setter for you, so you can omit it. You > only need it for the scoped_nsobject-backed properties. And since this is new code, it is recommended that you use ARC and not use scoped_nsobject altogether, meaning that you can synthesize all properties. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:114: - (BOOL)shouldAutoOpenLinks { On 2016/11/25 00:34:38, rohitrao wrote: > Consider ordering these methods to match the order that's in the protocol. This is a property, so you propably don't need to write implementation. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.mm (right): https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.mm:11: base::scoped_nsprotocol<id<NativeAppMetadata, NSObject>> _metadata; You should use ARC for new code and not use scoped_nsobject.
Patchset #6 (id:120001) has been deleted
PTAL Switched over to ARC and all tests pass with this current implementation. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/BUILD.gn:19: "native_app_launcher/fake_native_app_metadata.h", On 2016/11/25 08:41:57, sdefresne wrote: > On 2016/11/25 00:34:38, rohitrao wrote: > > These should move down into the "test_support" target. That will help ensure > we > > never accidentally ship them with the main app. > > +1 Right, didn't think of that! I've now created a new build.gn for native_app_launcher and enabled ARC on both source_sets. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.h (right): https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.h:16: @property(nonatomic, assign) NSString* appName; On 2016/11/25 00:34:38, rohitrao wrote: > This is personal preference, but I prefer to explicitly use "readwrite" on > properties. > > (nonatomic, readwrite, assign) Sounds good, it describes the property better. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.h:22: @property(getter=isGoogleOwnedApp) BOOL googleOwnedApp; On 2016/11/25 08:41:57, sdefresne wrote: > Is this readonly? Maybe add "readwrite" here too. > > You need "nonatomic" too as atomic is the default and we do not want this > default. Done. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.h:25: @property(getter=isInstalled) BOOL installed; On 2016/11/25 08:41:57, sdefresne wrote: > ditto "nonatomic". Done. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm (right): https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:71: return; On 2016/11/25 00:34:38, rohitrao wrote: > We should probably call the completion block with nil, in case the caller is > expecting a response. Done. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:90: return; On 2016/11/25 00:34:38, rohitrao wrote: > No need to write "return;" for methods that return void. Just leave the body > empty. Done. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.h (right): https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.h:15: // Holds the metadata object from initWithMetadata. On 2016/11/25 00:34:38, rohitrao wrote: > This comment should mention what metadata is used for. Maybe something like: > > // The metadata returned by calls to |newNativeAppForURL:|. Done. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.h:19: - (instancetype)initWithMetadata:(id<NativeAppMetadata>)metadata; On 2016/11/25 00:34:38, rohitrao wrote: > How about removing this initializer and making the property readwrite? You > could imagine a test wanting to change the returned metadata during the test. Was thinking the same thing, will change it. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.mm (right): https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.mm:11: base::scoped_nsprotocol<id<NativeAppMetadata, NSObject>> _metadata; On 2016/11/25 08:41:58, sdefresne wrote: > You should use ARC for new code and not use scoped_nsobject. Without the NSObject I wasn't able to call retain on _metadata. So apparently it needs to be added explicitly. I will switch to ARC. Stephan is converting upstream so I think is a good idea. Some GN refactoring will be needed. For the time being I'll add my new build.gn as a public dep. If we want to avoid that this CL needs to land before the one I've been trying to land downstream. Let me know what you both think. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.mm:18: - (instancetype)initWithMetadata:(id<NativeAppMetadata, NSObject>)metadata { On 2016/11/25 00:34:38, rohitrao wrote: > Should be able to leave out the NSObject. Acknowledged. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.mm:45: return; On 2016/11/25 00:34:39, rohitrao wrote: > No need for "return;". Done. https://codereview.chromium.org/2529763002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_whitelist_manager.mm:49: - (id<NativeAppMetadata, NSObject>)metadata { On 2016/11/25 00:34:38, rohitrao wrote: > Should be able to leave out the NSObject. Acknowledged. https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... File ios/public/provider/chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... ios/public/provider/chrome/browser/BUILD.gn:60: "//ios/public/provider/chrome/browser/native_app_launcher", This will be moved to deps after we update downstream build.gn paths to ios/public/provider/chrome/browser/native_app_launcher instead of ios/public/provider/chrome/browser https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm (right): https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:41: - (BOOL)canOpenURL:(const GURL&)url { This logic was used to satisfy TEST_F(GoogleAppsSettingsCollectionViewControllerTest, AppDidInstall)
The CQ bit was checked by sczs@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... File ios/public/provider/chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... ios/public/provider/chrome/browser/BUILD.gn:60: "//ios/public/provider/chrome/browser/native_app_launcher", On 2016/11/26 04:10:19, sczs1 wrote: > This will be moved to deps after we update downstream build.gn paths to > ios/public/provider/chrome/browser/native_app_launcher instead of > ios/public/provider/chrome/browser I'm already splitting that target with https://codereview.chromium.org/2527383002. https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... File ios/public/provider/chrome/browser/native_app_launcher/BUILD.gn (right): https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... ios/public/provider/chrome/browser/native_app_launcher/BUILD.gn:6: configs += [ "//build/config/compiler:enable_arc" ] nit: can you move "configs" at the end of "source_set" here and below https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm (right): https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:41: - (BOOL)canOpenURL:(const GURL&)url { On 2016/11/26 04:10:19, sczs1 wrote: > This logic was used to satisfy > TEST_F(GoogleAppsSettingsCollectionViewControllerTest, AppDidInstall) This can probably be changed to the following: return url.is_valid() && url.SchemeIs(base::SysNSStringToUTF8(_appName));
https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... File ios/public/provider/chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... ios/public/provider/chrome/browser/BUILD.gn:60: "//ios/public/provider/chrome/browser/native_app_launcher", On 2016/11/28 09:28:12, sdefresne wrote: > On 2016/11/26 04:10:19, sczs1 wrote: > > This will be moved to deps after we update downstream build.gn paths to > > ios/public/provider/chrome/browser/native_app_launcher instead of > > ios/public/provider/chrome/browser > > I'm already splitting that target with > https://codereview.chromium.org/2527383002. By that, I meant that you should leave those three files in the current target, I'm taking care of moving them. Just define the "test_support" target in ios/public/provider/chrome/browser/native_app_launcher/BUILD.gn.
Thanks Sylvain, will Rebase once your changes land. https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... File ios/public/provider/chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... ios/public/provider/chrome/browser/BUILD.gn:60: "//ios/public/provider/chrome/browser/native_app_launcher", On 2016/11/28 09:29:47, sdefresne wrote: > On 2016/11/28 09:28:12, sdefresne wrote: > > On 2016/11/26 04:10:19, sczs1 wrote: > > > This will be moved to deps after we update downstream build.gn paths to > > > ios/public/provider/chrome/browser/native_app_launcher instead of > > > ios/public/provider/chrome/browser > > > > I'm already splitting that target with > > https://codereview.chromium.org/2527383002. > > By that, I meant that you should leave those three files in the current target, > I'm taking care of moving them. Just define the "test_support" target in > ios/public/provider/chrome/browser/native_app_launcher/BUILD.gn. Acknowledged. https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... ios/public/provider/chrome/browser/BUILD.gn:60: "//ios/public/provider/chrome/browser/native_app_launcher", On 2016/11/28 09:29:47, sdefresne wrote: > On 2016/11/28 09:28:12, sdefresne wrote: > > On 2016/11/26 04:10:19, sczs1 wrote: > > > This will be moved to deps after we update downstream build.gn paths to > > > ios/public/provider/chrome/browser/native_app_launcher instead of > > > ios/public/provider/chrome/browser > > > > I'm already splitting that target with > > https://codereview.chromium.org/2527383002. > > By that, I meant that you should leave those three files in the current target, > I'm taking care of moving them. Just define the "test_support" target in > ios/public/provider/chrome/browser/native_app_launcher/BUILD.gn. Sounds good! Will revert these changes. https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... File ios/public/provider/chrome/browser/native_app_launcher/BUILD.gn (right): https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... ios/public/provider/chrome/browser/native_app_launcher/BUILD.gn:6: configs += [ "//build/config/compiler:enable_arc" ] On 2016/11/28 09:28:12, sdefresne wrote: > nit: can you move "configs" at the end of "source_set" here and below Done. https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm (right): https://codereview.chromium.org/2529763002/diff/140001/ios/public/provider/ch... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:41: - (BOOL)canOpenURL:(const GURL&)url { On 2016/11/28 09:28:12, sdefresne wrote: > On 2016/11/26 04:10:19, sczs1 wrote: > > This logic was used to satisfy > > TEST_F(GoogleAppsSettingsCollectionViewControllerTest, AppDidInstall) > > This can probably be changed to the following: > > return url.is_valid() && url.SchemeIs(base::SysNSStringToUTF8(_appName)); An invalid url needs to return yes all the time. Which won't be the case if we use that condition.
@sdefresne could you take a look at this today?
lgtm https://codereview.chromium.org/2529763002/diff/160001/ios/public/provider/ch... File ios/public/provider/chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2529763002/diff/160001/ios/public/provider/ch... ios/public/provider/chrome/browser/BUILD.gn:133: "//ios/public/provider/chrome/browser/native_app_launcher:test_support", Can you add a TODO that this public_deps need to be removed. I think I split this target, so maybe rebase too. https://codereview.chromium.org/2529763002/diff/160001/ios/public/provider/ch... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm (right): https://codereview.chromium.org/2529763002/diff/160001/ios/public/provider/ch... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:44: GURL appUrl(base::SysNSStringToUTF8(_appName) + ":"); Can you check if the following works? This will avoid parsing a GURL. If it does not work, current code look fine. return url.SchemeIs(base::SysNSStringToUTF8(_appName));
The CQ bit was checked by sczs@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Will CQ the CL if tests pass. https://codereview.chromium.org/2529763002/diff/160001/ios/public/provider/ch... File ios/public/provider/chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2529763002/diff/160001/ios/public/provider/ch... ios/public/provider/chrome/browser/BUILD.gn:133: "//ios/public/provider/chrome/browser/native_app_launcher:test_support", On 2016/11/29 18:19:33, sdefresne wrote: > Can you add a TODO that this public_deps need to be removed. I think I split > this target, so maybe rebase too. Yes you did, the rebase now includes your comment. https://codereview.chromium.org/2529763002/diff/160001/ios/public/provider/ch... File ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm (right): https://codereview.chromium.org/2529763002/diff/160001/ios/public/provider/ch... ios/public/provider/chrome/browser/native_app_launcher/fake_native_app_metadata.mm:44: GURL appUrl(base::SysNSStringToUTF8(_appName) + ":"); On 2016/11/29 18:19:33, sdefresne wrote: > Can you check if the following works? This will avoid parsing a GURL. If it does > not work, current code look fine. > > return url.SchemeIs(base::SysNSStringToUTF8(_appName)); It didn't work, leaving the current version.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sczs@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2529763002/#ps180001 (title: "Rebase and CL Feedback")
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_TIMED_OUT, no build URL)
The CQ bit was checked by rohitrao@chromium.org
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": 180001, "attempt_start_ts": 1480470053746820,
"parent_rev": "7e6c15e808bfad6d7642aab19d88160542553a78", "commit_rev":
"e5d98cac7b82ff7dc4967e61cfde4380db52bc33"}
Message was sent while issue was closed.
Description was changed from ========== Create FakeProviders for NativeAppMetadata and NativeAppManager These 2 classes will be used instead of Mocks for all tests where a basic NativeApp* implementation is needed. BUG=662573 ========== to ========== Create FakeProviders for NativeAppMetadata and NativeAppManager These 2 classes will be used instead of Mocks for all tests where a basic NativeApp* implementation is needed. BUG=662573 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Create FakeProviders for NativeAppMetadata and NativeAppManager These 2 classes will be used instead of Mocks for all tests where a basic NativeApp* implementation is needed. BUG=662573 ========== to ========== Create FakeProviders for NativeAppMetadata and NativeAppManager These 2 classes will be used instead of Mocks for all tests where a basic NativeApp* implementation is needed. BUG=662573 Committed: https://crrev.com/361b7f0fcda579b98df8e77090ff994bebcdd4cc Cr-Commit-Position: refs/heads/master@{#435116} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/361b7f0fcda579b98df8e77090ff994bebcdd4cc Cr-Commit-Position: refs/heads/master@{#435116} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
