[webkit-reviews] review granted: [Bug 34238] Send application ID and full URL to Chromium when requesting notification permissions : [Attachment 47588] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Aaron Boodman <aa at chromium.org>'s
request for review:
Bug 34238: Send application ID and full URL to Chromium when requesting
notification permissions
https://bugs.webkit.org/show_bug.cgi?id=34238

Attachment 47588: Patch
https://bugs.webkit.org/attachment.cgi?id=47588&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -    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