Instead of subclassing partial implementations like //components/signin/ios/browser/ios_sign_in_client.h //components/autofill/ios/browser/autofill_client_ios.h Create helper classes and extract common code instead.
+sdefresne@, this issue arose from https://chromium-review.googlesource.com/c/chromium/src/+/705636. Do you agree with me that composition is more appropriate than inheritance here (https://chromium-review.googlesource.com/c/chromium/src/+/705636#message-d17966d37bed5010ed7ad6b7b5db27da42938f95)?
Yes, I think composition is better.
SGTM. I'll make these changes soon.
Thanks!
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1f4edf555f5a9c6d394796b0c5c53df364537250 commit 1f4edf555f5a9c6d394796b0c5c53df364537250 Author: John Z Wu <jzw@chromium.org> Date: Thu Dec 07 19:46:50 2017 Remove AutofillClientIOS and refactor common code to util class. WebViewAutofillClientIOS and ChromeAutofillClientIOS should directly inherit from AutofillClient and share code through helpers. This is the preferred way to share code between //ios/web_view and //ios/chrome. Bug: 774517 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I52b55a03b3bb2038cb9af276a7ee7a208a469dcd Reviewed-on: https://chromium-review.googlesource.com/804474 Commit-Queue: John Wu <jzw@chromium.org> Reviewed-by: Mike Dougherty <michaeldo@chromium.org> Reviewed-by: Moe Ahmadi <mahmadi@chromium.org> Cr-Commit-Position: refs/heads/master@{#522511} [modify] https://crrev.com/1f4edf555f5a9c6d394796b0c5c53df364537250/components/autofill/ios/browser/BUILD.gn [delete] https://crrev.com/e4663906f13b834159b4b15e0798d52e6c04b68d/components/autofill/ios/browser/autofill_client_ios.mm [add] https://crrev.com/1f4edf555f5a9c6d394796b0c5c53df364537250/components/autofill/ios/browser/autofill_util.h [add] https://crrev.com/1f4edf555f5a9c6d394796b0c5c53df364537250/components/autofill/ios/browser/autofill_util.mm [modify] https://crrev.com/1f4edf555f5a9c6d394796b0c5c53df364537250/ios/chrome/browser/ui/autofill/chrome_autofill_client_ios.h [modify] https://crrev.com/1f4edf555f5a9c6d394796b0c5c53df364537250/ios/chrome/browser/ui/autofill/chrome_autofill_client_ios.mm [modify] https://crrev.com/1f4edf555f5a9c6d394796b0c5c53df364537250/ios/web_view/BUILD.gn [modify] https://crrev.com/1f4edf555f5a9c6d394796b0c5c53df364537250/ios/web_view/internal/autofill/cwv_autofill_controller.mm [rename] https://crrev.com/1f4edf555f5a9c6d394796b0c5c53df364537250/ios/web_view/internal/autofill/web_view_autofill_client_ios.h [add] https://crrev.com/1f4edf555f5a9c6d394796b0c5c53df364537250/ios/web_view/internal/autofill/web_view_autofill_client_ios.mm
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4140ab217e1ca1bec0c4b4d1b148f3361eb3a03e commit 4140ab217e1ca1bec0c4b4d1b148f3361eb3a03e Author: John Z Wu <jzw@chromium.org> Date: Sat Dec 09 00:19:23 2017 Introduce a helper that will be used by embedders of SigninClient to delay callbacks when network is offline and fire them when online. Both //ios/chrome and //ios/web_view needs to subclass SigninClient, and this helper will be used to share code between them. Bug: 774517 Change-Id: I1da0e5163ee65f0287483a022120ebc51045cd8b Reviewed-on: https://chromium-review.googlesource.com/807328 Reviewed-by: Colin Blundell <blundell@chromium.org> Reviewed-by: Mike Dougherty <michaeldo@chromium.org> Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: John Wu <jzw@chromium.org> Cr-Commit-Position: refs/heads/master@{#522932} [modify] https://crrev.com/4140ab217e1ca1bec0c4b4d1b148f3361eb3a03e/components/signin/ios/browser/BUILD.gn [modify] https://crrev.com/4140ab217e1ca1bec0c4b4d1b148f3361eb3a03e/components/signin/ios/browser/ios_signin_client.cc [modify] https://crrev.com/4140ab217e1ca1bec0c4b4d1b148f3361eb3a03e/components/signin/ios/browser/ios_signin_client.h [add] https://crrev.com/4140ab217e1ca1bec0c4b4d1b148f3361eb3a03e/components/signin/ios/browser/wait_for_network_callback_helper.cc [add] https://crrev.com/4140ab217e1ca1bec0c4b4d1b148f3361eb3a03e/components/signin/ios/browser/wait_for_network_callback_helper.h [add] https://crrev.com/4140ab217e1ca1bec0c4b4d1b148f3361eb3a03e/components/signin/ios/browser/wait_for_network_callback_helper_unittest.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bda65f4e1fceadb745a005f14c36886be4730cd0 commit bda65f4e1fceadb745a005f14c36886be4730cd0 Author: John Z Wu <jzw@chromium.org> Date: Tue Dec 12 16:46:39 2017 Remove IOSSigninClient and have IOSWebViewSigninClient and IOSChromeSigninClient directly inherit from SigninClient. Partial implementations are not preferred when sharing code between //ios/chrome and //ios/web_view. Instead, convert to using helper classes (using composition) wherever possible. Bug: 774517 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I0fa0ad558ee518b0eefb110f34f9480d69b59a6a Reviewed-on: https://chromium-review.googlesource.com/792252 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Reviewed-by: Mike Dougherty <michaeldo@chromium.org> Reviewed-by: Colin Blundell <blundell@chromium.org> Commit-Queue: John Wu <jzw@chromium.org> Cr-Commit-Position: refs/heads/master@{#523451} [modify] https://crrev.com/bda65f4e1fceadb745a005f14c36886be4730cd0/components/signin/ios/browser/BUILD.gn [delete] https://crrev.com/1d6547d38876429830505493e4cd745629351ffe/components/signin/ios/browser/ios_signin_client.cc [delete] https://crrev.com/1d6547d38876429830505493e4cd745629351ffe/components/signin/ios/browser/ios_signin_client.h [modify] https://crrev.com/bda65f4e1fceadb745a005f14c36886be4730cd0/ios/chrome/browser/signin/ios_chrome_signin_client.h [modify] https://crrev.com/bda65f4e1fceadb745a005f14c36886be4730cd0/ios/chrome/browser/signin/ios_chrome_signin_client.mm [modify] https://crrev.com/bda65f4e1fceadb745a005f14c36886be4730cd0/ios/web_view/internal/signin/ios_web_view_signin_client.h [modify] https://crrev.com/bda65f4e1fceadb745a005f14c36886be4730cd0/ios/web_view/internal/signin/ios_web_view_signin_client.mm
I appreciate your work here, John! I think the code is definitely cleaner now :).
Comment 1 by blundell@chromium.org
, Oct 16 2017