Define PushMessagingApplicationId and replace ad-hoc code.

Define PushMessagingApplicationId and replace ad-hoc code.
BUG=350377
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285571

[Michael van Ouwerkerk, 2014-07-22 11:18:32]

John, could you please review this from a feature perspective?

[Michael van Ouwerkerk, 2014-07-22 13:35:27]

Hi Filip, could you please review this as GCM owner?

[johnme, 2014-07-23 12:36:57]

lgtm with nits - thanks for the cleanup :)

https://codereview.chromium.org/404353003/diff/1/content/browser/push_messagi...
File content/browser/push_messaging_application_id_unittest.cc (right):

https://codereview.chromium.org/404353003/diff/1/content/browser/push_messagi...
content/browser/push_messaging_application_id_unittest.cc:47:
"push#https://www.example.com/#foo#1").is_valid());
Nit: maybe change this to "push#https://www.example.com/#1#1" (or add a separate
test for that)?

https://codereview.chromium.org/404353003/diff/1/content/public/browser/push_...
File content/public/browser/push_messaging_application_id.h (right):

https://codereview.chromium.org/404353003/diff/1/content/public/browser/push_...
content/public/browser/push_messaging_application_id.h:19: // Type used to
identify an "application" from a Push API perspective.
Nit: s/an "application"/a web app/ ?

https://codereview.chromium.org/404353003/diff/1/content/public/browser/push_...
content/public/browser/push_messaging_application_id.h:31: bool is_valid() {
Nit: this isn't a simple getter, so IIRC the definition should go in the .cc

https://codereview.chromium.org/404353003/diff/1/content/public/browser/push_...
content/public/browser/push_messaging_application_id.h:38: GURL origin;
Perhaps these should be immutable? You might even do the validity check in the
2nd constructor, then you could have is_valid() { return
service_worker_registration_id >= 0; }

[Michael van Ouwerkerk, 2014-07-23 15:05:14]

Thanks John!

https://codereview.chromium.org/404353003/diff/1/content/browser/push_messagi...
File content/browser/push_messaging_application_id_unittest.cc (right):

https://codereview.chromium.org/404353003/diff/1/content/browser/push_messagi...
content/browser/push_messaging_application_id_unittest.cc:47:
"push#https://www.example.com/#foo#1").is_valid());
On 2014/07/23 12:36:57, johnme wrote:
> Nit: maybe change this to "push#https://www.example.com/#1#1" (or add a
separate
> test for that)?

Done.

https://codereview.chromium.org/404353003/diff/1/content/public/browser/push_...
File content/public/browser/push_messaging_application_id.h (right):

https://codereview.chromium.org/404353003/diff/1/content/public/browser/push_...
content/public/browser/push_messaging_application_id.h:19: // Type used to
identify an "application" from a Push API perspective.
On 2014/07/23 12:36:57, johnme wrote:
> Nit: s/an "application"/a web app/ ?

Done.

https://codereview.chromium.org/404353003/diff/1/content/public/browser/push_...
content/public/browser/push_messaging_application_id.h:31: bool is_valid() {
On 2014/07/23 12:36:57, johnme wrote:
> Nit: this isn't a simple getter, so IIRC the definition should go in the .cc

Done.

https://codereview.chromium.org/404353003/diff/1/content/public/browser/push_...
content/public/browser/push_messaging_application_id.h:38: GURL origin;
Ok they're const now.

[Michael van Ouwerkerk, 2014-07-23 15:06:06]

jam, could you please review this as content owner?

[jam, 2014-07-23 17:24:47]

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

https://codereview.chromium.org/404353003/diff/20001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:91:
service()->Register(application_id.ToString(),
it seems that this is the only time where this new struct is used in content.

can we move the struct to chrome, and change the Register method to take the
origin.spec() and service worker id as parameters?

[fgorski, 2014-07-23 18:04:09]

gcm piece lgtm, one question to the unit tests file location.

https://codereview.chromium.org/404353003/diff/20001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/404353003/diff/20001/content/content_tests.gy...
content/content_tests.gypi:531:
'browser/push_messaging_application_id_unittest.cc',
your struct is in content/public/browser and tests are in content/browser
is that intended?

[Michael van Ouwerkerk, 2014-07-24 13:01:35]

Thanks for the reviews!

jam: please take another look.

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

https://codereview.chromium.org/404353003/diff/20001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:91:
service()->Register(application_id.ToString(),
On 2014/07/23 17:24:47, jam wrote:
> it seems that this is the only time where this new struct is used in content.
> 
> can we move the struct to chrome, and change the Register method to take the
> origin.spec() and service worker id as parameters?

Just to clarify: are you suggesting this so we can avoid adding a new type in
the public content API, and to make the unit test setup make more sense?
Assuming that's the reasoning: done.

https://codereview.chromium.org/404353003/diff/20001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/404353003/diff/20001/content/content_tests.gy...
content/content_tests.gypi:531:
'browser/push_messaging_application_id_unittest.cc',
On 2014/07/23 18:04:09, fgorski wrote:
> your struct is in content/public/browser and tests are in content/browser
> is that intended?

Yes I did that intentionally. There aren't really any unit tests for the public
content API because they are supposed to contain no logic, no implementations.
So I was kind of bending the rules here, hoping for guidance from jam. I think I
got that guidance :-)

[jam, 2014-07-24 20:09:42]

lgtm

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

https://codereview.chromium.org/404353003/diff/20001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:91:
service()->Register(application_id.ToString(),
On 2014/07/24 13:01:35, Michael van Ouwerkerk wrote:
> On 2014/07/23 17:24:47, jam wrote:
> > it seems that this is the only time where this new struct is used in
content.
> > 
> > can we move the struct to chrome, and change the Register method to take the
> > origin.spec() and service worker id as parameters?
> 
> Just to clarify: are you suggesting this so we can avoid adding a new type in
> the public content API, and to make the unit test setup make more sense?
> Assuming that's the reasoning: done.

to make the content api simpler

[Michael van Ouwerkerk, 2014-07-25 10:40:49]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2014-07-25 10:41:45]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/404353003/4...

[commit-bot: I haz the power, 2014-07-25 12:29:37]

Change committed as 285571