Update the PushEvent to have a nullable PushMessageData

content/public/common/push_event_payload.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_PUBLIC_COMMON_PUSH_EVENT_PAYLOAD_H_
+#define CONTENT_PUBLIC_COMMON_PUSH_EVENT_PAYLOAD_H_
+
+#include <stddef.h>
+
+#include "content/common/content_export.h"
+
+namespace content {
+
+// Structure representing the payload delivered as part of a push message.
+// This struct contains the decrypted information sent from the push
+// service as part of a PushEvent.

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

Another thing that would be worthwhile to document is why the difference between
an empty string and a null string is significant.

[harkness, 2016/01/26 12:07:19]

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. 

+

[Peter Beverloo, 2016/01/25 17:39:06]

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.

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

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

+struct CONTENT_EXPORT PushEventPayload {

[Peter Beverloo, 2016/01/25 17:39:06]

An alternative name for this structure could be "PushMessagePayload" - the
payload belongs to the message, and is only exposed through the event.

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

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.

+public:

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

micro nit: one space indent for the visibility identifiers

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

Removed, as it was no longer necessary.

+  PushEventPayload() :
+    m_isNull(true)

[Peter Beverloo, 2016/01/25 17:39:06]

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.

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

micro nit: format the constructor like this:

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

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

Done.

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

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?

+  {}
+  PushEventPayload(const std::string& data) :

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

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.

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

Removed it and updated the test file.

+    m_data(data),
+    m_isNull(false)
+  {}
+
+  ~PushEventPayload() {}
+
+  void setData(const std::string& data) {
+    m_data = data;
+    m_isNull = false;
+  }
+  const std::string& getData() const { return m_data; }

[Peter Beverloo, 2016/01/25 17:39:06]

micro nit: blank line above here

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

Done.

+
+  bool isNull() const { return m_isNull; }
+
+  // Data contained in the payload

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

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.

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

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.

+  std::string m_data;
+  bool m_isNull;
+

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

micro nit: no blank line

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

Done.

+};
+
+}  // namespace content
+
+#endif // CONTENT_PUBLIC_COMMON_PUSH_EVENT_PAYLOAD_H_