Define Stable API for WebRTC/Quartc

net/quic/quartc/quartc_at_exit.cc

+// Copyright (c) 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.
+
+#include "net/quic/quartc/quartc_at_exit.h"
+
+namespace {
+
+// A test mode AtExitManager which is allowed be create more than one.
+// This should only be used for testing! AtExitManagers are kept on a global
+// stack, and it will be removed during destruction. This allows you to shadow
+// another AtExitManager.
+// Create a subclass as the test mode constructor of the AtExitManager is
+// protected.

[pthatcher1, 2016/10/14 18:58:59]

"as the" => "because the"

[zhihuang1, 2016/10/16 00:45:27]

Done.

+class AtExitManagerForTest : public base::AtExitManager {
+ public:
+  AtExitManagerForTest() : AtExitManager(true) {}
+};
+}  // namespace
+
+namespace net {
+
+QuartcAtExitManager::QuartcAtExitManager() {}
+
+QuartcAtExitManager::~QuartcAtExitManager() {}
+
+void QuartcAtExitManager::SetAtExitManager(bool test_mode) {
+  if (at_exit_manager_) {

[pthatcher1, 2016/10/14 18:58:59]

If the QuartcAtExitManager itsn't usable until after SetAtExitManager is called,
and can't be changed after this is called, then basically this is an Init
method.  So why not call it that?

But then that brings up the question: why not just put it in the constructor?

[zhihuang1, 2016/10/16 00:45:27]

One reason I'm doing this when working in WebRTC, sometimes it is more flexible
to determine the mode of the AtExitManager later. But I think you are right,
this can be simplified.



I have an idea to simplify this by keeping a static at_exit_manager in the
quartc_factory since quartc_factory is the entry of all the quartc stuff. The
at_exit_manager in the quartc factory will only be created once. So we can also
get rid of the test_mode at_exit_manager. My only concern is the way to use
static stuff in Chromium correctly. Please pay extra attention on that.

+    LOG(WARNING) << "The AtExitManager has already been created";
+    return;
+  }
+  if (test_mode) {
+    // Only create ShadowingAtExitManager for unit tests.
+    LOG(WARNING) << "Creating an AtExitManager for tests.";
+    at_exit_manager_.reset(new AtExitManagerForTest);
+  } else {
+    at_exit_manager_.reset(new base::AtExitManager);
+  }

[pthatcher1, 2016/10/14 18:58:59]

An alternative is to rename AtExitManagerForTest to TestableAtExitManager and
then make this:

at_exit_manager_.reset(new TestableAtExitManager(test_mode));

[zhihuang1, 2016/10/16 00:45:27]

I think you mean we can have something like this:

"TestableAtExitManager(bool test_mode) :
   AtExitManager(test_mode) {}"

https://cs.chromium.org/chromium/src/base/at_exit.cc?sq=package:chromium&rcl=...

It seems that AtExitManager() is not equivalent to AtExitManager(false). So we
may need something like:

"TestableAtExitManager(bool test_mode) {
   if (test_mode) { AtExitManager(true); }
   else AtExitManager(); // This is the normal mode.
}"

+}
+
+std::unique_ptr<QuartcAtExitManagerInterface> CreateQuartcAtExitManager() {
+  return std::unique_ptr<QuartcAtExitManagerInterface>(new QuartcAtExitManager);

[pthatcher1, 2016/10/14 18:58:59]

What's the point of having a function that just calls the constructor?  Why not
just call the constructor?

[zhihuang1, 2016/10/16 00:45:27]

I thought this QuartcAtExitManager is something like the QuartcFactory. 
WebRTC can only depend on the interface and we need an additional function to
create instances of the QuartcAtExitManager.


I have another way to simplify this. Please check the update.

+}
+}  // namespace net