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

chrome/browser/extensions/api/web_request/post_data_parser.h

+// 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.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_API_WEB_REQUEST_POST_DATA_PARSER_H_
+#define CHROME_BROWSER_EXTENSIONS_API_WEB_REQUEST_POST_DATA_PARSER_H_
+
+#include <string>
+#include <vector>
+
+#include "base/compiler_specific.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/string_piece.h"
+
+namespace base {
+class DictionaryValue;
+}
+
+namespace net {
+class URLRequest;
+}
+
+namespace extensions {
+
+// Interface for parsers for the POST data.
+class PostDataParser {

[wtc, 2012/08/03 00:57:19]

This class probably should be named FormDataParser.

[vabr (Chromium), 2012/08/05 18:54:46]

Done, also for the inheriting classes.
The name of the file will also need to be changed, but I am not sure to what, it
depends on where the *Producer family will eventually sit.

+ public:
+  class Result {
+   public:
+    Result();
+    ~Result();
+    const std::string& key() const {

[wtc, 2012/08/03 00:57:19]

The specs of form data that you cited refer to these as "name/value" pairs
rather than "key/value" pairs. So it may be better to change key() to name()?

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

+      return key_;
+    }
+    const std::string& val() const {

[wtc, 2012/08/03 00:57:19]

Nit: it seems better to not abbreviate the getter and
the class member.  So name them value() and value_.

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

+      return val_;
+    }
+    void Reset();
+    void SetKey(const base::StringPiece& str);
+    void SetKey(const std::string& str);
+    void SetVal(const base::StringPiece& str);
+    void SetVal(const std::string& str);
+
+   private:
+    std::string key_;
+    std::string val_;
+  };
+
+  // Creates a correct parser instance based on the |request|. Returns NULL
+  // on failure.
+  static scoped_ptr<PostDataParser> CreatePostDataParser(

[wtc, 2012/08/03 00:57:19]

Shorten this method name to just "Create" because it'll be
invoked with the  PostDataParser:: prefix.

Why don't you just return a plain pointer?  What's the
purpose of returning a scoped_ptr?

[vabr (Chromium), 2012/08/05 18:54:46]

Renaming done.

I prefer the scoped_ptr to emphasise that the caller takes ownership. If you
know about some disadvantage of scoped_ptr against a raw one in this case,
please let me know.

[wtc, 2012/08/09 22:02:50]

If no other reviewers commented on this, it's fine to use
scoped_ptr.  I am a C programmer :-)

+      const net::URLRequest* request);
+
+  // Creates a correct parser instance based on the |content_type| of
+  // "Content-Type" URLRequest headers. If |content_type| is NULL, it defaults

[wtc, 2012/08/03 00:57:19]

Nit: this part of the comment doesn't read well:
  the |content_type| of "Content-Type" URLRequest headers

I suggest something like:
  the value of the "Content-Type" request header
or
  |content_type|, the "Content-Type" request header value
  

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

+  // to "application/x-www-form-urlencoded". Returns NULL on failure.
+  static scoped_ptr<PostDataParser> CreatePostDataParser(
+      const std::string* content_type);
+
+  // Tries to parse the POST data of |request| to extract encoded forms.
+  // On success returns the parsed data, otherwise NULL.
+  static scoped_ptr<base::DictionaryValue> ParseURLRequestData(

[wtc, 2012/08/03 00:57:19]

This method should be named ParseURLRequestPostData or
ParsePostData.

[vabr (Chromium), 2012/08/05 18:54:46]

Method removed completely in the meantime. Its job is now done by the
ParsedDataProducer class.

+      const net::URLRequest* request);
+
+  virtual ~PostDataParser();

[wtc, 2012/08/03 00:57:19]

Our Style Guide recommends that the destructor be declared
before methods, including static methods.

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

+
+  // Returns true if there was some data, it was well formed and all was read.
+  virtual bool AllDataReadOK() = 0;
+
+  // Returns the next key-value pair as |result|. After SetSource has succeeded,
+  // this allows to iterate over all pairs in the source.
+  // Returns true as long as a new pair was successfully found.
+  virtual bool GetNextPair(Result* result) = 0;

[wtc, 2012/08/03 00:57:19]

Nit: Pair => KeyValuePair

otherwise it may not be clear what "Pair" refers to.

[vabr (Chromium), 2012/08/05 18:54:46]

Done as Pair => NameValue, because key is now name, and also leaving the pair
there seems redundant and too long.

+
+  // Sets the |source| of the data to be parsed. The ownership is let with the

[wtc, 2012/08/03 00:57:19]

Nit: "is let with the caller" sounds a little strange.
Did you mean "is left with the caller"?

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

+  // caller and the source should live until |this| dies or |this->SetSource()|
+  // is called again, whatever comes sooner. Returns true on success.

[wtc, 2012/08/03 00:57:19]

Nit: whatever => whichever

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

+  virtual bool SetSource(const std::vector<char>* source) = 0;

[wtc, 2012/08/03 00:57:19]

Can we use a const reference here?
    const std::vector<char>& source

It seems that we can also represent the source as
    const char* source, size_t length

[vabr (Chromium), 2012/08/05 18:54:46]

Technically yes, but Dominic asked me to use a pointer for a reason I agree with
(see
https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext...).
Do you still think it is better to have the reference?
Yes, we can. But the source is originally always the vector from
UploadData::Element::bytes(), so it would not spare any conversion. Also, it
looks more readable to me when these two pieces of information are passed
coupled, not as two independent parameters. Moreover, one of the parsers
(URLencoded) already uses vector internally, so it would be converting back and
forth.
So, I would prefer to leave it as a vector. Do you still think it will be better
as const char* and size_t?

[wtc, 2012/08/09 22:02:50]

No. I am a C programmer.  I asked about using a const
reference only because the Style Guide seems to recommend
that.  Also, a pointer argument can be NULL.
No. Thank you for the explanation.

[vabr (Chromium), 2012/08/10 17:12:55]

Thanks for catching the missing NULL check! I added checking for source==NULL.

+
+ protected:
+  PostDataParser() {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PostDataParser);
+};
+
+// Parses URLencoded forms, see
+// http://www.w3.org/TR/REC-html40-971218/interact/forms.html#h-17.13.4.1 .
+class PostDataParserUrlEncoded : public PostDataParser {
+ public:
+  PostDataParserUrlEncoded();
+  virtual ~PostDataParserUrlEncoded();
+
+  // Implementation of PostDataParser.
+  virtual bool AllDataReadOK() OVERRIDE;
+  virtual bool GetNextPair(Result* result) OVERRIDE;
+  virtual bool SetSource(const std::vector<char>* source) OVERRIDE;
+
+ private:
+  const std::vector<char>* source_;
+  std::vector<char>::const_iterator offset_;
+  DISALLOW_COPY_AND_ASSIGN(PostDataParserUrlEncoded);
+};
+
+// Parses forms encoded as multipart, see RFC2388.
+class PostDataParserMultipart : public PostDataParser {
+ public:
+  explicit PostDataParserMultipart(const std::string& boundary_separator);
+  virtual ~PostDataParserMultipart();
+  // Implementation of PostDataParser.

[wtc, 2012/08/03 00:57:19]

Nit: add a blank line before this line.

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

+  virtual bool AllDataReadOK() OVERRIDE;
+  virtual bool GetNextPair(Result* result) OVERRIDE;
+  virtual bool SetSource(const std::vector<char>* source) OVERRIDE;
+
+ private:
+  // Note on implementation:
+  // This parser reads the source line by line. There are four types of lines:

[wtc, 2012/08/03 00:57:19]

This comment says "four types of lines", but there are five
types of lines defined below.

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

+  // - "Boundary" line, separating entries.
+  // - "End boundary" line, ends the data.
+  // - "Content-Disposition" line, containing the "key" for |result|.
+  // - Empty lines.
+  // - Other non-empty lines.
+  enum LineType {kBoundary, kEndBoundary, kDisposition, kEmpty, kOther};

[wtc, 2012/08/03 00:57:19]

Nit: it may be a good idea to add "Line" to all of these
enumerators.  Similarly, add "State" to all of the State
enumerators.

[vabr (Chromium), 2012/08/05 18:54:46]

My reasons for not adding "Line" or "State" to these yet were:
 * The names are long enough already.
 * The type-name of those enums already contain "Line" and "State", so the
context necessary to see that a kSomething is a line-type or a state is
sufficiently near.

Again, if you still think that I missed some advantage in adding "Line" and
"State" to the value names, please do not hesitate to let me know.

[wtc, 2012/08/09 22:02:50]

If the Style Guide allows this, it is fine for you to
deliberately omit "Line" and "State" in these enumerators.
(I guess the Style Guide allows this.)

[vabr (Chromium), 2012/08/10 17:12:55]

I believe this is fine with the style guide:
 * I found no mention in the Chromium SG, and
 * the Google C++ SG only speaks about kConstant/MACRO naming, and the enums in
the sample code there do not share a common suffix.

+
+  // The parser uses the following 7-state automaton to check for structure:
+  // kInit --Boundary--> kHeadStart, kInit --not(Boundary)--> kError
+  // kHeadStart --Disposition--> kHeadRead,
+  //     kHeadStart --not(Disposition)--> kHead
+  // kHead --Disposition--> kHeadRead, kHead --not(Disposition)-->kHead
+  // kHeadRead --Empty-->kBody, kHeadRead --not(Empty)--> kHeadRead
+  // kBody --Boundary--> kHeadStart, kBody --EndBoundary--> kFinal,
+  //     kBody --not(Boundary OR EndBoundary)--> kBody
+  enum State {kInit, kHeadStart, kHead, kHeadRead, kBody, kFinal, kError};

[wtc, 2012/08/03 00:57:19]

Throughout this class, change "Head" to "Header".  (I assume
by "Head" you mean "Header".)

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

+
+  // One-step of the automaton, based on |state_| and |line_type_|.
+  // This calls SeekNextLine() and returns it return value.

[wtc, 2012/08/03 00:57:19]

it => its

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

+  bool DoStep();
+
+  // Determine the |line_type_| of the current line_start_.
+  LineType GetLineType();
+
+  // Read one more line from |source_|, update line pointers.
+  bool SeekNextLine();
+
+  // Extracts "key" and possibly "val" from a DISP line. Returns success.

[wtc, 2012/08/03 00:57:19]

Nit: Returns success => Returns true on success ?

What does "DISP line" mean?

[vabr (Chromium), 2012/08/05 18:54:46]

Corrected both.

("Returns success" was an attempt to use "success" as a Boolean value :),
similarly as one writes, e.g., "Returns number of copied characters" instead of
"Returns 0 if no character was copied, 1 if one was copied, 2 if two, etc."  But
I agree it may sound confusing in the Boolean case.)

+  bool ParseHead(Result* result, bool* val_extracted);
+
+  // We parse the first |length_| bytes from |source_|.
+  const char* source_;
+  size_t length_;
+
+  // Offset of the current and next line from |source_|:
+  // [line_start_]...line... [line_end_]EOL [next_line_]...line...
+  size_t line_start_;  // Points at the first character of the current line.
+  size_t line_end_;  // Points right beyond the last character of the line.
+  size_t next_line_;  // First char. of the next line, or beyond source length.
+  const std::string boundary_;
+  const std::string final_boundary_;
+  State state_;
+  LineType line_type_;
+  DISALLOW_COPY_AND_ASSIGN(PostDataParserMultipart);
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_API_WEB_REQUEST_POST_DATA_PARSER_H_