Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2916473002:
[ObjC ARC] Converts ios/web:web to ARC. (Closed)
Description[ObjC ARC] Converts ios/web:web to ARC.
Automatically generated ARCMigrate commit
Notable issues:None
BUG=624363
TEST=None
Review-Url: https://codereview.chromium.org/2916473002
Cr-Commit-Position: refs/heads/master@{#480083}
Committed: https://chromium.googlesource.com/chromium/src/+/95903ec2a23312d86a8c26cdcf2721ce8b8f5d66
Patch Set 1 #Patch Set 2 : Fix unit test memory issue. #Patch Set 3 : Tweaks to unit test memory management #
Total comments: 5
Patch Set 4 : Remove CRWWebController from this CL #Patch Set 5 : Fix web integration tests #Patch Set 6 : Removing trivial boilerplate changes for a separate CL #Patch Set 7 : Fix ios web unit tests #
Total comments: 3
Patch Set 8 : Adoption of NS_VALID_UNTIL_END_OF_SCOPE #
Total comments: 12
Patch Set 9 : Tweaks to autorelease pool #
Total comments: 24
Patch Set 10 : Review feedback and change away from NSAutoreleasePool #
Total comments: 9
Patch Set 11 : Fix failing unit test #Patch Set 12 : (Rebase) #Patch Set 13 : Nits #Patch Set 14 : Fixing silly mistake #Messages
Total messages: 109 (75 generated)
The CQ bit was checked by peterlaurens@chromium.org 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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by peterlaurens@chromium.org 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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by peterlaurens@chromium.org 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: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by peterlaurens@chromium.org 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...
peterlaurens@chromium.org changed reviewers: + rohitrao@chromium.org, stkhapugin@chromium.org
Hello, This is an ARC conversion of ios/web. stkhapugin: For ARC conversion rohitrao: For ownership There are some more complex parts that needed manual intervention - I've called the critical ones out with comments. Thanks! https://codereview.chromium.org/2916473002/diff/40001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm (right): https://codereview.chromium.org/2916473002/diff/40001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm:168: EXPECT_NSEQ(@"App-ID", delegate->GetAppId()); Moving to ARC caused some objects to be deallocated in a different order than that which they were before. The problem here is that the web threads were being deallocated before the final autorelease pool that would release some higher-level objects. Those higher level objects would check which thread they were running on and they would fail to look up the UI web thread and DCHECK (because the web thread impl had been destroyed). https://codereview.chromium.org/2916473002/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2916473002/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4779: methodCallFunction(self, selector); This is one manual piece of conversion work that might be worth discussing. We can't easily change this code to not store selectors, that would be a large architectural change. The comment here explains what is happening, but we could also replace this with a pragma to turn off the selector warning. I feel that this is more explicit than that. Open to discussion! https://codereview.chromium.org/2916473002/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_script_message_router.mm (right): https://codereview.chromium.org/2916473002/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_script_message_router.mm:109: // ARC TODO: Discuss the lifecycle of handler here in ARC. This is something that needs discussion. We were keeping handler alive until the end of the runloop at least here, but we can't do anything like this in ARC. I need to dig in deeper through code analysis to try and understand the thinking here, because the comment is a bit cryptic to me. Would appreciate any guidance :-). https://codereview.chromium.org/2916473002/diff/40001/ios/web/web_state/web_s... File ios/web/web_state/web_state_impl.mm (right): https://codereview.chromium.org/2916473002/diff/40001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.mm:169: web_controller_.reset(web_controller); The converter created this, but can we just assign web_controller and get rid of .reset?
peterlaurens@chromium.org changed reviewers: + eugenebut@chromium.org - rohitrao@chromium.org
+ eugenebut as an owner: I think this is closer to his ownership than rohitrao's. Swapping reviewers! Thanks!
pkl@chromium.org changed reviewers: + pkl@chromium.org
native app launcher related code will be deleted within the next few weeks. https://codereview.chromium.org/2916473002/diff/40001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm (right): https://codereview.chromium.org/2916473002/diff/40001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm:126: TEST_F(NativeAppNavigationControllerTest, NativeAppInfoBar) { You can ignore this file since it will be deleted by https://codereview.chromium.org/2897213002/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/06/01 02:50:53, pkl -pls.ping.after.24h. wrote: > native app launcher related code will be deleted within the next few weeks. > > https://codereview.chromium.org/2916473002/diff/40001/ios/chrome/browser/nati... > File > ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm > (right): > > https://codereview.chromium.org/2916473002/diff/40001/ios/chrome/browser/nati... > ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm:126: > TEST_F(NativeAppNavigationControllerTest, NativeAppInfoBar) { > You can ignore this file since it will be deleted by > https://codereview.chromium.org/2897213002/ Could you please split this CL into smaller pieces: - CRWWebController - CRWWebControllerContainerView - files which only add |#if !defined(__has_feature) || !__has_feature(objc_arc)| all in one CL - the rest of the files, ideally split into a few groups I think we should also be prepared for reverts, especially for CL which converts CRWWebController :)
Thank you for doing this, but could you please break this down into smaller CLs? This is way too big to read in one sitting.
On 2017/06/01 09:55:29, stkhapugin wrote: > Thank you for doing this, but could you please break this down into smaller CLs? > This is way too big to read in one sitting. Absolutely! The majority of the change here is in the CRWWebController, it's more than all of the other changes put together. How about I break that out as a separate CL... we also don't need to change native_app_navigation_controller_unittest according to Peter as that's going away soon. Thanks! - Peter
The CQ bit was checked by peterlaurens@chromium.org 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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Thanks for making this smaller! The trybot failure seems related to your change, could you please investigate?
The CQ bit was checked by peterlaurens@chromium.org 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...
On 2017/06/02 10:12:22, stkhapugin wrote: > Thanks for making this smaller! > > The trybot failure seems related to your change, could you please investigate? Yeah, sorry I should have commented! There are issues with certain tests due to the order in which objects are now deallocated, objective-C objects are being deallocated later in the runloop by the final autorelease pool, but because they're deallocated after the test harness classes have been destroyed a number of DCHECKs fail because they cannot ensure they're being deallocated on the WebUI thread. I believe I have a solution which involves creating an NSAutoreleasePool for the test harness. Works locally, let's see what the trybot thinks!
On 2017/06/02 17:48:40, PL wrote: > On 2017/06/02 10:12:22, stkhapugin wrote: > > Thanks for making this smaller! > > > > The trybot failure seems related to your change, could you please investigate? > > Yeah, sorry I should have commented! There are issues with certain tests due to > the order in which objects are now deallocated, objective-C objects are being > deallocated later in the runloop by the final autorelease pool, but because > they're deallocated after the test harness classes have been destroyed a number > of DCHECKs fail because they cannot ensure they're being deallocated on the > WebUI thread. > > I believe I have a solution which involves creating an NSAutoreleasePool for the > test harness. Works locally, let's see what the trybot thinks! Would you mind splitting this CL further? There are files which only add |#if !defined(__has_feature)|, and those are save to land altogether in one CL. But this CL also have files with actual changes (like native_app_navigation_controller_unittest.mm, crw_web_controller_container_view.mm, wk_web_view_security_util.mm), which we should try to land separately.
On 2017/06/02 17:59:22, Eugene But wrote: > On 2017/06/02 17:48:40, PL wrote: > > On 2017/06/02 10:12:22, stkhapugin wrote: > > > Thanks for making this smaller! > > > > > > The trybot failure seems related to your change, could you please > investigate? > > > > Yeah, sorry I should have commented! There are issues with certain tests due > to > > the order in which objects are now deallocated, objective-C objects are being > > deallocated later in the runloop by the final autorelease pool, but because > > they're deallocated after the test harness classes have been destroyed a > number > > of DCHECKs fail because they cannot ensure they're being deallocated on the > > WebUI thread. > > > > I believe I have a solution which involves creating an NSAutoreleasePool for > the > > test harness. Works locally, let's see what the trybot thinks! > Would you mind splitting this CL further? There are files which only add |#if > !defined(__has_feature)|, and those are save to land altogether in one CL. But > this CL also have files with actual changes (like > native_app_navigation_controller_unittest.mm, > crw_web_controller_container_view.mm, wk_web_view_security_util.mm), which we > should try to land separately. Absolutely! Let me pull out the ones with only boilerplate which should be an easy review :-). Will post another CL.
On 2017/06/02 18:00:52, PL wrote: > On 2017/06/02 17:59:22, Eugene But wrote: > > On 2017/06/02 17:48:40, PL wrote: > > > On 2017/06/02 10:12:22, stkhapugin wrote: > > > > Thanks for making this smaller! > > > > > > > > The trybot failure seems related to your change, could you please > > investigate? > > > > > > Yeah, sorry I should have commented! There are issues with certain tests due > > to > > > the order in which objects are now deallocated, objective-C objects are > being > > > deallocated later in the runloop by the final autorelease pool, but because > > > they're deallocated after the test harness classes have been destroyed a > > number > > > of DCHECKs fail because they cannot ensure they're being deallocated on the > > > WebUI thread. > > > > > > I believe I have a solution which involves creating an NSAutoreleasePool for > > the > > > test harness. Works locally, let's see what the trybot thinks! > > Would you mind splitting this CL further? There are files which only add |#if > > !defined(__has_feature)|, and those are save to land altogether in one CL. But > > this CL also have files with actual changes (like > > native_app_navigation_controller_unittest.mm, > > crw_web_controller_container_view.mm, wk_web_view_security_util.mm), which we > > should try to land separately. > > Absolutely! Let me pull out the ones with only boilerplate which should be an > easy review :-). Will post another CL. ios_chrome_unittests and ios_web_inttests are now succesful, but I think they this is failing in ios_web_unittests. Investigating now!
The CQ bit was checked by peterlaurens@chromium.org 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.
Ok! Trybot is happy, and this should also be a much smaller CL than originally. I think we're ready for another look, PTAL! Thanks! - PL https://codereview.chromium.org/2916473002/diff/120001/ios/chrome/browser/nat... File ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm (right): https://codereview.chromium.org/2916473002/diff/120001/ios/chrome/browser/nat... ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm:169: } I know pkl already mentioned this class is obsolete, but until it's removed I figured I would keep the test fixed in this way to avoid the errors from the unit tests. https://codereview.chromium.org/2916473002/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_script_message_router.mm (right): https://codereview.chromium.org/2916473002/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_script_message_router.mm:109: // ARC TODO: Discuss the lifecycle of handler here in ARC. Would like to discuss this! I'm not sure quite what the issue is here that we'd need to do this, and [[x retain] autorelease] isn't something we can directly bring to ARC. It seems the author wanted to keep this object alive for a notification. Is the original concern that removing the |handler| from |_handlers| would deallocate it and then it would not hear the notification that it had been removed? If so, we could consider a new strong local variable to |handler| and make sure it existed until the method returns. E.g.: id local_handler = handler; <remove object for key> local_handler; // ensure ARC does not release the object until after this point return Or alternatively, keep it alive for one more runloop like: dispatch_async(dispatch_get_main_queue(), ^{ handler; } Both of these are gross, but not too much worse than the original?
https://codereview.chromium.org/2916473002/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_script_message_router.mm (right): https://codereview.chromium.org/2916473002/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_script_message_router.mm:109: // ARC TODO: Discuss the lifecycle of handler here in ARC. On 2017/06/02 21:42:12, PL wrote: > Would like to discuss this! I'm not sure quite what the issue is here that we'd > need to do this, and [[x retain] autorelease] isn't something we can directly > bring to ARC. > > It seems the author wanted to keep this object alive for a notification. Is the > original concern that removing the |handler| from |_handlers| would deallocate > it and then it would not hear the notification that it had been removed? > > If so, we could consider a new strong local variable to |handler| and make sure > it existed until the method returns. E.g.: > > id local_handler = handler; > <remove object for key> > local_handler; // ensure ARC does not release the object until after this point > return > > Or alternatively, keep it alive for one more runloop like: > > dispatch_async(dispatch_get_main_queue(), ^{ > handler; > } > > Both of these are gross, but not too much worse than the original? AHA! I think this is a candidate for NS_VALID_UNTIL_END_OF_SCOPE.
The CQ bit was checked by peterlaurens@chromium.org 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...
https://codereview.chromium.org/2916473002/diff/140001/ios/chrome/browser/nat... File ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm (right): https://codereview.chromium.org/2916473002/diff/140001/ios/chrome/browser/nat... ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm:136: @autoreleasepool { How about using ScopedNSAutoreleasePool? This will prevent code indentation. https://codereview.chromium.org/2916473002/diff/140001/ios/web/test/web_int_t... File ios/web/test/web_int_test.h (right): https://codereview.chromium.org/2916473002/diff/140001/ios/web/test/web_int_t... ios/web/test/web_int_test.h:81: NSAutoreleasePool* rootPool_; root_pool_ per C++ Style Guide. But also, should we add this pool to web::WebTest instead? Doing so, may also fix NativeAppNavigationControllerTest https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller_container_view.h (right): https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller_container_view.h:43: delegate; // weak Remove |// weak|. https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/w... File ios/web/web_state/ui/wk_web_view_configuration_provider.h (right): https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/w... ios/web/web_state/ui/wk_web_view_configuration_provider.h:8: #import "base/mac/scoped_nsobject.h" Do we still need this include? https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/wk_w... File ios/web/web_state/wk_web_view_security_util.mm (right): https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/wk_w... ios/web/web_state/wk_web_view_security_util.mm:60: SecCertificateRef initialCert = (__bridge SecCertificateRef)certs[0]; s/initialCert/root_cert https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/wk_w... ios/web/web_state/wk_web_view_security_util.mm:92: CFArrayRef certificatesArray = (__bridge CFArrayRef)certs; Do you want to drop this local variable? It's hard to name it. But if you choose to keep it then s/certificatesArray/certificates_array
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/06/02 23:18:38, Eugene But wrote: > https://codereview.chromium.org/2916473002/diff/140001/ios/chrome/browser/nat... > File > ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm > (right): > > https://codereview.chromium.org/2916473002/diff/140001/ios/chrome/browser/nat... > ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm:136: > @autoreleasepool { > How about using ScopedNSAutoreleasePool? This will prevent code indentation. > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/test/web_int_t... > File ios/web/test/web_int_test.h (right): > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/test/web_int_t... > ios/web/test/web_int_test.h:81: NSAutoreleasePool* rootPool_; > root_pool_ per C++ Style Guide. But also, should we add this pool to > web::WebTest instead? Doing so, may also fix NativeAppNavigationControllerTest > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/c... > File ios/web/web_state/ui/crw_web_controller_container_view.h (right): > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_web_controller_container_view.h:43: delegate; // weak > Remove |// weak|. > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/w... > File ios/web/web_state/ui/wk_web_view_configuration_provider.h (right): > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/w... > ios/web/web_state/ui/wk_web_view_configuration_provider.h:8: #import > "base/mac/scoped_nsobject.h" > Do we still need this include? > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/wk_w... > File ios/web/web_state/wk_web_view_security_util.mm (right): > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/wk_w... > ios/web/web_state/wk_web_view_security_util.mm:60: SecCertificateRef initialCert > = (__bridge SecCertificateRef)certs[0]; > s/initialCert/root_cert > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/wk_w... > ios/web/web_state/wk_web_view_security_util.mm:92: CFArrayRef certificatesArray > = (__bridge CFArrayRef)certs; > Do you want to drop this local variable? It's hard to name it. But if you choose > to keep it then s/certificatesArray/certificates_array Looking into the trybot results.
The CQ bit was checked by peterlaurens@chromium.org 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.
The CQ bit was checked by peterlaurens@chromium.org 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...
Thanks! Running another dry run as there may be some tests I've missed here. PTAL! https://codereview.chromium.org/2916473002/diff/140001/ios/chrome/browser/nat... File ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm (right): https://codereview.chromium.org/2916473002/diff/140001/ios/chrome/browser/nat... ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm:136: @autoreleasepool { On 2017/06/02 23:18:37, Eugene But wrote: > How about using ScopedNSAutoreleasePool? This will prevent code indentation. I think the @autoreleasepool is supposed to be more performant, but moot point as I took your advice and hoisted it all into the superclass! https://codereview.chromium.org/2916473002/diff/140001/ios/web/test/web_int_t... File ios/web/test/web_int_test.h (right): https://codereview.chromium.org/2916473002/diff/140001/ios/web/test/web_int_t... ios/web/test/web_int_test.h:81: NSAutoreleasePool* rootPool_; On 2017/06/02 23:18:37, Eugene But wrote: > root_pool_ per C++ Style Guide. But also, should we add this pool to > web::WebTest instead? Doing so, may also fix NativeAppNavigationControllerTest Done! This is a great suggestion, thanks! https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller_container_view.h (right): https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller_container_view.h:43: delegate; // weak On 2017/06/02 23:18:37, Eugene But wrote: > Remove |// weak|. Done! https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/w... File ios/web/web_state/ui/wk_web_view_configuration_provider.h (right): https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/w... ios/web/web_state/ui/wk_web_view_configuration_provider.h:8: #import "base/mac/scoped_nsobject.h" On 2017/06/02 23:18:38, Eugene But wrote: > Do we still need this include? Done! Nope, thanks! https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/wk_w... File ios/web/web_state/wk_web_view_security_util.mm (right): https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/wk_w... ios/web/web_state/wk_web_view_security_util.mm:60: SecCertificateRef initialCert = (__bridge SecCertificateRef)certs[0]; On 2017/06/02 23:18:38, Eugene But wrote: > s/initialCert/root_cert Done! https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/wk_w... ios/web/web_state/wk_web_view_security_util.mm:92: CFArrayRef certificatesArray = (__bridge CFArrayRef)certs; On 2017/06/02 23:18:38, Eugene But wrote: > Do you want to drop this local variable? It's hard to name it. But if you choose > to keep it then s/certificatesArray/certificates_array Done! Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/06/05 23:26:19, PL wrote: > Thanks! > > Running another dry run as there may be some tests I've missed here. > > PTAL! > > https://codereview.chromium.org/2916473002/diff/140001/ios/chrome/browser/nat... > File > ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm > (right): > > https://codereview.chromium.org/2916473002/diff/140001/ios/chrome/browser/nat... > ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm:136: > @autoreleasepool { > On 2017/06/02 23:18:37, Eugene But wrote: > > How about using ScopedNSAutoreleasePool? This will prevent code indentation. > > I think the @autoreleasepool is supposed to be more performant, but moot point > as I took your advice and hoisted it all into the superclass! > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/test/web_int_t... > File ios/web/test/web_int_test.h (right): > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/test/web_int_t... > ios/web/test/web_int_test.h:81: NSAutoreleasePool* rootPool_; > On 2017/06/02 23:18:37, Eugene But wrote: > > root_pool_ per C++ Style Guide. But also, should we add this pool to > > web::WebTest instead? Doing so, may also fix NativeAppNavigationControllerTest > > Done! This is a great suggestion, thanks! > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/c... > File ios/web/web_state/ui/crw_web_controller_container_view.h (right): > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_web_controller_container_view.h:43: delegate; // weak > On 2017/06/02 23:18:37, Eugene But wrote: > > Remove |// weak|. > > Done! > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/w... > File ios/web/web_state/ui/wk_web_view_configuration_provider.h (right): > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/w... > ios/web/web_state/ui/wk_web_view_configuration_provider.h:8: #import > "base/mac/scoped_nsobject.h" > On 2017/06/02 23:18:38, Eugene But wrote: > > Do we still need this include? > > Done! Nope, thanks! > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/wk_w... > File ios/web/web_state/wk_web_view_security_util.mm (right): > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/wk_w... > ios/web/web_state/wk_web_view_security_util.mm:60: SecCertificateRef initialCert > = (__bridge SecCertificateRef)certs[0]; > On 2017/06/02 23:18:38, Eugene But wrote: > > s/initialCert/root_cert > > Done! > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/wk_w... > ios/web/web_state/wk_web_view_security_util.mm:92: CFArrayRef certificatesArray > = (__bridge CFArrayRef)certs; > On 2017/06/02 23:18:38, Eugene But wrote: > > Do you want to drop this local variable? It's hard to name it. But if you > choose > > to keep it then s/certificatesArray/certificates_array > > Done! Thanks! (Looking at simulator failure)
The CQ bit was checked by peterlaurens@chromium.org 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...
On 2017/06/06 18:23:53, PL wrote: > On 2017/06/05 23:26:19, PL wrote: > > Thanks! > > > > Running another dry run as there may be some tests I've missed here. > > > > PTAL! > > > > > https://codereview.chromium.org/2916473002/diff/140001/ios/chrome/browser/nat... > > File > > > ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm > > (right): > > > > > https://codereview.chromium.org/2916473002/diff/140001/ios/chrome/browser/nat... > > > ios/chrome/browser/native_app_launcher/native_app_navigation_controller_unittest.mm:136: > > @autoreleasepool { > > On 2017/06/02 23:18:37, Eugene But wrote: > > > How about using ScopedNSAutoreleasePool? This will prevent code indentation. > > > > I think the @autoreleasepool is supposed to be more performant, but moot point > > as I took your advice and hoisted it all into the superclass! > > > > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/test/web_int_t... > > File ios/web/test/web_int_test.h (right): > > > > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/test/web_int_t... > > ios/web/test/web_int_test.h:81: NSAutoreleasePool* rootPool_; > > On 2017/06/02 23:18:37, Eugene But wrote: > > > root_pool_ per C++ Style Guide. But also, should we add this pool to > > > web::WebTest instead? Doing so, may also fix > NativeAppNavigationControllerTest > > > > Done! This is a great suggestion, thanks! > > > > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/c... > > File ios/web/web_state/ui/crw_web_controller_container_view.h (right): > > > > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/c... > > ios/web/web_state/ui/crw_web_controller_container_view.h:43: delegate; // > weak > > On 2017/06/02 23:18:37, Eugene But wrote: > > > Remove |// weak|. > > > > Done! > > > > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/w... > > File ios/web/web_state/ui/wk_web_view_configuration_provider.h (right): > > > > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/ui/w... > > ios/web/web_state/ui/wk_web_view_configuration_provider.h:8: #import > > "base/mac/scoped_nsobject.h" > > On 2017/06/02 23:18:38, Eugene But wrote: > > > Do we still need this include? > > > > Done! Nope, thanks! > > > > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/wk_w... > > File ios/web/web_state/wk_web_view_security_util.mm (right): > > > > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/wk_w... > > ios/web/web_state/wk_web_view_security_util.mm:60: SecCertificateRef > initialCert > > = (__bridge SecCertificateRef)certs[0]; > > On 2017/06/02 23:18:38, Eugene But wrote: > > > s/initialCert/root_cert > > > > Done! > > > > > https://codereview.chromium.org/2916473002/diff/140001/ios/web/web_state/wk_w... > > ios/web/web_state/wk_web_view_security_util.mm:92: CFArrayRef > certificatesArray > > = (__bridge CFArrayRef)certs; > > On 2017/06/02 23:18:38, Eugene But wrote: > > > Do you want to drop this local variable? It's hard to name it. But if you > > choose > > > to keep it then s/certificatesArray/certificates_array > > > > Done! Thanks! > > (Looking at simulator failure) (Simulator failed fixed - seems like flake)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2916473002/diff/160001/ios/web/public/test/we... File ios/web/public/test/web_test.h (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/public/test/we... ios/web/public/test/web_test.h:61: id root_pool_; Using |id| makes this header an Objective-C header so now it has to be imported everywhere, instead of included. https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller_container_view.mm (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller_container_view.mm:156: - (CRWWebViewContentView*)webViewContentView { Should we synthesize this method instead? https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller_container_view.mm:169: - (id<CRWNativeContent>)nativeController { ditto https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller_container_view.mm:187: - (CRWContentView*)transientContentView { ditto https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller_container_view.mm:205: - (UIView*)toolbarContainerView { ditto https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller_container_view.mm:219: - (id<CRWWebControllerContainerViewDelegate>)delegate { Should we synthesize getter and setter instead? Same question for toolbarContainerView https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_view_content_view.mm (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_view_content_view.mm:105: - (UIScrollView*)scrollView { Should we synthesize these accessors instead? https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_script_message_router.mm (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_script_message_router.mm:32: - (WKUserContentController*)userContentController { Should we just synthesize this? https://codereview.chromium.org/2916473002/diff/160001/ios/web/webui/web_ui_m... File ios/web/webui/web_ui_mojo_inttest.mm (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/webui/web_ui_m... ios/web/webui/web_ui_mojo_inttest.mm:152: } nit: Keep a linebreak after |TearDown|?
This is awesome! I agree with Eugene's comments and left a couple more. Thank you so much for doing this! https://codereview.chromium.org/2916473002/diff/160001/ios/web/public/test/we... File ios/web/public/test/web_test.h (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/public/test/we... ios/web/public/test/web_test.h:61: id root_pool_; On 2017/06/12 06:12:51, Eugene But (OOO till June 11) wrote: > Using |id| makes this header an Objective-C header so now it has to be imported > everywhere, instead of included. Please don't create a bare objc pointer in a header. Instead, do one of the following: 1. This header is included only from ARC files. Then mark this as __strong/__weak explicitly. In case of __strong, add a compilation guard to the header, too. 2. This header is included from both ARC and non-ARC. Then: a) This is a strong -> make it scoped_nsobject b) This is a weak -> make it __unsafe_unretained with a comment //weak to change it to __weak once all the including files are built with ARC c) This actually needs to be a zeroing weak - use WeakNSObject https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/w... File ios/web/web_state/ui/wk_web_view_configuration_provider.h (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/w... ios/web/web_state/ui/wk_web_view_configuration_provider.h:52: WKWebViewConfiguration* configuration_; ditto - See comment for web_test.h about unannotated objc pointers in headers. https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/wk_w... File ios/web/web_state/wk_web_view_security_util.mm (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/wk_w... ios/web/web_state/wk_web_view_security_util.mm:57: SecCertificateRef cert = (__bridge SecCertificateRef)certs[i]; nit: I think this will be better if you use a local for certs[i] to improve readability.
Many thanks for your guidance! Another CL with a change to the use of NSAutoreleasePool is up, some more comments inline. PTAL Thanks! - PL https://codereview.chromium.org/2916473002/diff/160001/ios/web/public/test/we... File ios/web/public/test/web_test.h (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/public/test/we... ios/web/public/test/web_test.h:61: id root_pool_; On 2017/06/12 12:36:23, stkhapugin wrote: > On 2017/06/12 06:12:51, Eugene But (OOO till June 11) wrote: > > Using |id| makes this header an Objective-C header so now it has to be > imported > > everywhere, instead of included. > > Please don't create a bare objc pointer in a header. Instead, do one of the > following: > > 1. This header is included only from ARC files. Then mark this as > __strong/__weak explicitly. In case of __strong, add a compilation guard to the > header, too. > 2. This header is included from both ARC and non-ARC. Then: > a) This is a strong -> make it scoped_nsobject > b) This is a weak -> make it __unsafe_unretained with a comment //weak to > change it to __weak once all the including files are built with ARC > c) This actually needs to be a zeroing weak - use WeakNSObject Thanks both, this is great guidance. In the next revision I've actually changed this significantly removing this altogether, because: • web_test.mm is now ARC so can't use NSAutoreleasePool at all, and @autorelease can't span methods. • We still have a fundamental problem with some test objects being captured by an autorelease pool that's released after all the C++ objects are destroyed. • One alternative is to have a new superclass that never becomes ARCified, and that wraps SetUp and TearDown in an autorelease pool. But this feels very heavy. • My proposed solution (in the revision) uses ARC autoreleasepool blocks instead, although it's needed in three places. I recommend that we do this, and file a bug on the gtest team(?) to have SetUp and TearDown to have its own pool (the Mac PlatformTest class already has a pool wrapping an entire test suite, so doesn't seem a stretch to add one per test). https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller_container_view.mm (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller_container_view.mm:156: - (CRWWebViewContentView*)webViewContentView { On 2017/06/12 06:12:51, Eugene But wrote: > Should we synthesize this method instead? Yes! Done! https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller_container_view.mm:169: - (id<CRWNativeContent>)nativeController { On 2017/06/12 06:12:51, Eugene But wrote: > ditto Done! https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller_container_view.mm:187: - (CRWContentView*)transientContentView { On 2017/06/12 06:12:51, Eugene But wrote: > ditto Done! https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller_container_view.mm:205: - (UIView*)toolbarContainerView { On 2017/06/12 06:12:51, Eugene But wrote: > ditto Done! https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller_container_view.mm:219: - (id<CRWWebControllerContainerViewDelegate>)delegate { On 2017/06/12 06:12:52, Eugene But wrote: > Should we synthesize getter and setter instead? Same question for > toolbarContainerView Done! We can do it easily for delegate, toolbarContainerView will still need to keep its overridden method as it's more than just a simple set of an object. https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_view_content_view.mm (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_view_content_view.mm:105: - (UIScrollView*)scrollView { On 2017/06/12 06:12:52, Eugene But wrote: > Should we synthesize these accessors instead? Yes! Done! https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_script_message_router.mm (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_script_message_router.mm:32: - (WKUserContentController*)userContentController { On 2017/06/12 06:12:52, Eugene But wrote: > Should we just synthesize this? Done! https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/w... File ios/web/web_state/ui/wk_web_view_configuration_provider.h (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/ui/w... ios/web/web_state/ui/wk_web_view_configuration_provider.h:52: WKWebViewConfiguration* configuration_; On 2017/06/12 12:36:23, stkhapugin wrote: > ditto - See comment for web_test.h about unannotated objc pointers in headers. Understood, thanks! I just reverted this back to how it was originally with the scoped objects as the header was imported by non-arc code. https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/wk_w... File ios/web/web_state/wk_web_view_security_util.mm (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/wk_w... ios/web/web_state/wk_web_view_security_util.mm:57: SecCertificateRef cert = (__bridge SecCertificateRef)certs[i]; On 2017/06/12 12:36:23, stkhapugin wrote: > nit: I think this will be better if you use a local for certs[i] to improve > readability. Thanks! I think I need a bit more guidance here, I'm not sure how to break this down further as |cert| is a local variable for certs[i]. What am I missing :-) ? https://codereview.chromium.org/2916473002/diff/160001/ios/web/webui/web_ui_m... File ios/web/webui/web_ui_mojo_inttest.mm (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/webui/web_ui_m... ios/web/webui/web_ui_mojo_inttest.mm:152: } On 2017/06/12 06:12:52, Eugene But wrote: > nit: Keep a linebreak after |TearDown|? Oh yes, thanks!
The CQ bit was checked by peterlaurens@chromium.org 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...
lgtm! Thanks for converting this. https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller_container_view.mm (right): https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller_container_view.mm:108: @property(weak, nonatomic, readonly) CRWWebViewProxyImpl* contentViewProxy; Really nit: (nonatomic, weak, readonly) https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_script_message_router.h (right): https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_script_message_router.h:17: @property(weak, nonatomic, readonly) Really nit: (nonatomic, weak, readonly) https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/web_... File ios/web/web_state/web_view_internal_creation_util.mm (right): https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/web_... ios/web/web_state/web_view_internal_creation_util.mm:70: void* associatedObjectKey = (__bridge void*)context_menu_controller; Sorry, missed in the previous round s/associatedObjectKey/associated_object_key https://codereview.chromium.org/2916473002/diff/180001/ios/web/webui/web_ui_m... File ios/web/webui/web_ui_mojo_inttest.mm (right): https://codereview.chromium.org/2916473002/diff/180001/ios/web/webui/web_ui_m... ios/web/webui/web_ui_mojo_inttest.mm:178: @autoreleasepool { Would ScopedAutoreleasePool work here to avoid indentations?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/wk_w... File ios/web/web_state/wk_web_view_security_util.mm (right): https://codereview.chromium.org/2916473002/diff/160001/ios/web/web_state/wk_w... ios/web/web_state/wk_web_view_security_util.mm:57: SecCertificateRef cert = (__bridge SecCertificateRef)certs[i]; On 2017/06/14 00:31:11, PL wrote: > On 2017/06/12 12:36:23, stkhapugin wrote: > > nit: I think this will be better if you use a local for certs[i] to improve > > readability. > > Thanks! I think I need a bit more guidance here, I'm not sure how to break this > down further as |cert| is a local variable for certs[i]. What am I missing :-) ? Oh, sorry, my comment is indeed confusing. I meant - create a local variable that's not an array, then cast it separately, so something like: id tmp_cert = certs[i]; SecCertificateRef cert = (__bridge SecCertiifcateRef)tmp_cert; But looking at this, this is even uglier, so disregard this! https://codereview.chromium.org/2916473002/diff/180001/ios/web/webui/web_ui_m... File ios/web/webui/web_ui_mojo_inttest.mm (right): https://codereview.chromium.org/2916473002/diff/180001/ios/web/webui/web_ui_m... ios/web/webui/web_ui_mojo_inttest.mm:178: @autoreleasepool { On 2017/06/14 01:27:59, Eugene But wrote: > Would ScopedAutoreleasePool work here to avoid indentations? We should probably not use ScopedAutoreleasePool and use @autoreleasepool instead. If the identations bother you, maybe we could not indent this specifically, like with namespace{} ?
https://codereview.chromium.org/2916473002/diff/180001/ios/web/webui/web_ui_m... File ios/web/webui/web_ui_mojo_inttest.mm (right): https://codereview.chromium.org/2916473002/diff/180001/ios/web/webui/web_ui_m... ios/web/webui/web_ui_mojo_inttest.mm:178: @autoreleasepool { On 2017/06/14 17:00:59, stkhapugin wrote: > On 2017/06/14 01:27:59, Eugene But wrote: > > Would ScopedAutoreleasePool work here to avoid indentations? > > We should probably not use ScopedAutoreleasePool and use @autoreleasepool > instead. If the identations bother you, maybe we could not indent this > specifically, like with namespace{} ? Indentation is fine. Just out of curiosity, why we should avoid ScopedAutoreleasePool?
On 2017/06/14 23:10:18, Eugene But wrote: > https://codereview.chromium.org/2916473002/diff/180001/ios/web/webui/web_ui_m... > File ios/web/webui/web_ui_mojo_inttest.mm (right): > > https://codereview.chromium.org/2916473002/diff/180001/ios/web/webui/web_ui_m... > ios/web/webui/web_ui_mojo_inttest.mm:178: @autoreleasepool { > On 2017/06/14 17:00:59, stkhapugin wrote: > > On 2017/06/14 01:27:59, Eugene But wrote: > > > Would ScopedAutoreleasePool work here to avoid indentations? > > > > We should probably not use ScopedAutoreleasePool and use @autoreleasepool > > instead. If the identations bother you, maybe we could not indent this > > specifically, like with namespace{} ? > Indentation is fine. Just out of curiosity, why we should avoid > ScopedAutoreleasePool? For the sim failure: There is something strange going on with WeakNSObject under ARC, I think it has different behavior (does not release immediately in the same scope) which is a bit scary. I think there's a way around it using __weak local variables, but will also try and document the strangeness.
The CQ bit was checked by peterlaurens@chromium.org 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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by peterlaurens@chromium.org 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 checked by peterlaurens@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller_container_view.mm (right): https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller_container_view.mm:108: @property(weak, nonatomic, readonly) CRWWebViewProxyImpl* contentViewProxy; On 2017/06/14 01:27:59, Eugene But wrote: > Really nit: (nonatomic, weak, readonly) Done! https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_script_message_router.h (right): https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_script_message_router.h:17: @property(weak, nonatomic, readonly) On 2017/06/14 01:27:59, Eugene But wrote: > Really nit: (nonatomic, weak, readonly) Done! https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/web_... File ios/web/web_state/web_view_internal_creation_util.mm (right): https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/web_... ios/web/web_state/web_view_internal_creation_util.mm:70: void* associatedObjectKey = (__bridge void*)context_menu_controller; On 2017/06/14 01:27:59, Eugene But wrote: > Sorry, missed in the previous round > s/associatedObjectKey/associated_object_key Done! Sorry, old habits!
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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by peterlaurens@chromium.org 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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by peterlaurens@chromium.org 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...
On 2017/06/15 22:51:32, PL wrote: > Thanks! > > https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/ui/c... > File ios/web/web_state/ui/crw_web_controller_container_view.mm (right): > > https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_web_controller_container_view.mm:108: @property(weak, > nonatomic, readonly) CRWWebViewProxyImpl* contentViewProxy; > On 2017/06/14 01:27:59, Eugene But wrote: > > Really nit: (nonatomic, weak, readonly) > > Done! > > https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/ui/c... > File ios/web/web_state/ui/crw_wk_script_message_router.h (right): > > https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_wk_script_message_router.h:17: @property(weak, > nonatomic, readonly) > On 2017/06/14 01:27:59, Eugene But wrote: > > Really nit: (nonatomic, weak, readonly) > > Done! > > https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/web_... > File ios/web/web_state/web_view_internal_creation_util.mm (right): > > https://codereview.chromium.org/2916473002/diff/180001/ios/web/web_state/web_... > ios/web/web_state/web_view_internal_creation_util.mm:70: void* > associatedObjectKey = (__bridge void*)context_menu_controller; > On 2017/06/14 01:27:59, Eugene But wrote: > > Sorry, missed in the previous round > > s/associatedObjectKey/associated_object_key > > Done! Sorry, old habits! (Investigating trybot failure, I'm not able to reproduce it locally).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by stkhapugin@chromium.org 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...
Note that "compile (without patch)" also failed. I've sent it for a second dry run, let's see if you were just unlucky.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/16 14:01:33, stkhapugin wrote: > Note that "compile (without patch)" also failed. I've sent it for a second dry > run, let's see if you were just unlucky. I think it was just luck (got rebased on top of a problem that was quickly fixed?). Let's get this in!
The CQ bit was checked by peterlaurens@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stkhapugin@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2916473002/#ps260001 (title: "Fixing silly mistake")
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": 260001, "attempt_start_ts": 1497632801527740,
"parent_rev": "aa3f1ca32be88440c7f71f0956e5427037b36123", "commit_rev":
"95903ec2a23312d86a8c26cdcf2721ce8b8f5d66"}
Message was sent while issue was closed.
Description was changed from ========== [ObjC ARC] Converts ios/web:web to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None ========== to ========== [ObjC ARC] Converts ios/web:web to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None Review-Url: https://codereview.chromium.org/2916473002 Cr-Commit-Position: refs/heads/master@{#480083} Committed: https://chromium.googlesource.com/chromium/src/+/95903ec2a23312d86a8c26cdcf27... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/95903ec2a23312d86a8c26cdcf27...
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/2948673002/ by gchatz@chromium.org. The reason for reverting is: Causes SettingsTestCase/testClearCookies to crash with error: FATAL:wk_web_view_configuration_provider.mm(111)] Check failed: !weak_router.
Message was sent while issue was closed.
On 2017/06/19 21:11:03, gchatz wrote: > A revert of this CL (patchset #14 id:260001) has been created in > https://codereview.chromium.org/2948673002/ by mailto:gchatz@chromium.org. > > The reason for reverting is: Causes SettingsTestCase/testClearCookies to crash > with error: > FATAL:wk_web_view_configuration_provider.mm(111)] Check failed: !weak_router. Thanks Greg, this is an autorelease under ARC problem tripping up a check that assumes a weak object is not put in the autoreleasepool. I will amend the test and then re-commit, thanks!
Message was sent while issue was closed.
On 2017/06/19 21:13:14, PL wrote: > On 2017/06/19 21:11:03, gchatz wrote: > > A revert of this CL (patchset #14 id:260001) has been created in > > https://codereview.chromium.org/2948673002/ by mailto:gchatz@chromium.org. > > > > The reason for reverting is: Causes SettingsTestCase/testClearCookies to crash > > with error: > > FATAL:wk_web_view_configuration_provider.mm(111)] Check failed: !weak_router. > > Thanks Greg, this is an autorelease under ARC problem tripping up a check that > assumes a weak object is not put in the autoreleasepool. > > I will amend the test and then re-commit, thanks! The fix for the test is here: https://codereview.chromium.org/2949673002/ I will file a new CL for this issue. |
