Push registration should read a "gcm_user_visible_only" key from the Manifest.

Push registration should read a "gcm_user_visible_only" key from the Manifest.

Per this implementation, the key will be read from the developer-supplied
manifest file as a boolean value (defaulting to FALSE), and passed through
to the browser process.

BUG=437929
Committed: https://crrev.com/94297a7db1826ba150b7d0766ddb52d4084e3914
Cr-Commit-Position: refs/heads/master@{#306824}

[Peter Beverloo, 2014-12-01 20:53:18]

peter@chromium.org changed reviewers:
+ mlamouri@chromium.org, mvanouwerkerk@chromium.org

[Peter Beverloo, 2014-12-01 20:53:18]

+mlamouri for manifest
+mvanouwerkerk for Push

[mlamouri (slow - plz ping), 2014-12-01 21:05:09]

About you maxing out IPC macros, I think you could simply remove |user_gesture|
from there. The stuff using the mojo permissions service don't use that anymore.
The |user_gesture| check should be done in the renderer process, see
https://crbug.com/423770

I'm not saying that you should remove it but it would make your life simpler:
you wouldn't have to add this struct.

https://codereview.chromium.org/770023002/diff/20001/content/browser/push_mes...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/770023002/diff/20001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:15: #include
"content/common/push_registration_params.h"
I think you forgot that file.

https://codereview.chromium.org/770023002/diff/20001/content/common/push_mess...
File content/common/push_messaging_messages.h (right):

https://codereview.chromium.org/770023002/diff/20001/content/common/push_mess...
content/common/push_messaging_messages.h:27:
IPC_STRUCT_TRAITS_MEMBER(user_gesture)
I wouldn't put |user_gesture| in that struct.

https://codereview.chromium.org/770023002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/770023002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:167: bool
ManifestParser::ParseBoolean(const base::DictionaryValue& dictionary,
Could you get ::ParseBoolean() on top, around ParseString() and ParseURL()? 
(Ideally: boolean / string / url)

https://codereview.chromium.org/770023002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser.h (right):

https://codereview.chromium.org/770023002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.h:55: // Returns the parsed boolean if
any, or |default_value| is parsing failed.
nit: s/is parsing failed/if parsing failed/.

https://codereview.chromium.org/770023002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/770023002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:849: }
Could you check that false is correctly parsed without parsing error.

[Miguel Garcia, 2014-12-03 14:02:51]

miguelg@chromium.org changed reviewers:
+ miguelg@chromium.org

[Miguel Garcia, 2014-12-03 14:02:51]

https://chromiumcodereview.appspot.com/770023002/diff/20001/chrome/browser/se...
File chrome/browser/services/gcm/push_messaging_service_impl.h (right):

https://chromiumcodereview.appspot.com/770023002/diff/20001/chrome/browser/se...
chrome/browser/services/gcm/push_messaging_service_impl.h:62:
blink::WebPushPermissionStatus GetPermissionStatus(
I think we need user_visible_only here as well so we can properly answer the
question. This might need to fed from storage (not just from the manifest) for
the case where the method is used within a service worker.

[Peter Beverloo, 2014-12-03 15:01:59]

All done. PTAL.

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

https://codereview.chromium.org/770023002/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.h:62:
blink::WebPushPermissionStatus GetPermissionStatus(
On 2014/12/03 14:02:51, Miguel Garcia wrote:
> I think we need user_visible_only here as well so we can properly answer the
> question. This might need to fed from storage (not just from the manifest) for
> the case where the method is used within a service worker.

Added a TODO in PushMessagingMessageFilter::OnRegisterFromDocument.

https://codereview.chromium.org/770023002/diff/20001/content/browser/push_mes...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/770023002/diff/20001/content/browser/push_mes...
content/browser/push_messaging/push_messaging_message_filter.cc:15: #include
"content/common/push_registration_params.h"
On 2014/12/01 21:05:09, Mounir Lamouri wrote:
> I think you forgot that file.

It's no longer necessary. Thanks for pointing it out, though.

https://codereview.chromium.org/770023002/diff/20001/content/common/push_mess...
File content/common/push_messaging_messages.h (right):

https://codereview.chromium.org/770023002/diff/20001/content/common/push_mess...
content/common/push_messaging_messages.h:27:
IPC_STRUCT_TRAITS_MEMBER(user_gesture)
On 2014/12/01 21:05:09, Mounir Lamouri wrote:
> I wouldn't put |user_gesture| in that struct.

Acknowledged.

https://codereview.chromium.org/770023002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/770023002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.cc:167: bool
ManifestParser::ParseBoolean(const base::DictionaryValue& dictionary,
On 2014/12/01 21:05:09, Mounir Lamouri wrote:
> Could you get ::ParseBoolean() on top, around ParseString() and ParseURL()? 
> (Ideally: boolean / string / url)

Done.

https://codereview.chromium.org/770023002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser.h (right):

https://codereview.chromium.org/770023002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser.h:55: // Returns the parsed boolean if
any, or |default_value| is parsing failed.
On 2014/12/01 21:05:09, Mounir Lamouri wrote:
> nit: s/is parsing failed/if parsing failed/.

Done.

https://codereview.chromium.org/770023002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/770023002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:849: }
On 2014/12/01 21:05:09, Mounir Lamouri wrote:
> Could you check that false is correctly parsed without parsing error.

Done.

[Michael van Ouwerkerk, 2014-12-03 15:18:12]

https://codereview.chromium.org/770023002/diff/40001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/770023002/diff/40001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:70:
ASSERT_TRUE(manifest.gcm_user_visible_only);
This should be ASSERT_FALSE, no?

https://codereview.chromium.org/770023002/diff/40001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:87:
ASSERT_TRUE(manifest.gcm_user_visible_only);
Ditto.

[Peter Beverloo, 2014-12-03 15:21:57]

https://codereview.chromium.org/770023002/diff/40001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/770023002/diff/40001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:70:
ASSERT_TRUE(manifest.gcm_user_visible_only);
On 2014/12/03 15:18:12, Michael van Ouwerkerk wrote:
> This should be ASSERT_FALSE, no?

Done.

https://codereview.chromium.org/770023002/diff/40001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:87:
ASSERT_TRUE(manifest.gcm_user_visible_only);
On 2014/12/03 15:18:12, Michael van Ouwerkerk wrote:
> Ditto.

Done.

[mlamouri (slow - plz ping), 2014-12-03 15:29:02]

lgtm with comments applied.

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

https://codereview.chromium.org/770023002/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:288: // re-introduce
the ability of |user_gesture| when bubbles require this.
Could you point to the bug. plz.

https://codereview.chromium.org/770023002/diff/60001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/770023002/diff/60001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:849: }
nit: can you add a test where you set the value to 1?

[Michael van Ouwerkerk, 2014-12-03 15:29:47]

lgtm

[johnme, 2014-12-03 15:36:31]

johnme@chromium.org changed reviewers:
+ johnme@chromium.org

[johnme, 2014-12-03 15:36:32]

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

https://codereview.chromium.org/770023002/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:290: web_contents,
id, embedding_origin, true /* user_gesture */,
It's not clear from https://crbug.com/423770 what the current status of
user_gesture is. Is the renderer now doing the checks like "on the renderer
side, any permission request after page load that isn't coming with a user
gesture will fail;" that are mentioned in that bug?

[Peter Beverloo, 2014-12-03 15:41:10]

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

https://codereview.chromium.org/770023002/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:288: // re-introduce
the ability of |user_gesture| when bubbles require this.
On 2014/12/03 15:29:01, Mounir Lamouri wrote:
> Could you point to the bug. plz.

Pointed to 423770.

https://codereview.chromium.org/770023002/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:290: web_contents,
id, embedding_origin, true /* user_gesture */,
On 2014/12/03 15:36:31, johnme wrote:
> It's not clear from https://crbug.com/423770 what the current status of
> user_gesture is. Is the renderer now doing the checks like "on the renderer
> side, any permission request after page load that isn't coming with a user
> gesture will fail;" that are mentioned in that bug?

Negative. Push is the feature using IPC-based permission requests, and I'm
hoping we'll be able to convert it to use Mojo before we ship (in fact, I'm
inclined to do it myself if Mounir doesn't beat me to it). This is an open issue
for all the Mojo code-paths, which will collectively be addressed there.

https://codereview.chromium.org/770023002/diff/60001/content/renderer/manifes...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/770023002/diff/60001/content/renderer/manifes...
content/renderer/manifest/manifest_parser_unittest.cc:849: }
On 2014/12/03 15:29:01, Mounir Lamouri wrote:
> nit: can you add a test where you set the value to 1?

Done.

[Peter Beverloo, 2014-12-03 19:06:46]

peter@chromium.org changed reviewers:
+ avi@chromium.org

[Peter Beverloo, 2014-12-03 19:06:47]

+avi, my content/ hero

[Peter Beverloo, 2014-12-03 19:07:52]

peter@chromium.org changed reviewers:
+ tsepez@chromium.org

[Peter Beverloo, 2014-12-03 19:07:52]

tsepez@chromium.org: for IPC stamp

[Avi (use Gerrit), 2014-12-03 19:20:19]

LGTM if that is indeed an explanatory comment.

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

https://codereview.chromium.org/770023002/diff/80001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:291: web_contents,
id, embedding_origin, true /* user_gesture */,
Is the |true| a user_gesture? If so, and the comment is explanatory, OK. If not,
then I don't get it.

[Peter Beverloo, 2014-12-03 19:26:36]

On 2014/12/03 19:20:19, Avi wrote:
> LGTM if that is indeed an explanatory comment.
> 
>
https://codereview.chromium.org/770023002/diff/80001/chrome/browser/services/...
> File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):
> 
>
https://codereview.chromium.org/770023002/diff/80001/chrome/browser/services/...
> chrome/browser/services/gcm/push_messaging_service_impl.cc:291: web_contents,
> id, embedding_origin, true /* user_gesture */,
> Is the |true| a user_gesture? If so, and the comment is explanatory, OK. If
not,
> then I don't get it.

Your understanding is correct. Just "true" by itself is unclear at the call
site, and since there's a corresponding TODO I included it.

Thanks!

[Tom Sepez, 2014-12-03 19:56:32]

Messages LGTM.

[Peter Beverloo, 2014-12-03 23:09:51]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-12-03 23:11:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/770023002/80001

[commit-bot: I haz the power, 2014-12-03 23:16:30]

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

[commit-bot: I haz the power, 2014-12-03 23:16:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Peter Beverloo, 2014-12-04 13:52:03]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-12-04 13:52:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/770023002/100001

[commit-bot: I haz the power, 2014-12-04 16:05:58]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-04 16:07:18]

Patchset 6 (id:??) landed as
https://crrev.com/94297a7db1826ba150b7d0766ddb52d4084e3914
Cr-Commit-Position: refs/heads/master@{#306824}