commit 77e77b840f1a5241a8d3338faa9706d23053ebf7
author bnc <bnc@google.com> Sun Apr 05 10:36:49 2020 -0700
committer Copybara-Service <copybara-worker@google.com> Sun Apr 05 10:37:35 2020 -0700
tree 8328f00547bff968398675f0f77c53ecda5c4b8a
parent cbd4ea311cc559b798db085421aa1a772d8d7db6

gfe-relnote:  Refactor QuicConnectionCloseFrame.  Not protected.

QuicConnectionCloseFrame::quic_error_code should not be accessed when using IETF
QUIC, however, as it is a public member, this is not enforced.  When read, it
returns the on-the-wire error code (transport or application) cast into a
QuicErrorCode.  This is bad.

Since public unions cannot enforce restrictions on access, I propose to remove
the union for clarity.  The price paid is type: wire_error_code will now be
uint64_t even for Google QUIC error codes which could be QuicErrorCode, and for
IETF QUIC transport errors, which could be QuicIetfTransportErrorCodes.

Also rename extracted_error_code to quic_error_code for clarity, because for
sent frames, it is not extracted.

This refactor is necessary for modifying QuicErrorCodeToTransportErrorCode() to
return IETF wire error codes.

When using Google QUIC, old fields |quic_error_code| and |extracted_error_code|
and new fields |wire_error_code| and |quic_error_code| should always have the
same value, therefore this refactor should not introduce any functional change.

PiperOrigin-RevId: 304902883
Change-Id: I586a92609fb8a6186310f6585c8d29ee1d60af4f

quic/core/quic_connection_test.cc

diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index e5faa35..ded114b 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -1670,8 +1670,11 @@
     const std::vector<QuicConnectionCloseFrame>& connection_close_frames =
         writer_->connection_close_frames();
     ASSERT_EQ(1u, connection_close_frames.size());
+
+    EXPECT_EQ(expected_code, connection_close_frames[0].quic_error_code);
+
     if (!VersionHasIetfQuicFrames(version().transport_version)) {
-      EXPECT_EQ(expected_code, connection_close_frames[0].quic_error_code);
+      EXPECT_EQ(expected_code, connection_close_frames[0].wire_error_code);
       EXPECT_EQ(GOOGLE_QUIC_CONNECTION_CLOSE,
                 connection_close_frames[0].close_type);
       return;
@@ -1685,16 +1688,13 @@
       EXPECT_EQ(IETF_QUIC_TRANSPORT_CONNECTION_CLOSE,
                 connection_close_frames[0].close_type);
       EXPECT_EQ(mapping.transport_error_code_,
-                connection_close_frames[0].transport_error_code);
-      // TODO(fkastenholz): when the extracted error code CL lands,
-      // need to test that extracted==expected.
+                connection_close_frames[0].wire_error_code);
     } else {
       // This maps to an application close.
-      EXPECT_EQ(expected_code, connection_close_frames[0].quic_error_code);
       EXPECT_EQ(IETF_QUIC_APPLICATION_CONNECTION_CLOSE,
                 connection_close_frames[0].close_type);
-      // TODO(fkastenholz): when the extracted error code CL lands,
-      // need to test that extracted==expected.
+      EXPECT_EQ(mapping.application_error_code_,
+                connection_close_frames[0].wire_error_code);
     }
   }
 
@@ -7621,7 +7621,7 @@
       kSelfAddress, kPeerAddress,
       QuicReceivedPacket(buffer, encrypted_length, QuicTime::Zero(), false));
   EXPECT_EQ(1, connection_close_frame_count_);
-  EXPECT_THAT(saved_connection_close_frame_.extracted_error_code,
+  EXPECT_THAT(saved_connection_close_frame_.quic_error_code,
               IsError(QUIC_PEER_GOING_AWAY));
 }
 
@@ -10134,7 +10134,7 @@
   ASSERT_EQ(1u, connection_close_frames.size());
   EXPECT_EQ(IETF_QUIC_TRANSPORT_CONNECTION_CLOSE,
             connection_close_frames[0].close_type);
-  EXPECT_EQ(kQuicErrorCode, connection_close_frames[0].extracted_error_code);
+  EXPECT_EQ(kQuicErrorCode, connection_close_frames[0].quic_error_code);
   EXPECT_EQ(kTransportCloseFrameType,
             connection_close_frames[0].transport_close_frame_type);
 }