Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2764773002:
Add CWVUserContentController which enables injecting JavaScripts. (Closed)
|
Created:
3 years, 9 months ago by Hiroshi Ichikawa Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews+web_chromium.org, ios-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd CWVUserContentController which enables injecting JavaScripts.
BUG=698656
Review-Url: https://codereview.chromium.org/2764773002
Cr-Commit-Position: refs/heads/master@{#459093}
Committed: https://chromium.googlesource.com/chromium/src/+/762b38b0d18bf44ad5f0381583f45c2598ab470d
Patch Set 1 #Patch Set 2 : Add license. #
Total comments: 91
Patch Set 3 : Apply new comments. #
Total comments: 4
Patch Set 4 : Apply review comments. #Patch Set 5 : Apply review comment. #Patch Set 6 : Apply review comments. #Patch Set 7 : Rebase. #Messages
Total messages: 36 (13 generated)
ichikawa@chromium.org changed reviewers: + eugenebut@chromium.org, michaeldo@chromium.org
Thanks for this CL, looks great overall! https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... File ios/web_view/internal/cwv_user_content_controller.mm (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:3: // found in the LICENSE file. Please add new line after license. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:13: __weak CWVWebViewConfiguration* _configuration; Please use properties here Ref: https://google.github.io/styleguide/objcguide.xml?showone=Properties#Properties https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:40: NSMutableString* joinedScript = [NSMutableString string]; please use alloc/init instead of "string" https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... File ios/web_view/internal/cwv_user_content_controller_internal.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller_internal.h:3: // found in the LICENSE file. please add empty line after license, before header guard https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller_internal.h:4: #ifndef IOS_WEB_VIEW_PUBLIC_CWV_USER_CONTENT_CONTROLLER_INTERNAL_H_ s/PUBLIC/INTERNAL IOS_WEB_VIEW_INTERNAL_CWV_USER_CONTENT_CONTROLLER_INTERNAL_H_ https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... File ios/web_view/internal/web_view_early_page_script_provider.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.h:35: void SetScript(NSString* script) { script_ = script; } Please document setter as script must be nonnull. This is documented as a requirement in web::WebClient on the GetEarlyPageScript method. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.h:38: WebViewEarlyPageScriptProvider(); please move to immediately after public: above https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.h:41: NSString* script_; Please use: base::scoped_nsobject<NSString> script_ and update setter above to use script_.reset(script) https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... File ios/web_view/internal/web_view_early_page_script_provider.mm (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.mm:28: DCHECK([NSThread isMainThread]); Why is this bound to the main thread? Should we DCHECK against one of the WebThread constants instead? If so, please update comment in header. For example, DCHECK_CURRENTLY_ON(WebThread::UI) (macro defined in ios/web/public/web_thread.h) https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.mm:32: std::unique_ptr<WebViewEarlyPageScriptProvider>( please use base::MakeUnique instead of new https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.mm:43: WebViewEarlyPageScriptProvider::~WebViewEarlyPageScriptProvider() {} please use "= default;" instead of {} https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_user_content_controller.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_content_controller.h:11: // Allows injecting custom scripts into CWVWebView created with nit: re-wrap comment: move "the" to line 11 https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_content_controller.h:16: @property(copy, readonly) NSArray<CWVUserScript*>* userScripts; Please add nullability annotations to this newly created header. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_user_script.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_script.h:11: // loaded (i.e., at the same timing as please re-wrap this comment https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_script.h:13: __attribute__((visibility("default"))) I think we shuold remove the visibility attribute for now. It is missing from most of the other classes, and if we want to add them to all the classes, we should do that in a separate CL. If you want to add it here since this is a new header, please add to cwv_user_content_controller.h as well. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_script.h:17: @property(copy, readonly) NSString* source; Please add nullability annotations to this newly created header. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:23: @property(readonly, copy) CWVWebViewConfiguration* configuration; Is exposing this needed? https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view_configuration.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view_configuration.h:24: // The user content controller to associate with the web view. nit: I think "the web view" seems a bit arbitrary as this is the configuration object, not the web view itself. WDYT about: The user content controller to associate with web views created using this configuration.
Generally looks good https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... File ios/web_view/internal/cwv_user_content_controller.mm (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:23: _configuration = configuration; nit: Consider adding DCHECK(configuration) https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:30: [_userScripts addObject:userScript]; nit: Consider adding DCHECK(userScript) https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:39: - (void)updateEarlyPageScript { nit: Could you please add comment. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:42: [joinedScript appendString:script.source]; This can potentially be a performance problem. Do you want to file a bug and add TODO to refactor WebClient::GetEarlyPageScript so it returns NSArray<NSString*>*? https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... File ios/web_view/internal/cwv_user_content_controller_internal.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller_internal.h:14: (__weak CWVWebViewConfiguration*)configuration; Do you want to specify that |configuration| is nonnull? https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... File ios/web_view/internal/cwv_user_script.mm (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_script.mm:18: _source = source; _source = [source copy]; https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... File ios/web_view/internal/web_view_early_page_script_provider.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.h:35: void SetScript(NSString* script) { script_ = script; } script_ = [script copy]; https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... File ios/web_view/internal/web_view_early_page_script_provider.mm (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.mm:39: WebViewEarlyPageScriptProvider::WebViewEarlyPageScriptProvider() { WebViewEarlyPageScriptProvider::WebViewEarlyPageScriptProvider() : script_([[NSString ] init]) {} https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_user_content_controller.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_content_controller.h:3: // found in the LICENSE file. Add a linebreak https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_content_controller.h:16: @property(copy, readonly) NSArray<CWVUserScript*>* userScripts; nonatomic ? https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_user_script.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_script.h:17: @property(copy, readonly) NSString* source; nonatomic? https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_script.h:21: - (instancetype)initWithSource:(NSString*)source; Do you want to add nonnull to |source|? https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:23: @property(readonly, copy) CWVWebViewConfiguration* configuration; On 2017/03/21 16:12:35, michaeldo wrote: > Is exposing this needed? This matches our API spec. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:23: @property(readonly, copy) CWVWebViewConfiguration* configuration; nonatomic? https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:26: @property(nonatomic, readonly) UIView* view; Do we have a bug to remove this property? https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view_configuration.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view_configuration.h:29: @property(readonly, getter=isPersistent) BOOL persistent; nonatomic ?
https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:23: @property(readonly, copy) CWVWebViewConfiguration* configuration; On 2017/03/21 16:47:55, Eugene But wrote: > On 2017/03/21 16:12:35, michaeldo wrote: > > Is exposing this needed? > This matches our API spec. Of course, sorry.
https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... File ios/web_view/internal/cwv_user_content_controller.mm (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:3: // found in the LICENSE file. On 2017/03/21 16:12:34, michaeldo wrote: > Please add new line after license. Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:13: __weak CWVWebViewConfiguration* _configuration; On 2017/03/21 16:12:34, michaeldo wrote: > Please use properties here > > Ref: > https://google.github.io/styleguide/objcguide.xml?showone=Properties#Properties Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:23: _configuration = configuration; On 2017/03/21 16:47:55, Eugene But wrote: > nit: Consider adding DCHECK(configuration) Done. But I also added nonnull annotation for |configuration|, so maybe DCHECK is not necessary? Are there cases where nil is passed to nonnull argument? https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:30: [_userScripts addObject:userScript]; On 2017/03/21 16:47:55, Eugene But wrote: > nit: Consider adding DCHECK(userScript) Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:39: - (void)updateEarlyPageScript { On 2017/03/21 16:47:55, Eugene But wrote: > nit: Could you please add comment. Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:40: NSMutableString* joinedScript = [NSMutableString string]; On 2017/03/21 16:12:34, michaeldo wrote: > please use alloc/init instead of "string" Done. Just curious, why? Is it just a preferred style? https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:42: [joinedScript appendString:script.source]; On 2017/03/21 16:47:55, Eugene But wrote: > This can potentially be a performance problem. Do you want to file a bug and add > TODO to refactor WebClient::GetEarlyPageScript so it returns > NSArray<NSString*>*? Done. I believe it would be rare that it becomes a performance problem because this method is called just initially in most cases, though. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... File ios/web_view/internal/cwv_user_content_controller_internal.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller_internal.h:3: // found in the LICENSE file. On 2017/03/21 16:12:34, michaeldo wrote: > please add empty line after license, before header guard Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller_internal.h:4: #ifndef IOS_WEB_VIEW_PUBLIC_CWV_USER_CONTENT_CONTROLLER_INTERNAL_H_ On 2017/03/21 16:12:34, michaeldo wrote: > s/PUBLIC/INTERNAL > > IOS_WEB_VIEW_INTERNAL_CWV_USER_CONTENT_CONTROLLER_INTERNAL_H_ Oops. Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller_internal.h:14: (__weak CWVWebViewConfiguration*)configuration; On 2017/03/21 16:47:55, Eugene But wrote: > Do you want to specify that |configuration| is nonnull? Done. Added nonnull/nullable for other pointers in this class too. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... File ios/web_view/internal/cwv_user_script.mm (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_script.mm:18: _source = source; On 2017/03/21 16:47:55, Eugene But wrote: > _source = [source copy]; Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... File ios/web_view/internal/web_view_early_page_script_provider.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.h:35: void SetScript(NSString* script) { script_ = script; } On 2017/03/21 16:12:34, michaeldo wrote: > Please document setter as script must be nonnull. This is documented as a > requirement in web::WebClient on the GetEarlyPageScript method. I *documented* the requirement by adding _Nonnull annotation. Is it OK, to use _Nonnull annontation in Objective-C++ code? https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.h:35: void SetScript(NSString* script) { script_ = script; } On 2017/03/21 16:47:55, Eugene But wrote: > script_ = [script copy]; Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.h:38: WebViewEarlyPageScriptProvider(); On 2017/03/21 16:12:34, michaeldo wrote: > please move to immediately after public: above Shouldn't the constructor be private so that it can be only instantiated via FromBrowserState()? I followed this class as an example: https://cs.chromium.org/chromium/src/ios/web/web_state/ui/wk_web_view_configu... which also declares the constructor as private. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.h:41: NSString* script_; On 2017/03/21 16:12:34, michaeldo wrote: > Please use: > > base::scoped_nsobject<NSString> script_ > > and update setter above to use script_.reset(script) Done. What is the benefit using scoped_nsobject compared to a raw pointer? I believe raw pointers are also reference-counted under ARC. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... File ios/web_view/internal/web_view_early_page_script_provider.mm (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.mm:28: DCHECK([NSThread isMainThread]); On 2017/03/21 16:12:34, michaeldo wrote: > Why is this bound to the main thread? Should we DCHECK against one of the > WebThread constants instead? If so, please update comment in header. > > For example, DCHECK_CURRENTLY_ON(WebThread::UI) > > (macro defined in ios/web/public/web_thread.h) Well, I just copied implementation of: https://cs.chromium.org/chromium/src/ios/web/web_state/ui/wk_web_view_configu... I believe the check is needed because the class is not safe against access from multiple threads. Used DCHECK_CURRENTLY_ON(WebThread::UI) instead. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.mm:32: std::unique_ptr<WebViewEarlyPageScriptProvider>( On 2017/03/21 16:12:34, michaeldo wrote: > please use base::MakeUnique instead of new MakeUnique() causes an error in this case because the constructor of this class is currently private so MakeUnique() cannot access the constructor. Should I make the constructor public? https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.mm:39: WebViewEarlyPageScriptProvider::WebViewEarlyPageScriptProvider() { On 2017/03/21 16:47:55, Eugene But wrote: > WebViewEarlyPageScriptProvider::WebViewEarlyPageScriptProvider() : > script_([[NSString ] init]) {} Done. But just curious, why? Is [[NSString alloc] init] preferred to @""? https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.mm:43: WebViewEarlyPageScriptProvider::~WebViewEarlyPageScriptProvider() {} On 2017/03/21 16:12:34, michaeldo wrote: > please use "= default;" instead of {} Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_user_content_controller.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_content_controller.h:3: // found in the LICENSE file. On 2017/03/21 16:47:55, Eugene But wrote: > Add a linebreak Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_content_controller.h:11: // Allows injecting custom scripts into CWVWebView created with On 2017/03/21 16:12:35, michaeldo wrote: > nit: re-wrap comment: move "the" to line 11 Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_content_controller.h:16: @property(copy, readonly) NSArray<CWVUserScript*>* userScripts; On 2017/03/21 16:12:35, michaeldo wrote: > Please add nullability annotations to this newly created header. Done. But adding nonnull annotation caused a weird error when synthesizing it: ../../ios/web_view/internal/cwv_user_content_controller.mm:21:13: error: type of property 'userScripts' ('NSArray<CWVUserScript *> * _Nonnull') does not match type of instance variable '_userScripts' ('NSMutableArray<CWVUserScript *> *__strong') I manually wrote -userScripts method instead of synthesizing it for now. Let me know if you know a better solution. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_content_controller.h:16: @property(copy, readonly) NSArray<CWVUserScript*>* userScripts; On 2017/03/21 16:47:55, Eugene But wrote: > nonatomic ? Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_user_script.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_script.h:11: // loaded (i.e., at the same timing as On 2017/03/21 16:12:35, michaeldo wrote: > please re-wrap this comment Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_script.h:13: __attribute__((visibility("default"))) On 2017/03/21 16:12:35, michaeldo wrote: > I think we shuold remove the visibility attribute for now. It is missing from > most of the other classes, and if we want to add them to all the classes, we > should do that in a separate CL. If you want to add it here since this is a new > header, please add to cwv_user_content_controller.h as well. Sure. Reverted. I added this here because my test code using this this feature didn't work without this attribute. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_script.h:17: @property(copy, readonly) NSString* source; On 2017/03/21 16:12:35, michaeldo wrote: > Please add nullability annotations to this newly created header. Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_script.h:17: @property(copy, readonly) NSString* source; On 2017/03/21 16:47:55, Eugene But wrote: > nonatomic? Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_script.h:21: - (instancetype)initWithSource:(NSString*)source; On 2017/03/21 16:47:55, Eugene But wrote: > Do you want to add nonnull to |source|? Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:23: @property(readonly, copy) CWVWebViewConfiguration* configuration; On 2017/03/21 16:47:55, Eugene But wrote: > nonatomic? Done. BTW Many properties including this one are marked as (readonly, copy) in the API, but isn't it meaningless? Doesn't "copy" only affect the setter, not getter? https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:26: @property(nonatomic, readonly) UIView* view; On 2017/03/21 16:47:55, Eugene But wrote: > Do we have a bug to remove this property? I believe I have just forgotten to delete it. Filed crbug.com/703981. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view_configuration.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view_configuration.h:24: // The user content controller to associate with the web view. On 2017/03/21 16:12:35, michaeldo wrote: > nit: I think "the web view" seems a bit arbitrary as this is the configuration > object, not the web view itself. WDYT about: > > The user content controller to associate with web views created using this > configuration. > Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view_configuration.h:29: @property(readonly, getter=isPersistent) BOOL persistent; On 2017/03/21 16:47:55, Eugene But wrote: > nonatomic ? Done.
lgtm https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... File ios/web_view/internal/web_view_early_page_script_provider.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.h:38: WebViewEarlyPageScriptProvider(); On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > On 2017/03/21 16:12:34, michaeldo wrote: > > please move to immediately after public: above > > Shouldn't the constructor be private so that it can be only instantiated via > FromBrowserState()? I followed this class as an example: > https://cs.chromium.org/chromium/src/ios/web/web_state/ui/wk_web_view_configu... > which also declares the constructor as private. Private constructor seems like a better choice https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... File ios/web_view/internal/web_view_early_page_script_provider.mm (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.mm:39: WebViewEarlyPageScriptProvider::WebViewEarlyPageScriptProvider() { On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > On 2017/03/21 16:47:55, Eugene But wrote: > > WebViewEarlyPageScriptProvider::WebViewEarlyPageScriptProvider() : > > script_([[NSString ] init]) {} > > Done. But just curious, why? Is [[NSString alloc] init] preferred to @""? Sorry, my comment was mostly about using initialization list. There is probably no practical different between |[[NSString alloc] init]| and |@""|. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:23: @property(readonly, copy) CWVWebViewConfiguration* configuration; On 2017/03/22 04:52:53, Hiroshi Ichikawa wrote: > On 2017/03/21 16:47:55, Eugene But wrote: > > nonatomic? > > Done. > > BTW Many properties including this one are marked as (readonly, copy) in the > API, but isn't it meaningless? Doesn't "copy" only affect the setter, not > getter? Copy affects getter, and will generate code similar to this: - (CWVWebViewConfiguration*)configuration { return [[_configuration copy] autorelease]; } The main value for using |copy| is documenting the ownership https://codereview.chromium.org/2764773002/diff/40001/ios/web/public/web_clie... File ios/web/public/web_client.h (right): https://codereview.chromium.org/2764773002/diff/40001/ios/web/public/web_clie... ios/web/public/web_client.h:113: // improve performance. nit: please remove extra spaces before "improve" to align the comment
Thanks! lgtm with a few suggestions. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... File ios/web_view/internal/cwv_user_content_controller.mm (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:23: _configuration = configuration; On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > On 2017/03/21 16:47:55, Eugene But wrote: > > nit: Consider adding DCHECK(configuration) > > Done. But I also added nonnull annotation for |configuration|, so maybe DCHECK > is not necessary? Are there cases where nil is passed to nonnull argument? We commonly use DCHECKS where we expect a nonnull item, but that practice was prior to annotating with nonnnull. I don't think the DCHECK is necessary with the annotated method marked as nonnull, but I don't know if our team has any specific guidance on this case. Nullability annotations are referenced from the style guides at all from what I could see. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:40: NSMutableString* joinedScript = [NSMutableString string]; On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > On 2017/03/21 16:12:34, michaeldo wrote: > > please use alloc/init instead of "string" > > Done. Just curious, why? Is it just a preferred style? Correct, I suggested this because our style prefers alloc/init over new and string is a similar Class method shortcut initializer. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... File ios/web_view/internal/cwv_user_content_controller_internal.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller_internal.h:14: (__weak CWVWebViewConfiguration*)configuration; On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > On 2017/03/21 16:47:55, Eugene But wrote: > > Do you want to specify that |configuration| is nonnull? > > Done. Added nonnull/nullable for other pointers in this class too. Thanks for adding these, based on other classes, like UIView's header, this initializer should return "nonnull instancetype". I believe nullable is usually only used for initializers that could commonly return nil, like initWithCoder methods. Even though initializers have the "if (self)" check within their implementation, I believe nonnull is the correct annotation for the return value. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... File ios/web_view/internal/web_view_early_page_script_provider.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.h:35: void SetScript(NSString* script) { script_ = script; } On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > On 2017/03/21 16:12:34, michaeldo wrote: > > Please document setter as script must be nonnull. This is documented as a > > requirement in web::WebClient on the GetEarlyPageScript method. > > I *documented* the requirement by adding _Nonnull annotation. Is it OK, to use > _Nonnull annontation in Objective-C++ code? I found at least one other reference to _Nonnull so yes I think it is OK to use in terms of use in the repo. In terms of use in general, it looks like it is a compiler feature, here is a little more info: http://stackoverflow.com/questions/40892529/nullability-rules-for-c-objects-i... https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.h:38: WebViewEarlyPageScriptProvider(); On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > On 2017/03/21 16:12:34, michaeldo wrote: > > please move to immediately after public: above > > Shouldn't the constructor be private so that it can be only instantiated via > FromBrowserState()? I followed this class as an example: > https://cs.chromium.org/chromium/src/ios/web/web_state/ui/wk_web_view_configu... > which also declares the constructor as private. Sorry, you're right. I didn't see why it was private, but yes. It makes sense because it is created by FromBrowserState(). https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.h:41: NSString* script_; On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > On 2017/03/21 16:12:34, michaeldo wrote: > > Please use: > > > > base::scoped_nsobject<NSString> script_ > > > > and update setter above to use script_.reset(script) > > Done. What is the benefit using scoped_nsobject compared to a raw pointer? I > believe raw pointers are also reference-counted under ARC. I mostly suggested based on style and consistency. There may be other benefits, but I'm not sure of them all. However, these are good questions for Chromium-dev. I'm sure many people there would have deeper knowledge of all these types of things and could identify improvements/cleanups. Or at least suggest a modification to the comments within scoped_nsobject.h to be more explicit about some of this items with relation to newer compiler features like ARC. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... File ios/web_view/internal/web_view_early_page_script_provider.mm (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.mm:28: DCHECK([NSThread isMainThread]); On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > On 2017/03/21 16:12:34, michaeldo wrote: > > Why is this bound to the main thread? Should we DCHECK against one of the > > WebThread constants instead? If so, please update comment in header. > > > > For example, DCHECK_CURRENTLY_ON(WebThread::UI) > > > > (macro defined in ios/web/public/web_thread.h) > > Well, I just copied implementation of: > https://cs.chromium.org/chromium/src/ios/web/web_state/ui/wk_web_view_configu... > I believe the check is needed because the class is not safe against access from > multiple threads. > Used DCHECK_CURRENTLY_ON(WebThread::UI) instead. Got it, that makes sense, either is likely ok. However, maybe some of the WebKit calls require mainThread explicity which could be the reason to use NSThread instead. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.mm:32: std::unique_ptr<WebViewEarlyPageScriptProvider>( On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > On 2017/03/21 16:12:34, michaeldo wrote: > > please use base::MakeUnique instead of new > > MakeUnique() causes an error in this case because the constructor of this class > is currently private so MakeUnique() cannot access the constructor. Should I > make the constructor public? I see, no using new is OK then. However, is the std::unique_ptr needed at all, in the link above to wk_web_view_configuration_provider.mm, it doesn't have the std::unique_ptr wrapper https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_user_content_controller.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_content_controller.h:16: @property(copy, readonly) NSArray<CWVUserScript*>* userScripts; On 2017/03/22 04:52:53, Hiroshi Ichikawa wrote: > On 2017/03/21 16:12:35, michaeldo wrote: > > Please add nullability annotations to this newly created header. > > Done. But adding nonnull annotation caused a weird error when synthesizing it: > > ../../ios/web_view/internal/cwv_user_content_controller.mm:21:13: error: type of > property 'userScripts' ('NSArray<CWVUserScript *> * _Nonnull') does not match > type of instance variable '_userScripts' ('NSMutableArray<CWVUserScript *> > *__strong') > > I manually wrote -userScripts method instead of synthesizing it for now. Let me > know if you know a better solution. I believe if you make userScripts a property in the implementation file (also marked as nonnull) instead of an ivar, it should autosynthesize correctly. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_user_script.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_script.h:13: __attribute__((visibility("default"))) On 2017/03/22 04:52:53, Hiroshi Ichikawa wrote: > On 2017/03/21 16:12:35, michaeldo wrote: > > I think we shuold remove the visibility attribute for now. It is missing from > > most of the other classes, and if we want to add them to all the classes, we > > should do that in a separate CL. If you want to add it here since this is a > new > > header, please add to cwv_user_content_controller.h as well. > > Sure. Reverted. I added this here because my test code using this this feature > didn't work without this attribute. Understandable, thank you for creating the separate CL. https://codereview.chromium.org/2764773002/diff/40001/ios/web_view/internal/w... File ios/web_view/internal/web_view_early_page_script_provider.mm (right): https://codereview.chromium.org/2764773002/diff/40001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.mm:43: script_.reset((NSString * _Nonnull)[script copy]); Is this cast needed? I would expect this to be OK: script_.reset([script copy]);
https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... File ios/web_view/internal/cwv_user_content_controller.mm (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:23: _configuration = configuration; On 2017/03/22 23:03:32, michaeldo wrote: > On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > > On 2017/03/21 16:47:55, Eugene But wrote: > > > nit: Consider adding DCHECK(configuration) > > > > Done. But I also added nonnull annotation for |configuration|, so maybe DCHECK > > is not necessary? Are there cases where nil is passed to nonnull argument? > > We commonly use DCHECKS where we expect a nonnull item, but that practice was > prior to annotating with nonnnull. I don't think the DCHECK is necessary with > the annotated method marked as nonnull, but I don't know if our team has any > specific guidance on this case. Nullability annotations are referenced from the > style guides at all from what I could see. That's a good point. Maybe DCHECKs are indeed unnecessary :)
https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... File ios/web_view/internal/web_view_early_page_script_provider.mm (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.mm:32: std::unique_ptr<WebViewEarlyPageScriptProvider>( On 2017/03/22 23:03:32, michaeldo wrote: > On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > > On 2017/03/21 16:12:34, michaeldo wrote: > > > please use base::MakeUnique instead of new > > > > MakeUnique() causes an error in this case because the constructor of this > class > > is currently private so MakeUnique() cannot access the constructor. Should I > > make the constructor public? > > I see, no using new is OK then. However, is the std::unique_ptr needed at all, > in the link above to wk_web_view_configuration_provider.mm, it doesn't have the > std::unique_ptr wrapper That's true. But I found this comment: https://cs.chromium.org/chromium/src/base/supports_user_data.h?l=40&rcl=89365... So I assumed using the version which takes a raw pointer is not recommended. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.mm:39: WebViewEarlyPageScriptProvider::WebViewEarlyPageScriptProvider() { Thanks for clarification! https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:23: @property(readonly, copy) CWVWebViewConfiguration* configuration; On 2017/03/22 15:08:50, Eugene But wrote: > On 2017/03/22 04:52:53, Hiroshi Ichikawa wrote: > > On 2017/03/21 16:47:55, Eugene But wrote: > > > nonatomic? > > > > Done. > > > > BTW Many properties including this one are marked as (readonly, copy) in the > > API, but isn't it meaningless? Doesn't "copy" only affect the setter, not > > getter? > Copy affects getter, and will generate code similar to this: > - (CWVWebViewConfiguration*)configuration { > return [[_configuration copy] autorelease]; > } > The main value for using |copy| is documenting the ownership Hmm it looks it doesn't affect the getter as far as I tested (see the test code below). Am I missing something? https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Pr... also mentions only about the setter but not getter for |copy|. Maybe |retain| is better for documentation purpose if it actually doesn't copy? #import <Foundation/Foundation.h> @interface Foo : NSObject @property(nonatomic, readonly, copy) NSObject* bar; - (instancetype)init; @end @implementation Foo - (instancetype)init { self = [super init]; if (self) { _bar = [[NSObject alloc] init]; } return self; } @end int main() { Foo* foo = [[Foo alloc] init]; NSObject* bar1 = foo.bar; NSObject* bar2 = foo.bar; NSLog(@"bar1 = %@", bar1); // => bar1 = <NSObject: 0x7fce2f400430> NSLog(@"bar2 = %@", bar2); // => bar2 = <NSObject: 0x7fce2f400430> return 0; } https://codereview.chromium.org/2764773002/diff/40001/ios/web/public/web_clie... File ios/web/public/web_client.h (right): https://codereview.chromium.org/2764773002/diff/40001/ios/web/public/web_clie... ios/web/public/web_client.h:113: // improve performance. On 2017/03/22 15:08:50, Eugene But wrote: > nit: please remove extra spaces before "improve" to align the comment Done. https://codereview.chromium.org/2764773002/diff/40001/ios/web_view/internal/w... File ios/web_view/internal/web_view_early_page_script_provider.mm (right): https://codereview.chromium.org/2764773002/diff/40001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.mm:43: script_.reset((NSString * _Nonnull)[script copy]); On 2017/03/22 23:03:33, michaeldo wrote: > Is this cast needed? I would expect this to be OK: > script_.reset([script copy]); It worked, thanks. Done.
The CQ bit was checked by ichikawa@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeldo@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2764773002/#ps80001 (title: "Apply review comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ichikawa@chromium.org
https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... File ios/web_view/internal/cwv_user_content_controller.mm (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:23: _configuration = configuration; On 2017/03/22 23:09:50, Eugene But wrote: > On 2017/03/22 23:03:32, michaeldo wrote: > > On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > > > On 2017/03/21 16:47:55, Eugene But wrote: > > > > nit: Consider adding DCHECK(configuration) > > > > > > Done. But I also added nonnull annotation for |configuration|, so maybe > DCHECK > > > is not necessary? Are there cases where nil is passed to nonnull argument? > > > > We commonly use DCHECKS where we expect a nonnull item, but that practice was > > prior to annotating with nonnnull. I don't think the DCHECK is necessary with > > the annotated method marked as nonnull, but I don't know if our team has any > > specific guidance on this case. Nullability annotations are referenced from > the > > style guides at all from what I could see. > That's a good point. Maybe DCHECKs are indeed unnecessary :) Thanks. Deleted the DCHECKs. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller.mm:40: NSMutableString* joinedScript = [NSMutableString string]; On 2017/03/22 23:03:32, michaeldo wrote: > On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > > On 2017/03/21 16:12:34, michaeldo wrote: > > > please use alloc/init instead of "string" > > > > Done. Just curious, why? Is it just a preferred style? > > Correct, I suggested this because our style prefers alloc/init over new and > string is a similar Class method shortcut initializer. Acknowledged. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... File ios/web_view/internal/cwv_user_content_controller_internal.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/c... ios/web_view/internal/cwv_user_content_controller_internal.h:14: (__weak CWVWebViewConfiguration*)configuration; On 2017/03/22 23:03:32, michaeldo wrote: > On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > > On 2017/03/21 16:47:55, Eugene But wrote: > > > Do you want to specify that |configuration| is nonnull? > > > > Done. Added nonnull/nullable for other pointers in this class too. > > Thanks for adding these, based on other classes, like UIView's header, this > initializer should return "nonnull instancetype". I believe nullable is usually > only used for initializers that could commonly return nil, like initWithCoder > methods. Even though initializers have the "if (self)" check within their > implementation, I believe nonnull is the correct annotation for the return > value. Good to know, thanks. Done. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... File ios/web_view/internal/web_view_early_page_script_provider.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.h:35: void SetScript(NSString* script) { script_ = script; } On 2017/03/22 23:03:32, michaeldo wrote: > On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > > On 2017/03/21 16:12:34, michaeldo wrote: > > > Please document setter as script must be nonnull. This is documented as a > > > requirement in web::WebClient on the GetEarlyPageScript method. > > > > I *documented* the requirement by adding _Nonnull annotation. Is it OK, to use > > _Nonnull annontation in Objective-C++ code? > > I found at least one other reference to _Nonnull so yes I think it is OK to use > in terms of use in the repo. In terms of use in general, it looks like it is a > compiler feature, here is a little more info: > http://stackoverflow.com/questions/40892529/nullability-rules-for-c-objects-i... Acknowledged. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/internal/w... ios/web_view/internal/web_view_early_page_script_provider.h:41: NSString* script_; On 2017/03/22 23:03:32, michaeldo wrote: > On 2017/03/22 04:52:52, Hiroshi Ichikawa wrote: > > On 2017/03/21 16:12:34, michaeldo wrote: > > > Please use: > > > > > > base::scoped_nsobject<NSString> script_ > > > > > > and update setter above to use script_.reset(script) > > > > Done. What is the benefit using scoped_nsobject compared to a raw pointer? I > > believe raw pointers are also reference-counted under ARC. > > I mostly suggested based on style and consistency. There may be other benefits, > but I'm not sure of them all. However, these are good questions for > Chromium-dev. I'm sure many people there would have deeper knowledge of all > these types of things and could identify improvements/cleanups. Or at least > suggest a modification to the comments within scoped_nsobject.h to be more > explicit about some of this items with relation to newer compiler features like > ARC. Acknowledged. https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_user_content_controller.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_user_content_controller.h:16: @property(copy, readonly) NSArray<CWVUserScript*>* userScripts; On 2017/03/22 23:03:32, michaeldo wrote: > On 2017/03/22 04:52:53, Hiroshi Ichikawa wrote: > > On 2017/03/21 16:12:35, michaeldo wrote: > > > Please add nullability annotations to this newly created header. > > > > Done. But adding nonnull annotation caused a weird error when synthesizing it: > > > > ../../ios/web_view/internal/cwv_user_content_controller.mm:21:13: error: type > of > > property 'userScripts' ('NSArray<CWVUserScript *> * _Nonnull') does not match > > type of instance variable '_userScripts' ('NSMutableArray<CWVUserScript *> > > *__strong') > > > > I manually wrote -userScripts method instead of synthesizing it for now. Let > me > > know if you know a better solution. > > I believe if you make userScripts a property in the implementation file (also > marked as nonnull) instead of an ivar, it should autosynthesize correctly. Well, its type as a public property should be NSArray while its type as an ivar should be NSMutableArray. So I cannot omit ivar definition for this, I believe.
The CQ bit was checked by ichikawa@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeldo@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2764773002/#ps100001 (title: "Apply review comments.")
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: 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-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by ichikawa@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeldo@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2764773002/#ps120001 (title: "Rebase.")
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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by michaeldo@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": 120001, "attempt_start_ts": 1490282180468560,
"parent_rev": "0e90d09b207c43aa17afb52580d8fc899a49160c", "commit_rev":
"762b38b0d18bf44ad5f0381583f45c2598ab470d"}
Message was sent while issue was closed.
Description was changed from ========== Add CWVUserContentController which enables injecting JavaScripts. BUG=698656 ========== to ========== Add CWVUserContentController which enables injecting JavaScripts. BUG=698656 Review-Url: https://codereview.chromium.org/2764773002 Cr-Commit-Position: refs/heads/master@{#459093} Committed: https://chromium.googlesource.com/chromium/src/+/762b38b0d18bf44ad5f0381583f4... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/762b38b0d18bf44ad5f0381583f4...
Message was sent while issue was closed.
https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... File ios/web_view/public/cwv_web_view.h (right): https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... ios/web_view/public/cwv_web_view.h:23: @property(readonly, copy) CWVWebViewConfiguration* configuration; On 2017/03/23 02:18:40, Hiroshi Ichikawa wrote: > On 2017/03/22 15:08:50, Eugene But wrote: > > On 2017/03/22 04:52:53, Hiroshi Ichikawa wrote: > > > On 2017/03/21 16:47:55, Eugene But wrote: > > > > nonatomic? > > > > > > Done. > > > > > > BTW Many properties including this one are marked as (readonly, copy) in the > > > API, but isn't it meaningless? Doesn't "copy" only affect the setter, not > > > getter? > > Copy affects getter, and will generate code similar to this: > > - (CWVWebViewConfiguration*)configuration { > > return [[_configuration copy] autorelease]; > > } > > The main value for using |copy| is documenting the ownership > > Hmm it looks it doesn't affect the getter as far as I tested (see the test code > below). Am I missing something? > https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Pr... > also mentions only about the setter but not getter for |copy|. > > Maybe |retain| is better for documentation purpose if it actually doesn't copy? > > #import <Foundation/Foundation.h> > > @interface Foo : NSObject > @property(nonatomic, readonly, copy) NSObject* bar; > - (instancetype)init; > @end > > @implementation Foo > > - (instancetype)init { > self = [super init]; > if (self) { > _bar = [[NSObject alloc] init]; > } > return self; > } > > @end > > int main() { > Foo* foo = [[Foo alloc] init]; > NSObject* bar1 = foo.bar; > NSObject* bar2 = foo.bar; > NSLog(@"bar1 = %@", bar1); > // => bar1 = <NSObject: 0x7fce2f400430> > NSLog(@"bar2 = %@", bar2); > // => bar2 = <NSObject: 0x7fce2f400430> > return 0; > } Maybe this is an optimization for immutable objects, like NSObject? Or maybe I don't understand Objective-C :)
Message was sent while issue was closed.
On 2017/03/23 16:17:02, Eugene But wrote: > https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... > File ios/web_view/public/cwv_web_view.h (right): > > https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... > ios/web_view/public/cwv_web_view.h:23: @property(readonly, copy) > CWVWebViewConfiguration* configuration; > On 2017/03/23 02:18:40, Hiroshi Ichikawa wrote: > > On 2017/03/22 15:08:50, Eugene But wrote: > > > On 2017/03/22 04:52:53, Hiroshi Ichikawa wrote: > > > > On 2017/03/21 16:47:55, Eugene But wrote: > > > > > nonatomic? > > > > > > > > Done. > > > > > > > > BTW Many properties including this one are marked as (readonly, copy) in > the > > > > API, but isn't it meaningless? Doesn't "copy" only affect the setter, not > > > > getter? > > > Copy affects getter, and will generate code similar to this: > > > - (CWVWebViewConfiguration*)configuration { > > > return [[_configuration copy] autorelease]; > > > } > > > The main value for using |copy| is documenting the ownership > > > > Hmm it looks it doesn't affect the getter as far as I tested (see the test > code > > below). Am I missing something? > > > https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Pr... > > also mentions only about the setter but not getter for |copy|. > > > > Maybe |retain| is better for documentation purpose if it actually doesn't > copy? > > > > #import <Foundation/Foundation.h> > > > > @interface Foo : NSObject > > @property(nonatomic, readonly, copy) NSObject* bar; > > - (instancetype)init; > > @end > > > > @implementation Foo > > > > - (instancetype)init { > > self = [super init]; > > if (self) { > > _bar = [[NSObject alloc] init]; > > } > > return self; > > } > > > > @end > > > > int main() { > > Foo* foo = [[Foo alloc] init]; > > NSObject* bar1 = foo.bar; > > NSObject* bar2 = foo.bar; > > NSLog(@"bar1 = %@", bar1); > > // => bar1 = <NSObject: 0x7fce2f400430> > > NSLog(@"bar2 = %@", bar2); > > // => bar2 = <NSObject: 0x7fce2f400430> > > return 0; > > } > Maybe this is an optimization for immutable objects, like NSObject? Or maybe I > don't understand Objective-C :) Here is (I believe) a clearer example that it's not copied in the getter: #import <Foundation/Foundation.h> @interface Foo : NSObject @property(nonatomic, readonly, copy) NSMutableString* bar; - (instancetype)init; @end @implementation Foo - (instancetype)init { self = [super init]; if (self) { _bar = [[NSMutableString alloc] init]; [_bar appendString:@"aaa"]; } return self; } @end int main() { Foo* foo = [[Foo alloc] init]; NSMutableString* bar1 = foo.bar; [bar1 appendString:@"bbb"]; NSLog(@"bar1 = %@", bar1); // => bar1 = aaabbb NSLog(@"foo.bar = %@", foo.bar); // => foo.bar = aaabbb return 0; }
Message was sent while issue was closed.
On 2017/03/24 02:49:04, Hiroshi Ichikawa wrote: > On 2017/03/23 16:17:02, Eugene But wrote: > > > https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... > > File ios/web_view/public/cwv_web_view.h (right): > > > > > https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... > > ios/web_view/public/cwv_web_view.h:23: @property(readonly, copy) > > CWVWebViewConfiguration* configuration; > > On 2017/03/23 02:18:40, Hiroshi Ichikawa wrote: > > > On 2017/03/22 15:08:50, Eugene But wrote: > > > > On 2017/03/22 04:52:53, Hiroshi Ichikawa wrote: > > > > > On 2017/03/21 16:47:55, Eugene But wrote: > > > > > > nonatomic? > > > > > > > > > > Done. > > > > > > > > > > BTW Many properties including this one are marked as (readonly, copy) in > > the > > > > > API, but isn't it meaningless? Doesn't "copy" only affect the setter, > not > > > > > getter? > > > > Copy affects getter, and will generate code similar to this: > > > > - (CWVWebViewConfiguration*)configuration { > > > > return [[_configuration copy] autorelease]; > > > > } > > > > The main value for using |copy| is documenting the ownership > > > > > > Hmm it looks it doesn't affect the getter as far as I tested (see the test > > code > > > below). Am I missing something? > > > > > > https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Pr... > > > also mentions only about the setter but not getter for |copy|. > > > > > > Maybe |retain| is better for documentation purpose if it actually doesn't > > copy? > > > > > > #import <Foundation/Foundation.h> > > > > > > @interface Foo : NSObject > > > @property(nonatomic, readonly, copy) NSObject* bar; > > > - (instancetype)init; > > > @end > > > > > > @implementation Foo > > > > > > - (instancetype)init { > > > self = [super init]; > > > if (self) { > > > _bar = [[NSObject alloc] init]; > > > } > > > return self; > > > } > > > > > > @end > > > > > > int main() { > > > Foo* foo = [[Foo alloc] init]; > > > NSObject* bar1 = foo.bar; > > > NSObject* bar2 = foo.bar; > > > NSLog(@"bar1 = %@", bar1); > > > // => bar1 = <NSObject: 0x7fce2f400430> > > > NSLog(@"bar2 = %@", bar2); > > > // => bar2 = <NSObject: 0x7fce2f400430> > > > return 0; > > > } > > Maybe this is an optimization for immutable objects, like NSObject? Or maybe I > > don't understand Objective-C :) > > Here is (I believe) a clearer example that it's not copied in the getter: > > #import <Foundation/Foundation.h> > > @interface Foo : NSObject > @property(nonatomic, readonly, copy) NSMutableString* bar; > - (instancetype)init; > @end > > @implementation Foo > > - (instancetype)init { > self = [super init]; > if (self) { > _bar = [[NSMutableString alloc] init]; > [_bar appendString:@"aaa"]; > } > return self; > } > > @end > > int main() { > Foo* foo = [[Foo alloc] init]; > NSMutableString* bar1 = foo.bar; > [bar1 appendString:@"bbb"]; > NSLog(@"bar1 = %@", bar1); > // => bar1 = aaabbb > mailto:NSLog(@"foo.bar = %@", foo.bar); > // => foo.bar = aaabbb > return 0; > } Thank you for sharing this. Apparently I do not understand Objective-C :)
Message was sent while issue was closed.
On 2017/03/24 15:39:51, Eugene But wrote: > On 2017/03/24 02:49:04, Hiroshi Ichikawa wrote: > > On 2017/03/23 16:17:02, Eugene But wrote: > > > > > > https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... > > > File ios/web_view/public/cwv_web_view.h (right): > > > > > > > > > https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... > > > ios/web_view/public/cwv_web_view.h:23: @property(readonly, copy) > > > CWVWebViewConfiguration* configuration; > > > On 2017/03/23 02:18:40, Hiroshi Ichikawa wrote: > > > > On 2017/03/22 15:08:50, Eugene But wrote: > > > > > On 2017/03/22 04:52:53, Hiroshi Ichikawa wrote: > > > > > > On 2017/03/21 16:47:55, Eugene But wrote: > > > > > > > nonatomic? > > > > > > > > > > > > Done. > > > > > > > > > > > > BTW Many properties including this one are marked as (readonly, copy) > in > > > the > > > > > > API, but isn't it meaningless? Doesn't "copy" only affect the setter, > > not > > > > > > getter? > > > > > Copy affects getter, and will generate code similar to this: > > > > > - (CWVWebViewConfiguration*)configuration { > > > > > return [[_configuration copy] autorelease]; > > > > > } > > > > > The main value for using |copy| is documenting the ownership > > > > > > > > Hmm it looks it doesn't affect the getter as far as I tested (see the test > > > code > > > > below). Am I missing something? > > > > > > > > > > https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Pr... > > > > also mentions only about the setter but not getter for |copy|. > > > > > > > > Maybe |retain| is better for documentation purpose if it actually doesn't > > > copy? > > > > > > > > #import <Foundation/Foundation.h> > > > > > > > > @interface Foo : NSObject > > > > @property(nonatomic, readonly, copy) NSObject* bar; > > > > - (instancetype)init; > > > > @end > > > > > > > > @implementation Foo > > > > > > > > - (instancetype)init { > > > > self = [super init]; > > > > if (self) { > > > > _bar = [[NSObject alloc] init]; > > > > } > > > > return self; > > > > } > > > > > > > > @end > > > > > > > > int main() { > > > > Foo* foo = [[Foo alloc] init]; > > > > NSObject* bar1 = foo.bar; > > > > NSObject* bar2 = foo.bar; > > > > NSLog(@"bar1 = %@", bar1); > > > > // => bar1 = <NSObject: 0x7fce2f400430> > > > > NSLog(@"bar2 = %@", bar2); > > > > // => bar2 = <NSObject: 0x7fce2f400430> > > > > return 0; > > > > } > > > Maybe this is an optimization for immutable objects, like NSObject? Or maybe > I > > > don't understand Objective-C :) > > > > Here is (I believe) a clearer example that it's not copied in the getter: > > > > #import <Foundation/Foundation.h> > > > > @interface Foo : NSObject > > @property(nonatomic, readonly, copy) NSMutableString* bar; > > - (instancetype)init; > > @end > > > > @implementation Foo > > > > - (instancetype)init { > > self = [super init]; > > if (self) { > > _bar = [[NSMutableString alloc] init]; > > [_bar appendString:@"aaa"]; > > } > > return self; > > } > > > > @end > > > > int main() { > > Foo* foo = [[Foo alloc] init]; > > NSMutableString* bar1 = foo.bar; > > [bar1 appendString:@"bbb"]; > > NSLog(@"bar1 = %@", bar1); > > // => bar1 = aaabbb > > mailto:NSLog(@"foo.bar = %@", foo.bar); > > // => foo.bar = aaabbb > > return 0; > > } > Thank you for sharing this. Apparently I do not understand Objective-C :) No problem. Should we change (readonly, copy) to (readonly, retain) for clarification?
Message was sent while issue was closed.
On 2017/03/27 04:25:29, Hiroshi Ichikawa wrote: > On 2017/03/24 15:39:51, Eugene But wrote: > > On 2017/03/24 02:49:04, Hiroshi Ichikawa wrote: > > > On 2017/03/23 16:17:02, Eugene But wrote: > > > > > > > > > > https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... > > > > File ios/web_view/public/cwv_web_view.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... > > > > ios/web_view/public/cwv_web_view.h:23: @property(readonly, copy) > > > > CWVWebViewConfiguration* configuration; > > > > On 2017/03/23 02:18:40, Hiroshi Ichikawa wrote: > > > > > On 2017/03/22 15:08:50, Eugene But wrote: > > > > > > On 2017/03/22 04:52:53, Hiroshi Ichikawa wrote: > > > > > > > On 2017/03/21 16:47:55, Eugene But wrote: > > > > > > > > nonatomic? > > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > BTW Many properties including this one are marked as (readonly, > copy) > > in > > > > the > > > > > > > API, but isn't it meaningless? Doesn't "copy" only affect the > setter, > > > not > > > > > > > getter? > > > > > > Copy affects getter, and will generate code similar to this: > > > > > > - (CWVWebViewConfiguration*)configuration { > > > > > > return [[_configuration copy] autorelease]; > > > > > > } > > > > > > The main value for using |copy| is documenting the ownership > > > > > > > > > > Hmm it looks it doesn't affect the getter as far as I tested (see the > test > > > > code > > > > > below). Am I missing something? > > > > > > > > > > > > > > > https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Pr... > > > > > also mentions only about the setter but not getter for |copy|. > > > > > > > > > > Maybe |retain| is better for documentation purpose if it actually > doesn't > > > > copy? > > > > > > > > > > #import <Foundation/Foundation.h> > > > > > > > > > > @interface Foo : NSObject > > > > > @property(nonatomic, readonly, copy) NSObject* bar; > > > > > - (instancetype)init; > > > > > @end > > > > > > > > > > @implementation Foo > > > > > > > > > > - (instancetype)init { > > > > > self = [super init]; > > > > > if (self) { > > > > > _bar = [[NSObject alloc] init]; > > > > > } > > > > > return self; > > > > > } > > > > > > > > > > @end > > > > > > > > > > int main() { > > > > > Foo* foo = [[Foo alloc] init]; > > > > > NSObject* bar1 = foo.bar; > > > > > NSObject* bar2 = foo.bar; > > > > > NSLog(@"bar1 = %@", bar1); > > > > > // => bar1 = <NSObject: 0x7fce2f400430> > > > > > NSLog(@"bar2 = %@", bar2); > > > > > // => bar2 = <NSObject: 0x7fce2f400430> > > > > > return 0; > > > > > } > > > > Maybe this is an optimization for immutable objects, like NSObject? Or > maybe > > I > > > > don't understand Objective-C :) > > > > > > Here is (I believe) a clearer example that it's not copied in the getter: > > > > > > #import <Foundation/Foundation.h> > > > > > > @interface Foo : NSObject > > > @property(nonatomic, readonly, copy) NSMutableString* bar; > > > - (instancetype)init; > > > @end > > > > > > @implementation Foo > > > > > > - (instancetype)init { > > > self = [super init]; > > > if (self) { > > > _bar = [[NSMutableString alloc] init]; > > > [_bar appendString:@"aaa"]; > > > } > > > return self; > > > } > > > > > > @end > > > > > > int main() { > > > Foo* foo = [[Foo alloc] init]; > > > NSMutableString* bar1 = foo.bar; > > > [bar1 appendString:@"bbb"]; > > > NSLog(@"bar1 = %@", bar1); > > > // => bar1 = aaabbb > > > mailto:NSLog(@"foo.bar = %@", foo.bar); > > > // => foo.bar = aaabbb > > > return 0; > > > } > > Thank you for sharing this. Apparently I do not understand Objective-C :) > > No problem. Should we change (readonly, copy) to (readonly, retain) for > clarification? Keeping copy may still be useful. F.e. if the implementation wants to redefine the property as read-write, then the property needs be changed from "strong" to "copy" in the header. However if you want to change properties from "copy" to "strong" feel free to to so (unless those properties and NSString*, in which case it still should be "copy" to match Objective-C Style Guide).
Message was sent while issue was closed.
On 2017/03/27 16:31:50, Eugene But wrote: > On 2017/03/27 04:25:29, Hiroshi Ichikawa wrote: > > On 2017/03/24 15:39:51, Eugene But wrote: > > > On 2017/03/24 02:49:04, Hiroshi Ichikawa wrote: > > > > On 2017/03/23 16:17:02, Eugene But wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... > > > > > File ios/web_view/public/cwv_web_view.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2764773002/diff/20001/ios/web_view/public/cwv... > > > > > ios/web_view/public/cwv_web_view.h:23: @property(readonly, copy) > > > > > CWVWebViewConfiguration* configuration; > > > > > On 2017/03/23 02:18:40, Hiroshi Ichikawa wrote: > > > > > > On 2017/03/22 15:08:50, Eugene But wrote: > > > > > > > On 2017/03/22 04:52:53, Hiroshi Ichikawa wrote: > > > > > > > > On 2017/03/21 16:47:55, Eugene But wrote: > > > > > > > > > nonatomic? > > > > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > BTW Many properties including this one are marked as (readonly, > > copy) > > > in > > > > > the > > > > > > > > API, but isn't it meaningless? Doesn't "copy" only affect the > > setter, > > > > not > > > > > > > > getter? > > > > > > > Copy affects getter, and will generate code similar to this: > > > > > > > - (CWVWebViewConfiguration*)configuration { > > > > > > > return [[_configuration copy] autorelease]; > > > > > > > } > > > > > > > The main value for using |copy| is documenting the ownership > > > > > > > > > > > > Hmm it looks it doesn't affect the getter as far as I tested (see the > > test > > > > > code > > > > > > below). Am I missing something? > > > > > > > > > > > > > > > > > > > > > https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Pr... > > > > > > also mentions only about the setter but not getter for |copy|. > > > > > > > > > > > > Maybe |retain| is better for documentation purpose if it actually > > doesn't > > > > > copy? > > > > > > > > > > > > #import <Foundation/Foundation.h> > > > > > > > > > > > > @interface Foo : NSObject > > > > > > @property(nonatomic, readonly, copy) NSObject* bar; > > > > > > - (instancetype)init; > > > > > > @end > > > > > > > > > > > > @implementation Foo > > > > > > > > > > > > - (instancetype)init { > > > > > > self = [super init]; > > > > > > if (self) { > > > > > > _bar = [[NSObject alloc] init]; > > > > > > } > > > > > > return self; > > > > > > } > > > > > > > > > > > > @end > > > > > > > > > > > > int main() { > > > > > > Foo* foo = [[Foo alloc] init]; > > > > > > NSObject* bar1 = foo.bar; > > > > > > NSObject* bar2 = foo.bar; > > > > > > NSLog(@"bar1 = %@", bar1); > > > > > > // => bar1 = <NSObject: 0x7fce2f400430> > > > > > > NSLog(@"bar2 = %@", bar2); > > > > > > // => bar2 = <NSObject: 0x7fce2f400430> > > > > > > return 0; > > > > > > } > > > > > Maybe this is an optimization for immutable objects, like NSObject? Or > > maybe > > > I > > > > > don't understand Objective-C :) > > > > > > > > Here is (I believe) a clearer example that it's not copied in the getter: > > > > > > > > #import <Foundation/Foundation.h> > > > > > > > > @interface Foo : NSObject > > > > @property(nonatomic, readonly, copy) NSMutableString* bar; > > > > - (instancetype)init; > > > > @end > > > > > > > > @implementation Foo > > > > > > > > - (instancetype)init { > > > > self = [super init]; > > > > if (self) { > > > > _bar = [[NSMutableString alloc] init]; > > > > [_bar appendString:@"aaa"]; > > > > } > > > > return self; > > > > } > > > > > > > > @end > > > > > > > > int main() { > > > > Foo* foo = [[Foo alloc] init]; > > > > NSMutableString* bar1 = foo.bar; > > > > [bar1 appendString:@"bbb"]; > > > > NSLog(@"bar1 = %@", bar1); > > > > // => bar1 = aaabbb > > > > mailto:NSLog(@"foo.bar = %@", foo.bar); > > > > // => foo.bar = aaabbb > > > > return 0; > > > > } > > > Thank you for sharing this. Apparently I do not understand Objective-C :) > > > > No problem. Should we change (readonly, copy) to (readonly, retain) for > > clarification? > Keeping copy may still be useful. F.e. if the implementation wants to redefine > the property as read-write, then the property needs be changed from "strong" to > "copy" in the header. However if you want to change properties from "copy" to > "strong" feel free to to so (unless those properties and NSString*, in which > case it still should be "copy" to match Objective-C Style Guide). That makes sense. Then let's keep them as "copy". Thanks for the explanation. |
