Implement a permission check for push.

Implement a permission check for push.

Extracted common functionality from the midi permission check.
A subsequent CL will move the common classes to chrome/browser/content_settings/
and implement the midi permission check in terms of these classes.

Note that bubbles are not yet implemented so only infobars work now.
Note also that only way to rever the permission decision right now
is to clear browser settings.

BUG=350378
NOTRY=true

(note, the mac release bot has been stuck for over an hour in the compile step, all other release builds and all other mac bots are ok)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279437

[Miguel Garcia, 2014-06-19 00:51:58]

First pass review before adding the different owners.

Takashi, my idea would be to refactor the midi implementation so that it uses
the classes in permission_context_base.h and permission_infobar_delegate.cc I
would first implement generic permission bubbles and move all these files to a
common place but I thought it would be good for you to have a look to see if you
see this happening.

[Michael van Ouwerkerk, 2014-06-19 10:16:11]

Nice! You'll need to rebase though as r278087 has possible merge conflicts.

https://codereview.chromium.org/343743004/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/343743004/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:14396: +   <message
name="IDS_PUSH_MESSAGES_PERMISSION_QUESTION" desc="Question asked on the info
bar whenever a domain wants to send push messages">
Technically it's an origin, not a domain, correct?

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl.cc:382: return NULL;
Add a TODO like above?
// TODO(miguelg): Support push messaging in incognito if possible.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:16: class GCMInfoBarDelegate
: public PermissionInfobarDelegate {
I'm wondering whether we want to maybe have a PushMessagingInfoBarDelegate (and
friends) that does not mention GCM. It would still have to live in the chrome
layer, but really it is not GCM specific is it?

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_context_base.cc (right):

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/permission_context_base.cc:28: void
PermissionContextBase::RequestPermission(
Let's add a TODO for doing full UMA instrumentation for the Push Messaging
infobar flow. I'd love to know the response rates.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_context_base.h (right):

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/permission_context_base.h:45:
base::Callback<void(bool)> callback);
Should this also be a "const BrowserPermissionCallback&" ?

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:41: return;
We can't only return. We must also send a rejection back to the renderer,
otherwise the promise never resolves.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:50:
gcm::GCMPermissionContext* context =
We have other contexts (ServiceWorkerContextWrapper, ServiceWorkerContextCore,
BrowserContext). Please rename this one to permission_context.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:73: void
PushMessagingServiceImpl::ShouldProceed(
This method name is not very clear. Let's rename to something like
DidRequestPermission.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:76: const
content::PushMessagingService::RegisterCallback& callback,
The argument and method names are starting to conflict. Let's rename this
callback to register_callback.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:79: // NOT_SIGNED_IN
is the closest error I can find when not allowed.
We really need to rewire all the error codes so ppl know why things are failing.
Currently we end up with a boolean in the content layer :-/ This is for a
follow-up patch.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:80:
DidRegister(callback, "", GCMClient::NOT_SIGNED_IN);
The signed-in requirement is going away. Maybe just use UNKNOWN for now.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.h (right):

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.h:8: #include <vector>
Nit: move this to push_messaging_service_impl.cc where it is used.

[Miguel Garcia, 2014-06-19 17:49:29]

PTAL, adding reviewers for the different pieces

fgorski: for gcm
bauerb: for content settings (jochen seems to be travelling)
erg: for profile
pkasting: for common/ and because it's dealing with infobars

https://codereview.chromium.org/343743004/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/343743004/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:14396: +   <message
name="IDS_PUSH_MESSAGES_PERMISSION_QUESTION" desc="Question asked on the info
bar whenever a domain wants to send push messages">
On 2014/06/19 10:16:10, Michael van Ouwerkerk wrote:
> Technically it's an origin, not a domain, correct?

I don't think the translators understand origin in this context, domain seems
easier to get.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl.cc:382: return NULL;
On 2014/06/19 10:16:10, Michael van Ouwerkerk wrote:
> Add a TODO like above?
> // TODO(miguelg): Support push messaging in incognito if possible.

There is already a TODO in GetPushMessagingService above I think that's enough
right? We don't need add TODOs for this on every method.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:16: class GCMInfoBarDelegate
: public PermissionInfobarDelegate {
On 2014/06/19 10:16:10, Michael van Ouwerkerk wrote:
> I'm wondering whether we want to maybe have a PushMessagingInfoBarDelegate
(and
> friends) that does not mention GCM. It would still have to live in the chrome
> layer, but really it is not GCM specific is it?

I thought about it as well, and in fact renamed the class/file a few times, in
the end I went with this because it's shorter and it's inside the gcm directory
already so it's pretty specific to GCM at this point.

The way I see it push_message_service_impl.cc is where we become gcm specific.
If other embedders need to implement this, that is the layer at which they will
have to add their own push server.

I don't feel very strongly about it, this is the reasoning I did to myself but
if after reading this you still prefer a more generic name (for this and the
permission context I guess?) I can change it.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_profile_service.h (right):

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.h:53: Profile* profile() const {
return profile_; }
Since after the rebase profile is now injected into PushMessagingServiceImpl
directly I don't need this any more so I am reverting this change.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_context_base.cc (right):

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/permission_context_base.cc:28: void
PermissionContextBase::RequestPermission(
On 2014/06/19 10:16:11, Michael van Ouwerkerk wrote:
> Let's add a TODO for doing full UMA instrumentation for the Push Messaging
> infobar flow. I'd love to know the response rates.

It's actually a pretty neat idea to do it here, that way all new permissions
will get it for free. Addded the TODO

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_context_base.h (right):

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/permission_context_base.h:45:
base::Callback<void(bool)> callback);
On 2014/06/19 10:16:11, Michael van Ouwerkerk wrote:
> Should this also be a "const BrowserPermissionCallback&" ?

indeed

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:41: return;
On 2014/06/19 10:16:11, Michael van Ouwerkerk wrote:
> We can't only return. We must also send a rejection back to the renderer,
> otherwise the promise never resolves.
But if web_contents does not exist, it means the renderer is gone so there is
really no one to execute the promise.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:50:
gcm::GCMPermissionContext* context =
On 2014/06/19 10:16:11, Michael van Ouwerkerk wrote:
> We have other contexts (ServiceWorkerContextWrapper, ServiceWorkerContextCore,
> BrowserContext). Please rename this one to permission_context.

Done.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:73: void
PushMessagingServiceImpl::ShouldProceed(
On 2014/06/19 10:16:11, Michael van Ouwerkerk wrote:
> This method name is not very clear. Let's rename to something like
> DidRequestPermission.

Done.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:76: const
content::PushMessagingService::RegisterCallback& callback,
On 2014/06/19 10:16:11, Michael van Ouwerkerk wrote:
> The argument and method names are starting to conflict. Let's rename this
> callback to register_callback.

Done.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:79: // NOT_SIGNED_IN
is the closest error I can find when not allowed.
On 2014/06/19 10:16:11, Michael van Ouwerkerk wrote:
> We really need to rewire all the error codes so ppl know why things are
failing.
> Currently we end up with a boolean in the content layer :-/ This is for a
> follow-up patch.

Agreed

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:80:
DidRegister(callback, "", GCMClient::NOT_SIGNED_IN);
On 2014/06/19 10:16:11, Michael van Ouwerkerk wrote:
> The signed-in requirement is going away. Maybe just use UNKNOWN for now.

Done.

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.h (right):

https://codereview.chromium.org/343743004/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.h:8: #include <vector>
On 2014/06/19 10:16:11, Michael van Ouwerkerk wrote:
> Nit: move this to push_messaging_service_impl.cc where it is used.

Done.

[Peter Kasting, 2014-06-19 17:58:21]

I'm not actually a chrome/common/ owner, but here are some minor comments on the
infobars.

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_infobar_delegate.cc (right):

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.cc:41:
net::FormatUrl(requesting_frame_.GetOrigin(), display_languages_));
Perhaps we should change |requesting_frame_| to |requesting_origin_| and do the
GetOrigin() call in the constructor, rather than here.

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:43: }  // namespace gcm
Nit: Blank line below this

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_infobar_delegate.cc (right):

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.cc:29: const {
Nit: "const" should be on same line as last arg (in this case, as the function
name).  So break after the return type above instead.

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.cc:36: :
IDS_PERMISSION_DENY);
Nit: clang-format error; instead, break after "?" and indent 4:

  return xyz ?
      foo : bar;

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.cc:52:
InfoBarService::WebContentsFromInfoBar(infobar());
Nit: You can inline this below if you start the args indented 4 instead of even
with the first paren.

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.cc:58: }
Nit: Blank line below this

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.h:23: class
PermissionInfobarDelegate : public ConfirmInfoBarDelegate {
Do you have this base class because your next patch is going to add more
subclasses of it, or something?  It's a bit odd to have this class with only one
subclass that's what everyone actually wants to use.

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.h:45:
DISALLOW_COPY_AND_ASSIGN(PermissionInfobarDelegate);
Nit: Blank line above this so it doesn't blend in with the members

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.h:48: }  // namespace
gcm
Nit: Blank line below this

