Bug 76570 - [WK2] Sync call for notifications permissions causes flashes on gmail.com
Summary: [WK2] Sync call for notifications permissions causes flashes on gmail.com
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.7
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-01-18 14:12 PST by Jon Lee
Modified: 2012-01-18 17:46 PST (History)
3 users (show)

See Also:


Attachments
[WK2] Sync call for notifications permissions causes flashes on gmail.com (11.65 KB, patch)
2012-01-18 15:27 PST, Jon Lee
no flags Details | Formatted Diff | Diff
[WK2] Sync call for notifications permissions causes flashes on gmail.com (14.44 KB, patch)
2012-01-18 15:27 PST, Jon Lee
no flags Details | Formatted Diff | Diff
[WK2] Sync call for notifications permissions causes flashes on gmail.com (9.17 KB, patch)
2012-01-18 15:27 PST, Jon Lee
no flags Details | Formatted Diff | Diff
[WK2] Sync call for notifications permissions causes flashes on gmail.com (15.36 KB, patch)
2012-01-18 15:27 PST, Jon Lee
no flags Details | Formatted Diff | Diff
[WK2] Sync call for notifications permissions causes flashes on gmail.com (12.42 KB, patch)
2012-01-18 15:27 PST, Jon Lee
no flags Details | Formatted Diff | Diff
Source/WebCore: [WK2] Sync call for notifications permissions causes flashes on gmail.com (5.63 KB, patch)
2012-01-18 15:31 PST, Jon Lee
andersca: review+
Details | Formatted Diff | Diff
[WK2] Sync call for notifications permissions causes flashes on gmail.com (11.65 KB, patch)
2012-01-18 15:31 PST, Jon Lee
sam: review+
Details | Formatted Diff | Diff
[WK2] Sync call for notifications permissions causes flashes on gmail.com (14.44 KB, patch)
2012-01-18 15:31 PST, Jon Lee
andersca: review+
Details | Formatted Diff | Diff
[WK2] Sync call for notifications permissions causes flashes on gmail.com (9.17 KB, patch)
2012-01-18 15:31 PST, Jon Lee
sam: review+
Details | Formatted Diff | Diff
[WK2] Sync call for notifications permissions causes flashes on gmail.com (15.36 KB, patch)
2012-01-18 15:31 PST, Jon Lee
andersca: review+
Details | Formatted Diff | Diff
[WK2] Sync call for notifications permissions causes flashes on gmail.com (12.42 KB, patch)
2012-01-18 15:31 PST, Jon Lee
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2012-01-18 14:12:22 PST
Need to store a cache of the permissions in the web process to get rid of sync call.

<rdar://problem/10647155>
Comment 1 Jon Lee 2012-01-18 15:27:45 PST
Created attachment 123009 [details]
[WK2] Sync call for notifications permissions causes flashes on gmail.com
Comment 2 Jon Lee 2012-01-18 15:27:48 PST
Created attachment 123010 [details]
[WK2] Sync call for notifications permissions causes flashes on gmail.com
Comment 3 Jon Lee 2012-01-18 15:27:51 PST
Created attachment 123011 [details]
[WK2] Sync call for notifications permissions causes flashes on gmail.com
Comment 4 Jon Lee 2012-01-18 15:27:53 PST
Created attachment 123012 [details]
[WK2] Sync call for notifications permissions causes flashes on gmail.com
Comment 5 Jon Lee 2012-01-18 15:27:56 PST
Created attachment 123013 [details]
[WK2] Sync call for notifications permissions causes flashes on gmail.com
Comment 6 Jon Lee 2012-01-18 15:31:19 PST
Created attachment 123014 [details]
Source/WebCore: [WK2] Sync call for notifications permissions causes flashes on gmail.com
Comment 7 Jon Lee 2012-01-18 15:31:22 PST
Created attachment 123015 [details]
[WK2] Sync call for notifications permissions causes flashes on gmail.com
Comment 8 Jon Lee 2012-01-18 15:31:24 PST
Created attachment 123016 [details]
[WK2] Sync call for notifications permissions causes flashes on gmail.com
Comment 9 Jon Lee 2012-01-18 15:31:27 PST
Created attachment 123017 [details]
[WK2] Sync call for notifications permissions causes flashes on gmail.com
Comment 10 Jon Lee 2012-01-18 15:31:30 PST
Created attachment 123018 [details]
[WK2] Sync call for notifications permissions causes flashes on gmail.com
Comment 11 Jon Lee 2012-01-18 15:31:33 PST
Created attachment 123019 [details]
[WK2] Sync call for notifications permissions causes flashes on gmail.com
Comment 12 WebKit Review Bot 2012-01-18 15:32:19 PST
Attachment 123012 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/WebProcess/Notifications/WebNotificationManager.h:65:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 WebKit Review Bot 2012-01-18 15:34:31 PST
Attachment 123014 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Jon Lee 2012-01-18 15:36:02 PST
Comment on attachment 123014 [details]
Source/WebCore: [WK2] Sync call for notifications permissions causes flashes on gmail.com

