Bug 99251 – Make subclassing of WKView safer by namespacing methods and eliminating its use as an observer and delegate

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit2

[Darin Adler]

Make subclassing of WKView safer by namespacing methods and eliminating its use as an observer and delegate

[Darin Adler]

Created attachment 168558 [details]
Patch

[Darin Adler]

One minus to this approach is that these prefixed method names won’t work well as getter/setter pairs for Objective-C properties.

[Maciej Stachowiak]

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.

[Darin Adler]

(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_.

[Darin Adler]

(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.

[Darin Adler]

Created attachment 168730 [details]
Patch, revised based on feedback from Anders and Maciej

[Darin Adler]

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.

[Darin Adler]

Created attachment 168843 [details]
Patch, revised based on additional feedback from Anders

[Darin Adler]

Anders, I made the changes you requested. Would you consider reviewing?

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch appears to have broken Lion builds:
http://build.webkit.org/builders/Apple%20Lion%20Debug%20%28Build%29/builds/6232

[Ryosuke Niwa]

Reverted the part of the patch that was causing the build failure in http://trac.webkit.org/changeset/131711.

[Sam Weinig]

I completely rolled this out in r132738 due to continued crashes.  Reopening.

[Darin Adler]

I don’t think I want to do this.