[Elliot Glaysher, 2014-06-19 18:07:15]

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:1036: return
gcm::GCMPermissionContextFactory::GetForProfile(this);
Is there a reason you're adding more methods to the Profile instead of just
calling GCM...::GetProfile() at the actual call site?

[Bernhard Bauer, 2014-06-19 18:13:18]

Content settinsg LGTM with a nit:

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_queue_controller.cc:122: infobar_ =
gcm::GCMInfoBarDelegate::Create(GetInfoBarService(id_),
Nit: Other ::Create methods here put multiple parameters on one line.

[Miguel Garcia, 2014-06-19 21:49:22]

Thanks fall for the quick review!

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_queue_controller.cc:122: infobar_ =
gcm::GCMInfoBarDelegate::Create(GetInfoBarService(id_),
On 2014/06/19 18:13:18, Bernhard Bauer wrote:
> Nit: Other ::Create methods here put multiple parameters on one line.

Done.

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:1036: return
gcm::GCMPermissionContextFactory::GetForProfile(this);
On 2014/06/19 18:07:14, Elliot Glaysher wrote:
> Is there a reason you're adding more methods to the Profile instead of just
> calling GCM...::GetProfile() at the actual call site?

Not really, mainly because that's what the midi and the geolocation permissions
are doing but you are right there is absolutely no need. Removed

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_infobar_delegate.cc (right):

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.cc:41:
net::FormatUrl(requesting_frame_.GetOrigin(), display_languages_));
On 2014/06/19 17:58:20, Peter Kasting wrote:
> Perhaps we should change |requesting_frame_| to |requesting_origin_| and do
the
> GetOrigin() call in the constructor, rather than here.

Done.

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:43: }  // namespace gcm
On 2014/06/19 17:58:20, Peter Kasting wrote:
> Nit: Blank line below this