View in context: https://bugs.webkit.org/attachment.cgi?id=123014&action=review

>> Source/WebKit2/ChangeLog:4
>> +		https://bugs.webkit.org/show_bug.cgi?id=76570
> 
> Line contains tab character.  [whitespace/tab] [5]

This is fixed in a later patch.

>> Source/WebKit2/ChangeLog:5
>> +		<rdar://problem/10647155>
> 
> Line contains tab character.  [whitespace/tab] [5]

This is fixed in a later patch.

>> Source/WebKit2/ChangeLog:9
>> +		Add WK API calls to change notificationsEnabled bit in WebCore::Settings.
> 
> Line contains tab character.  [whitespace/tab] [5]

This is fixed in a later patch.
Comment 15 WebKit Review Bot 2012-01-18 15:39:31 PST
Attachment 123018 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/WebProcess/Notifications/WebNotificationManager.h:65:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Sam Weinig 2012-01-18 15:50:12 PST
Comment on attachment 123015 [details]
[WK2] Sync call for notifications permissions causes flashes on gmail.com

View in context: https://bugs.webkit.org/attachment.cgi?id=123015&action=review

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3309
> -    RefPtr<WebSecurityOrigin> origin = WebSecurityOrigin::create(originIdentifier);
> +    RefPtr<WebSecurityOrigin> origin = WebSecurityOrigin::createFromDatabaseIdentifier(originIdentifier);

Please put a comment here about how we probably should not be using database identifier for geolocation.
Comment 17 Jon Lee 2012-01-18 16:03:44 PST
The website code figures out the permission level for its security origin by making a JS call (called checkPermission()) that is synchronous. The way this was implemented was to make a synchronous call from the WebNotificationManager to its proxy. That call goes to the WK API layer to find the policy, and returns that policy back to the JS.

The synchronous nature of this call causes the white flash to appear in certain cases.

To fix this, the checkPermission() call is handled all within the web process, instead of going up into the UI process. To do this, the web process initializes the WebNotificationManager with a copy of the notification permissions. Any time the WK client makes a change to the permissions, that gets sent down asynchronously, and the cached copy in WebNotificationManager gets updated.

A page's settings may disable notifications altogether. Before, this would have been handled by the WK client, since retrieving the permissions were also handled there. Now that the lookup happens in the web process, we need to add that setting in WebCore.
Comment 18 Anders Carlsson 2012-01-18 16:25:44 PST
Comment on attachment 123016 [details]
[WK2] Sync call for notifications permissions causes flashes on gmail.com

View in context: https://bugs.webkit.org/attachment.cgi?id=123016&action=review

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.h:56
> +    void initialize(const HashMap<WTF::String, bool>& permissions);

No need for WTF:: here.

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.h:83
> +    typedef HashMap<WTF::String, bool> PermissionsMap;
> +    PermissionsMap m_permissionsMap;

No need for WTF:: here. Also, I don't think that the typedef is necessary here.
Comment 19 Sam Weinig 2012-01-18 16:30:47 PST
Comment on attachment 123017 [details]
[WK2] Sync call for notifications permissions causes flashes on gmail.com

View in context: https://bugs.webkit.org/attachment.cgi?id=123017&action=review

> Source/WebKit2/UIProcess/API/C/WKNotificationManager.cpp:59
> +void WKNotificationManagerProviderDidUpdateNotificationPolicy(WKNotificationManagerRef managerRef, WKStringRef originString, bool allowed)

We should be passing the origins as origins, not strings.

> Source/WebKit2/UIProcess/API/C/WKNotificationManager.cpp:64
> +void WKNotificationManagerProviderDidRemoveNotificationPolicies(WKNotificationManagerRef managerRef, WKArrayRef originStrings)

Same here.

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:175
> +    Vector<WTF::String> vectorOriginStrings;

Don't need the WTF::

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:79
> +    for (Vector<String>::const_iterator it = originStrings.begin(); it != originStrings.end(); ++it)

We usually iterate vectors using array style indexing.
Comment 20 Jon Lee 2012-01-18 17:17:55 PST
Committed r105364: http://trac.webkit.org/changeset/105364