Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(71)

Issue 758603002: Use Mojo Permission Service when requesting Notifications permission. (Closed)

Created:
6 years ago by mlamouri (slow - plz ping)
Modified:
6 years ago
CC:
chromium-reviews, creis+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, nasko+codewatch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, darin (slow to review), ben+mojo_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@permission_geolocation_follow_up
Project:
chromium
Visibility:
Public.

Description

Use Mojo Permission Service when requesting Notifications permission. This is also enabling the Notification code to be able to return a tri-state value when a permission request is handled, see bug 434547. The Permission Service does not yet expose those three values. The NotificationPermissionDispatcher will need to be updated as soon as the service is ready. BUG=430238, 434547 Committed: https://crrev.com/26ce71cf9df19f2da9ae404d81fcb577d2969ce9 Cr-Commit-Position: refs/heads/master@{#305654}

Patch Set 1 #

Total comments: 12

Patch Set 2 : fixes to pass tests #

Messages

Total messages: 23 (8 generated)
mlamouri (slow - plz ping)
Peter, can you have an early look at this?
6 years ago (2014-11-24 18:23:41 UTC) #3
mlamouri (slow - plz ping)
tsepez@chromium.org: Please review changes in: * content/common/platform_notification_messages.h * content/common/permission_service.mojom
6 years ago (2014-11-24 18:24:14 UTC) #5
mlamouri (slow - plz ping)
nasko@chromium.org: Please review changes in: * content/*
6 years ago (2014-11-24 18:25:14 UTC) #7
Peter Beverloo
Notifications lgtm. Since Web Notifications require a sync IPC for the Notification.permission getter, we can't ...
6 years ago (2014-11-24 18:37:34 UTC) #8
Tom Sepez
lgtm https://codereview.chromium.org/758603002/diff/1/content/renderer/notification_permission_dispatcher.cc File content/renderer/notification_permission_dispatcher.cc (right): https://codereview.chromium.org/758603002/diff/1/content/renderer/notification_permission_dispatcher.cc#newcode58 content/renderer/notification_permission_dispatcher.cc:58: } Nit: default: NOTREACHED(); break; or similar?
6 years ago (2014-11-24 18:58:06 UTC) #9
nasko
LGTM with a question. https://codereview.chromium.org/758603002/diff/1/content/renderer/notification_permission_dispatcher.h File content/renderer/notification_permission_dispatcher.h (right): https://codereview.chromium.org/758603002/diff/1/content/renderer/notification_permission_dispatcher.h#newcode20 content/renderer/notification_permission_dispatcher.h:20: class NotificationPermissionDispatcher : public RenderFrameObserver ...
6 years ago (2014-11-25 00:44:58 UTC) #10
nasko
https://codereview.chromium.org/758603002/diff/1/content/renderer/notification_permission_dispatcher.h File content/renderer/notification_permission_dispatcher.h (right): https://codereview.chromium.org/758603002/diff/1/content/renderer/notification_permission_dispatcher.h#newcode20 content/renderer/notification_permission_dispatcher.h:20: class NotificationPermissionDispatcher : public RenderFrameObserver { On 2014/11/25 00:44:58, ...
6 years ago (2014-11-25 00:48:03 UTC) #11
mlamouri (slow - plz ping)
https://codereview.chromium.org/758603002/diff/1/content/renderer/notification_permission_dispatcher.cc File content/renderer/notification_permission_dispatcher.cc (right): https://codereview.chromium.org/758603002/diff/1/content/renderer/notification_permission_dispatcher.cc#newcode33 content/renderer/notification_permission_dispatcher.cc:33: permission_service_->RequestPermission( On 2014/11/24 18:37:34, Peter Beverloo wrote: > Is ...
6 years ago (2014-11-25 12:19:55 UTC) #12
Peter Beverloo
https://codereview.chromium.org/758603002/diff/1/content/renderer/notification_permission_dispatcher.cc File content/renderer/notification_permission_dispatcher.cc (right): https://codereview.chromium.org/758603002/diff/1/content/renderer/notification_permission_dispatcher.cc#newcode33 content/renderer/notification_permission_dispatcher.cc:33: permission_service_->RequestPermission( On 2014/11/25 12:19:55, Mounir Lamouri wrote: > On ...
6 years ago (2014-11-25 13:10:34 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/758603002/1
6 years ago (2014-11-25 13:29:02 UTC) #15
mlamouri (slow - plz ping)
https://codereview.chromium.org/758603002/diff/1/content/renderer/notification_permission_dispatcher.cc File content/renderer/notification_permission_dispatcher.cc (right): https://codereview.chromium.org/758603002/diff/1/content/renderer/notification_permission_dispatcher.cc#newcode33 content/renderer/notification_permission_dispatcher.cc:33: permission_service_->RequestPermission( On 2014/11/25 13:10:34, Peter Beverloo wrote: > On ...
6 years ago (2014-11-25 13:30:22 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tests_recipe/builds/31748)
6 years ago (2014-11-25 13:52:22 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/758603002/20001
6 years ago (2014-11-25 14:58:40 UTC) #21
commit-bot: I haz the power
Committed patchset #2 (id:20001)
6 years ago (2014-11-25 16:41:25 UTC) #22
commit-bot: I haz the power
6 years ago (2014-11-25 16:42:52 UTC) #23
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/26ce71cf9df19f2da9ae404d81fcb577d2969ce9
Cr-Commit-Position: refs/heads/master@{#305654}

Powered by Google App Engine
This is Rietveld 408576698