Done.

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_infobar_delegate.cc (right):

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.cc:29: const {
On 2014/06/19 17:58:20, Peter Kasting wrote:
> Nit: "const" should be on same line as last arg (in this case, as the function
> name).  So break after the return type above instead.

Done.

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.cc:36: :
IDS_PERMISSION_DENY);
On 2014/06/19 17:58:20, Peter Kasting wrote:
> Nit: clang-format error; instead, break after "?" and indent 4:
> 
>   return xyz ?
>       foo : bar;

Done.

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.cc:52:
InfoBarService::WebContentsFromInfoBar(infobar());
On 2014/06/19 17:58:20, Peter Kasting wrote:
> Nit: You can inline this below if you start the args indented 4 instead of
even
> with the first paren.

Done.

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.cc:58: }
On 2014/06/19 17:58:20, Peter Kasting wrote:
> Nit: Blank line below this

Done.

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.h:23: class
PermissionInfobarDelegate : public ConfirmInfoBarDelegate {
On 2014/06/19 17:58:21, Peter Kasting wrote:
> Do you have this base class because your next patch is going to add more
> subclasses of it, or something?  It's a bit odd to have this class with only
one
> subclass that's what everyone actually wants to use.

Yes, Midi Geolocation and other permission classes do almost exactly the same
thing so the idea is to move this to a common place and reuse it in all of them.
I need to implement bubble support before I do it though. I added a explanation
in the description.

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.h:45:
DISALLOW_COPY_AND_ASSIGN(PermissionInfobarDelegate);
On 2014/06/19 17:58:21, Peter Kasting wrote:
> Nit: Blank line above this so it doesn't blend in with the members

Done.

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.h:48: }  // namespace
gcm
On 2014/06/19 17:58:20, Peter Kasting wrote:
> Nit: Blank line below this

Done.

[Miguel Garcia, 2014-06-19 21:58:37]

+jam for content OWNERS

[Peter Kasting, 2014-06-19 22:04:32]

