Add read-only access to POST data for webRequest's onBeforeRequest

chrome/browser/extensions/api/web_request/upload_data_presenter.cc

+// Copyright (c) 2012 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 "chrome/browser/extensions/api/web_request/upload_data_presenter.h"
+
+#include "base/base64.h"
+#include "base/file_path.h"
+#include "base/string_util.h"
+#include "base/values.h"
+#include "chrome/browser/extensions/api/web_request/form_data_parser.h"
+#include "net/url_request/url_request.h"
+
+using base::BinaryValue;
+using base::DictionaryValue;
+using base::ListValue;
+using base::StringValue;
+using base::Value;
+
+namespace {
+
+// Takes |dictionary| of <string, list of strings> pairs, and gets the list
+// for |key|, creating it if necessary.
+ListValue* GetOrCreateList(DictionaryValue* dictionary,
+                           const std::string& key) {
+  ListValue* list = NULL;
+  if (!dictionary->GetList(key, &list)) {
+    list = new ListValue();
+    dictionary->Set(key, list);
+  }
+  return list;
+}
+
+}  // namespace
+
+namespace extensions {
+
+// Implementation of UploadDataPresenter.
+
+UploadDataPresenter::~UploadDataPresenter() {}
+
+// Implementation of ChunkedErrorPresenter.
+
+ChunkedErrorPresenter::ChunkedErrorPresenter(const net::URLRequest* request)
+    : chunks_found_(TransferEncodingChunked(request)) {
+}
+
+ChunkedErrorPresenter::~ChunkedErrorPresenter() {}
+
+// static
+bool ChunkedErrorPresenter::TransferEncodingChunked(
+    const net::URLRequest* request){
+  std::string transfer_encoding;
+  if (!request->extra_request_headers().GetHeader(
+      net::HttpRequestHeaders::kTransferEncoding, &transfer_encoding))

[battre, 2012/08/16 19:18:03]

nit: +4 indent

[vabr (Chromium), 2012/08/17 18:29:57]

Are you sure? Does the +4 indent rule here apply to the start of "if", or to the
start of the conditional expression? It looks funny being 6 spaces off the
return, and also the Vim autoindent settings for Google, which otherwise work
fine, suggest only 4 spaces in total.

[battre, 2012/08/20 12:09:38]

The reasoning is that the line would usually continue below the !, but that we
are now dealing with a wrapped function call. Maybe you can ask anybody for a
third opinion and go with it. I would not block the CL on this. ;-)

[vabr (Chromium), 2012/08/23 15:41:54]

:) I sacrificed one more line to get the headers first, and then used the
arguments-start-under-each-other style. Hopefully that makes it easier to read.

+    return false;
+  return base::strcasecmp(transfer_encoding.c_str(), "chunked") == 0;
+}
+
+void ChunkedErrorPresenter::FeedNext(const net::UploadData::Element& element) {
+  if (chunks_found_)
+    return;  // We already found a reason to report an error.
+
+  if (element.type() == net::UploadData::TYPE_CHUNK)
+    chunks_found_ = true;
+}
+
+bool ChunkedErrorPresenter::Succeeded() {
+  return chunks_found_;

[battre, 2012/08/16 19:18:03]

is this correct? Shouldn't this always return true?

[vabr (Chromium), 2012/08/17 18:29:57]

It should only return true if there were some elements of TYPE_CHUNK among those
fed to this presenter.
So I believe this is correct. But please speak up if you feel this is a
misunderstanding.

+}
+
+scoped_ptr<Value> ChunkedErrorPresenter::Result() {
+  if (!chunks_found_)
+    return scoped_ptr<Value>();
+
+  scoped_ptr<StringValue> error_string(new StringValue(
+      "Not supported: data is uploaded chunked."));
+  return error_string.PassAs<Value>();
+}
+
+// Implementation of RawDataPresenter.
+
+RawDataPresenter::RawDataPresenter()
+  : success_(true),
+    list_(new base::ListValue) {
+}
+RawDataPresenter::~RawDataPresenter() {}
+
+void RawDataPresenter::FeedNext(const net::UploadData::Element& element) {
+  if (!success_)
+    return;
+
+  if (element.type() == net::UploadData::TYPE_BYTES) {
+    FeedNextBytes(element.bytes());
+  } else if (element.type() == net::UploadData::TYPE_CHUNK) {
+    Abort();  // Chunks are not supported (yet).
+  } else if (element.type() == net::UploadData::TYPE_FILE) {
+    // Insert the file path instead of the contents, which may be too large.
+    FeedNextFile(element.file_path().AsUTF8Unsafe());
+  } // TYPE_BLOB is silently ignored.

[battre, 2012/08/16 19:18:03]

how about:
switch (element.type()) {
  case TYPE_BYTES;
    FeedNextBytes(...);
    break;
  ...
  case TYPE_BLOB:
    break;  // TYPE_BLOB is silently ignored.
}

Note that I did not add a default branch so that this generates a compiler error
in case a new file type is added but not considered here.

[battre, 2012/08/16 19:18:03]

Why silently ignored? Should we not generate a string or something that
indicates that some data would be there but that they are not presented?

[vabr (Chromium), 2012/08/17 18:29:57]

Good idea.

[vabr (Chromium), 2012/08/17 18:29:57]

Given the comment at UploadData::Element::SetToBlobUrl in base/upload_data.h,
TYPE_BLOB is not expected to appear among upload data. I will add a sentence
that blobs are ignored to the JSON documentation. Would that be sufficient?

[battre, 2012/08/20 12:09:38]

Add a NOTREACHED()?

[vabr (Chromium), 2012/08/23 15:41:54]

Well, CL https://chromiumcodereview.appspot.com/10834289 solved this for me by
removing TYPE_BLOB completely. :)

