[Push] Register: require user-visible to be present in the manifest

[Push] Register: require user-visible to be present in the manifest

Prompt for notifications if the Notification permission was not
previously granted.

Refactor a bit the browser tests to accomodate pages with
different manifests.

BUG=432930
TBR=fgorski

Committed: https://crrev.com/202115f03eb3c90a7139e69e0a1cb2062c0754cf
Cr-Commit-Position: refs/heads/master@{#308075}

[Miguel Garcia, 2014-12-09 11:47:15]

miguelg@chromium.org changed reviewers:
+ bauerb@chromium.org, mvanouwerkerk@chromium.org

[Miguel Garcia, 2014-12-09 11:48:12]

Michael for OWNERS approval of the /gcm pieces and the tests
Bernhard for Owners approval on /content_settings

[Bernhard Bauer, 2014-12-09 12:02:08]

https://codereview.chromium.org/785993003/diff/1/chrome/browser/services/gcm/...
File chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc
(right):

https://codereview.chromium.org/785993003/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:48:
PushMessagingPermissionContextTest(){};
Space before the braces, and no semicolon afterwards (like it was before).

https://codereview.chromium.org/785993003/diff/1/chrome/test/data/push_messag...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/785993003/diff/1/chrome/test/data/push_messag...
chrome/test/data/push_messaging/push_test.js:85:
pushManager.register().then(function(pushRegistration) {
If you return this (the result of the then() call), an error will be propagated
to the result of the outer then() call, which you could catch(). That way, you
only need sendErrorToTest once.

[Miguel Garcia, 2014-12-09 14:24:32]

Thanks for the quick review!

https://codereview.chromium.org/785993003/diff/1/chrome/browser/services/gcm/...
File chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc
(right):

https://codereview.chromium.org/785993003/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:48:
PushMessagingPermissionContextTest(){};
On 2014/12/09 12:02:07, Bernhard Bauer wrote:
> Space before the braces, and no semicolon afterwards (like it was before).

Done.

https://codereview.chromium.org/785993003/diff/1/chrome/test/data/push_messag...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/785993003/diff/1/chrome/test/data/push_messag...
chrome/test/data/push_messaging/push_test.js:85:
pushManager.register().then(function(pushRegistration) {
On 2014/12/09 12:02:08, Bernhard Bauer wrote:
> If you return this (the result of the then() call), an error will be
propagated
> to the result of the outer then() call, which you could catch(). That way, you
> only need sendErrorToTest once.

Done.

[Bernhard Bauer, 2014-12-09 16:22:27]

lgtm

[Michael van Ouwerkerk, 2014-12-10 18:36:33]

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

https://codereview.chromium.org/785993003/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_browsertest.cc:165: protected:
This should still be private. We have public getters for the server and service.

https://codereview.chromium.org/785993003/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_browsertest.cc:173: void
loadTestPage() override {
Instead of subclassing, you could add a loadTestPage method that accepts a url
for the page to load. Just call it at the start of your test method to load a
different page. I don't think it's overly expensive to load an extra page, but
if you do, we can talk about other approaches.

Calling loadTestPage with a specific url at the start of the test also seems
easier to read than using a different test class, I tend to gloss over those.

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

https://codereview.chromium.org/785993003/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_permission_context.cc:79:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
Why are we (sometimes) checking for threads? This entire class is expected to
run on the same thread (UI). That can be documented at the class level, in the
header, if you feel it's not clear enough.

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

https://codereview.chromium.org/785993003/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:278: if
(permission_context == NULL || !user_visible_only) {
nit: how about
if (!permission_context || !user_visible_only)

https://codereview.chromium.org/785993003/diff/20001/chrome/test/data/push_me...
File chrome/test/data/push_messaging/bad_manifest.json (right):

https://codereview.chromium.org/785993003/diff/20001/chrome/test/data/push_me...
chrome/test/data/push_messaging/bad_manifest.json:3: }
It seems there can be more than one way of having a bad manifest: 1) no sender
id and 2) no user visible only. Please name this manifest to make clear what the
difference is e.g. 'manifest_without_user_visible_only.json'.

https://codereview.chromium.org/785993003/diff/20001/chrome/test/data/push_me...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/785993003/diff/20001/chrome/test/data/push_me...
chrome/test/data/push_messaging/push_test.js:1: var pushData = new FutureData();
Please turn on strict mode for this file -
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode

https://codereview.chromium.org/785993003/diff/20001/chrome/test/data/push_me...
File chrome/test/data/push_messaging/test.html (right):

https://codereview.chromium.org/785993003/diff/20001/chrome/test/data/push_me...
chrome/test/data/push_messaging/test.html:7: </script>
nit: this can now go on the same line as the opening tag.

https://codereview.chromium.org/785993003/diff/20001/chrome/test/data/push_me...
File chrome/test/data/push_messaging/test_bad_manifest.html (right):

https://codereview.chromium.org/785993003/diff/20001/chrome/test/data/push_me...
chrome/test/data/push_messaging/test_bad_manifest.html:7: </script>
Also on same line as opening tag.

[Miguel Garcia, 2014-12-11 11:37:08]

https://chromiumcodereview.appspot.com/785993003/diff/20001/chrome/browser/se...
File chrome/browser/services/gcm/push_messaging_browsertest.cc (right):

https://chromiumcodereview.appspot.com/785993003/diff/20001/chrome/browser/se...
chrome/browser/services/gcm/push_messaging_browsertest.cc:165: protected:
On 2014/12/10 18:36:33, Michael van Ouwerkerk wrote:
> This should still be private. We have public getters for the server and
service.

Done.

https://chromiumcodereview.appspot.com/785993003/diff/20001/chrome/browser/se...
chrome/browser/services/gcm/push_messaging_browsertest.cc:173: void
loadTestPage() override {
mm not sure I agree here, the way this is structured we now have a page per test
which seems sensible and removes a lot of boiler plate code (having to load the
page on every test case.


On 2014/12/10 18:36:33, Michael van Ouwerkerk wrote:
> Instead of subclassing, you could add a loadTestPage method that accepts a url
> for the page to load. Just call it at the start of your test method to load a
> different page. I don't think it's overly expensive to load an extra page, but
> if you do, we can talk about other approaches.
> 
> Calling loadTestPage with a specific url at the start of the test also seems
> easier to read than using a different test class, I tend to gloss over those.

https://chromiumcodereview.appspot.com/785993003/diff/20001/chrome/browser/se...
File chrome/browser/services/gcm/push_messaging_permission_context.cc (right):

https://chromiumcodereview.appspot.com/785993003/diff/20001/chrome/browser/se...
chrome/browser/services/gcm/push_messaging_permission_context.cc:79:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2014/12/10 18:36:33, Michael van Ouwerkerk wrote:
> Why are we (sometimes) checking for threads? This entire class is expected to
> run on the same thread (UI). That can be documented at the class level, in the
> header, if you feel it's not clear enough.

This was actually not strictly required before we could have run it on the IO
thread since we were just checking preferences and not prompting. Now that we
are using the notification service it is actually requierd, hence the DCHECK

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

https://chromiumcodereview.appspot.com/785993003/diff/20001/chrome/browser/se...
chrome/browser/services/gcm/push_messaging_service_impl.cc:278: if
(permission_context == NULL || !user_visible_only) {
On 2014/12/10 18:36:33, Michael van Ouwerkerk wrote:
> nit: how about
> if (!permission_context || !user_visible_only)

well that makes it look like both variables are the same type which is not
really true, this seems more explicit. I don't feel strongly about it though.

https://chromiumcodereview.appspot.com/785993003/diff/20001/chrome/test/data/...
File chrome/test/data/push_messaging/bad_manifest.json (right):

https://chromiumcodereview.appspot.com/785993003/diff/20001/chrome/test/data/...
chrome/test/data/push_messaging/bad_manifest.json:3: }
On 2014/12/10 18:36:33, Michael van Ouwerkerk wrote:
> It seems there can be more than one way of having a bad manifest: 1) no sender
> id and 2) no user visible only. Please name this manifest to make clear what
the
> difference is e.g. 'manifest_without_user_visible_only.json'.

Done.

https://chromiumcodereview.appspot.com/785993003/diff/20001/chrome/test/data/...
File chrome/test/data/push_messaging/push_test.js (right):

https://chromiumcodereview.appspot.com/785993003/diff/20001/chrome/test/data/...
chrome/test/data/push_messaging/push_test.js:1: var pushData = new FutureData();
On 2014/12/10 18:36:33, Michael van Ouwerkerk wrote:
> Please turn on strict mode for this file -
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode

Done.

https://chromiumcodereview.appspot.com/785993003/diff/20001/chrome/test/data/...
File chrome/test/data/push_messaging/test.html (right):

https://chromiumcodereview.appspot.com/785993003/diff/20001/chrome/test/data/...
chrome/test/data/push_messaging/test.html:7: </script>
On 2014/12/10 18:36:33, Michael van Ouwerkerk wrote:
> nit: this can now go on the same line as the opening tag.

Acknowledged.

https://chromiumcodereview.appspot.com/785993003/diff/20001/chrome/test/data/...
File chrome/test/data/push_messaging/test_bad_manifest.html (right):

https://chromiumcodereview.appspot.com/785993003/diff/20001/chrome/test/data/...
chrome/test/data/push_messaging/test_bad_manifest.html:7: </script>
On 2014/12/10 18:36:33, Michael van Ouwerkerk wrote:
> Also on same line as opening tag.

Done.

[Michael van Ouwerkerk, 2014-12-11 15:55:36]

lgtm with nits

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

https://codereview.chromium.org/785993003/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_browsertest.cc:176:
https_server()->GetURL("files/push_messaging/test_bad_manifest.html"));
Well, if you really prefer to use a subclass: all you really need is a different
url. You could have a smaller override footprint by using a virtual method for
GetTestPageUrl.

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

https://codereview.chromium.org/785993003/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_permission_context.cc:92:
weak_factory_.GetWeakPtr(), id, requesting_origin,
Weak pointers and their factories are not thread safe, they're bound to a
specific thread. Please rename this factory to indicate it is for the UI thread
only e.g. weak_factory_ui_thread_.

[Miguel Garcia, 2014-12-11 16:20:07]

https://chromiumcodereview.appspot.com/785993003/diff/40001/chrome/browser/se...
File chrome/browser/services/gcm/push_messaging_browsertest.cc (right):

https://chromiumcodereview.appspot.com/785993003/diff/40001/chrome/browser/se...
chrome/browser/services/gcm/push_messaging_browsertest.cc:176:
https_server()->GetURL("files/push_messaging/test_bad_manifest.html"));
On 2014/12/11 15:55:35, Michael van Ouwerkerk wrote:
> Well, if you really prefer to use a subclass: all you really need is a
different
> url. You could have a smaller override footprint by using a virtual method for
> GetTestPageUrl.

Done.

https://chromiumcodereview.appspot.com/785993003/diff/40001/chrome/browser/se...
File chrome/browser/services/gcm/push_messaging_permission_context.cc (right):

https://chromiumcodereview.appspot.com/785993003/diff/40001/chrome/browser/se...
chrome/browser/services/gcm/push_messaging_permission_context.cc:92:
weak_factory_.GetWeakPtr(), id, requesting_origin,
On 2014/12/11 15:55:35, Michael van Ouwerkerk wrote:
> Weak pointers and their factories are not thread safe, they're bound to a
> specific thread. Please rename this factory to indicate it is for the UI
thread
> only e.g. weak_factory_ui_thread_.

Done.

[Miguel Garcia, 2014-12-11 16:20:13]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-12-11 16:21:26]

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

[commit-bot: I haz the power, 2014-12-11 16:26:20]

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

[commit-bot: I haz the power, 2014-12-11 16:26:21]

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

[Miguel Garcia, 2014-12-11 16:37:43]

TBR fgorski for the two removed files

[Miguel Garcia, 2014-12-11 16:37:53]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-12-11 16:38:45]

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

[commit-bot: I haz the power, 2014-12-11 17:44:32]

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

[commit-bot: I haz the power, 2014-12-11 17:44:33]

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

[Michael van Ouwerkerk, 2014-12-11 17:47:21]

On 2014/12/11 17:44:33, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Looks like you need a rebase.

[Miguel Garcia, 2014-12-11 18:31:52]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-12-11 18:32:44]

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

[commit-bot: I haz the power, 2014-12-11 19:59:39]

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

[commit-bot: I haz the power, 2014-12-11 19:59:40]

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

[Miguel Garcia, 2014-12-12 10:27:14]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-12-12 10:27:33]

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

[commit-bot: I haz the power, 2014-12-12 14:40:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-12 14:41:36]

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