Add net::SdchSourceStream and net::SdchPolicyDelegate

net/filter/sdch_source_stream.cc

+// 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.
+
+#include "net/filter/sdch_source_stream.h"
+
+#include "base/auto_reset.h"
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/numerics/safe_conversions.h"
+#include "net/base/io_buffer.h"
+#include "sdch/open-vcdiff/src/google/vcdecoder.h"
+
+namespace net {
+
+namespace {
+
+const size_t kServerIdLength = 9;
+const char kSDCH[] = "SDCH";
+
+}  // namespace
+
+SdchSourceStream::SdchSourceStream(std::unique_ptr<SourceStream> previous,
+                                   Delegate* delegate)
+    : FilterSourceStream(SourceStream::TYPE_SDCH, std::move(previous)),
+      delegate_(delegate),
+      error_occurred_(false),
+      input_state_(STATE_LOAD_DICTIONARY) {
+  CHECK(delegate);
+}
+
+SdchSourceStream::~SdchSourceStream() {}
+
+std::string SdchSourceStream::GetTypeAsString() const {
+  return kSDCH;
+}
+
+int SdchSourceStream::FilterData(IOBuffer* output_buffer,
+                                 int output_buffer_size,
+                                 IOBuffer* input_buffer,
+                                 int input_buffer_size,
+                                 int* consumed_bytes,
+                                 bool /*upstream_end_reached*/) {
+  DCHECK_LE(0, input_buffer_size);
+  size_t input_data_size = input_buffer_size;
+  char* input_data = input_buffer->data();
+  int bytes_out = 0;
+  while ((input_data_size > 0 || !buffered_output_.empty()) &&
+         output_buffer_size - bytes_out > 0) {
+    switch (input_state_) {
+      case STATE_LOAD_DICTIONARY: {
+        // Copy at most |kServerIdLength| from |input_buffer|.
+        size_t to_copy = std::min(
+            kServerIdLength - dictionary_server_id_.length(), input_data_size);
+        dictionary_server_id_.append(input_data, to_copy);
+        input_data_size -= to_copy;
+        input_data += to_copy;
+
+        // Not enough bytes for a dictionary ID accumulated yet.
+        if (dictionary_server_id_.length() != kServerIdLength) {
+          *consumed_bytes = input_buffer_size - input_data_size;

[Randy Smith (Not in Mondays), 2016/10/04 20:06:29]

Though (i.e. not even a suggestion :-}): I believe that in this conditional
input_data_size will always be zero (because otherwise we would have copied more
data into dictionary_server_id_).  Would it be clearer to just set
*consumed_bytes to input_buffer_size and have a DCHECK?

[xunjieli, 2016/10/05 13:44:57]

Done.

+          return 0;
+        }
+        // To avoid passing a std::string with a null terminator into
+        // GetDictionary(), server hash here removes the last byte blindly,
+        // and this method only calls GetDictionary() with it if
+        // CouldBeDictionaryId() returns true.

[Randy Smith (Not in Mondays), 2016/10/04 20:06:29]

Is this comment still valid/positioned appropriately?  I remember reading it and
it making sense on the first round, but the code that I remember it referring to
is well below this line.

[xunjieli, 2016/10/05 13:44:57]

Done. Good catch!

+        const std::string* dictionary_text = nullptr;
+
+        if (!CouldBeDictionaryId(dictionary_server_id_)) {
+          // If |dictionary_server_id_| is bogus, it should appear in output
+          // stream, so append it to |buffered_output_| here.
+          buffered_output_.append(dictionary_server_id_);
+          if (!HandleError(
+                  delegate_->OnDictionaryIdError(this, &buffered_output_))) {
+            return ERR_CONTENT_DECODING_FAILED;
+          }
+          break;
+        }
+        if (!delegate_->OnGetDictionary(
+                dictionary_server_id_.substr(0, kServerIdLength - 1),
+                &dictionary_text)) {
+          // If GetDictionaryId fails and delegate chooses to pass through,
+          // preserve the dictionary id in the output.
+          buffered_output_.append(dictionary_server_id_);
+          if (!HandleError(
+                  delegate_->OnGetDictionaryError(this, &buffered_output_))) {
+            return ERR_CONTENT_DECODING_FAILED;
+          }
+          break;
+        }
+        decoder_.reset(new open_vcdiff::VCDiffStreamingDecoder);
+        decoder_->SetAllowVcdTarget(false);
+        decoder_->StartDecoding(dictionary_text->data(),
+                                dictionary_text->length());
+        input_state_ = STATE_DECODE;
+        break;
+      }
+      case STATE_DECODE: {
+        DCHECK(buffered_output_.empty());
+        bool ok = decoder_->DecodeChunk(input_data, input_data_size,
+                                        &buffered_output_);
+        // Calls to DecodeChunk always consume all their input, so this always
+        // drains the entire buffer.
+        input_data += input_data_size;
+        input_data_size = 0;
+        if (!ok) {
+          decoder_.reset();
+          bool handled =
+              HandleError(delegate_->OnDecodingError(this, &buffered_output_));
+          if (!handled)
+            return ERR_CONTENT_DECODING_FAILED;
+          break;
+        }
+        input_state_ = STATE_FLUSH_INTERNAL_BUFFER;
+        break;
+      }
+      case STATE_FLUSH_INTERNAL_BUFFER: {
+        bytes_out += FlushBufferedOutput(output_buffer->data() + bytes_out,
+                                         output_buffer_size - bytes_out);
+        if (!buffered_output_.empty())

[Randy Smith (Not in Mondays), 2016/10/04 20:06:29]

Suggestion: I'm uncomfortable with the layering semi-violation here. 
buffered_output_ is engaged with both in FlushBufferredOutput and here,
requiring modifications to each location to be aware of the code at the other
location.  It's true, they're both part of the same class.  But I'd be happier
if the variable was only touched in one of inside FlushBufferredOutput() or out
here.

Given the use in STATE_PASS_THROUGH, how about passing bufferred_output_ as an
argument, and pulling FLushBufferredOutput into a static or file-local function?

[xunjieli, 2016/10/05 13:44:57]

Done.

+          break;
+        if (!error_occurred_)

[Randy Smith (Not in Mondays), 2016/10/04 20:06:29]

I'm confused about how this works in the |error_occurred_ == true| case.  Won't
the while loop around the switch statement keep returning us to this state,
which won't do anything?  (Presuming there's still data available from
upstream.)

[xunjieli, 2016/10/05 13:44:57]

Done.

+          input_state_ = STATE_DECODE;

[Randy Smith (Not in Mondays), 2016/10/04 20:06:29]

Ok, I think I'm clearer on this condition now; basically, in normal operation we
ping-pong back and forth between STATE_DECODE and STATE_FLUSH_INTERNAL_BUFFER
depending on whether we information buffered.

However, I'm a bit uncomfortable about consistency among the states here.  What
we have is:

* STATE_PASS_THROUGH: Takes care of flushing bufferred_output_ itself.
* STATE_DECODE: Ping-pongs back and forth with FLUSH_INTERNAL_BUFFER as needed.
* STATE_FLUSH_INTERNAL_BUFFER: Does double duty as its own state (meaning "we've
had the output replaced, and we need to flush it and do nothing more") and as a
helper state for DECODE.
The two problems I see with that are a) the states don't actually map to
different states of processing the input, and b) they aren't consistent in the
relationship between the use of FlushBufferedOutput() and use of
STATE_FLUSH_INTERNAL_BUFFER.

My suggestion for fixing this would be:
* Put a prefix at the beginning of the code for STATE_DECODE similar to the one
at the beginning of STATE_PASS_THROUGH that flushes anything in the buffer.  
* Get rid of the error_occurred_ boolean (:-}) and rename
STATE_FLUSH_INTERNAL_BUFFER to STATE_OUTPUT_REPLACED.

WDYT?

[xunjieli, 2016/10/05 13:44:57]

Done.

+        break;
+      }
+      case STATE_PASS_THROUGH: {
+        if (!buffered_output_.empty()) {
+          bytes_out += FlushBufferedOutput(output_buffer->data() + bytes_out,
+                                           output_buffer_size - bytes_out);
+        }
+        if (!buffered_output_.empty())
+          break;
+        size_t to_copy =
+            std::min(base::checked_cast<size_t>(output_buffer_size - bytes_out),
+                     input_data_size);
+        memcpy(output_buffer->data() + bytes_out, input_data, to_copy);
+        input_data += to_copy;
+        input_data_size -= to_copy;
+        break;
+      }
+    }
+  }
+  *consumed_bytes = input_buffer_size - input_data_size;
+  return bytes_out;
+}
+
+int SdchSourceStream::FlushBufferedOutput(char* output_buffer,
+                                          int output_buffer_size) {
+  // DCHECK_LT(0u, buffered_output_.length());
+  size_t to_flush = std::min(base::checked_cast<size_t>(output_buffer_size),
+                             buffered_output_.length());
+  memcpy(output_buffer, buffered_output_.data(), to_flush);
+  buffered_output_.erase(0, to_flush);
+  return to_flush;
+}
+bool SdchSourceStream::CouldBeDictionaryId(const std::string& id) const {
+  for (size_t i = 0; i < kServerIdLength - 1; i++) {
+    char base64_char = id[i];
+    if (!isalnum(base64_char) && '-' != base64_char && '_' != base64_char)
+      return false;
+  }
+  if (id[kServerIdLength - 1] != '\0')
+    return false;
+  return true;
+}
+
+bool SdchSourceStream::HandleError(Delegate::ErrorRecover error_recover) {
+  error_occurred_ = true;
+  switch (error_recover) {
+    case Delegate::NONE:
+      return false;
+    case Delegate::PASS_THROUGH:
+      input_state_ = STATE_PASS_THROUGH;
+      break;
+    case Delegate::REPLACE_OUTPUT:
+      input_state_ = STATE_FLUSH_INTERNAL_BUFFER;
+      break;
+  }
+  return true;
+}
+
+}  // namespace net