Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
648623003:
PushMessagingBrowserTest for registration success and failure (Closed)
|
Created:
6 years, 2 months ago by Michael van Ouwerkerk Modified:
6 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git/+/master Project:
chromium Visibility:
Public. |
DescriptionPushMessagingBrowserTest for registration success and failure.
BUG=350377
Committed: https://crrev.com/0900680fd9312ff57f1fc25045e29ee176be376a
Cr-Commit-Position: refs/heads/master@{#300898}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Extract endpoint constant and tweak RunScript signature. #Patch Set 3 : Rebase. #
Total comments: 25
Patch Set 4 : Rebase and address Peter's feedback. #
Total comments: 2
Patch Set 5 : Move profile setter. #
Total comments: 9
Patch Set 6 : Rebase and address Filip's feedback. #Patch Set 7 : Add copyright comment. #Patch Set 8 : Fix ASAN failure. #Patch Set 9 : Rebase. #Patch Set 10 : Respond to the infobar asynchronously, like a user. #Messages
Total messages: 49 (20 generated)
mvanouwerkerk@chromium.org changed reviewers: + peter@chromium.org
mvanouwerkerk@chromium.org changed required reviewers: + peter@chromium.org
Peter, could you please take an overall look?
Whoohoo! Two tests :-). https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/fake_gcm_profile_service.cc (right): https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/fake_gcm_profile_service.cc:117: ->SetProfileForTesting(profile); It looks like PushMessagingServiceImpl is only ever constructed with a NULL profile from this code-path, which has now been changed with a late initialization of the profile through the SetProfileForTesting method. That's a bit silly. I see that there are two constructors for the GCMProfileService: GCMProfileService(Profile) GCMProfileService(Profile, GCMClientFactory) The former is used by Android, the latter by all other platforms. The PushMessagingServiceImpl constructor has no side effects, so passing this in at constructor time should be fine, aside from conflicting with the Android constructor. I'm not sure if there's a clean way to get this to work. Perhaps we could come up with some distinct signature for the testing constructor to avoid the duplication, allowing us to initialize the PushMessagingServiceImpl directly with the Profile*? https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:29: browser->tab_strip_model()->GetActiveWebContents())), nit: I'd initialize this in the constructor's body since it's a non-trivial initialization. That's preference, though. https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:57: // InProcessBrowserTest: micro nit: Perhaps group the three "InProcessBrowserTest:" comments into a single "// InProcessBrowserTest overrides."? https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:86: void RunScript(const std::string& script, const std::string& expected) { I would prefer if we could factor |expected| out of here. RunScript doesn't sound like something that takes an expected result. Especially since it ASSERTs instead of EXPECTs. Perhaps: std::string RunScript(const std::string& script); std::string RegisterServiceWorker(); std::string RegisterPush(); EXPECT_EQ("ok", RegisterServiceWorker()); Alternatively, you could move the ASSERT_EQ() to the Register{ServiceWorker,Push}AndExpect() instead. https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:100: return RunScript("registerPush('1234567890')", expected); nit: Will we always pass 1234567890 for tests, or does it make sense to test for validity of sender Ids? I recall that you wanted to serialize it somewhere, but it looks like we don't validate it anywhere at all. (Is it even validatable?) Relatedly, why do we still pass up a senderId through the Blink API? It's even passed to the browser process if the manifest key is not available. Mind if I send you two patches to nuke that code path? https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:113: RegisterPushAndExpect("https://android.googleapis.com/gcm/send - 1"); Is there some kind of constant or method we can use to refer to the registration end point rather than hard-coding it as a string in this test? https://codereview.chromium.org/648623003/diff/1/chrome/test/data/push_messag... File chrome/test/data/push_messaging/sw.js (right): https://codereview.chromium.org/648623003/diff/1/chrome/test/data/push_messag... chrome/test/data/push_messaging/sw.js:1: // Empty service worker script. Please name this file service_worker.js. (The initialism "SW" is a bit overloaded.) nit: Add a TODO to indicate that a "push" event listener has to be added here for future tests? https://codereview.chromium.org/648623003/diff/1/chrome/test/data/push_messag... File chrome/test/data/push_messaging/test.html (right): https://codereview.chromium.org/648623003/diff/1/chrome/test/data/push_messag... chrome/test/data/push_messaging/test.html:1: <html> nit: <!DOCTYPE html> https://codereview.chromium.org/648623003/diff/1/chrome/test/data/push_messag... chrome/test/data/push_messaging/test.html:3: <title>Push API</title> micro nit: This is not the Push API, it's a test runner for the Push API. Dito with the page's body -- for people randomly coming across this file it might be nice to add a little bit (half a sentence) of explanation.
Thanks Peter! Please take another look. https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/fake_gcm_profile_service.cc (right): https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/fake_gcm_profile_service.cc:117: ->SetProfileForTesting(profile); On 2014/10/17 14:04:07, Peter Beverloo wrote: > It looks like PushMessagingServiceImpl is only ever constructed with a NULL > profile from this code-path, which has now been changed with a late > initialization of the profile through the SetProfileForTesting method. That's a > bit silly. > > I see that there are two constructors for the GCMProfileService: > GCMProfileService(Profile) > GCMProfileService(Profile, GCMClientFactory) > > The former is used by Android, the latter by all other platforms. The > PushMessagingServiceImpl constructor has no side effects, so passing this in at > constructor time should be fine, aside from conflicting with the Android > constructor. > > I'm not sure if there's a clean way to get this to work. Perhaps we could come > up with some distinct signature for the testing constructor to avoid the > duplication, allowing us to initialize the PushMessagingServiceImpl directly > with the Profile*? It would be nice to just inject the dependencies it needs using the constructor, but it's a pain because of the platform differences. The late setting of the dependency seems as reasonable as any other approach. The driver for example is already set in this way as well. https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:29: browser->tab_strip_model()->GetActiveWebContents())), On 2014/10/17 14:04:07, Peter Beverloo wrote: > nit: I'd initialize this in the constructor's body since it's a non-trivial > initialization. That's preference, though. I don't mind strongly one way or the other. It's not trivial syntax but in the end it's just a chain of getters. If there are no strong arguments either way I'd rather leave it as is for now. https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:57: // InProcessBrowserTest: On 2014/10/17 14:04:08, Peter Beverloo wrote: > micro nit: Perhaps group the three "InProcessBrowserTest:" comments into a > single "// InProcessBrowserTest overrides."? I find the current pattern easier to read. If you don't mind, I'd rather leave it as is. https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:86: void RunScript(const std::string& script, const std::string& expected) { On 2014/10/17 14:04:08, Peter Beverloo wrote: > I would prefer if we could factor |expected| out of here. RunScript doesn't > sound like something that takes an expected result. Especially since it ASSERTs > instead of EXPECTs. > > Perhaps: > std::string RunScript(const std::string& script); > std::string RegisterServiceWorker(); > std::string RegisterPush(); > > EXPECT_EQ("ok", RegisterServiceWorker()); > > Alternatively, you could move the ASSERT_EQ() to the > Register{ServiceWorker,Push}AndExpect() instead. Done. https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:100: return RunScript("registerPush('1234567890')", expected); On 2014/10/17 14:04:07, Peter Beverloo wrote: > nit: Will we always pass 1234567890 for tests, or does it make sense to test for > validity of sender Ids? I recall that you wanted to serialize it somewhere, but > it looks like we don't validate it anywhere at all. (Is it even validatable?) The sender id is opaque from the browser perspective. We might eventually change the API to handle multiple senders, but for now this seems like a clean solution. > Relatedly, why do we still pass up a senderId through the Blink API? It's even > passed to the browser process if the manifest key is not available. Mind if I > send you two patches to nuke that code path? Sure, we should get rid of the non-manifest path. But as agreed, tests first. https://codereview.chromium.org/648623003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:113: RegisterPushAndExpect("https://android.googleapis.com/gcm/send - 1"); On 2014/10/17 14:04:07, Peter Beverloo wrote: > Is there some kind of constant or method we can use to refer to the registration > end point rather than hard-coding it as a string in this test? Done.
mvanouwerkerk@chromium.org changed reviewers: + fgorski@chromium.org
mvanouwerkerk@chromium.org changed required reviewers: + fgorski@chromium.org
Filip: please review as owner of the gcm directory.
mvanouwerkerk@chromium.org changed reviewers: + phajdan.jr@chromium.org
mvanouwerkerk@chromium.org changed required reviewers: + phajdan.jr@chromium.org
Pawel: please review as owner of net/tools/testserver/testserver.py
net/tools LGTM
Michael, please check whether you replied to all comments before asking a reviewer to take another look. I carried three forwards. https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:28: InfoBarObserver(Browser* browser, bool accept) Is it much more noise to pass in a WebContents* rather than a Browser*? https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:28: InfoBarObserver(Browser* browser, bool accept) I don't really like the name of InfoBarObserver given that it also accepts or cancels the permission request. Perhaps something like InfoBarPermissionGranter? https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:40: infobar->delegate()->AsConfirmInfoBarDelegate(); Can we check somehow if the infobar which you're dealing with here is the one you're expecting it to be? Also, please add a DCHECK(delegate). https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:61: switches::kEnableExperimentalWebPlatformFeatures); + InProcessBrowserTest::SetUpCommandLine() https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:84: browser(), https_server_->GetURL("files/push_messaging/test.html")); + InProcessBrowserTest::SetUpOnMainThread() https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:88: ASSERT_TRUE(content::ExecuteScriptAndExtractString( This still has the problem that an ASSERT_*() in a called function will not actually stop the test from running. At the very least, this should be an EXPECT_TRUE(), but you could also opt to return the boolean instead and ASSERT_TRUE on RunScript() in the test body. https://codereview.chromium.org/648623003/diff/40001/chrome/test/data/push_me... File chrome/test/data/push_messaging/sw.js (right): https://codereview.chromium.org/648623003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/sw.js:1: // Empty service worker script. Carry-forward since you did not reply to this. Please carefully check if you replied to all comments before asking a reviewer to take another look. ===== Please name this file service_worker.js. (The initialism "SW" is a bit overloaded.) nit: Add a TODO to indicate that a "push" event listener has to be added here for future tests? https://codereview.chromium.org/648623003/diff/40001/chrome/test/data/push_me... File chrome/test/data/push_messaging/test.html (right): https://codereview.chromium.org/648623003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:1: <html> Carry-forward since you did not reply. ===== nit: <!DOCTYPE html> https://codereview.chromium.org/648623003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:3: <title>Push API</title> Carry-forward since you did not reply. ===== micro nit: This is not the Push API, it's a test runner for the Push API. Dito with the page's body -- for people randomly coming across this file it might be nice to add a little bit (half a sentence) of explanation.
Thanks Peter. Please take another look. https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:28: InfoBarObserver(Browser* browser, bool accept) On 2014/10/20 12:45:22, Peter Beverloo wrote: > I don't really like the name of InfoBarObserver given that it also accepts or > cancels the permission request. Perhaps something like InfoBarPermissionGranter? InfoBarResponder then, as it can deny as well as grant. https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:28: InfoBarObserver(Browser* browser, bool accept) On 2014/10/20 12:45:22, Peter Beverloo wrote: > Is it much more noise to pass in a WebContents* rather than a Browser*? I think so. https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:40: infobar->delegate()->AsConfirmInfoBarDelegate(); On 2014/10/20 12:45:22, Peter Beverloo wrote: > Can we check somehow if the infobar which you're dealing with here is the one > you're expecting it to be? Also, please add a DCHECK(delegate). That seems to require Run-Time Type Information. But, I've changed it to be a one-shot, by removing itself when it does observe an infobar. This means that if it somehow (not sure in what case) observes a different infobar than intended, the intended infobar will not be handled and the test will fail. https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:61: switches::kEnableExperimentalWebPlatformFeatures); On 2014/10/20 12:45:22, Peter Beverloo wrote: > + InProcessBrowserTest::SetUpCommandLine() Done. https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:84: browser(), https_server_->GetURL("files/push_messaging/test.html")); On 2014/10/20 12:45:22, Peter Beverloo wrote: > + InProcessBrowserTest::SetUpOnMainThread() Done. https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:88: ASSERT_TRUE(content::ExecuteScriptAndExtractString( On 2014/10/20 12:45:22, Peter Beverloo wrote: > This still has the problem that an ASSERT_*() in a called function will not > actually stop the test from running. At the very least, this should be an > EXPECT_TRUE(), but you could also opt to return the boolean instead and > ASSERT_TRUE on RunScript() in the test body. It should be an ASSERT, as it would not make sense to continue running the test if js execution failed. Still, pulling it up to the test level makes sense, so I did that. https://codereview.chromium.org/648623003/diff/40001/chrome/test/data/push_me... File chrome/test/data/push_messaging/sw.js (right): https://codereview.chromium.org/648623003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/sw.js:1: // Empty service worker script. On 2014/10/20 12:45:22, Peter Beverloo wrote: > Carry-forward since you did not reply to this. Please carefully check if you > replied to all comments before asking a reviewer to take another look. > > ===== > > Please name this file service_worker.js. (The initialism "SW" is a bit > overloaded.) > > nit: Add a TODO to indicate that a "push" event listener has to be added here > for future tests? Done. https://codereview.chromium.org/648623003/diff/40001/chrome/test/data/push_me... File chrome/test/data/push_messaging/test.html (right): https://codereview.chromium.org/648623003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:1: <html> On 2014/10/20 12:45:22, Peter Beverloo wrote: > Carry-forward since you did not reply. > > ===== > > nit: <!DOCTYPE html> Done. https://codereview.chromium.org/648623003/diff/40001/chrome/test/data/push_me... chrome/test/data/push_messaging/test.html:3: <title>Push API</title> On 2014/10/20 12:45:22, Peter Beverloo wrote: > Carry-forward since you did not reply. > > ===== > > micro nit: This is not the Push API, it's a test runner for the Push API. Dito > with the page's body -- for people randomly coming across this file it might be > nice to add a little bit (half a sentence) of explanation. It's a test, so I marked it as such now. Not sure what else you would like me to write in terms of explanation, it looks fine to me.
lgtm I'll defer to Filip for the GCM specific bits. https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:28: InfoBarObserver(Browser* browser, bool accept) On 2014/10/20 15:30:37, Michael van Ouwerkerk wrote: > On 2014/10/20 12:45:22, Peter Beverloo wrote: > > I don't really like the name of InfoBarObserver given that it also accepts or > > cancels the permission request. Perhaps something like > InfoBarPermissionGranter? > > InfoBarResponder then, as it can deny as well as grant. Acknowledged, that sounds better :-). https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:28: InfoBarObserver(Browser* browser, bool accept) On 2014/10/20 15:30:37, Michael van Ouwerkerk wrote: > On 2014/10/20 12:45:22, Peter Beverloo wrote: > > Is it much more noise to pass in a WebContents* rather than a Browser*? > > I think so. Did you try this? The following does not look that much worse to me. ----- auto accepting_responder(browser()->tab_strip_model()->GetActiveWebContents(), true); ----- ("I think so." is a very dismissive response.) https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:40: infobar->delegate()->AsConfirmInfoBarDelegate(); On 2014/10/20 15:30:37, Michael van Ouwerkerk wrote: > On 2014/10/20 12:45:22, Peter Beverloo wrote: > > Can we check somehow if the infobar which you're dealing with here is the one > > you're expecting it to be? Also, please add a DCHECK(delegate). > > That seems to require Run-Time Type Information. But, I've changed it to be a > one-shot, by removing itself when it does observe an infobar. This means that if > it somehow (not sure in what case) observes a different infobar than intended, > the intended infobar will not be handled and the test will fail. I was hoping there'd be some way to get the owner or some kind of non-textual, consistent ID. If there isn't then that's OK. https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:88: ASSERT_TRUE(content::ExecuteScriptAndExtractString( On 2014/10/20 15:30:37, Michael van Ouwerkerk wrote: > On 2014/10/20 12:45:22, Peter Beverloo wrote: > > This still has the problem that an ASSERT_*() in a called function will not > > actually stop the test from running. At the very least, this should be an > > EXPECT_TRUE(), but you could also opt to return the boolean instead and > > ASSERT_TRUE on RunScript() in the test body. > > It should be an ASSERT, as it would not make sense to continue running the test > if js execution failed. Still, pulling it up to the test level makes sense, so I > did that. My point is that ASSERTs don't work in called methods :-). https://codereview.chromium.org/648623003/diff/60001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/648623003/diff/60001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_service_impl.cc:259: void PushMessagingServiceImpl::SetProfileForTesting(Profile* profile) { nit: Move this to *before* the DeliverMessageCallback() implementation. Header files and implementation files should follow the same order.
Thanks Peter. https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:28: InfoBarObserver(Browser* browser, bool accept) On 2014/10/20 16:35:52, Peter Beverloo wrote: > On 2014/10/20 15:30:37, Michael van Ouwerkerk wrote: > > On 2014/10/20 12:45:22, Peter Beverloo wrote: > > > Is it much more noise to pass in a WebContents* rather than a Browser*? > > > > I think so. > > Did you try this? The following does not look that much worse to me. > > ----- > auto accepting_responder(browser()->tab_strip_model()->GetActiveWebContents(), > true); > ----- > > ("I think so." is a very dismissive response.) I didn't mean to dismiss a serious concern, this looked like a minor nit to me, sorry if it wasn't. I tried Peter, I do think it's needlessly verbose. The class being instantiated is only used in this test, so passing in a Browser* is a trivial requirement. Were it a general purpose class I would probably agree with you. Anyway, using auto does not work like that, you need to hint at the type a bit more explicitly. https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:40: infobar->delegate()->AsConfirmInfoBarDelegate(); On 2014/10/20 16:35:52, Peter Beverloo wrote: > On 2014/10/20 15:30:37, Michael van Ouwerkerk wrote: > > On 2014/10/20 12:45:22, Peter Beverloo wrote: > > > Can we check somehow if the infobar which you're dealing with here is the > one > > > you're expecting it to be? Also, please add a DCHECK(delegate). > > > > That seems to require Run-Time Type Information. But, I've changed it to be a > > one-shot, by removing itself when it does observe an infobar. This means that > if > > it somehow (not sure in what case) observes a different infobar than intended, > > the intended infobar will not be handled and the test will fail. > > I was hoping there'd be some way to get the owner or some kind of non-textual, > consistent ID. If there isn't then that's OK. Acknowledged. https://codereview.chromium.org/648623003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:88: ASSERT_TRUE(content::ExecuteScriptAndExtractString( On 2014/10/20 16:35:52, Peter Beverloo wrote: > On 2014/10/20 15:30:37, Michael van Ouwerkerk wrote: > > On 2014/10/20 12:45:22, Peter Beverloo wrote: > > > This still has the problem that an ASSERT_*() in a called function will not > > > actually stop the test from running. At the very least, this should be an > > > EXPECT_TRUE(), but you could also opt to return the boolean instead and > > > ASSERT_TRUE on RunScript() in the test body. > > > > It should be an ASSERT, as it would not make sense to continue running the > test > > if js execution failed. Still, pulling it up to the test level makes sense, so > I > > did that. > > My point is that ASSERTs don't work in called methods :-). Acknowledged. https://codereview.chromium.org/648623003/diff/60001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/648623003/diff/60001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_service_impl.cc:259: void PushMessagingServiceImpl::SetProfileForTesting(Profile* profile) { On 2014/10/20 16:35:52, Peter Beverloo wrote: > nit: Move this to *before* the DeliverMessageCallback() implementation. Header > files and implementation files should follow the same order. Done.
mostly looks ok. A bunch of nits and question regarding the consts file. https://codereview.chromium.org/648623003/diff/80001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://codereview.chromium.org/648623003/diff/80001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:57: class PushMessagingBrowserTest : public InProcessBrowserTest { Don't you want your tests to be in ::gcm namespace? why? https://codereview.chromium.org/648623003/diff/80001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:112: gcm::FakeGCMProfileService* gcm_service_; nit: make both private and expose through getters. FakeGCMProfileService* gcm_service() const { return gcm_service_; } https://codereview.chromium.org/648623003/diff/80001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:130: EXPECT_EQ(gcm_service_->last_registered_app_id(), expected_id.ToString()); expected goes first, actual follows. Please fix everywhere you make a call to EXPECT. https://codereview.chromium.org/648623003/diff/80001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_constants.h (right): https://codereview.chromium.org/648623003/diff/80001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_constants.h:10: extern const char kPushMessagingEndpoint[]; Why do you put this const into a separate file? did you consider push_messaging_service.h or _impl.h?
Thanks Filip! Please take another look. https://codereview.chromium.org/648623003/diff/80001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://codereview.chromium.org/648623003/diff/80001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:57: class PushMessagingBrowserTest : public InProcessBrowserTest { On 2014/10/20 22:29:07, fgorski wrote: > Don't you want your tests to be in ::gcm namespace? why? No reason not to. Done! https://codereview.chromium.org/648623003/diff/80001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:112: gcm::FakeGCMProfileService* gcm_service_; On 2014/10/20 22:29:08, fgorski wrote: > nit: make both private and expose through getters. > > FakeGCMProfileService* gcm_service() const { return gcm_service_; } Done. https://codereview.chromium.org/648623003/diff/80001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_browsertest.cc:130: EXPECT_EQ(gcm_service_->last_registered_app_id(), expected_id.ToString()); On 2014/10/20 22:29:08, fgorski wrote: > expected goes first, actual follows. > > Please fix everywhere you make a call to EXPECT. Done. https://codereview.chromium.org/648623003/diff/80001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_constants.h (right): https://codereview.chromium.org/648623003/diff/80001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_constants.h:10: extern const char kPushMessagingEndpoint[]; On 2014/10/20 22:29:08, fgorski wrote: > Why do you put this const into a separate file? > > did you consider push_messaging_service.h or _impl.h? It seems cleaner. If you need the constant, you only need to pull in this file. I don't feel strongly about it, except I'd rather avoid adding it to the public content API, there's always a lot of pressure to avoid adding things to it.
lgtm https://codereview.chromium.org/648623003/diff/80001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_constants.h (right): https://codereview.chromium.org/648623003/diff/80001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_constants.h:10: extern const char kPushMessagingEndpoint[]; On 2014/10/21 10:32:01, Michael van Ouwerkerk wrote: > On 2014/10/20 22:29:08, fgorski wrote: > > Why do you put this const into a separate file? > > > > did you consider push_messaging_service.h or _impl.h? > > It seems cleaner. If you need the constant, you only need to pull in this file. > I don't feel strongly about it, except I'd rather avoid adding it to the public > content API, there's always a lot of pressure to avoid adding things to it. Acknowledged.
The CQ bit was checked by mvanouwerkerk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/648623003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by mvanouwerkerk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/648623003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mvanouwerkerk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/648623003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mvanouwerkerk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/648623003/120001
The CQ bit was checked by mvanouwerkerk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/648623003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mvanouwerkerk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/648623003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32 on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mvanouwerkerk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/648623003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mvanouwerkerk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/648623003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/0900680fd9312ff57f1fc25045e29ee176be376a Cr-Commit-Position: refs/heads/master@{#300898} |
