Bug 34238 - Send application ID and full URL to Chromium when requesting notification permissions
Summary: Send application ID and full URL to Chromium when requesting notification per...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Aaron Boodman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-27 19:42 PST by Aaron Boodman
Modified: 2010-01-28 12:12 PST (History)
4 users (show)

See Also:


Attachments
Patch (15.30 KB, patch)
2010-01-27 20:27 PST, Aaron Boodman
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Boodman 2010-01-27 19:42:16 PST
Send application ID and full URL to Chromium when requesting notification permissions
Comment 1 Aaron Boodman 2010-01-27 20:10:37 PST
We are experimenting with the concept of an "application id" in Chromium to group permission requests together so that they can be made all at once.

The application ID should be settable by authors using either an HTTP header or a meta tag using http-equiv.
Comment 2 Aaron Boodman 2010-01-27 20:27:14 PST
Created attachment 47588 [details]
Patch
Comment 3 WebKit Review Bot 2010-01-27 20:29:49 PST
Attachment 47588 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/notifications/NotificationPresenter.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Aaron Boodman 2010-01-27 20:34:09 PST
(In reply to comment #3)
> Attachment 47588 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebCore/notifications/NotificationPresenter.h:41:  Code inside a namespace
> should not be indented.  [whitespace/indent] [4]
> Total errors found: 1
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Note: the file was already indented incorrectly, so just followed along.
Comment 5 Darin Adler 2010-01-27 20:44:03 PST
Comment on attachment 47588 [details]
Patch

> -    if (m_presenter->checkPermission(context->securityOrigin()) != NotificationPresenter::PermissionAllowed) {
> +    if (m_presenter->checkPermission(context->url(),
> +                                     context->isDocument() ? static_cast<Document*>(context) : 0)
> +            != NotificationPresenter::PermissionAllowed) {

I'm surprised to discover that the style guide doesn't specifically recommends against lining up with parentheses in this fashion. Renaming will make lines like this no longer line up, so we have been avoiding this kind of formatting, but I can't find that in writing anywhere.

Maybe a local variable for the document will keep things a bit more compact.

>          virtual void cancel(Notification* object) = 0;

>          virtual void notificationObjectDestroyed(Notification* object) = 0;

>          virtual void requestPermission(SecurityOrigin* origin, PassRefPtr<VoidCallback> callback) = 0;

> +        virtual Permission checkPermission(const KURL& url, Document* document) = 0;

In every single one of these cases the argument names should be omitted, because all they do is repeat the type name.

I didn't review style much in the Chromium-specific part of the patch, but the code seems OK.

r=me
Comment 6 Aaron Boodman 2010-01-28 11:24:31 PST
Committed r54008: <http://trac.webkit.org/changeset/54008>
Comment 7 Eric Seidel (no email) 2010-01-28 12:12:03 PST
Broke Chromium builders:
  CXX(target) out/Release/obj.target/webcore/../../WebCore/bindings/v8/WorkerContextExecutionProxy.o
  CXX(target) out/Release/obj.target/webcore/../../WebCore/notifications/Notification.o
../../WebCore/notifications/Notification.cpp: In constructor ‘WebCore::Notification::Notification(const WebCore::String&, WebCore::ScriptExecutionContext*, WebCore::ExceptionCode&, WebCore::NotificationPresenter*)’:
../../WebCore/notifications/Notification.cpp:53: error: expected primary-expression before ‘!=’ token
../../WebCore/notifications/Notification.cpp:53: error: expected `;' before ‘)’ token
../../WebCore/notifications/Notification.cpp: In constructor ‘WebCore::Notification::Notification(const WebCore::NotificationContents&, WebCore::ScriptExecutionContext*, WebCore::ExceptionCode&, WebCore::NotificationPresenter*)’:
../../WebCore/notifications/Notification.cpp:74: error: expected primary-expression before ‘!=’ token
../../WebCore/notifications/Notification.cpp:74: error: expected `;' before ‘)’ token
make[1]: *** [out/Release/obj.target/webcore/../../WebCore/notifications/Notification.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make[1]: Leaving directory `/WebKit-BuildSlave/chromium-linux-release/build/WebKit/chromium'
program finished with exit code 2
elapsedTime=57.465181