Update the PushEvent to have a nullable PushMessageData

Update the PushEvent to have a nullable PushMessageData

The specification now allows a PushMessage to have valid non-empty data,
valid empty data, and null data. To support that, the content layer now uses
a PushEventPayload which can be null.

This also adds and updates relevant unit tests for each of the layers.

BUG=578036
Committed: https://crrev.com/dd4d2b253d29963aacb80e3a0dea42061f1f47c4
Cr-Commit-Position: refs/heads/master@{#371839}

[harkness, 2016-01-25 16:20:01]

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

[harkness, 2016-01-25 16:20:01]

The specification now allows a PushMessage to have valid non-empty data,
valid empty data, and null data. To support that, the content layer now uses
a PushEventPayload which can be null.

This also adds and updates relevant unit tests for each of the layers.

BUG=578036

[harkness, 2016-01-25 17:20:24]

The CQ bit was checked by harkness@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-25 17:21:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1636483002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1636483002/1

[commit-bot: I haz the power, 2016-01-25 17:21:57]

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

[commit-bot: I haz the power, 2016-01-25 17:21:58]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Peter Beverloo, 2016-01-25 17:39:07]

Cool, thank you! :)

https://codereview.chromium.org/1636483002/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1636483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:224:
payload.setData(message.raw_data);
nit: we don't use curly brackets for one-line conditionals in C++ (we do in
Java).

https://codereview.chromium.org/1636483002/diff/1/content/browser/browser_con...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/1636483002/diff/1/content/browser/browser_con...
content/browser/browser_context.cc:258: const content::PushEventPayload&
payload,
nit: no need for content:: (we are in the content namespace)

Applies to all other uses of content:: in files in this directory as well.

https://codereview.chromium.org/1636483002/diff/1/content/browser/push_messag...
File content/browser/push_messaging/push_messaging_router.h (right):

https://codereview.chromium.org/1636483002/diff/1/content/browser/push_messag...
content/browser/push_messaging/push_messaging_router.h:14: #include
"content/public/common/push_event_payload.h"
nit: You can forward declare PushEventPayload to avoid including the file in the
header. This does mean you have to include it in each file that uses it
separately.

https://codereview.chromium.org/1636483002/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/1636483002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_test_helper.h:120: const
PushEventPayload& data);
note that the following CL will create a conflict with your work:

https://codereview.chromium.org/1579413004/

You may have to rebase this CL once that one has landed.

https://codereview.chromium.org/1636483002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/1636483002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version_unittest.cc:712:
PushEventPayload(std::string()));
Mm. It doesn't seem to be significant for these tests that there is a non-NULL
payload. Perhaps just use the default constructor?

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
File content/public/common/push_event_payload.h (right):

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:16: // service as part of a
PushEvent.
Another thing that would be worthwhile to document is why the difference between
an empty string and a null string is significant.

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:17: 
micro nit: no need for the blank line

I think you've already used the "git cl format" tool on some of the other
directories (as they don't allow you to upload your CL unless it's properly
formatted, yay automation), but you can use this for automatic formatting of any
file you'd like.

For example, you could ask the tool to look at this file by typing:

$ git cl format content/public/common/push_event_payload.h