infobars LGTM with one substantive issue

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:43: }  // namespace gcm
On 2014/06/19 21:49:22, Miguel Garcia wrote:
> On 2014/06/19 17:58:20, Peter Kasting wrote:
> > Nit: Blank line below this
> 
> Done.

Doesn't appear like you did this, in either this file or the other two where I
made this comment?

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:38: const std::string&
display_languages_;
Having this as a const ref makes me nervous, since it means the creator must
pass a string that outlives the delegate.

I'd prefer this as a const std::string (no ref).

[Elliot Glaysher, 2014-06-19 22:11:40]

(My lgtm is no longer needed since there aren't any changes to the c/b/p/.)

[fgorski, 2014-06-20 14:14:15]

lgtm, a few nits and questions though.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_infobar_delegate.cc (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.cc:21: return
infobar_service->AddInfoBar(ConfirmInfoBarDelegate::CreateInfoBar(
are checks for infobar_service != NULL and controller != NULL not necessary
here?

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:25: GURL& requesting_frame,
pass GURL as const &.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:31: GURL& requesting_frame,
same here

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:38: const std::string&
display_languages_;
On 2014/06/19 22:04:31, Peter Kasting wrote:
> Having this as a const ref makes me nervous, since it means the creator must
> pass a string that outlives the delegate.
> 
> I'd prefer this as a const std::string (no ref).
+1

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:43: }  // namespace gcm
nit: Please add empty line below and make it consistent across all files.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_context_base.cc (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_context_base.cc:75:
base::Unretained(this),
Will this callback be passed somewhere outside of the
permission_queue_controller_? If so, I think weak_ptr would be better.
Otherwise, could you add a comment explaining what the expectation is?

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.h:30: GURL&
requesting_frame);
const &

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:172:
DidRegister(app_id, callback, "", GCMClient::UNKNOWN_ERROR);
nit: std::string()

you are using it in earlier files as opposed to ""

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:211:
DidRegister(app_id, register_callback, "", GCMClient::UNKNOWN_ERROR);
in case you think there will be a specific error you want to give to developers
in response you can add a todo to extend the result enum accordingly.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:211:
DidRegister(app_id, register_callback, "", GCMClient::UNKNOWN_ERROR);
nit: std::string()

[jam, 2014-06-20 15:55:01]

https://codereview.chromium.org/343743004/diff/60001/content/browser/push_mes...
File content/browser/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/343743004/diff/60001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:42: //                      by
receiving the IPC on the UI thread.
this should be a WCO. why not do that now? it's a simple change.

also why is WebPushClient on WebViewClient instead of WebFrameClient. if the
latter, the content implementation would be a RenderFrameObserver instead of 
RenderViewObserver. This is how we're converting existing code as part of site
isolation code. any new code should be done using that model.

https://codereview.chromium.org/343743004/diff/60001/content/public/browser/p...
File content/public/browser/push_messaging_service.h (right):

https://codereview.chromium.org/343743004/diff/60001/content/public/browser/p...
content/public/browser/push_messaging_service.h:29: const int routing_id,
nit: what is routing_id? usually we disambiguate by naming it render_view_id or
render_frame_id.

[jianli, 2014-06-20 17:19:30]

I am wondering if it is appropriate to put lots of UI related stuff under
chrome/browser/services/gcm. Would it be better to  put them under
chrome/browser/ui/push_messaging?

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:16: class GCMInfoBarDelegate
: public PermissionInfobarDelegate {
The info bar seems to be targeted only for web push messaging. Should we make it
more clear by renaming the class name to PushMessagingInfoBarDelegate?

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_permission_context.cc (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_permission_context.cc:15: }  // namespace gcm
nit: empty line before this line

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_permission_context.h (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_permission_context.h:13: class
GCMPermissionContext : public PermissionContextBase {
Should we name this class as PushMessagingPermissionContext?

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_permission_context.h:18:
DISALLOW_COPY_AND_ASSIGN(GCMPermissionContext);
nit: add private scope

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_permission_context_factory.cc (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_permission_context_factory.cc:13: // static
nit: empty line before this line

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_permission_context_factory.h (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_permission_context_factory.h:17: class
GCMPermissionContextFactory : public BrowserContextKeyedServiceFactory {
ditto for class name

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_context_base.cc (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_context_base.cc:24:
DCHECK(!permission_queue_controller_);
Please include logging.h.

Since permission_queue_controller_ is delay created, do we need to check for
this? If we do, there is no need to make permission_queue_controller_ delay
created.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_context_base.cc:34: // TODO(miguelg) Add
UMA instrumentation
nit: add ':' after ')' and end the sentence with period.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_context_base.cc:65: // TODO(miguelg)
implement bubble support
ditto

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_context_base.h (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_context_base.h:83: };
nit: empty line after this

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.h:13: class GURL;
Is this needed? It seems that you need the definition for requesting_frame_.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.h:42:
PermissionQueueController* controller_;
Document the lifetime or ownership for this pointer.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:145: // The page
doesn't exist any more.
Not sure I understand this comment.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:169:
gcm::GCMPermissionContextFactory::GetForProfile(profile_);
It seems that GCMPermissionContext is only tied with PushMessagingServiceImpl.
Do we really need to make it KeyedService based?

https://codereview.chromium.org/343743004/diff/60001/content/public/browser/p...
File content/public/browser/push_messaging_service.h (right):

https://codereview.chromium.org/343743004/diff/60001/content/public/browser/p...
content/public/browser/push_messaging_service.h:28: const int renderer_id,
nit: no need to add const

[Miguel Garcia, 2014-06-20 20:50:13]

Thanks a lot for all the comments

>I am wondering if it is appropriate to put lots of UI related stuff under
>chrome/browser/services/gcm. Would it be better to  put them under
>chrome/browser/ui/push_messaging?

I am planning to move all the base classes to chrome/browser/content_settings
and implement midi permissions (and later geolocation) on top of it. However I
need to implement bubble support here before doing that. So bear with me 2 more
CLs and the result will almost be identical.


All others inline

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_infobar_delegate.cc (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.cc:21: return
infobar_service->AddInfoBar(ConfirmInfoBarDelegate::CreateInfoBar(
On 2014/06/20 14:14:15, fgorski wrote:
> are checks for infobar_service != NULL and controller != NULL not necessary
> here?

I added DCHECKS for both since they should always be non null.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:16: class GCMInfoBarDelegate
: public PermissionInfobarDelegate {
On 2014/06/20 17:19:29, jianli wrote:
> The info bar seems to be targeted only for web push messaging. Should we make
it
> more clear by renaming the class name to PushMessagingInfoBarDelegate?

Ok, since two people already asked for the same I'll go with that. I was on the
fence since this is really a permission to register for GCM

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:25: GURL& requesting_frame,
On 2014/06/20 14:14:15, fgorski wrote:
> pass GURL as const &.

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:31: GURL& requesting_frame,
On 2014/06/20 14:14:15, fgorski wrote:
> same here

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:38: const std::string&
display_languages_;
On 2014/06/19 22:04:31, Peter Kasting wrote:
> Having this as a const ref makes me nervous, since it means the creator must
> pass a string that outlives the delegate.
> 
> I'd prefer this as a const std::string (no ref).

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:38: const std::string&
display_languages_;
On 2014/06/20 14:14:15, fgorski wrote:
> On 2014/06/19 22:04:31, Peter Kasting wrote:
> > Having this as a const ref makes me nervous, since it means the creator must
> > pass a string that outlives the delegate.
> > 
> > I'd prefer this as a const std::string (no ref).
> +1

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_infobar_delegate.h:43: }  // namespace gcm
On 2014/06/20 14:14:15, fgorski wrote:
> nit: Please add empty line below and make it consistent across all files.

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_permission_context.cc (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_permission_context.cc:15: }  // namespace gcm
On 2014/06/20 17:19:29, jianli wrote:
> nit: empty line before this line

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_permission_context.h (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_permission_context.h:13: class
GCMPermissionContext : public PermissionContextBase {
On 2014/06/20 17:19:29, jianli wrote:
> Should we name this class as PushMessagingPermissionContext?

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_permission_context.h:18:
DISALLOW_COPY_AND_ASSIGN(GCMPermissionContext);
On 2014/06/20 17:19:29, jianli wrote:
> nit: add private scope

Oops, thanks for catching this one!

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_permission_context_factory.cc (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_permission_context_factory.cc:13: // static
On 2014/06/20 17:19:29, jianli wrote:
> nit: empty line before this line

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_permission_context_factory.h (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_permission_context_factory.h:17: class
GCMPermissionContextFactory : public BrowserContextKeyedServiceFactory {
On 2014/06/20 17:19:29, jianli wrote:
> ditto for class name

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_context_base.cc (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_context_base.cc:24:
DCHECK(!permission_queue_controller_);
Actually let's create permission_queue_controller in the constructor. I think
this is done this way because once we implement permission bubbles (coming soon)
the queue does not need to be created at all but I think it's just clearer if it
is created in the constructor just like anything else.


On 2014/06/20 17:19:29, jianli wrote:
> Please include logging.h.
> 
> Since permission_queue_controller_ is delay created, do we need to check for
> this? If we do, there is no need to make permission_queue_controller_ delay
> created.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_context_base.cc:34: // TODO(miguelg) Add
UMA instrumentation
On 2014/06/20 17:19:29, jianli wrote:
> nit: add ':' after ')' and end the sentence with period.

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_context_base.cc:65: // TODO(miguelg)
implement bubble support
On 2014/06/20 17:19:29, jianli wrote:
> ditto

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_context_base.cc:75:
base::Unretained(this),
On 2014/06/20 14:14:15, fgorski wrote:
> Will this callback be passed somewhere outside of the
> permission_queue_controller_? If so, I think weak_ptr would be better.
> Otherwise, could you add a comment explaining what the expectation is?
Once we implement bubbles it will be passed to the bubble as well but it seems a
similar case, i.e this object will be alive anyway. I'll change it to weak_ptr
as suggested

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_context_base.h (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_context_base.h:83: };
On 2014/06/20 17:19:29, jianli wrote:
> nit: empty line after this

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.h:13: class GURL;
On 2014/06/20 17:19:30, jianli wrote:
> Is this needed? It seems that you need the definition for requesting_frame_.

Now that it's not a reference anymore I in fact don't need it.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.h:30: GURL&
requesting_frame);
On 2014/06/20 14:14:15, fgorski wrote:
> const &

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.h:42:
PermissionQueueController* controller_;
On 2014/06/20 17:19:29, jianli wrote:
> Document the lifetime or ownership for this pointer.

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:145: // The page
doesn't exist any more.
On 2014/06/20 17:19:30, jianli wrote:
> Not sure I understand this comment.
No wonder, it's an artifact of a bad merge. Removed

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:169:
gcm::GCMPermissionContextFactory::GetForProfile(profile_);
On 2014/06/20 17:19:30, jianli wrote:
> It seems that GCMPermissionContext is only tied with PushMessagingServiceImpl.
> Do we really need to make it KeyedService based?

All other permissions seems to be tied to it so they can be fetched through a
keyed factory and so that they can behave in the right way when being called
from an incognito tab. I think it makes sense to keep it like this.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:172:
DidRegister(app_id, callback, "", GCMClient::UNKNOWN_ERROR);
On 2014/06/20 14:14:15, fgorski wrote:
> nit: std::string()
> 
> you are using it in earlier files as opposed to ""

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:211:
DidRegister(app_id, register_callback, "", GCMClient::UNKNOWN_ERROR);
On 2014/06/20 14:14:15, fgorski wrote:
> nit: std::string()

Done.

https://codereview.chromium.org/343743004/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:211:
DidRegister(app_id, register_callback, "", GCMClient::UNKNOWN_ERROR);
On 2014/06/20 14:14:15, fgorski wrote:
> in case you think there will be a specific error you want to give to
developers
> in response you can add a todo to extend the result enum accordingly.

Great, added the TODO and will be extending the enum in a subsuquent CL, in a
first iteration it will be useful for UMA collection, we are still figuring out
how to properly expose errors in the API

https://codereview.chromium.org/343743004/diff/60001/content/browser/push_mes...
File content/browser/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/343743004/diff/60001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:42: //                      by
receiving the IPC on the UI thread.
It feels out of scope for this change. I can upgrade the TODO with your
suggestions so Michael can get to it (he works in my team and will happily do
it). All the permission stuff I am adding here will be once he changes it.

On 2014/06/20 15:55:01, jam wrote:
> this should be a WCO. why not do that now? it's a simple change.
> 
> also why is WebPushClient on WebViewClient instead of WebFrameClient. if the
> latter, the content implementation would be a RenderFrameObserver instead of 
> RenderViewObserver. This is how we're converting existing code as part of site
> isolation code. any new code should be done using that model.

https://codereview.chromium.org/343743004/diff/60001/content/public/browser/p...
File content/public/browser/push_messaging_service.h (right):

https://codereview.chromium.org/343743004/diff/60001/content/public/browser/p...
content/public/browser/push_messaging_service.h:28: const int renderer_id,
On 2014/06/20 17:19:30, jianli wrote:
> nit: no need to add const

Done.

https://codereview.chromium.org/343743004/diff/60001/content/public/browser/p...
content/public/browser/push_messaging_service.h:29: const int routing_id,
On 2014/06/20 15:55:01, jam wrote:
> nit: what is routing_id? usually we disambiguate by naming it render_view_id
or
> render_frame_id.

Done.

[jam, 2014-06-23 06:53:48]

content lgtm

the sooner this is converted to use frame instead of view, the better, thanks.

[Takashi Toyoshima, 2014-06-23 10:29:16]

lgtm.
defer to owners.

https://codereview.chromium.org/343743004/diff/80001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/343743004/diff/80001/chrome/app/generated_res...
chrome/app/generated_resources.grd:14398: +   <!-- Push messaging infobar -->
indent looks wrong

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_default_provider.cc:171:
CONTENT_SETTING_NUM_SETTINGS);
Add UMA for Push here?

[Michael van Ouwerkerk, 2014-06-23 12:34:10]

lgtm Thanks for all the changes!

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.h:21: // provide an
incon and a message text to the infobar.
Nit: s/incon/icon/

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_infobar_delegate.h:30: const
PermissionRequestID& id,
Nit: align indentation with first argument.

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:179: false,
Nit: What does false stand for? Maybe add a comment behind it.

[Miguel Garcia, 2014-06-23 18:23:14]

https://codereview.chromium.org/343743004/diff/80001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/343743004/diff/80001/chrome/app/generated_res...
chrome/app/generated_resources.grd:14398: +   <!-- Push messaging infobar -->
On 2014/06/23 10:29:16, Takashi Toyoshima (chromium) wrote:
> indent looks wrong

Done.

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_default_provider.cc:171:
CONTENT_SETTING_NUM_SETTINGS);
On 2014/06/23 10:29:16, Takashi Toyoshima (chromium) wrote:
> Add UMA for Push here?

I added a todo since it would require me to pull histograms.xm which requires
another owner, I rather send a smaller patch with that.

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/services/...
File chrome/browser/services/gcm/permission_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/services/...
chrome/browser/services/gcm/permission_infobar_delegate.h:21: // provide an
incon and a message text to the infobar.
On 2014/06/23 12:34:10, Michael van Ouwerkerk wrote:
> Nit: s/incon/icon/

Done.

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_infobar_delegate.h (right):

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_infobar_delegate.h:30: const
PermissionRequestID& id,
On 2014/06/23 12:34:10, Michael van Ouwerkerk wrote:
> Nit: align indentation with first argument.

Done.

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/343743004/diff/80001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:179: false,
On 2014/06/23 12:34:10, Michael van Ouwerkerk wrote:
> Nit: What does false stand for? Maybe add a comment behind it.

Done.

[Miguel Garcia, 2014-06-23 18:24:13]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-06-23 18:26:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miguelg@chromium.org/343743004/100001

[commit-bot: I haz the power, 2014-06-23 20:30:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/20430)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/24272)

[commit-bot: I haz the power, 2014-06-23 20:32:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-23 20:32:34]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Takashi Toyoshima, 2014-06-24 02:18:37]

Ah, you need to handle switch-case for mac and win.

[Miguel Garcia, 2014-06-24 08:52:40]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-06-24 08:53:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miguelg@chromium.org/343743004/160001

[Miguel Garcia, 2014-06-24 13:11:46]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-06-24 13:13:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miguelg@chromium.org/343743004/180001

[commit-bot: I haz the power, 2014-06-24 16:19:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Miguel Garcia, 2014-06-24 17:50:34]

The CQ bit was unchecked by miguelg@chromium.org

[Miguel Garcia, 2014-06-24 17:50:35]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-06-24 17:51:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miguelg@chromium.org/343743004/180001

[commit-bot: I haz the power, 2014-06-24 17:53:27]

Change committed as 279437

[Jason Robbins -- corp, 2014-12-03 22:01:16]

jrobbins@google.com changed reviewers:
- erg@chromium.or