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.
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 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.
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 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.
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 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 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.
2012-01-18 15:27 PST, Jon Lee
2012-01-18 15:27 PST, Jon Lee
2012-01-18 15:27 PST, Jon Lee
2012-01-18 15:27 PST, Jon Lee
2012-01-18 15:27 PST, Jon Lee
2012-01-18 15:31 PST, Jon Lee
2012-01-18 15:31 PST, Jon Lee
2012-01-18 15:31 PST, Jon Lee
2012-01-18 15:31 PST, Jon Lee
2012-01-18 15:31 PST, Jon Lee
2012-01-18 15:31 PST, Jon Lee