+}
+
+bool RawDataPresenter::Succeeded() {
+  return success_;
+}
+
+scoped_ptr<Value> RawDataPresenter::Result() {
+  if (!success_)
+    return scoped_ptr<Value>();
+
+  return list_.PassAs<Value>();
+}
+
+void RawDataPresenter::Abort() {
+  success_ = false;
+  list_.reset();
+}
+
+void RawDataPresenter::FeedNextBytes(const std::vector<char>& bytes) {
+  list_->Append(
+      BinaryValue::CreateWithCopiedBuffer(&(bytes[0]), bytes.size()));
+}
+
+void RawDataPresenter::FeedNextFile(const std::string& filename) {
+  // Insert the file path instead of the contents, which may be too large.
+  std::string bytes_encoded;
+  success_ = base::Base64Encode(filename, &bytes_encoded);

[battre, 2012/08/16 19:18:03]

Why do you pass the file path as base64 encoded data? Shouldn't this be UTF-8?

[vabr (Chromium), 2012/08/17 18:29:57]

I removed the encoding. At some point I saw it transform some Japanese
characters to '?', but that might have been a problem with my fonts. The
element.file_path().AsUTF8Unsafe() should indeed return UTF8, and the
StringValue constructor should be able to deal with it.

+  if (success_)
+    list_->Append(new StringValue(bytes_encoded));
+}
+
+// Implementation of ParsedDataPresenter.
+
+ParsedDataPresenter::ParsedDataPresenter(const net::URLRequest* request)
+  : parser_(FormDataParser::Create(request).release()),

[battre, 2012/08/16 19:18:03]

is this .release() necessary?

[vabr (Chromium), 2012/08/17 18:29:57]

No :). Removed.

+    success_(parser_.get() != NULL),
+    dictionary_(success_ ? new DictionaryValue() : NULL) {
+}
+
+ParsedDataPresenter::~ParsedDataPresenter() {}
+
+void ParsedDataPresenter::FeedNext(const net::UploadData::Element& element) {
+  if (!success_)
+    return;
+
+  if (element.type() != net::UploadData::TYPE_BYTES) {
+    if (element.type() != net::UploadData::TYPE_FILE) {
+      Abort();  // We do not handle blobs nor chunks.
+    }
+    return;  // But we just ignore files.
+  }
+  if (!parser_->SetSource(&(element.bytes()))) {
+    Abort();
+    return;
+  }
+
+  FormDataParser::Result result;
+  while (parser_->GetNextNameValue(&result)) {
+    GetOrCreateList(dictionary_.get(), result.name())->Append(
+        new StringValue(result.value()));
+  }
+}
+
+bool ParsedDataPresenter::Succeeded() {
+  if (success_ && !parser_->AllDataReadOK())
+    Abort();
+  return success_;
+}
+
+scoped_ptr<Value> ParsedDataPresenter::Result() {
+  if (success_)
+    return dictionary_.PassAs<Value>();

[battre, 2012/08/16 19:18:03]

I think this looks wrong. You return the ownership of the dictionary_ and reset
dictionary_ to NULL. At the same time you call requestBody->Set(kKeys[i],
presenters[i]->Result().release()); in web_request_api.cc multiple times.

[vabr (Chromium), 2012/08/17 18:29:57]

I start with dictionary_, which is a scoped_ptr<DictionaryValue>.
I need to return scoped_ptr<Value>, so I use PassAs to cast dictionary_.
Then I need to release the raw pointer held in the scoped_ptr<Value> because
ListValue is going to own it.

Which of these steps is/looks wrong or could be left out?

[battre, 2012/08/20 12:09:38]

Never mind. I misunderstood your code in web_request_api.cc.

+  else
+    return scoped_ptr<Value>();
+}
+
+void ParsedDataPresenter::Abort() {
+  success_ = false;
+  dictionary_.reset();
+  parser_.reset();
+}
+
+}  // namespace extensions