Reviewer feedback

src/core/ext/filters/client_channel/client_channel_channelz.cc

@@ -26,14 +26,15 @@
 
 namespace grpc_core {
 namespace channelz {
+namespace {
 
-static void* client_channel_channelz_copy(void* p) { return p; }
+void* client_channel_channelz_copy(void* p) { return p; }
 
-static void client_channel_channelz_destroy(void* p) {}
+void client_channel_channelz_destroy(void* p) {}
 
-static int client_channel_channelz_cmp(void* a, void* b) {
+int client_channel_channelz_cmp(void* a, void* b) { return GPR_ICMP(a, b); }
-  return GPR_ICMP(a, b);
+
-}
+}  // namespace
 
 static const grpc_arg_pointer_vtable client_channel_channelz_vtable = {
     client_channel_channelz_copy, client_channel_channelz_destroy,
@@ -62,17 +63,17 @@ void ClientChannelNode::PopulateConnectivityState(grpc_json* json) {
                          false);
 }
 
-grpc_arg ClientChannelNode::CreateArg() {
+grpc_arg ClientChannelNode::CreateChannelArg() {
   return grpc_channel_arg_pointer_create(
       const_cast<char*>(GRPC_ARG_CHANNELZ_CHANNEL_NODE_CREATION_FUNC),
       reinterpret_cast<void*>(MakeClientChannelNode),
       &client_channel_channelz_vtable);
 }
 
-RefCountedPtr<ChannelNode> MakeClientChannelNode(
+RefCountedPtr<ChannelNode> ClientChannelNode::MakeClientChannelNode(
     grpc_channel* channel, size_t channel_tracer_max_nodes) {
-  return RefCountedPtr<ChannelNode>(
+  return MakePolymorphicRefCounted<ChannelNode, ClientChannelNode>(
-      New<ClientChannelNode>(channel, channel_tracer_max_nodes));
+      channel, channel_tracer_max_nodes);
 }
 
 }  // namespace channelz

src/core/ext/filters/client_channel/client_channel_channelz.h

@@ -31,8 +31,8 @@ namespace channelz {
 // functionality like querying for connectivity_state and subchannel data.
 class ClientChannelNode : public ChannelNode {
  public:
-  ClientChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes);
+  static RefCountedPtr<ChannelNode> MakeClientChannelNode(
-  virtual ~ClientChannelNode() {}
+      grpc_channel* channel, size_t channel_tracer_max_nodes);
 
   // Override this functionality since client_channels have a notion of
   // channel connectivity.
@@ -40,17 +40,18 @@ class ClientChannelNode : public ChannelNode {
 
   // Helper to create a channel arg to ensure this type of ChannelNode is
   // created.
-  static grpc_arg CreateArg();
+  static grpc_arg CreateChannelArg();
+
+ protected:
+  GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
+  GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW
+  ClientChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes);
+  virtual ~ClientChannelNode() {}
 
  private:
   grpc_channel_element* client_channel_;
 };
 
-RefCountedPtr<ChannelNode> MakeClientChannelNode(
-    grpc_channel* channel, size_t channel_tracer_max_nodes);
-
-grpc_arg BlahBlah();
-
 }  // namespace channelz
 }  // namespace grpc_core
 

src/core/ext/filters/client_channel/client_channel_plugin.cc

@@ -25,6 +25,7 @@
 #include <grpc/support/alloc.h>
 
 #include "src/core/ext/filters/client_channel/client_channel.h"
+#include "src/core/ext/filters/client_channel/client_channel_channelz.h"
 #include "src/core/ext/filters/client_channel/http_connect_handshaker.h"
 #include "src/core/ext/filters/client_channel/http_proxy.h"
 #include "src/core/ext/filters/client_channel/lb_policy_registry.h"
@@ -35,6 +36,14 @@
 #include "src/core/lib/surface/channel_init.h"
 
 static bool append_filter(grpc_channel_stack_builder* builder, void* arg) {
+  const grpc_channel_args* args =
+      grpc_channel_stack_builder_get_channel_arguments(builder);
+  grpc_arg args_to_add[] = {
+      grpc_core::channelz::ClientChannelNode::CreateChannelArg()};
+  grpc_channel_args* new_args = grpc_channel_args_copy_and_add(
+      args, args_to_add, GPR_ARRAY_SIZE(args_to_add));
+  grpc_channel_stack_builder_set_channel_arguments(builder, new_args);
+  grpc_channel_args_destroy(new_args);
   return grpc_channel_stack_builder_append_filter(
       builder, static_cast<const grpc_channel_filter*>(arg), nullptr, nullptr);
 }

src/core/ext/transport/chttp2/client/insecure/channel_create.cc

@@ -26,7 +26,6 @@
 #include <grpc/support/string_util.h>
 
 #include "src/core/ext/filters/client_channel/client_channel.h"
-#include "src/core/ext/filters/client_channel/client_channel_channelz.h"
 #include "src/core/ext/filters/client_channel/resolver_registry.h"
 #include "src/core/ext/transport/chttp2/client/authority.h"
 #include "src/core/ext/transport/chttp2/client/chttp2_connector.h"
@@ -93,11 +92,10 @@ grpc_channel* grpc_insecure_channel_create(const char* target,
       "grpc_insecure_channel_create(target=%s, args=%p, reserved=%p)", 3,
       (target, args, reserved));
   GPR_ASSERT(reserved == nullptr);
-  grpc_arg args_to_add[] = {
+  // Add channel arg containing the client channel factory.
-      grpc_client_channel_factory_create_channel_arg(&client_channel_factory),
+  grpc_arg arg =
-      grpc_core::channelz::ClientChannelNode::CreateArg()};
+      grpc_client_channel_factory_create_channel_arg(&client_channel_factory);
-  grpc_channel_args* new_args = grpc_channel_args_copy_and_add(
+  grpc_channel_args* new_args = grpc_channel_args_copy_and_add(args, &arg, 1);
-      args, args_to_add, GPR_ARRAY_SIZE(args_to_add));
   // Create channel.
   grpc_channel* channel = client_channel_factory_create_channel(
       &client_channel_factory, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR,

src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc

@@ -26,7 +26,6 @@
 #include <grpc/support/string_util.h>
 
 #include "src/core/ext/filters/client_channel/client_channel.h"
-#include "src/core/ext/filters/client_channel/client_channel_channelz.h"
 #include "src/core/ext/filters/client_channel/resolver_registry.h"
 #include "src/core/ext/filters/client_channel/uri_parser.h"
 #include "src/core/ext/transport/chttp2/client/chttp2_connector.h"
@@ -214,8 +213,7 @@ grpc_channel* grpc_secure_channel_create(grpc_channel_credentials* creds,
     // credentials.
     grpc_arg args_to_add[] = {
         grpc_client_channel_factory_create_channel_arg(&client_channel_factory),
-        grpc_channel_credentials_to_arg(creds),
+        grpc_channel_credentials_to_arg(creds)};
-        grpc_core::channelz::ClientChannelNode::CreateArg()};
     grpc_channel_args* new_args = grpc_channel_args_copy_and_add(
         args, args_to_add, GPR_ARRAY_SIZE(args_to_add));
     // Create channel.

src/core/lib/channel/channelz.cc

@@ -129,7 +129,6 @@ char* ChannelNode::RenderJSON() {
                                            GRPC_JSON_OBJECT, false);
   json = data;
   json_iterator = nullptr;
-
   PopulateConnectivityState(json);
   json_iterator = grpc_json_create_child(
       json_iterator, json, "target", target_.get(), GRPC_JSON_STRING, false);
@@ -166,8 +165,8 @@ char* ChannelNode::RenderJSON() {
   return json_str;
 }
 
-RefCountedPtr<ChannelNode> MakeChannelNode(grpc_channel* channel,
+RefCountedPtr<ChannelNode> ChannelNode::MakeChannelNode(
-                                           size_t channel_tracer_max_nodes) {
+    grpc_channel* channel, size_t channel_tracer_max_nodes) {
   return MakeRefCounted<grpc_core::channelz::ChannelNode>(
       channel, channel_tracer_max_nodes);
 }

src/core/lib/channel/channelz.h

@@ -44,8 +44,8 @@ class ChannelNodePeer;
 
 class ChannelNode : public RefCounted<ChannelNode> {
  public:
-  ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes);
+  static RefCountedPtr<ChannelNode> MakeChannelNode(
-  virtual ~ChannelNode();
+      grpc_channel* channel, size_t channel_tracer_max_nodes);
 
   void RecordCallStarted();
   void RecordCallFailed() {
@@ -73,6 +73,12 @@ class ChannelNode : public RefCounted<ChannelNode> {
 
   intptr_t channel_uuid() { return channel_uuid_; }
 
+ protected:
+  GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
+  GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW
+  ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes);
+  virtual ~ChannelNode();
+
  private:
   // testing peer friend.
   friend class testing::ChannelNodePeer;
@@ -92,9 +98,6 @@ class ChannelNode : public RefCounted<ChannelNode> {
 typedef RefCountedPtr<ChannelNode> (*ChannelNodeCreationFunc)(grpc_channel*,
                                                               size_t);
 
-RefCountedPtr<ChannelNode> MakeChannelNode(grpc_channel* channel,
-                                           size_t channel_tracer_max_nodes);
-
 }  // namespace channelz
 }  // namespace grpc_core
 

src/core/lib/gprpp/memory.h

@@ -31,12 +31,12 @@
 // protected destructor.
 #define GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE \
   template <typename T>                           \
-  friend void Delete(T*);
+  friend void grpc_core::Delete(T*);
 // Add this to a class that want to use New(), but has a private or
 // protected constructor.
 #define GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW \
   template <typename T, typename... Args>      \
-  friend T* New(Args&&...);
+  friend T* grpc_core::New(Args&&...);
 
 namespace grpc_core {
 

src/core/lib/gprpp/ref_counted_ptr.h

@@ -107,6 +107,11 @@ inline RefCountedPtr<T> MakeRefCounted(Args&&... args) {
   return RefCountedPtr<T>(New<T>(std::forward<Args>(args)...));
 }
 
+template <typename Parent, typename Child, typename... Args>
+inline RefCountedPtr<Parent> MakePolymorphicRefCounted(Args&&... args) {
+  return RefCountedPtr<Parent>(New<Child>(std::forward<Args>(args)...));
+}
+
 }  // namespace grpc_core
 
 #endif /* GRPC_CORE_LIB_GPRPP_REF_COUNTED_PTR_H */

src/core/lib/surface/channel.cc

@@ -108,7 +108,7 @@ grpc_channel* grpc_channel_create_with_builder(
   // this creates the default ChannelNode. Different types of channels may
   // override this to ensure a correct ChannelNode is created.
   grpc_core::channelz::ChannelNodeCreationFunc channel_node_create_func =
-      grpc_core::channelz::MakeChannelNode;
+      grpc_core::channelz::ChannelNode::MakeChannelNode;
   gpr_mu_init(&channel->registered_call_mu);
   channel->registered_calls = nullptr;