Bug 34238 – Send application ID and full URL to Chromium when requesting notification permissions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Aaron Boodman]

Send application ID and full URL to Chromium when requesting notification permissions

[Aaron Boodman]

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.

[Aaron Boodman]

Created attachment 47588 [details]
Patch

[WebKit Review Bot]

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.

[Aaron Boodman]

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

[Darin Adler]

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

[Aaron Boodman]

Committed r54008: <http://trac.webkit.org/changeset/54008>

[Eric Seidel (no email)]

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