Make subclassing of WKView safer by namespacing methods and eliminating its use as an observer and delegate
Created attachment 168558 [details] Patch
One minus to this approach is that these prefixed method names won’t work well as getter/setter pairs for Objective-C properties.
Comment on attachment 168558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168558&action=review > Source/WebKit2/ChangeLog:9 > + we chose to use internally in WKView's own implementation. Thus, it's a good idea to add a "wk_" prefix I am not sure that wk_ as the prefix best aligns the advice Apple gives to third-party developers, which is that Cocoa frameworks reserve methods starting with "_". So Apple's advice is that application code should never use methods starting with "_" and should instead use a two-letter prefix on method names of the form "XX_". By using "wk_" in WebKit2, a Cocoa framework, we're stepping on what our guidelines say is Application namespace: https://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingMethods.html On the other hand, in practice many apps use underscore-prefixed names freely, which is presumably the reason for your change. If we want to be extra kind to apps that use WebKit, then perhaps an _wk_ prefix would be a good choice (or _WK_). That way we would both be using what is supposed to be the system framework namespace for private methods (start with underscore) but also using a unique prefix to reduce risk of collision from app developers not following the advice.
(In reply to comment #3) > If we want to be extra kind to apps that use WebKit We do. > then perhaps an _wk_ prefix would be a good choice (or _WK_). Sounds fine. Would be easy for me to change wk_ to _wk_.
(In reply to comment #3) > Cocoa frameworks reserve methods starting with "_" It’s kind of unfortunate that future additions to the public APIs for Cocoa frameworks effectively reserve methods with simple names that do not start with "_" and the internals of Cocoa frameworks reserve methods with names that do start with "_". Guess that means app developers do need to use a prefix.
Created attachment 168730 [details] Patch, revised based on feedback from Anders and Maciej
Comment on attachment 168730 [details] Patch, revised based on feedback from Anders and Maciej View in context: https://bugs.webkit.org/attachment.cgi?id=168730&action=review > Source/WebKit2/UIProcess/API/mac/WKView.mm:1889 > + #define ADD_OBSERVER(selectorName, notificationName, notificationObject) \ > + [[NSNotificationCenter defaultCenter] addObserver:_data selector:@selector(selectorName:) \ > + name:notificationName object:notificationObject] Anders asked me to not use the WKViewData as the observer, because he plans to eliminate that object entirely in the future, once we can take advantage of the more modern Objective-C feature where we can add data without touching the header file. So I’ll use the block form of observer instead. > Source/WebKit2/UIProcess/API/mac/WKView.mm:2538 > + _data->_lastToolTipTag = [self addToolTipRect:wideOpenRect owner:_data userData:NULL]; Similarly, Anders asked me not to use the WKViewData as the owner of the tooltip, so I’ll create a separate object just for that.
Created attachment 168843 [details] Patch, revised based on additional feedback from Anders
Anders, I made the changes you requested. Would you consider reviewing?
Comment on attachment 168843 [details] Patch, revised based on additional feedback from Anders Clearing flags on attachment: 168843 Committed r131686: <http://trac.webkit.org/changeset/131686>
All reviewed patches have been landed. Closing bug.
This patch appears to have broken Lion builds: http://build.webkit.org/builders/Apple%20Lion%20Debug%20%28Build%29/builds/6232
Reverted the part of the patch that was causing the build failure in http://trac.webkit.org/changeset/131711.
I completely rolled this out in r132738 due to continued crashes. Reopening.
I don’t think I want to do this.