... and it'd fix most of the formatting style things for you.

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:18: struct CONTENT_EXPORT
PushEventPayload {
An alternative name for this structure could be "PushMessagePayload" - the
payload belongs to the message, and is only exposed through the event.

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:19: public:
micro nit: one space indent for the visibility identifiers

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:21: m_isNull(true)
naming: Blink style prefixes members with "m_", and then uses capitalization to
distinguish words, e.g.

m_myMemberProperty

Chromium style uses underscores to distinguish words, and uses an underscore
suffix for members, e.g.

my_member_property_

We're in Chromium here, so these should be named data_ and is_null_
respectively.

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:21: m_isNull(true)
micro nit: format the constructor like this:

class MyClass {
 public:
  MyClass()
      : is_null_(true) {}
};

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:23: PushEventPayload(const
std::string& data) :
We only use this constructor for the service_worker_version_unittest.cc tests,
which don't really care about the data itself. Perhaps remove it?

As a FYI nit, constructors with only a single argument in Chromium should almost
always be marked with the "explicit" keyword.

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:34: const std::string& getData()
const { return m_data; }
micro nit: blank line above here

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:38: // Data contained in the payload
I don't mind too strongly, but given that this struct has methods and accessors
for the data it, semantically, feels more like a class (in which the data_ and
is_null_ properties would be private) than a structure. Since visibility in a
struct is public by default, line 19 currently has no purpose.

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:41: 
micro nit: no blank line

https://codereview.chromium.org/1636483002/diff/1/content/renderer/service_wo...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1636483002/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:771: if
(!payload.isNull()) {
micro nit: no curly brackets

https://codereview.chromium.org/1636483002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/push_messaging/PushMessageData.cpp
(right):

https://codereview.chromium.org/1636483002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/push_messaging/PushMessageData.cpp:24: if
(messageString.isNull()) {
micro nit: no curly brackets here either

https://codereview.chromium.org/1636483002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp
(right):

https://codereview.chromium.org/1636483002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:13:
const char kPushMessageData[] = "Push Message valid data string.";
micro nit: namespaces are not indented in either of our styles

https://codereview.chromium.org/1636483002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:15:
class PushMessageDataTest : public ::testing::Test {
Since you don't need anything special from the fixture, rather than using
TEST_F() below you could remove this class entirely and use TEST() instead,
which creates a new suite with the given name.

TEST(PushMessageDataTest, ValidPayload) {}  // does not require PushMessageData
to be defined, assumes a ::testing::Test instead.

https://codereview.chromium.org/1636483002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:26: if
(data) {
GTest supports most comparisons using two directives:

ASSERT_*()
EXPECT_*()

The difference is that an ASSERT_*() will abort the test completely (like
throwing an exception), where EXPECT_*() will continue with the next steps in
the test.

In this case, |data| not being a nullptr is a prerequisite for the next
verification to be able to run, so you could rewrite this if-statement by using
an assert/expect combination instead:

ASSERT_NE(data, nullptr);
EXPECT_TRUE(s.equals(data->text()));  // won't run if |data| is a nullptr

[harkness, 2016-01-26 12:07:20]

Addressed comments.

https://codereview.chromium.org/1636483002/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1636483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:224:
payload.setData(message.raw_data);
On 2016/01/25 17:39:06, Peter Beverloo wrote:
> nit: we don't use curly brackets for one-line conditionals in C++ (we do in
> Java).

Done.

https://codereview.chromium.org/1636483002/diff/1/content/browser/browser_con...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/1636483002/diff/1/content/browser/browser_con...
content/browser/browser_context.cc:258: const content::PushEventPayload&
payload,
On 2016/01/25 17:39:06, Peter Beverloo wrote:
> nit: no need for content:: (we are in the content namespace)
> 
> Applies to all other uses of content:: in files in this directory as well.

Done.

https://codereview.chromium.org/1636483002/diff/1/content/browser/push_messag...
File content/browser/push_messaging/push_messaging_router.h (right):

https://codereview.chromium.org/1636483002/diff/1/content/browser/push_messag...
content/browser/push_messaging/push_messaging_router.h:14: #include
"content/public/common/push_event_payload.h"
On 2016/01/25 17:39:06, Peter Beverloo wrote:
> nit: You can forward declare PushEventPayload to avoid including the file in
the
> header. This does mean you have to include it in each file that uses it
> separately.

Done.

https://codereview.chromium.org/1636483002/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/1636483002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_test_helper.h:120: const
PushEventPayload& data);
On 2016/01/25 17:39:06, Peter Beverloo wrote:
> note that the following CL will create a conflict with your work:
> 
> https://codereview.chromium.org/1579413004/
> 
> You may have to rebase this CL once that one has landed.

Acknowledged.

https://codereview.chromium.org/1636483002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/1636483002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version_unittest.cc:712:
PushEventPayload(std::string()));
On 2016/01/25 17:39:06, Peter Beverloo wrote:
> Mm. It doesn't seem to be significant for these tests that there is a non-NULL
> payload. Perhaps just use the default constructor?

Done.

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
File content/public/common/push_event_payload.h (right):

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:16: // service as part of a
PushEvent.
On 2016/01/25 17:39:07, Peter Beverloo wrote:
> Another thing that would be worthwhile to document is why the difference
between
> an empty string and a null string is significant.

I've added a brief description of the fact that we have metadata in the overall
struct description, and a more explicit description of the use of the is_null_
flag where it's defined.

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:17: 
On 2016/01/25 17:39:06, Peter Beverloo wrote:
> micro nit: no need for the blank line
> 
> I think you've already used the "git cl format" tool on some of the other
> directories (as they don't allow you to upload your CL unless it's properly
> formatted, yay automation), but you can use this for automatic formatting of
any
> file you'd like.
> 
> For example, you could ask the tool to look at this file by typing:
> 
> $ git cl format content/public/common/push_event_payload.h
> 
> ... and it'd fix most of the formatting style things for you.

Interestingly, I tried a cl format on this file and it didn't resolve the extra
blank lines. I've fixed them manually.

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:18: struct CONTENT_EXPORT
PushEventPayload {
On 2016/01/25 17:39:06, Peter Beverloo wrote:
> An alternative name for this structure could be "PushMessagePayload" - the
> payload belongs to the message, and is only exposed through the event.

My goal for the structure was for it to be a possibly extensible package for all
the things we may want to surface to the Web designer as part of a PushEvent. At
that level, the PushEvent has a PushMessageData pointer which could either be a
valid value with (possibly empty) content, or a null. Because this struct wraps
both the PushMessageData content and whether it is null or not, it feels like
the correct scope for the name is at the PushEvent level.

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:19: public:
On 2016/01/25 17:39:07, Peter Beverloo wrote:
> micro nit: one space indent for the visibility identifiers

Removed, as it was no longer necessary.

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:21: m_isNull(true)
On 2016/01/25 17:39:06, Peter Beverloo wrote:
> naming: Blink style prefixes members with "m_", and then uses capitalization
to
> distinguish words, e.g.
> 
> m_myMemberProperty
> 
> Chromium style uses underscores to distinguish words, and uses an underscore
> suffix for members, e.g.
> 
> my_member_property_
> 
> We're in Chromium here, so these should be named data_ and is_null_
> respectively.

Done.

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:21: m_isNull(true)
On 2016/01/25 17:39:07, Peter Beverloo wrote:
> micro nit: format the constructor like this:
> 
> class MyClass {
>  public:
>   MyClass()
>       : is_null_(true) {}
> };

This is actually counter to the Google coding style, and I don't see anything in
the chromium coding style. Should I add it to a list to maybe one day get added
to the chromium guide?

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:23: PushEventPayload(const
std::string& data) :
On 2016/01/25 17:39:07, Peter Beverloo wrote:
> We only use this constructor for the service_worker_version_unittest.cc tests,
> which don't really care about the data itself. Perhaps remove it?
> 
> As a FYI nit, constructors with only a single argument in Chromium should
almost
> always be marked with the "explicit" keyword.

Removed it and updated the test file.

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:34: const std::string& getData()
const { return m_data; }
On 2016/01/25 17:39:06, Peter Beverloo wrote:
> micro nit: blank line above here

Done.

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:38: // Data contained in the payload
On 2016/01/25 17:39:07, Peter Beverloo wrote:
> I don't mind too strongly, but given that this struct has methods and
accessors
> for the data it, semantically, feels more like a class (in which the data_ and
> is_null_ properties would be private) than a structure. Since visibility in a
> struct is public by default, line 19 currently has no purpose.

Unfortunately, using private members makes IPC unable to simply access the data
members. 

I'm going to leave the setData accessor, but otherwise remove the get* accessors
so that this is a truer struct.

https://codereview.chromium.org/1636483002/diff/1/content/public/common/push_...
content/public/common/push_event_payload.h:41: 
On 2016/01/25 17:39:07, Peter Beverloo wrote:
> micro nit: no blank line

Done.

https://codereview.chromium.org/1636483002/diff/1/content/renderer/service_wo...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1636483002/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:771: if
(!payload.isNull()) {
On 2016/01/25 17:39:07, Peter Beverloo wrote:
> micro nit: no curly brackets

Done.

https://codereview.chromium.org/1636483002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/push_messaging/PushMessageData.cpp
(right):

https://codereview.chromium.org/1636483002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/push_messaging/PushMessageData.cpp:24: if
(messageString.isNull()) {
On 2016/01/25 17:39:07, Peter Beverloo wrote:
> micro nit: no curly brackets here either

Done.

https://codereview.chromium.org/1636483002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp
(right):

https://codereview.chromium.org/1636483002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:13:
const char kPushMessageData[] = "Push Message valid data string.";
On 2016/01/25 17:39:07, Peter Beverloo wrote:
> micro nit: namespaces are not indented in either of our styles

Done.

https://codereview.chromium.org/1636483002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:15:
class PushMessageDataTest : public ::testing::Test {
On 2016/01/25 17:39:07, Peter Beverloo wrote:
> Since you don't need anything special from the fixture, rather than using
> TEST_F() below you could remove this class entirely and use TEST() instead,
> which creates a new suite with the given name.
> 
> TEST(PushMessageDataTest, ValidPayload) {}  // does not require
PushMessageData
> to be defined, assumes a ::testing::Test instead.

Done.

https://codereview.chromium.org/1636483002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:26: if
(data) {
On 2016/01/25 17:39:07, Peter Beverloo wrote:
> GTest supports most comparisons using two directives:
> 
> ASSERT_*()
> EXPECT_*()
> 
> The difference is that an ASSERT_*() will abort the test completely (like
> throwing an exception), where EXPECT_*() will continue with the next steps in
> the test.
> 
> In this case, |data| not being a nullptr is a prerequisite for the next
> verification to be able to run, so you could rewrite this if-statement by
using
> an assert/expect combination instead:
> 
> ASSERT_NE(data, nullptr);
> EXPECT_TRUE(s.equals(data->text()));  // won't run if |data| is a nullptr

Done.

[Peter Beverloo, 2016-01-26 12:35:18]

https://codereview.chromium.org/1636483002/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/service_worker_handler.cc (right):

https://codereview.chromium.org/1636483002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/service_worker_handler.cc:445:
payload.setData(data);
Perhaps only setData() if |data.size() > 0|? (The empty-but-not-NULL case is
much less interesting for DevTools, the only user of this path.)

https://codereview.chromium.org/1636483002/diff/20001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_router.cc (right):

https://codereview.chromium.org/1636483002/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_router.cc:38: const
content::PushEventPayload& payload,
s/content:://

Elsewhere in this file too.

https://codereview.chromium.org/1636483002/diff/20001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_router.h (right):

https://codereview.chromium.org/1636483002/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_router.h:35: const
content::PushEventPayload& payload,
s/content:://

Elsewhere in this file too.

https://codereview.chromium.org/1636483002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/1636483002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:376: const
content::PushEventPayload& payload) {
s/content:://

https://codereview.chromium.org/1636483002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1636483002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:662: const
content::PushEventPayload& payload) {
s/content:://

https://codereview.chromium.org/1636483002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1636483002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.h:280: const
content::PushEventPayload& payload);
s/content:://

https://codereview.chromium.org/1636483002/diff/20001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/1636483002/diff/20001/content/public/browser/...
content/public/browser/browser_context.h:112: const content::PushEventPayload&
payload,
s/content:://

https://codereview.chromium.org/1636483002/diff/20001/content/public/common/p...
File content/public/common/push_event_payload.h (right):

https://codereview.chromium.org/1636483002/diff/20001/content/public/common/p...
content/public/common/push_event_payload.h:8: #include <stddef.h>
What do we use stddef.h for?

We should include <string>

https://codereview.chromium.org/1636483002/diff/20001/content/public/common/p...
content/public/common/push_event_payload.h:28: std::string data_;
Sorry! Since this is a struct, we should not have the trailing underscore -

https://google.github.io/styleguide/cppguide.html#Variable_Names

https://codereview.chromium.org/1636483002/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1636483002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:766: const
content::PushEventPayload& payload) {
s/content:://

https://codereview.chromium.org/1636483002/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/1636483002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:196: void
OnPushEvent(int request_id, const content::PushEventPayload& payload);
s/content:://

https://codereview.chromium.org/1636483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp
(right):

https://codereview.chromium.org/1636483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:13:
const char kPushMessageData[] = "Push Message valid data string.";
No indentation in the namespace please.

[harkness, 2016-01-26 14:49:20]

Code review updates.

https://codereview.chromium.org/1636483002/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/service_worker_handler.cc (right):

https://codereview.chromium.org/1636483002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/service_worker_handler.cc:445:
payload.setData(data);
On 2016/01/26 12:35:18, Peter Beverloo wrote:
> Perhaps only setData() if |data.size() > 0|? (The empty-but-not-NULL case is
> much less interesting for DevTools, the only user of this path.)

Done.

https://codereview.chromium.org/1636483002/diff/20001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_router.cc (right):

https://codereview.chromium.org/1636483002/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_router.cc:38: const
content::PushEventPayload& payload,
On 2016/01/26 12:35:18, Peter Beverloo wrote:
> s/content:://
> 
> Elsewhere in this file too.

Done.

https://codereview.chromium.org/1636483002/diff/20001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_router.h (right):

https://codereview.chromium.org/1636483002/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_router.h:35: const
content::PushEventPayload& payload,
On 2016/01/26 12:35:18, Peter Beverloo wrote:
> s/content:://
> 
> Elsewhere in this file too.

Done.

https://codereview.chromium.org/1636483002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/1636483002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:376: const
content::PushEventPayload& payload) {
On 2016/01/26 12:35:18, Peter Beverloo wrote:
> s/content:://

Done.

https://codereview.chromium.org/1636483002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1636483002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:662: const
content::PushEventPayload& payload) {
On 2016/01/26 12:35:18, Peter Beverloo wrote:
> s/content:://

Done.

https://codereview.chromium.org/1636483002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1636483002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.h:280: const
content::PushEventPayload& payload);
On 2016/01/26 12:35:18, Peter Beverloo wrote:
> s/content:://

Done.

https://codereview.chromium.org/1636483002/diff/20001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/1636483002/diff/20001/content/public/browser/...
content/public/browser/browser_context.h:112: const content::PushEventPayload&
payload,
On 2016/01/26 12:35:18, Peter Beverloo wrote:
> s/content:://

Done.

https://codereview.chromium.org/1636483002/diff/20001/content/public/common/p...
File content/public/common/push_event_payload.h (right):

https://codereview.chromium.org/1636483002/diff/20001/content/public/common/p...
content/public/common/push_event_payload.h:8: #include <stddef.h>
On 2016/01/26 12:35:18, Peter Beverloo wrote:
> What do we use stddef.h for?
> 
> We should include <string>

Done.

https://codereview.chromium.org/1636483002/diff/20001/content/public/common/p...
content/public/common/push_event_payload.h:28: std::string data_;
On 2016/01/26 12:35:18, Peter Beverloo wrote:
> Sorry! Since this is a struct, we should not have the trailing underscore -
> 
> https://google.github.io/styleguide/cppguide.html#Variable_Names

Done.

https://codereview.chromium.org/1636483002/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1636483002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:766: const
content::PushEventPayload& payload) {
On 2016/01/26 12:35:18, Peter Beverloo wrote:
> s/content:://

Done.

https://codereview.chromium.org/1636483002/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/1636483002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:196: void
OnPushEvent(int request_id, const content::PushEventPayload& payload);
On 2016/01/26 12:35:18, Peter Beverloo wrote:
> s/content:://

Done.

https://codereview.chromium.org/1636483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp
(right):

https://codereview.chromium.org/1636483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:13:
const char kPushMessageData[] = "Push Message valid data string.";
On 2016/01/26 12:35:18, Peter Beverloo wrote:
> No indentation in the namespace please.

I had to double check this one, because I knew I had made the changes, but it
looks like git cl format adds the indenting back in.

That does seem to match my reading of the Blink style guidelines.

[Peter Beverloo, 2016-01-26 14:53:28]

lgtm % PushMessageDataTest.cpp. Thank you!!

https://codereview.chromium.org/1636483002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp
(right):

https://codereview.chromium.org/1636483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:11:
namespace {
Let's chat about this in person. I really don't think this is correct.

[harkness, 2016-01-26 15:41:39]

harkness@chromium.org changed reviewers:
+ avi@chromium.org, falken@chromium.org, mkwst@chromium.org

[harkness, 2016-01-26 15:41:39]

mkwst@chromium.org: Please review changes in the IPC layer and WebKit code.

falken@chromium.org: Please review changes in service_worker code.

avi@chromium.org: Please review changes in Content API and browser context

Thanks!

[Marijn Kruisselbrink, 2016-01-26 17:05:39]

drive-by comment: you probably want to rebase your changes to current HEAD
(after which you wouldn't need to make any changes to service_worker_version*)

[Avi (use Gerrit), 2016-01-27 02:51:42]

lgtm with nits.

https://codereview.chromium.org/1636483002/diff/80001/content/public/common/p...
File content/public/common/push_event_payload.h (right):

https://codereview.chromium.org/1636483002/diff/80001/content/public/common/p...
content/public/common/push_event_payload.h:27: // Data contained in the payload
full sentences end in a .

https://codereview.chromium.org/1636483002/diff/80001/content/public/common/p...
content/public/common/push_event_payload.h:29: // Whether the payload is null or
not. Payloads can be valid with non-empty
blank line separating variables.

[Mike West, 2016-01-27 06:32:14]

IPC LGTM. WebKit LGTM % nits.

https://codereview.chromium.org/1636483002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp
(right):

https://codereview.chromium.org/1636483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:11:
namespace {
Why do you need the extra namespace?

https://codereview.chromium.org/1636483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:23:
EXPECT_TRUE(s.equals(data->text()));
Nit: Why not EXPECT_STREQ?

https://codereview.chromium.org/1636483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:46: }
// namespace blink
Nit: If you keep the extra namespace, label the closing `}`. It feels like there
should be some whitespace here as well, at least between the last test and the
namespace-closing braces.

[falken, 2016-01-27 10:22:58]

service_worker lgtm

[harkness, 2016-01-27 11:52:18]

https://codereview.chromium.org/1636483002/diff/80001/content/public/common/p...
File content/public/common/push_event_payload.h (right):

https://codereview.chromium.org/1636483002/diff/80001/content/public/common/p...
content/public/common/push_event_payload.h:27: // Data contained in the payload
On 2016/01/27 02:51:42, Avi (OOO until 2 Feb) wrote:
> full sentences end in a .

Done.

https://codereview.chromium.org/1636483002/diff/80001/content/public/common/p...
content/public/common/push_event_payload.h:29: // Whether the payload is null or
not. Payloads can be valid with non-empty
On 2016/01/27 02:51:42, Avi (OOO until 2 Feb) wrote:
> blank line separating variables.

Done.

https://codereview.chromium.org/1636483002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp
(right):

https://codereview.chromium.org/1636483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:11:
namespace {
On 2016/01/27 06:32:14, Mike West wrote:
> Why do you need the extra namespace?

I was trying to take the approach of other tests, and the ones I looked at had
the functional area namespace and the anonymous namespace. I did a bit of
looking around and it does look like this is inconsistent across other tests.
Peter@ pointed me to this thread which discusses the appropriate style:
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/anonym...

I don't personally have a strong opinion on the namespace wrappings. My main
goal is to have consistency, and it does seem that other related tests have the
double wrapping.

https://codereview.chromium.org/1636483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:23:
EXPECT_TRUE(s.equals(data->text()));
On 2016/01/27 06:32:14, Mike West wrote:
> Nit: Why not EXPECT_STREQ?

Done.

https://codereview.chromium.org/1636483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:46: }
// namespace blink
On 2016/01/27 06:32:14, Mike West wrote:
> Nit: If you keep the extra namespace, label the closing `}`. It feels like
there
> should be some whitespace here as well, at least between the last test and the
> namespace-closing braces.

Done.

[Peter Beverloo, 2016-01-27 12:33:54]

Still lgtm, thanks :)

https://codereview.chromium.org/1636483002/diff/100001/content/browser/push_m...
File content/browser/push_messaging/push_messaging_router.h (right):

https://codereview.chromium.org/1636483002/diff/100001/content/browser/push_m...
content/browser/push_messaging/push_messaging_router.h:66: const
PushEventPayload& data,
nit: s/data/payload/?

https://codereview.chromium.org/1636483002/diff/100001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/1636483002/diff/100001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:129: const
PushEventPayload& data);
dito

[harkness, 2016-01-27 17:18:03]

The CQ bit was checked by harkness@chromium.org

[harkness, 2016-01-27 17:18:04]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
avi@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1636483002/#ps100001
(title: "Rebase and integrate OWNERS code review comments")

[commit-bot: I haz the power, 2016-01-27 17:19:07]

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

[commit-bot: I haz the power, 2016-01-27 17:29:13]

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

[commit-bot: I haz the power, 2016-01-27 17:29:13]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Beverloo, 2016-01-27 17:30:49]

https://codereview.chromium.org/1636483002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp
(right):

https://codereview.chromium.org/1636483002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:8:
#include "third_party/WebKit/public/platform/WebString.h"
Ugh.

This should read:

#include "public/platform/WebString.h"

[harkness, 2016-01-27 17:49:07]

https://codereview.chromium.org/1636483002/diff/100001/content/browser/push_m...
File content/browser/push_messaging/push_messaging_router.h (right):

https://codereview.chromium.org/1636483002/diff/100001/content/browser/push_m...
content/browser/push_messaging/push_messaging_router.h:66: const
PushEventPayload& data,
On 2016/01/27 12:33:54, Peter Beverloo wrote:
> nit: s/data/payload/?

Done.

https://codereview.chromium.org/1636483002/diff/100001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/1636483002/diff/100001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:129: const
PushEventPayload& data);
On 2016/01/27 12:33:54, Peter Beverloo wrote:
> dito

Done.

https://codereview.chromium.org/1636483002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp
(right):

https://codereview.chromium.org/1636483002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/push_messaging/PushMessageDataTest.cpp:8:
#include "third_party/WebKit/public/platform/WebString.h"
On 2016/01/27 17:30:49, Peter Beverloo wrote:
> Ugh.
> 
> This should read:
> 
> #include "public/platform/WebString.h"

oops.

[Peter Beverloo, 2016-01-27 17:57:13]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2016-01-27 17:57:13]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
peter@chromium.org, avi@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1636483002/#ps140001
(title: "Code review comments and include")

[commit-bot: I haz the power, 2016-01-27 17:59:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1636483002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1636483002/140001

[commit-bot: I haz the power, 2016-01-27 19:26:50]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-01-27 19:27:58]

Description was changed from

==========
Update the PushEvent to have a nullable PushMessageData

The specification now allows a PushMessage to have valid non-empty data,
valid empty data, and null data. To support that, the content layer now uses
a PushEventPayload which can be null.

This also adds and updates relevant unit tests for each of the layers.

BUG=578036
==========

to

==========
Update the PushEvent to have a nullable PushMessageData

The specification now allows a PushMessage to have valid non-empty data,
valid empty data, and null data. To support that, the content layer now uses
a PushEventPayload which can be null.

This also adds and updates relevant unit tests for each of the layers.

BUG=578036

Committed: https://crrev.com/dd4d2b253d29963aacb80e3a0dea42061f1f47c4
Cr-Commit-Position: refs/heads/master@{#371839}
==========

[commit-bot: I haz the power, 2016-01-27 19:28:00]

Patchset 8 (id:??) landed as
https://crrev.com/dd4d2b253d29963aacb80e3a0dea42061f1f47c4
Cr-Commit-Position: refs/heads/master@{#371839}