WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
95465
[Mac] Registering web views for notifications is unbalanced
https://bugs.webkit.org/show_bug.cgi?id=95465
Summary
[Mac] Registering web views for notifications is unbalanced
Jon Lee
Reported
2012-08-30 08:48:10 PDT
Setting the notification provider for a WK1 WebView registers that web view to the provider. When the WebView is closed, WebCore is essentially responsible for unregistering the web view. This leads to an imbalance on Lion since notifications are not supported, and the NotificationController responsible for making the unregistration call doesn't exist. Instead, the unregisterWebView method should be called when the web view is closed, keeping both calls within WebKit.
Attachments
Patch
(4.10 KB, patch)
2012-08-30 09:48 PDT
,
Jon Lee
darin
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2012-08-30 08:48:20 PDT
<
rdar://problem/12206765
>
Jon Lee
Comment 2
2012-08-30 09:48:59 PDT
Created
attachment 161499
[details]
Patch
Darin Adler
Comment 3
2012-08-30 10:09:39 PDT
Comment on
attachment 161499
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=161499&action=review
> Source/WebKit/mac/WebView/WebView.mm:1143 > + [[self _notificationProvider] unregisterWebView:self];
What about the _closeWithFastTeardown case? Are we OK not unregistering in that case? Could something bad happen during the rest of the application termination process?
> Source/WebKit/mac/WebView/WebView.mm:6526 > - (void)_setNotificationProvider:(id<WebNotificationProvider>)notificationProvider > { > - if (_private) { > + if (_private && !_private->_notificationProvider) { > _private->_notificationProvider = notificationProvider; > [_private->_notificationProvider registerWebView:self]; > } > }
It seems a little non-obvious that this method does nothing once a notification provider is set. Are the callers able to tolerate this behavior? The alternative would be to teach this function to unregister the old provider when setting a new one.
Jon Lee
Comment 4
2012-08-30 10:56:41 PDT
(In reply to
comment #3
)
> (From update of
attachment 161499
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=161499&action=review
> > > Source/WebKit/mac/WebView/WebView.mm:1143 > > + [[self _notificationProvider] unregisterWebView:self]; > > What about the _closeWithFastTeardown case? Are we OK not unregistering in that case? Could something bad happen during the rest of the application termination process?
I considered that, but did not see any harm with not unregistering in this situation. The registered web view is called either from WebCore or when there is some user interaction with the platform notification, but in the latter case that is code in the provider.
> > > Source/WebKit/mac/WebView/WebView.mm:6526 > > - (void)_setNotificationProvider:(id<WebNotificationProvider>)notificationProvider > > { > > - if (_private) { > > + if (_private && !_private->_notificationProvider) { > > _private->_notificationProvider = notificationProvider; > > [_private->_notificationProvider registerWebView:self]; > > } > > } > > It seems a little non-obvious that this method does nothing once a notification provider is set. Are the callers able to tolerate this behavior? The alternative would be to teach this function to unregister the old provider when setting a new one.
The provider is set once, typically during initialization of the web view. There is no reason I can think of that one would want to switch providers in the middle of the web view's lifetime. The case is similar with other kinds of providers like geolocation. Because of the use case, I opted to just go with initialize-once rather than unregister-the-old-provider, since that is not the intended use for this method. Maybe renaming this to _initializeNotificationProvider would be more descriptive.
Jon Lee
Comment 5
2012-08-30 10:59:17 PDT
Committed
r127161
: <
http://trac.webkit.org/changeset/127161
>
Darin Adler
Comment 6
2012-08-31 10:39:09 PDT
Comment on
attachment 161499
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=161499&action=review
>>> Source/WebKit/mac/WebView/WebView.mm:6526 >>> } >> >> It seems a little non-obvious that this method does nothing once a notification provider is set. Are the callers able to tolerate this behavior? The alternative would be to teach this function to unregister the old provider when setting a new one. > > The provider is set once, typically during initialization of the web view. There is no reason I can think of that one would want to switch providers in the middle of the web view's lifetime. The case is similar with other kinds of providers like geolocation. > > Because of the use case, I opted to just go with initialize-once rather than unregister-the-old-provider, since that is not the intended use for this method. Maybe renaming this to _initializeNotificationProvider would be more descriptive.
The name change is one possible way to increase clarity. Another would be to add an ASSERT_NOT_REACHED along with early return instead of nesting the function inside an if. The _private == nil case seems expected and harmless while the !_private->_notificationProvider seems like it always reflects a programming error. Might also want to assert that the provider is non-nil or something.
Jon Lee
Comment 7
2012-08-31 10:53:34 PDT
Comment on
attachment 161499
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=161499&action=review
>>>> Source/WebKit/mac/WebView/WebView.mm:6526 >>>> } >>> >>> It seems a little non-obvious that this method does nothing once a notification provider is set. Are the callers able to tolerate this behavior? The alternative would be to teach this function to unregister the old provider when setting a new one. >> >> The provider is set once, typically during initialization of the web view. There is no reason I can think of that one would want to switch providers in the middle of the web view's lifetime. The case is similar with other kinds of providers like geolocation. >> >> Because of the use case, I opted to just go with initialize-once rather than unregister-the-old-provider, since that is not the intended use for this method. Maybe renaming this to _initializeNotificationProvider would be more descriptive. > > The name change is one possible way to increase clarity. Another would be to add an ASSERT_NOT_REACHED along with early return instead of nesting the function inside an if. The _private == nil case seems expected and harmless while the !_private->_notificationProvider seems like it always reflects a programming error. Might also want to assert that the provider is non-nil or something.
Filed
bug 95589
to track this work.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug