Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10694055:
Add read-only access to POST data for webRequest's onBeforeRequest (Closed)
|
Created:
8 years, 5 months ago by vabr (Chromium) Modified:
8 years, 3 months ago CC:
chromium-reviews, mihaip-chromium-reviews_chromium.org, satorux1 Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionThis adds read-only access to POST data for webRequest's onBeforeRequest listeners.
browser_tests: ExtensionWebRequestApiTest.WebRequestPost; unit_tests: ExtensionWebRequestPostDataParserTest.Parsing, ExtensionWebRequestTest.AccessPostData
TBR=jhawkins@chromium.org
BUG=91191
TEST=Please follow the instructions from comment 23 on the BUG (http://code.google.com/p/chromium/issues/detail?id=91191#c23). Note that the feature only works for requests with an HTTP or HTTPS scheme in the URL.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156547
Patch Set 1 #
Total comments: 42
Patch Set 2 : Unit test extended + 1st round of review addressed #Patch Set 3 : Documentation and a browsertest added #Patch Set 4 : Deleting a forgotten comment #
Total comments: 76
Patch Set 5 : Parsers separated and review comments addressed #Patch Set 6 : Minor fixes #Patch Set 7 : Repairing ParseHead() #Patch Set 8 : Initializing member variables with base types #Patch Set 9 : Typo fixed #
Total comments: 36
Patch Set 10 : Comments addressed, unit tests split #
Total comments: 33
Patch Set 11 : Comments addressed #Patch Set 12 : I will not tell language jokes to the compiler #Patch Set 13 : Naming as "experimental" #Patch Set 14 : Minor simplification in the unit-test #Patch Set 15 : Trading ASSERT for CHECK + unused header removed #Patch Set 16 : Re-uploading #Patch Set 17 : Rejecting CHUNKS and BLOBS, adding formData #Patch Set 18 : Rebased & added missing docs update #
Total comments: 3
Patch Set 19 : Adding an example value of formData to the docs #
Total comments: 10
Patch Set 20 : Matt's comments + minor correction on no parseable data #Patch Set 21 : Remove what accidentally slipped in during rebasing #Patch Set 22 : Matt's comments + error reporting for chunked + unit_test refactoring + browser_test correction #Patch Set 23 : Trying to fix the android build #Patch Set 24 : Corrected broken unit-tests #
Total comments: 1
Patch Set 25 : Missing header for strcasecmp #Patch Set 26 : Wrong strcasecmp :) #
Total comments: 21
Patch Set 27 : #
Total comments: 4
Patch Set 28 : Forgot to take care of parser unittest #Patch Set 29 : Adding the channel restriction #
Total comments: 3
Patch Set 30 : Now checking the channel for real #
Total comments: 31
Patch Set 31 : Matt's comments #Patch Set 32 : Forgot to correct the browser test #Patch Set 33 : Correcting InitFromValue unit-test #Patch Set 34 : Further changes on channel selection + most of wtc's comments #Patch Set 35 : Windows, what's your problem with scoped_ptr? #
Total comments: 97
Patch Set 36 : Comments from wtc and mpcomplete + raw POST and PUT support + change in 'details' structure #Patch Set 37 : Rebased + some corrections #
Total comments: 40
Patch Set 38 : Comments from Aaron, Matt and Wan-teh, splitting parsers and (ex-)producers, test improvements... #Patch Set 39 : Corrected the multipart parser + parsedForm->formData #
Total comments: 81
Patch Set 40 : Dominic's comments #
Total comments: 6
Patch Set 41 : Dominic's comments + adjusting to the recent move of UploadElement out of UploadData. #
Total comments: 6
Patch Set 42 : Removing support for TYPE_CHUNK after chunks were taken away #Patch Set 43 : Kent's first comments #
Total comments: 21
Patch Set 44 : Rewritten parsers to use re2 #Patch Set 45 : One more bug fixed, enum style corrected, strlen's removed, UTF8 restriction added to the docs #Patch Set 46 : Making non-trivial data-members non-static #
Total comments: 1
Patch Set 47 : Unescaping field name + corresponding api tests enhanced #
Total comments: 2
Patch Set 48 : One more static RE2 object made non-static #
Total comments: 4
Patch Set 49 : Correcting method names pointed out by Kent #Patch Set 50 : Introducing LazyInstance for "static" RE2 #
Total comments: 2
Patch Set 51 : Indents corrected + LazyInstance made Leaky #
Total comments: 46
Patch Set 52 : Dominic's comments #Patch Set 53 : More comments from Dominic #
Total comments: 1
Patch Set 54 : Removing spaces before full-stops after function names in comments. #Patch Set 55 : No change in code, but with a patch generated without copy detection #Messages
Total messages: 102 (0 generated)
Hi Dominic, Since I return to work first on Friday, I wanted to give you access to the first version of this before I leave, so that you can split your reviewing effort. But there is no rush. If you don't find time to look at it, I will ping you again, once the rest (browser test+docs) is ready. Thanks, Vaclav
Here is a first set of comments (I did not check the correctness of the approach, which will be easier once the style matches the guide lines -> less tasks for my brain to process ;-)). In general the line breaking in Chrome is if (foo) bar(); not if (foo) bar(); The style guide does not prevent it but the latter is uncommon. I use it in cases where I test a lot of exit criteria at the beginning of a function. Best regards, Dominic http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:178: size_t key_length; This looks like a usecase of StringPiece http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:182: // Attempts to set the |length| bytes at |source| as the data to be parsed. s/Attempts to set/Sets/ http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:190: virtual bool AllDataReadOK() = 0; new line before protected: and private: http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:199: PostDataParserUrlEncoded() : source_(NULL) {} length_ and offset_ should be initialized http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:201: virtual bool SetSource(const char* source, size_t length) OVERRIDE; Please add a new line // Implementation of PostDataParser between 200 and 201 http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:203: virtual bool AllDataReadOK() OVERRIDE; new line before private: http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:229: while (seek < length_ && *(source_ + seek) != '=') ++seek; I think this is legal but unconventional style. Typically you would move the ++seek; to the next line. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:250: state_(kInit) {} initialize all atomic variables http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:255: private: newline before private: http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:310: while (state_ != kSkip) if (!DoStep()) return false; This should go into multiple lines http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:342: else if (strncmp(line, "Content-Disposition:", 20) == 0) static const char kContentDisposition[] = "Content-Disposition:"; ... strncmp(line, kContentDisposition, arraysize(kContentDisposition) - 1) (please check whether this should be -1 or not, not sure whether this appends a \0 by default) http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:367: if (line_type_ == kBound) state_ = kFirst; \n after ) http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:458: if (parser_ != NULL) return parser_->GetNextPair(result); newline before return. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:475: list = reinterpret_cast<ListValue*>(receive); // Safe due to contract. you can get rid of the else clause if you use if (!dictionary->GetList(key, &list)) { above. Then you don't need receive either. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:478: list->Append(string_value); I would inline string_value. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:489: DictionaryValue* post_data = new DictionaryValue(); please use a scoped_ptr<DictionaryValue>, this way the pointer cannot get lost. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:495: if (!parser.SetSource(bytes, size)) continue; Any reason why you don't pass a const std::vector<char>& here? http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:500: UpdateDictionaryWithString(post_data, key, val); How about GetOrCreateList(post_data, key)->Append(new StringValue(val)); ? "Update" is kind of abstract. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:503: if (parser.AllDataReadOK()) out->Set(keys::kPostDataKey, post_data); newline before out->Set http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:842: extra_info_spec | ExtraInfoSpec::REQUEST_POST_DATA) this should probably be extra_info_spec & ... I would reverse the order of the check. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:79: enum TestMessageResult {kPostDataFound, kNoPostData, kError}; style: enum TestMessageResult { kPostDataFound, kNoPostData, kError };
Hi Dominic, I addressed most of your comments (thanks!), except for the two about initialization, see my comments. I would be happy to discuss this further, in person or here. I also extended the unit test, but did not add the browser test yet. This is my very next TODO. Cheers, Vaclav http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:178: size_t key_length; On 2012/07/06 07:36:24, battre wrote: > This looks like a usecase of StringPiece Done. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:182: // Attempts to set the |length| bytes at |source| as the data to be parsed. On 2012/07/06 07:36:24, battre wrote: > s/Attempts to set/Sets/ Done. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:190: virtual bool AllDataReadOK() = 0; On 2012/07/06 07:36:24, battre wrote: > new line before protected: and private: Done. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:199: PostDataParserUrlEncoded() : source_(NULL) {} On 2012/07/06 07:36:24, battre wrote: > length_ and offset_ should be initialized I did not initialize them on purpose. Their value does not make sense until the source is set, and I found it confusing to assign anything to them. While setting the source, length_ and offset_ get initialized properly, and before that these two values are never used. Do you insist on initializing them? http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:201: virtual bool SetSource(const char* source, size_t length) OVERRIDE; On 2012/07/06 07:36:24, battre wrote: > Please add a new line > > // Implementation of PostDataParser between 200 and 201 Done. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:203: virtual bool AllDataReadOK() OVERRIDE; On 2012/07/06 07:36:24, battre wrote: > new line before private: Done. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:229: while (seek < length_ && *(source_ + seek) != '=') ++seek; On 2012/07/06 07:36:24, battre wrote: > I think this is legal but unconventional style. Typically you would move the > ++seek; to the next line. Done, also at the similar while-cycle below. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:250: state_(kInit) {} On 2012/07/06 07:36:24, battre wrote: > initialize all atomic variables Similarly to PostDataParserUrlEncoded, the variables left out from initialization have no meaningful value to be initialized with at the time of the object's construction. They get initialized before the first use, line_type_ in GetLineType() via DoStep(), and the rest in SetSource(). In my opinion it would hurt the logic of the code to assign random values to them here, please let me know any counterarguments you have. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:255: private: On 2012/07/06 07:36:24, battre wrote: > newline before private: Done. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:310: while (state_ != kSkip) if (!DoStep()) return false; On 2012/07/06 07:36:24, battre wrote: > This should go into multiple lines Done. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:342: else if (strncmp(line, "Content-Disposition:", 20) == 0) On 2012/07/06 07:36:24, battre wrote: > static const char kContentDisposition[] = "Content-Disposition:"; > ... strncmp(line, kContentDisposition, arraysize(kContentDisposition) - 1) > > (please check whether this should be -1 or not, not sure whether this appends a > \0 by default) Done via strlen(), as it appears to be evaluated at compile time on string constants under GCC. Unlike sizeof or arraysize, strlen does not include the trailing zero. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:367: if (line_type_ == kBound) state_ = kFirst; On 2012/07/06 07:36:24, battre wrote: > \n after ) Done. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:458: if (parser_ != NULL) return parser_->GetNextPair(result); On 2012/07/06 07:36:24, battre wrote: > newline before return. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... Done. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:475: list = reinterpret_cast<ListValue*>(receive); // Safe due to contract. On 2012/07/06 07:36:24, battre wrote: > you can get rid of the else clause if you use > if (!dictionary->GetList(key, &list)) { > above. Then you don't need receive either. Done. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:478: list->Append(string_value); On 2012/07/06 07:36:24, battre wrote: > I would inline string_value. Done. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:489: DictionaryValue* post_data = new DictionaryValue(); On 2012/07/06 07:36:24, battre wrote: > please use a scoped_ptr<DictionaryValue>, this way the pointer cannot get lost. Done, including using get()/release() where appropriate and removing the delete clause. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:495: if (!parser.SetSource(bytes, size)) continue; On 2012/07/06 07:36:24, battre wrote: > Any reason why you don't pass a const std::vector<char>& here? No, good point. Done. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:500: UpdateDictionaryWithString(post_data, key, val); On 2012/07/06 07:36:24, battre wrote: > How about > GetOrCreateList(post_data, key)->Append(new StringValue(val)); > ? > "Update" is kind of abstract. Done. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:503: if (parser.AllDataReadOK()) out->Set(keys::kPostDataKey, post_data); On 2012/07/06 07:36:24, battre wrote: > newline before out->Set > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... Done. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api.cc:842: extra_info_spec | ExtraInfoSpec::REQUEST_POST_DATA) On 2012/07/06 07:36:24, battre wrote: > this should probably be extra_info_spec & ... > > I would reverse the order of the check. Done. I agree that reversing the order could be a nice speed-up, thanks for pointing that out. http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:79: enum TestMessageResult {kPostDataFound, kNoPostData, kError}; On 2012/07/06 07:36:24, battre wrote: > style: > enum TestMessageResult { > kPostDataFound, > kNoPostData, > kError > }; Done.
Random drive by: Could you get someone familiar with the net/ stack (particularly net/http) to give a once-over on this? I'm particularly concerned with how things like chunked-uploads (Transfer-Encoding: Chunked) will work, since the form body may be spread over multiple disparate chunks.
Ryan, Thanks for your comment. I'll make sure to have someone familiar with net/ take a look at this before committing. Before that, I need to speak with Dominic to have an understanding on how much support for POST data access we want to offer now. Based on that this CL might change a lot. Currently this code just ignores chunked uploads. Dominic, I added the browser test and the (generated) documentation. We should speak about multiple things once you return from vacations, e.g.: * I no longer believe blobs can be used to simulate file uploads, * we should also take a look at the issue with chunks, * the browser test might be inappropriately long (or too small?) * I need a reminder what exactly should I have prefixed with "experimental" -- the "requestPostData" keyword, or the postData key in event details? * etc. Cheers, Vaclav
First comments on web_request_api.cc (did not look at the rest, yet) https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:176: class PostDataParser { could you move all of this new code to c/b/e/api/web_request/post_data_parser.{h,cc}? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:183: // Returns true on success. There is no |length| anymore. https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:184: virtual bool SetSource(const std::vector<char>& source) = 0; I think you should pass a const std::vector<char>* to demonstrate that the class may keep a pointer to |source|. My assumption of this function definition would be that |source| needs to be valid for the life time of the PostDataParser. You should also document that life time requirements of |source|. https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:184: virtual bool SetSource(const std::vector<char>& source) = 0; What do you think of making SetSource part of the constructor rather than a separate function? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:193: PostDataParser() {} You need to declare a virtual destructor. https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:199: class PostDataParserUrlEncoded : public PostDataParser { Can you add a reference to the RFC according to which this data is parsed? What do you think of de-uuencoding the data returns by GetNextPair? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:202: ~PostDataParserUrlEncoded() {} virtual https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:209: // We parse the first |length_| bytes from |source_|. There is no |length_| anymore. https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:215: bool PostDataParserUrlEncoded::AllDataReadOK() { nit: the function order should match the function order in the class definition. https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:246: offset_ = seek == source_->end() ? seek : seek+1; nit: spaces around + opt: I think I would put parentheses around the (seek == ... + 1) https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:250: class PostDataParserMultipart : public PostDataParser { Can you mention the RFC according to which this parses? Is this RFC2388? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:256: ~PostDataParserMultipart() {} virtual https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:270: enum Lines {kBound, kFBound, kDisp, kEmpty, kOther}; Can you expand these names? We prefer to have no abbreviations in Chrome. -> kBoundary, kFinalBoundary, kContentDisposition, kEmpty, kOther https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:350: const size_t line_length = line_end_ - line_; How about this? StringPiece line = (source_ + line_, line_end_ - line_); // maybe line_ becomes line_start_. bool boundary_test = line.starts_with("--" + boundary_); alternatively: if (line == "--" + bounary_) line_type = kBound; else if (line == "--" + boundary_ + "--") line_type_ = kFBound; instead of calculating "--" + bounadry_ and "--" + boundary_ + "--" each time you may store them in the class. https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:372: size_t seek = line_ = next_line_; could please you split variable declarations and assignment into separate lines here? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:377: if (seek < length_ && *(source_ + seek + 1) != '\n') should this be if (seek + 1 < length_ ...) ? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:423: bool PostDataParserMultipart::ParseHead(Result* result) { You can verify the contract via DCHECK_EQ(kDisp, line_type_) https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:424: std::string line(source_ + line_, line_end_-line_); nit: space around - https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:425: size_t key_offset = line.find(" name=\"") + 7; what happens if find returns std::string::npos? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:426: if (key_offset == std::string::npos) won't be the case because of the +7 https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:442: class PostDataParserProxy : public PostDataParser { Would it not be simpler to write a scoped_ptr<PostDataParser> CreatePostDataParser(net::URLRequest* request) function? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api.cc:447: delete parser_; The better way to do this is using a scoped_ptr
some more comments https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:83: }; nit: newline https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:84: TestMessageResult TestMessage(IPC::Message* message, std::string* post_data) { Can you rename this to something more descriptive? E.g. GetPostData()? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:85: if (message->type() != ExtensionMsg_MessageInvoke::ID) return kError; can you move the return statements into the next line? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:85: if (message->type() != ExtensionMsg_MessageInvoke::ID) return kError; I think all cases where you return kError are cases where the input is invalid. How about using assertions instead? I.e. ASSERT_TRUE(ExtensionMsg_MessageInvoke::Read(message, ¶m)); ASSERT_EQ(2, param.c.GetSize()); ASSERT_TRUE(param.c.Get(1, &temp_value)); Then it would be sufficient to return true and false. https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:90: if (!param.c.Get(1,&temp_value)) return kError; nit: space before & https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:96: post_data_start += sizeof(kPostDataHead) - 1; //-1 for trailing '\0' nit: strlen? 2 spaces before //, 1 space after // https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:490: {kResultMultipart, kResultMultipart, kResultUrlEncoded, NULL, NULL}; Rename this to kExpectedResults? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/browser/ext... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:577: EXPECT_FALSE(i == ipc_sender_.sent_end()); does EXPECT_NE work? Otherwise, I think it is simpler to understand EXPECT_TRUE(i != ipc_Sender_.send_end()); https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/test/data/e... File chrome/test/data/extensions/api_test/webrequest/framework.js (right): https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/test/data/e... chrome/test/data/extensions/api_test/webrequest/framework.js:271: }, filter, intersect(extraInfoSpec, ["blocking","requestPostData"])); nit: space before " https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/test/data/e... File chrome/test/data/extensions/api_test/webrequest/postData/multipart.html (right): https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/test/data/e... chrome/test/data/extensions/api_test/webrequest/postData/multipart.html:12: --> remove these lines? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/test/data/e... chrome/test/data/extensions/api_test/webrequest/postData/multipart.html:15: <input name="pw" id="pw" type="password" value="pw" /> rename to password? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/test/data/e... chrome/test/data/extensions/api_test/webrequest/postData/multipart.html:18: <input name="chck" id="chck" type="checkbox" value="option_A" checked /> A rename to check? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/test/data/e... File chrome/test/data/extensions/api_test/webrequest/postData/no-enctype.html (right): https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/test/data/e... chrome/test/data/extensions/api_test/webrequest/postData/no-enctype.html:12: --> delete? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/test/data/e... File chrome/test/data/extensions/api_test/webrequest/postData/plaintext.html (right): https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/test/data/e... chrome/test/data/extensions/api_test/webrequest/postData/plaintext.html:12: --> delete? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/test/data/e... File chrome/test/data/extensions/api_test/webrequest/postData/urlencoded.html (right): https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/test/data/e... chrome/test/data/extensions/api_test/webrequest/postData/urlencoded.html:12: --> delete? https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/test/data/e... File chrome/test/data/extensions/api_test/webrequest/test_post.js (right): https://chromiumcodereview.appspot.com/10694055/diff/17019/chrome/test/data/e... chrome/test/data/extensions/api_test/webrequest/test_post.js:22: //frameId: 0, nit: space after // (also below)
Hi Dominic, I addressed your comments and reorganized the code. Please have another look. Cheers, Vaclav http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:176: class PostDataParser { On 2012/07/11 10:59:50, battre wrote: > could you move all of this new code to > c/b/e/api/web_request/post_data_parser.{h,cc}? Done, except for ExtractRequestInfoPost which I feel belongs here. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:183: // Returns true on success. On 2012/07/11 10:59:50, battre wrote: > There is no |length| anymore. Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:184: virtual bool SetSource(const std::vector<char>& source) = 0; On 2012/07/11 10:59:50, battre wrote: > What do you think of making SetSource part of the constructor rather than a > separate function? That would not work, as SetSource is called multiple times (see PostDataParsetMultipart). http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:184: virtual bool SetSource(const std::vector<char>& source) = 0; On 2012/07/11 10:59:50, battre wrote: > I think you should pass a const std::vector<char>* to demonstrate that the class > may keep a pointer to |source|. My assumption of this function definition would > be that |source| needs to be valid for the life time of the PostDataParser. You > should also document that life time requirements of |source|. Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:193: PostDataParser() {} On 2012/07/11 10:59:50, battre wrote: > You need to declare a virtual destructor. Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:199: class PostDataParserUrlEncoded : public PostDataParser { On 2012/07/11 10:59:50, battre wrote: > Can you add a reference to the RFC according to which this data is parsed? What > do you think of de-uuencoding the data returns by GetNextPair? This is not covered in a RFC, but it is described in the HTML standard: http://www.w3.org/TR/REC-html40-971218/interact/forms.html#h-17.13.4.1. Decoding sounds good to me, once adding the write-support, we will have to encode back anyway. I had to modify the PostDataParser::Result to make this possible (up to now Result was only holding a pointer to original data, making it impossible to alter that). http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:202: ~PostDataParserUrlEncoded() {} On 2012/07/11 10:59:50, battre wrote: > virtual Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:209: // We parse the first |length_| bytes from |source_|. On 2012/07/11 10:59:50, battre wrote: > There is no |length_| anymore. Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:215: bool PostDataParserUrlEncoded::AllDataReadOK() { On 2012/07/11 10:59:50, battre wrote: > nit: the function order should match the function order in the class definition. Done, converted all to alphabetical order. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:246: offset_ = seek == source_->end() ? seek : seek+1; On 2012/07/11 10:59:50, battre wrote: > nit: spaces around + > opt: I think I would put parentheses around the (seek == ... + 1) Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:250: class PostDataParserMultipart : public PostDataParser { On 2012/07/11 10:59:50, battre wrote: > Can you mention the RFC according to which this parses? Is this RFC2388? Done. Yes. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:256: ~PostDataParserMultipart() {} On 2012/07/11 10:59:50, battre wrote: > virtual Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:270: enum Lines {kBound, kFBound, kDisp, kEmpty, kOther}; On 2012/07/11 10:59:50, battre wrote: > Can you expand these names? We prefer to have no abbreviations in Chrome. -> > kBoundary, kFinalBoundary, kContentDisposition, kEmpty, kOther Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:350: const size_t line_length = line_end_ - line_; On 2012/07/11 10:59:50, battre wrote: > How about this? > > StringPiece line = (source_ + line_, line_end_ - line_); // maybe line_ becomes > line_start_. > bool boundary_test = line.starts_with("--" + boundary_); > > alternatively: > > if (line == "--" + bounary_) > line_type = kBound; > else if (line == "--" + boundary_ + "--") > line_type_ = kFBound; > > instead of calculating "--" + bounadry_ and "--" + boundary_ + "--" each time > you may store them in the class. Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:372: size_t seek = line_ = next_line_; On 2012/07/11 10:59:50, battre wrote: > could please you split variable declarations and assignment into separate lines > here? Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:377: if (seek < length_ && *(source_ + seek + 1) != '\n') On 2012/07/11 10:59:50, battre wrote: > should this be if (seek + 1 < length_ ...) ? No, it should equal to the first part of the condition in the while-cycle above. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:423: bool PostDataParserMultipart::ParseHead(Result* result) { On 2012/07/11 10:59:50, battre wrote: > You can verify the contract via > DCHECK_EQ(kDisp, line_type_) Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:424: std::string line(source_ + line_, line_end_-line_); On 2012/07/11 10:59:50, battre wrote: > nit: space around - Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:425: size_t key_offset = line.find(" name=\"") + 7; On 2012/07/11 10:59:50, battre wrote: > what happens if find returns std::string::npos? Corrected to: 1. Check against npos. 2. Increment if offset != npos. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:426: if (key_offset == std::string::npos) On 2012/07/11 10:59:50, battre wrote: > won't be the case because of the +7 Solved, see above. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:442: class PostDataParserProxy : public PostDataParser { On 2012/07/11 10:59:50, battre wrote: > Would it not be simpler to write a scoped_ptr<PostDataParser> > CreatePostDataParser(net::URLRequest* request) function? Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:447: delete parser_; On 2012/07/11 10:59:50, battre wrote: > The better way to do this is using a scoped_ptr Done via replacing the whole parser-proxy thing with the scoped_ptr. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:83: }; On 2012/07/11 12:28:43, battre wrote: > nit: newline Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:84: TestMessageResult TestMessage(IPC::Message* message, std::string* post_data) { On 2012/07/11 12:28:43, battre wrote: > Can you rename this to something more descriptive? E.g. GetPostData()? Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:85: if (message->type() != ExtensionMsg_MessageInvoke::ID) return kError; On 2012/07/11 12:28:43, battre wrote: > I think all cases where you return kError are cases where the input is invalid. > How about using assertions instead? I.e. > > ASSERT_TRUE(ExtensionMsg_MessageInvoke::Read(message, ¶m)); > ASSERT_EQ(2, param.c.GetSize()); > ASSERT_TRUE(param.c.Get(1, &temp_value)); > > Then it would be sufficient to return true and false. Done. Had to move the return bool to (output) arguments, because ASSERT_* cannot be used in non-void functions. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:85: if (message->type() != ExtensionMsg_MessageInvoke::ID) return kError; On 2012/07/11 12:28:43, battre wrote: > can you move the return statements into the next line? Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:90: if (!param.c.Get(1,&temp_value)) return kError; On 2012/07/11 12:28:43, battre wrote: > nit: space before & Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:96: post_data_start += sizeof(kPostDataHead) - 1; //-1 for trailing '\0' On 2012/07/11 12:28:43, battre wrote: > nit: strlen? 2 spaces before //, 1 space after // Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:490: {kResultMultipart, kResultMultipart, kResultUrlEncoded, NULL, NULL}; On 2012/07/11 12:28:43, battre wrote: > Rename this to kExpectedResults? Done. http://codereview.chromium.org/10694055/diff/17019/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:577: EXPECT_FALSE(i == ipc_sender_.sent_end()); On 2012/07/11 12:28:43, battre wrote: > does EXPECT_NE work? Otherwise, I think it is simpler to understand > EXPECT_TRUE(i != ipc_Sender_.send_end()); Done. http://codereview.chromium.org/10694055/diff/17019/chrome/test/data/extension... File chrome/test/data/extensions/api_test/webrequest/framework.js (right): http://codereview.chromium.org/10694055/diff/17019/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/framework.js:271: }, filter, intersect(extraInfoSpec, ["blocking","requestPostData"])); On 2012/07/11 12:28:43, battre wrote: > nit: space before " Done. http://codereview.chromium.org/10694055/diff/17019/chrome/test/data/extension... File chrome/test/data/extensions/api_test/webrequest/postData/multipart.html (right): http://codereview.chromium.org/10694055/diff/17019/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/postData/multipart.html:12: --> On 2012/07/11 12:28:43, battre wrote: > remove these lines? Done. http://codereview.chromium.org/10694055/diff/17019/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/postData/multipart.html:15: <input name="pw" id="pw" type="password" value="pw" /> On 2012/07/11 12:28:43, battre wrote: > rename to password? Done. http://codereview.chromium.org/10694055/diff/17019/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/postData/multipart.html:18: <input name="chck" id="chck" type="checkbox" value="option_A" checked /> A On 2012/07/11 12:28:43, battre wrote: > rename to check? Done. http://codereview.chromium.org/10694055/diff/17019/chrome/test/data/extension... File chrome/test/data/extensions/api_test/webrequest/postData/no-enctype.html (right): http://codereview.chromium.org/10694055/diff/17019/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/postData/no-enctype.html:12: --> On 2012/07/11 12:28:43, battre wrote: > delete? Done. http://codereview.chromium.org/10694055/diff/17019/chrome/test/data/extension... File chrome/test/data/extensions/api_test/webrequest/postData/plaintext.html (right): http://codereview.chromium.org/10694055/diff/17019/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/postData/plaintext.html:12: --> On 2012/07/11 12:28:43, battre wrote: > delete? Done. http://codereview.chromium.org/10694055/diff/17019/chrome/test/data/extension... File chrome/test/data/extensions/api_test/webrequest/postData/urlencoded.html (right): http://codereview.chromium.org/10694055/diff/17019/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/postData/urlencoded.html:12: --> On 2012/07/11 12:28:43, battre wrote: > delete? Done. http://codereview.chromium.org/10694055/diff/17019/chrome/test/data/extension... File chrome/test/data/extensions/api_test/webrequest/test_post.js (right): http://codereview.chromium.org/10694055/diff/17019/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/test_post.js:22: //frameId: 0, On 2012/07/11 12:28:43, battre wrote: > nit: space after // (also below) Actually, those comments were there by mistake. Removed now.
Hi Dominic, Eventually I added the member value initialization for basic types, as I can see your point of making debugging easier by preventing random values to appear there. Once we are finished with the current state of the code, I should address Ryan's comment about dealing with chunks. Thanks, Vaclav
Great. I think we are almost there. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... File chrome/browser/extensions/api/web_request/post_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:90: result->get_key() = net::UnescapeURLComponent(encoded_key, unescape_rules); I think this would be more natural as result->set_key(...) http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:95: result->get_val() = net::UnescapeURLComponent(encoded_val, unescape_rules); same as above. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:118: final_boundary_(boundary_+"--"), nit: spaces around + http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:129: bool PostDataParserMultipart::GetNextPair(Result* result) { Can you add more explanation what this function does? http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:133: while (state_ != kSkip) { Can you explain this logic, maybe in a comment. Why do loop until a state kSkip is reached? Maybe the name kSkip is not very suitable. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:211: // Contract: only to be called from GetNextLine(). optional: What do you think of changing this to Lines void PostDataParserMultipart::GetLineType(const StringPiece& line); and removing this contract? http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:212: void PostDataParserMultipart::GetLineType() { I think if you don't return anything I would rename this to DetermineLineType() http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:228: bool PostDataParserMultipart::GetNextLine() { opt: SeekNextLine? http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:237: line_end_ = seek; so line_end_ points to after the end of the line, right? I think you should document that in the header file. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:239: if (seek < length_ && *(source_ + seek + 1) != '\n') source_ + seek + 1 could point beyond the end of the string http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... File chrome/browser/extensions/api/web_request/post_data_parser.h (right): http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.h:21: class PostDataParser { now that we have separated this into separate files, I think there should be some unit tests in post_data_parser_unittest.cc, see my comment in web_request_api_unittest.cc. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.h:25: Result(); you need to define a destructor, otherwise the destructor is generated multiple times (wired C++ behavior). http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.h:26: std::string& get_key() { I would remove this and the following function and rely on SetKey, this is more common. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.h:39: void SetKey(const char* s, size_t n); I would change this to void set_key(const std::string& key) { key_ = key; } same below. The current implementation looks complicated. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.h:51: virtual ~PostDataParser() {} no, our rule is that virtual functions should be never inlined. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.h:65: PostDataParser() {} clang might complain about that. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.h:132: const std::string boundary_; Add #include <string> for this. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:431: TEST_F(ExtensionWebRequestTest, AccessPostData) { What do you think of moving most of this to post_data_parser_unittest.cc and leave just enough here to have coverage for ExtractRequestInfoPost here (while keeping the overall structure)?
Hi Dominic, Next round of changes. Please note that I accidentally rebased in between, but the picked-up changes are isolated from mine. PTAL Thanks, Vaclav http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... File chrome/browser/extensions/api/web_request/post_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:90: result->get_key() = net::UnescapeURLComponent(encoded_key, unescape_rules); On 2012/07/13 15:20:04, battre wrote: > I think this would be more natural as > result->set_key(...) Done. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:95: result->get_val() = net::UnescapeURLComponent(encoded_val, unescape_rules); On 2012/07/13 15:20:04, battre wrote: > same as above. Done. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:118: final_boundary_(boundary_+"--"), On 2012/07/13 15:20:04, battre wrote: > nit: spaces around + Done. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:129: bool PostDataParserMultipart::GetNextPair(Result* result) { On 2012/07/13 15:20:04, battre wrote: > Can you add more explanation what this function does? Done. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:133: while (state_ != kSkip) { On 2012/07/13 15:20:04, battre wrote: > Can you explain this logic, maybe in a comment. Why do loop until a state kSkip > is reached? Maybe the name kSkip is not very suitable. I changed kSkip and other state names to describe better what the states are. Is the while-loop now more readable? http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:211: // Contract: only to be called from GetNextLine(). On 2012/07/13 15:20:04, battre wrote: > optional: What do you think of changing this to > Lines void PostDataParserMultipart::GetLineType(const StringPiece& line); and > removing this contract? Done. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:212: void PostDataParserMultipart::GetLineType() { On 2012/07/13 15:20:04, battre wrote: > I think if you don't return anything I would rename this to DetermineLineType() Changed to returning Lines. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:228: bool PostDataParserMultipart::GetNextLine() { On 2012/07/13 15:20:04, battre wrote: > opt: SeekNextLine? Done. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:237: line_end_ = seek; On 2012/07/13 15:20:04, battre wrote: > so line_end_ points to after the end of the line, right? I think you should > document that in the header file. Done. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.cc:239: if (seek < length_ && *(source_ + seek + 1) != '\n') On 2012/07/13 15:20:04, battre wrote: > source_ + seek + 1 could point beyond the end of the string Corrected. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... File chrome/browser/extensions/api/web_request/post_data_parser.h (right): http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.h:21: class PostDataParser { On 2012/07/13 15:20:04, battre wrote: > now that we have separated this into separate files, I think there should be > some unit tests in post_data_parser_unittest.cc, see my comment in > web_request_api_unittest.cc. Done. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.h:25: Result(); On 2012/07/13 15:20:04, battre wrote: > you need to define a destructor, otherwise the destructor is generated multiple > times (wired C++ behavior). Done. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.h:26: std::string& get_key() { On 2012/07/13 15:20:04, battre wrote: > I would remove this and the following function and rely on SetKey, this is more > common. Done. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.h:39: void SetKey(const char* s, size_t n); On 2012/07/13 15:20:04, battre wrote: > I would change this to > void set_key(const std::string& key) { > key_ = key; > } > same below. The current implementation looks complicated. After speaking to Dominic, I added a new version of SetKey, taking std::string, in addition to the original version, which aimed at sparing one copying pass. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.h:51: virtual ~PostDataParser() {} On 2012/07/13 15:20:04, battre wrote: > no, our rule is that virtual functions should be never inlined. Done (and especially ashamed about this one). http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.h:65: PostDataParser() {} On 2012/07/13 15:20:04, battre wrote: > clang might complain about that. The trybot linux_clang did not issue a warning about this. If you still want me to put this into .cc, let me know. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/post_data_parser.h:132: const std::string boundary_; On 2012/07/13 15:20:04, battre wrote: > Add #include <string> for this. Done. http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/4043/chrome/browser/extensions/a... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:431: TEST_F(ExtensionWebRequestTest, AccessPostData) { On 2012/07/13 15:20:04, battre wrote: > What do you think of moving most of this to post_data_parser_unittest.cc and > leave just enough here to have coverage for ExtractRequestInfoPost here (while > keeping the overall structure)? Done. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.h (right): http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:51: static scoped_ptr<PostDataParser> CreatePostDataParser( I separated the two steps in the original CreatePostDataParser to aid unit-testability. (So that I do not have to construct the URLRequest in the post_data_parser_unittest.cc .)
looking good. I'll have a final pass on the final version. Have you looked for a potential reviewer from net/. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.h (right): http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:52: const std::string* value); can you rename |value| to content_type? http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:108: enum Lines {kBoundary, kEndBoundary, kDisposition, kEmpty, kOther}; nit: rename to LineType? http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:117: enum States {kInit, kHeadStart, kHead, kHeadRead, kBody, kFinal, kError}; nit: rename to State? http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser_unittest.cc (right): http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser_unittest.cc:129: EXPECT_TRUE(std::equal(output.begin(), output.end(), kExpected.begin())); Can you add an ASSERT_LE(output.size(), kExpected.size()) (also below) to prevent segfaults. Or better, we should check that we generate the right number of outputs. The condition is true if output.size() == 0. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:78: void GetPostData(IPC::Message* message, std::string* post_data, bool* success) { nit: return boolean instead of passing bool* success? http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:81: ASSERT_EQ(message->type(), ExtensionMsg_MessageInvoke::ID); as message is provided and not generated by the tested code, I would use a CHECK here. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:88: ASSERT_TRUE(temp_value->GetAsString(&args)); nit: new line after 88? http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:93: post_data_start += strlen(kPostDataHead); nit: new line after 93? http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:433: // and the data will be |bytes|. the comment goes to the declaration above. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:437: // The request URL can be arbitrary but must have a HTTP or HTTPS scheme. nit: s/a HTTP/an HTTP/ http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:445: } nit: empty line http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:478: &profile_, extension_id, extension_id, kEventName, kEventName + "/1", nit: +2 spaces http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:489: &profile_, extension_id, extension_id, kEventName, kEventName + "/1", +2 spaces http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:505: EXPECT_TRUE(post_data_found); if you write EXPECT_TRUE(post_data_found) << test; an error message is slightly simpler to analyze. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:512: EXPECT_TRUE(i == ipc_sender_.sent_end()); Does EXPECT_EQ work?
Hi Asanka, Do you think you could review this? The main code review is (being) done by Dominic, and I will soon add somebody from the extensions team, but I need your net/ expertise. Ryan expressed concerns about my handling of forms uploaded as chunks (Transfer-Encoding: Chunked). Currently I ignore those (see line 201 in c/b/extensions/api/web_request/web_request_api.cc). Do you know under which circumstances gets an HTML form uploaded in a chunked way? And, if this happens, then is the form spread across multiple URLRequests, or do I just see one request with multiple TYPE_CHUNK data elements? Looking forward to your insights and/or redirection to somebody else. Hi Dominic, Please take a look at my response to your comments. Thank you all, Vaclav http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.h (right): http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:52: const std::string* value); On 2012/07/16 17:39:45, battre wrote: > can you rename |value| to content_type? Done. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:108: enum Lines {kBoundary, kEndBoundary, kDisposition, kEmpty, kOther}; On 2012/07/16 17:39:45, battre wrote: > nit: rename to LineType? Done. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:117: enum States {kInit, kHeadStart, kHead, kHeadRead, kBody, kFinal, kError}; On 2012/07/16 17:39:45, battre wrote: > nit: rename to State? Done. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser_unittest.cc (right): http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser_unittest.cc:129: EXPECT_TRUE(std::equal(output.begin(), output.end(), kExpected.begin())); On 2012/07/16 17:39:45, battre wrote: > Can you add an ASSERT_LE(output.size(), kExpected.size()) (also below) to > prevent segfaults. Or better, we should check that we generate the right number > of outputs. The condition is true if output.size() == 0. Thanks for catching this! I avoided the ASSERT by first EXPECTing equal sizes of |output| and |kExpected|, and then testing further if they are equal. Thus the test avoids segfaults from overflowing |kExpected|, and still should be correctly checking that |kExpected| == |output|. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:78: void GetPostData(IPC::Message* message, std::string* post_data, bool* success) { On 2012/07/16 17:39:45, battre wrote: > nit: return boolean instead of passing bool* success? That would not work, ASSERT_* cannot be used in non-void functions. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:81: ASSERT_EQ(message->type(), ExtensionMsg_MessageInvoke::ID); On 2012/07/16 17:39:45, battre wrote: > as message is provided and not generated by the tested code, I would use a CHECK > here. Done. QUESTION: Do you want me to change this also for the first line of TestIPCSender::Send() ? http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:88: ASSERT_TRUE(temp_value->GetAsString(&args)); On 2012/07/16 17:39:45, battre wrote: > nit: new line after 88? Done. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:93: post_data_start += strlen(kPostDataHead); On 2012/07/16 17:39:45, battre wrote: > nit: new line after 93? Done. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:433: // and the data will be |bytes|. On 2012/07/16 17:39:45, battre wrote: > the comment goes to the declaration above. Done. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:437: // The request URL can be arbitrary but must have a HTTP or HTTPS scheme. On 2012/07/16 17:39:45, battre wrote: > nit: s/a HTTP/an HTTP/ Done. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:445: } On 2012/07/16 17:39:45, battre wrote: > nit: empty line Done. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:478: &profile_, extension_id, extension_id, kEventName, kEventName + "/1", On 2012/07/16 17:39:45, battre wrote: > nit: +2 spaces Done. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:489: &profile_, extension_id, extension_id, kEventName, kEventName + "/1", On 2012/07/16 17:39:45, battre wrote: > +2 spaces Done. http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:505: EXPECT_TRUE(post_data_found); On 2012/07/16 17:39:45, battre wrote: > if you write > EXPECT_TRUE(post_data_found) << test; > an error message is slightly simpler to analyze. Done. Thanks for teaching me this. :) http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:512: EXPECT_TRUE(i == ipc_sender_.sent_end()); On 2012/07/16 17:39:45, battre wrote: > Does EXPECT_EQ work? Done. It seems to work, just as the EXPECT_NE a couple of lines above. I don't know why I started with EXPECT_TRUE.
one final comment :-) http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:78: void GetPostData(IPC::Message* message, std::string* post_data, bool* success) { On 2012/07/17 11:11:11, vabr (Chromium) wrote: > On 2012/07/16 17:39:45, battre wrote: > > nit: return boolean instead of passing bool* success? > > That would not work, ASSERT_* cannot be used in non-void functions. You can replace all ASSERT_* statements with CHECK statements because they would indicate that the input is invalid.
Dominic, Thanks for all your comments. Your most recent one has just been addressed. Vaclav http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:78: void GetPostData(IPC::Message* message, std::string* post_data, bool* success) { On 2012/07/17 12:46:07, battre wrote: > On 2012/07/17 11:11:11, vabr (Chromium) wrote: > > On 2012/07/16 17:39:45, battre wrote: > > > nit: return boolean instead of passing bool* success? > > > > That would not work, ASSERT_* cannot be used in non-void functions. > > You can replace all ASSERT_* statements with CHECK statements because they would > indicate that the input is invalid. Done.
Please upload the CL again. The new web_request_api_unittest.cc is not accessible.
Re-uploaded, now both files changed since #14 should be available. Sorry for that inconvenience. Vaclav
On 2012/07/17 11:11:11, vabr (Chromium) wrote: > Hi Asanka, > > Do you think you could review this? > The main code review is (being) done by Dominic, and I will soon add somebody > from the extensions team, but I need your net/ expertise. > > Ryan expressed concerns about my handling of forms uploaded as chunks > (Transfer-Encoding: Chunked). Currently I ignore those (see line 201 in > c/b/extensions/api/web_request/web_request_api.cc). Do you know under which > circumstances gets an HTML form uploaded in a chunked way? And, if this happens, > then is the form spread across multiple URLRequests, or do I just see one > request with multiple TYPE_CHUNK data elements? > > Looking forward to your insights and/or redirection to somebody else. +satorux who knows the UploadData/UploadDataStream logic better.
Chuncked encoding support in UploadData/UploadDataStream is a very dirty hack, so it's best to avoid using it. I wanted to clean this up but got distracted by other tasks. The chunked uploading is only useful when you don't know the upload data size beforehand. This is used for voice search, so that chrome can start uploading the user voice data as soon as the speech starts, rather than waiting for the completion.
Satoru-san, Thanks for your explanation. Could you please review my code from the networking perspective? More details follow here: In this code I am actually a consumer of uploaded data -- each time the browser sends POST data to a server, my code attempts to see if it is an encoded HTML form, and if so, decodes it. Currently I ignore chunked uploads (UploadData::Element objects of type TYPE_CHUNK), and also uploads with blobs (see the for-loop in ExtractRequestInfoPost in c/b/extensions/api/web_request/web_request_api.cc). As for chunks, based on your explanation, I guess that a HTML form would hardly be uploaded as chunked data, correct? As for ignoring blobs, I found this comment [http://code.google.com/searchframe#OAMlx_jo-ck/src/net/base/upload_data.h&exact_package=chromium&q=TYPE_BLOB&l=101] which indicates that blobs also should not appear in normal POST data uploads. Dominic, Please let me know what you think about the recent changes with explicit ignoring of CHUNKs and BLOBs, and adding the formData parameter. Cheers, Vaclav
http://codereview.chromium.org/10694055/diff/51001/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/51001/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:119: "additionalProperties": { I don't understand this section
Answering Dominic's comment. http://codereview.chromium.org/10694055/diff/51001/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/51001/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:119: "additionalProperties": { On 2012/07/19 11:12:54, battre wrote: > I don't understand this section The 'additionalProperties' say that this dictionary can contain arbitrary key-value pairs. Without it the schema validator would complain each time the formData would be non-empty.
Satoru-san, Do you think you could take a look at this from the networking perspective? More details are in my previous e-mail to you on this CL. Matt, Could you please review this for the extensions part? Dominic, Please take a look at the example value of formData I added to the docs. Thanks to all of you, Vaclav http://codereview.chromium.org/10694055/diff/51001/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/51001/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:119: "additionalProperties": { On 2012/07/19 11:44:40, vabr (Chromium) wrote: > On 2012/07/19 11:12:54, battre wrote: > > I don't understand this section > > The 'additionalProperties' say that this dictionary can contain arbitrary > key-value pairs. Without it the schema validator would complain each time the > formData would be non-empty. OK, sorry I misunderstood your question. The type of formData is a dictionary where values are arrays of strings. I will amend the description to include a short example.
On 2012/07/23 17:01:21, vabr (Chromium) wrote: > Satoru-san, > > Do you think you could take a look at this from the networking perspective? More > details are in my previous e-mail to you on this CL. Sorry for the belated response. I'm not a chrome networking guy. I was just a random person who refactored UploadData stuff, so I don't think I'm not qualified. wtc@ and willchan@ would be good people to talk to. > > > Matt, > > Could you please review this for the extensions part? > > > Dominic, > > Please take a look at the example value of formData I added to the docs. > > Thanks to all of you, > Vaclav > > http://codereview.chromium.org/10694055/diff/51001/chrome/common/extensions/a... > File chrome/common/extensions/api/web_request.json (right): > > http://codereview.chromium.org/10694055/diff/51001/chrome/common/extensions/a... > chrome/common/extensions/api/web_request.json:119: "additionalProperties": { > On 2012/07/19 11:44:40, vabr (Chromium) wrote: > > On 2012/07/19 11:12:54, battre wrote: > > > I don't understand this section > > > > The 'additionalProperties' say that this dictionary can contain arbitrary > > key-value pairs. Without it the schema validator would complain each time the > > formData would be non-empty. > > OK, sorry I misunderstood your question. The type of formData is a dictionary > where values are arrays of strings. I will amend the description to include a > short example.
> Sorry for the belated response. I'm not a chrome networking guy. I was just a > random person who refactored UploadData stuff, so I don't think I'm not > qualified. wtc@ and willchan@ would be good people to talk to. Thanks for the pointer! I will ask one of them for review. Just one question, given what you wrote here earlier -- do you think it is probable that a HTML form would be uploaded as chunked data? From what you wrote I guess not, could you confirm it? Thanks, Vaclav
On 2012/07/23 17:13:00, vabr (Chromium) wrote: > > Sorry for the belated response. I'm not a chrome networking guy. I was just a > > random person who refactored UploadData stuff, so I don't think I'm not > > qualified. wtc@ and willchan@ would be good people to talk to. > > Thanks for the pointer! I will ask one of them for review. > > Just one question, given what you wrote here earlier -- do you think it is > probable that a HTML form would be uploaded as chunked data? From what you wrote > I guess not, could you confirm it? From HTML forms, I think chunked encoding won't be used. The file size is always known for uploading from HTML forms > > Thanks, > > Vaclav
Hi Wan-Teh, Do you think you could review the networking part of this? I touch networking here: I scan each URLRequest with POST data, looking for encoded HTML forms being sent to a server. When a request contains chunked data or blobs I ignore it. See the for-loop in ExtractRequestInfoPost in c/b/extensions/api/web_request/web_request_api.cc . Ryan raised concern about the ignoring of chunked data. Do you think forms can be uploaded in this way? Thanks, Vaclav
On 2012/07/23 17:23:31, vabr (Chromium) wrote: > Hi Wan-Teh, > > Do you think you could review the networking part of this? > > I touch networking here: > I scan each URLRequest with POST data, looking for encoded HTML forms being sent > to a server. > When a request contains chunked data or blobs I ignore it. See the for-loop in > ExtractRequestInfoPost in c/b/extensions/api/web_request/web_request_api.cc . > > Ryan raised concern about the ignoring of chunked data. Do you think forms can > be uploaded in this way? > > Thanks, > > Vaclav Vaclav, The concern was that while we only use chunked request bodies /today/ under certain limited conditions (eg: the UploadDataStream has been explicitly indicated as chunked, which only happens when doing speech recognition and which should only happen over SPDY in practice), chunked request bodies are valid HTTP/1.1, and there are proposals (eg: http://tools.ietf.org/html/draft-nottingham-http-browser-hints-03 ) that seek to promote their usage. Because the current implementation requires low-level parsing (ie, there is no API to extract the FormDataList from WebKit), any such changes would break this extension in subtle and surprising ways.
note: I did not review the PostDataParser This mostly looks good. Just a few comments on the API. http://codereview.chromium.org/10694055/diff/44028/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/44028/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:194: scoped_ptr<DictionaryValue> post_data(new DictionaryValue()); The rest of this function seems like it should be a helper method on PostDataParser. http://codereview.chromium.org/10694055/diff/44028/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/44028/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:109: "experimentalPostData": { Drop the "experimental" prefix. We're moving away from that convention. http://codereview.chromium.org/10694055/diff/44028/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:145: "enum": ["blocking", "requestPostData"] imo "request" is unnecessary here. There's only 1 kind of "postData", so I think that would be a fine name. http://codereview.chromium.org/10694055/diff/44028/chrome/test/data/extension... File chrome/test/data/extensions/api_test/webrequest/test_post.js (right): http://codereview.chromium.org/10694055/diff/44028/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/test_post.js:82: (includePostData) ? { label: "b-onBeforeRequest", I think you could make this more succinct like so: { details: { method: "POST", ..., postData: includePostData ? <postData> : undefined } }
On 2012/07/23 17:23:31, vabr (Chromium) wrote: > Hi Wan-Teh, > > Do you think you could review the networking part of this? Hi vabr, I'll be happy to review the networking part of this CL, but I won't be able to review it until Thursday. Is that OK?
http://codereview.chromium.org/10694055/diff/44028/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/44028/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:109: "experimentalPostData": { On 2012/07/23 21:19:01, Matt Perry wrote: > Drop the "experimental" prefix. We're moving away from that convention. Don't you want to indicate to developers that this feature may change? I am concerned that we might not get it right in the first try.
On 2012/07/23 23:54:30, wtc wrote: > On 2012/07/23 17:23:31, vabr (Chromium) wrote: > > Hi Wan-Teh, > > > > Do you think you could review the networking part of this? > > Hi vabr, > > I'll be happy to review the networking part of this CL, but > I won't be able to review it until Thursday. Is that OK? Thank you very much! Thursday is more than fine, this is not really that urgent. Currently I am working on comments from Matt, Ryan and Dominic, once I am done with that, I will send a mail through this CL clarifying the crucial parts to be reviewed by respective people. So it is good that you will be able to review this not sooner than on Thursday, it will be more polished by then. Vaclav
http://codereview.chromium.org/10694055/diff/44028/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/44028/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:109: "experimentalPostData": { On 2012/07/24 09:28:53, battre wrote: > On 2012/07/23 21:19:01, Matt Perry wrote: > > Drop the "experimental" prefix. We're moving away from that convention. > > Don't you want to indicate to developers that this feature may change? I am > concerned that we might not get it right in the first try. The description indicates that. Also, the feature is restricted to the dev channel. I think that's enough, and is the convention we're moving towards.
On 2012/07/24 15:45:06, vabr (Chromium) wrote: > On 2012/07/23 23:54:30, wtc wrote: > > On 2012/07/23 17:23:31, vabr (Chromium) wrote: > > > Hi Wan-Teh, > > > > > > Do you think you could review the networking part of this? > > > > Hi vabr, > > > > I'll be happy to review the networking part of this CL, but > > I won't be able to review it until Thursday. Is that OK? > > Thank you very much! > Thursday is more than fine, this is not really that urgent. > > Currently I am working on comments from Matt, Ryan and Dominic, once I am done > with that, I will send a mail through this CL clarifying the crucial parts to be > reviewed by respective people. > So it is good that you will be able to review this not sooner than on Thursday, > it will be more polished by then. > > Vaclav One more update -- during implementing some review comments, this CL got blocked by some yak-shaving (CL 10834004 and 10832003). So currently it is not in a reviewable state. I will send a notice once it can be reviewed again. Sorry for that, Vaclav
Dear reviewers, thanks for your patience! Ryan, Thanks for your explanation. I can see the fragility in not supporting chunked uploads. As a fast "solution" I added error reporting to notify the user about forms uploaded in chunks. (See chrome/browser/extensions/api/web_request/web_request_api.cc:171 .) The other way would be to just support this, but I would like to postpone that for two reasons. First, it will be easier to develop and test this once the forms are actually uploaded chunked by the browser. Second, the API has not been used yet, and I would like to see the actual use-cases before deciding where to invest more time on this. If that does not sound acceptable to you, please let me know. Matt, Are you satisfied with how I addressed your comments? Wan-Teh, If you are still willing to review the network part, please feel free to proceed. I would appreciate if you could take a look at the parsers for encoded forms (chrome/browser/extensions/api/web_request/post_data_parser.*). Dominic, I added the error-logging for chunked forms and changed the structure of "details" passed to the listener, based on our conversation. I also extended the unit-tests accordingly. Please let me know what you think. Thanks to all! Vaclav http://codereview.chromium.org/10694055/diff/44028/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/44028/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:194: scoped_ptr<DictionaryValue> post_data(new DictionaryValue()); On 2012/07/23 21:19:01, Matt Perry wrote: > The rest of this function seems like it should be a helper method on > PostDataParser. Fair point, I tried to move as much as I felt is in the parser's scope. http://codereview.chromium.org/10694055/diff/44028/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/44028/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:109: "experimentalPostData": { On 2012/07/24 20:46:57, Matt Perry wrote: > On 2012/07/24 09:28:53, battre wrote: > > On 2012/07/23 21:19:01, Matt Perry wrote: > > > Drop the "experimental" prefix. We're moving away from that convention. > > > > Don't you want to indicate to developers that this feature may change? I am > > concerned that we might not get it right in the first try. > > The description indicates that. Also, the feature is restricted to the dev > channel. I think that's enough, and is the convention we're moving towards. I removed the 'experimental' prefix based on the above discussion. http://codereview.chromium.org/10694055/diff/44028/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:145: "enum": ["blocking", "requestPostData"] On 2012/07/23 21:19:01, Matt Perry wrote: > imo "request" is unnecessary here. There's only 1 kind of "postData", so I think > that would be a fine name. Done. http://codereview.chromium.org/10694055/diff/44028/chrome/test/data/extension... File chrome/test/data/extensions/api_test/webrequest/test_post.js (right): http://codereview.chromium.org/10694055/diff/44028/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/test_post.js:82: (includePostData) ? { label: "b-onBeforeRequest", On 2012/07/23 21:19:01, Matt Perry wrote: > I think you could make this more succinct like so: > { > details: { > method: "POST", > ..., > postData: includePostData ? <postData> : undefined > } > } I tried that but it did not work. Even if the expected details.postData is undefined, such 'details' still has a property 'postData'. So the expected details is different from the real details, which does not even have the property 'postData'. Luckily, the test was not quite right -- when no form data can be parsed, but the request contains POST data, the postData field still has to be present, only does not contain formData. I corrected this behaviour in the code, and made this test shorter eventually. http://codereview.chromium.org/10694055/diff/65001/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/65001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:131: // TODO(vabr): Add const once ListValue getters get corrected. I am concurrently taking care of correction the ListValue (already did for DictionaryValue), but it may take longer than this CL, as it is lower priority.
the web_request stuff LGTM http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.h (right): http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:32: const std::string& get_key() const { just key() and val() for accessors http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:39: void SetKey(const char* s, size_t n); do you know about StringPiece? That's generally preferred when you need raw_string+size. I think it would also serve for the std::string version too. http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:48: // Creates a correct parser instance based on the |request|. Returns NULL This class could use some vertical whitespace. Put a newline before each of these comment blocks.
On Mon, Jul 30, 2012 at 9:05 AM, <vabr@chromium.org> wrote: > Dear reviewers, thanks for your patience! > > Ryan, > Thanks for your explanation. I can see the fragility in not supporting > chunked > uploads. As a fast "solution" I added error reporting to notify the user > about > forms uploaded in chunks. (See > chrome/browser/extensions/api/**web_request/web_request_api.**cc:171 .) > The other way would be to just support this, but I would like to postpone > that > for two reasons. First, it will be easier to develop and test this once the > forms are actually uploaded chunked by the browser. Second, the API has > not been > used yet, and I would like to see the actual use-cases before deciding > where to > invest more time on this. > If that does not sound acceptable to you, please let me know. > LG/SGTM. Erroring out is a perfectly acceptable form of handling in my book :) > > Matt, > Are you satisfied with how I addressed your comments? > > Wan-Teh, > If you are still willing to review the network part, please feel free to > proceed. I would appreciate if you could take a look at the parsers for > encoded > forms (chrome/browser/extensions/**api/web_request/post_data_**parser.*). > > Dominic, > I added the error-logging for chunked forms and changed the structure of > "details" passed to the listener, based on our conversation. I also > extended the > unit-tests accordingly. Please let me know what you think. > > > Thanks to all! > Vaclav > > > > http://codereview.chromium.**org/10694055/diff/44028/** > chrome/browser/extensions/api/**web_request/web_request_api.cc<http://codereview.chromium.org/10694055/diff/44028/chrome/browser/extensions/api/web_request/web_request_api.cc> > File chrome/browser/extensions/api/**web_request/web_request_api.cc > (right): > > http://codereview.chromium.**org/10694055/diff/44028/** > chrome/browser/extensions/api/**web_request/web_request_api.** > cc#newcode194<http://codereview.chromium.org/10694055/diff/44028/chrome/browser/extensions/api/web_request/web_request_api.cc#newcode194> > chrome/browser/extensions/api/**web_request/web_request_api.**cc:194: > scoped_ptr<DictionaryValue> post_data(new DictionaryValue()); > On 2012/07/23 21:19:01, Matt Perry wrote: > >> The rest of this function seems like it should be a helper method on >> PostDataParser. >> > > Fair point, I tried to move as much as I felt is in the parser's scope. > > > http://codereview.chromium.**org/10694055/diff/44028/** > chrome/common/extensions/api/**web_request.json<http://codereview.chromium.org/10694055/diff/44028/chrome/common/extensions/api/web_request.json> > File chrome/common/extensions/api/**web_request.json (right): > > http://codereview.chromium.**org/10694055/diff/44028/** > chrome/common/extensions/api/**web_request.json#newcode109<http://codereview.chromium.org/10694055/diff/44028/chrome/common/extensions/api/web_request.json#newcode109> > chrome/common/extensions/api/**web_request.json:109: > "experimentalPostData": { > On 2012/07/24 20:46:57, Matt Perry wrote: > >> On 2012/07/24 09:28:53, battre wrote: >> > On 2012/07/23 21:19:01, Matt Perry wrote: >> > > Drop the "experimental" prefix. We're moving away from that >> > convention. > >> > >> > Don't you want to indicate to developers that this feature may >> > change? I am > >> > concerned that we might not get it right in the first try. >> > > The description indicates that. Also, the feature is restricted to the >> > dev > >> channel. I think that's enough, and is the convention we're moving >> > towards. > > I removed the 'experimental' prefix based on the above discussion. > > > http://codereview.chromium.**org/10694055/diff/44028/** > chrome/common/extensions/api/**web_request.json#newcode145<http://codereview.chromium.org/10694055/diff/44028/chrome/common/extensions/api/web_request.json#newcode145> > chrome/common/extensions/api/**web_request.json:145: "enum": ["blocking", > "requestPostData"] > On 2012/07/23 21:19:01, Matt Perry wrote: > >> imo "request" is unnecessary here. There's only 1 kind of "postData", >> > so I think > >> that would be a fine name. >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/44028/** > chrome/test/data/extensions/**api_test/webrequest/test_post.**js<http://codereview.chromium.org/10694055/diff/44028/chrome/test/data/extensions/api_test/webrequest/test_post.js> > File chrome/test/data/extensions/**api_test/webrequest/test_post.**js > (right): > > http://codereview.chromium.**org/10694055/diff/44028/** > chrome/test/data/extensions/**api_test/webrequest/test_post.**js#newcode82<http://codereview.chromium.org/10694055/diff/44028/chrome/test/data/extensions/api_test/webrequest/test_post.js#newcode82> > chrome/test/data/extensions/**api_test/webrequest/test_post.**js:82: > (includePostData) ? { label: "b-onBeforeRequest", > On 2012/07/23 21:19:01, Matt Perry wrote: > >> I think you could make this more succinct like so: >> { >> details: { >> method: "POST", >> ..., >> postData: includePostData ? <postData> : undefined >> } >> } >> > > I tried that but it did not work. Even if the expected details.postData > is undefined, such 'details' still has a property 'postData'. So the > expected details is different from the real details, which does not even > have the property 'postData'. > > Luckily, the test was not quite right -- when no form data can be > parsed, but the request contains POST data, the postData field still has > to be present, only does not contain formData. I corrected this > behaviour in the code, and made this test shorter eventually. > > http://codereview.chromium.**org/10694055/diff/65001/** > chrome/browser/extensions/api/**web_request/web_request_api_**unittest.cc<http://codereview.chromium.org/10694055/diff/65001/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc> > File > chrome/browser/extensions/api/**web_request/web_request_api_**unittest.cc > (right): > > http://codereview.chromium.**org/10694055/diff/65001/** > chrome/browser/extensions/api/**web_request/web_request_api_** > unittest.cc#newcode131<http://codereview.chromium.org/10694055/diff/65001/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc#newcode131> > chrome/browser/extensions/api/**web_request/web_request_api_** > unittest.cc:131: > // TODO(vabr): Add const once ListValue getters get corrected. > I am concurrently taking care of correction the ListValue (already did > for DictionaryValue), but it may take longer than this CL, as it is > lower priority. > > http://codereview.chromium.**org/10694055/<http://codereview.chromium.org/106... >
Matt, should we disable the postData extraInfoSpec from channels other than DEV? http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:129: if (!parser->SetSource(&(element->bytes()))) continue; nit: could you move the continue to the next line? http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:177: post_data->Set(keys::kPostDataErrorKey, error); nit: post_data->SetString(keys::kPostDataErrorKey, "chunked_encoding"); http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:440: else if (str == "postData") I think at this place we need to check whether we are on the dev branch. I think we don't want to enable this to all branches instantly. Do we? Matt? http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:476: const char* bytes, size_t bytes_length) { nit: next line. http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:588: const Value* result; = NULL http://codereview.chromium.org/10694055/diff/64003/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/64003/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:112: "description": "Experimental feature. Container for representations of data sent by the request via POST method. Only provided if extraInfoSpec contains 'postData'.", s/Experimental feature/Experimental feature, only available on DEV branch/? http://codereview.chromium.org/10694055/diff/64003/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:114: "error": {"type": "string", "optional": true, "enum": ["chunked_encoding"], "description": "Errors when obtaining POST data: 'chunked_encoding' means that the data was uploaded chunked, which is not currently supported by WebRequest."}, s/by WebRequest/by the WebRequest API/?
Matt and Dominic, I followed your comments mostly. Two exceptions: * We should clarify whether we guard this feature against deploying in stable/beta. * for Dominic, in chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:476 -- see my comment. Thanks, Vaclav http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:129: if (!parser->SetSource(&(element->bytes()))) continue; On 2012/07/30 17:54:25, battre wrote: > nit: could you move the continue to the next line? Done. http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.h (right): http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:32: const std::string& get_key() const { On 2012/07/30 16:14:15, Matt Perry wrote: > just key() and val() for accessors Done. http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:39: void SetKey(const char* s, size_t n); On 2012/07/30 16:14:15, Matt Perry wrote: > do you know about StringPiece? That's generally preferred when you need > raw_string+size. I think it would also serve for the std::string version too. I changed the char*+size_t to StringPiece because it certainly increases the readability, and does the same thing behind scenes. However, I would like to retain the std::string version, it spares one copying of the string, compared to going through StringPiece, and I don't think the code is obfuscated by this. http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:48: // Creates a correct parser instance based on the |request|. Returns NULL On 2012/07/30 16:14:15, Matt Perry wrote: > This class could use some vertical whitespace. Put a newline before each of > these comment blocks. Done. http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:177: post_data->Set(keys::kPostDataErrorKey, error); On 2012/07/30 17:54:25, battre wrote: > nit: post_data->SetString(keys::kPostDataErrorKey, "chunked_encoding"); Done. http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:476: const char* bytes, size_t bytes_length) { On 2012/07/30 17:54:25, battre wrote: > nit: next line. Sorry, I don't understand. Did you mean to split bytes and bytes_length in two lines? I can do that if you want me to, but I wanted to suggest they belong together (see the style guide, search for "char* str, int str_len"). http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:588: const Value* result; On 2012/07/30 17:54:25, battre wrote: > = NULL Done. http://codereview.chromium.org/10694055/diff/64003/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/64003/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:114: "error": {"type": "string", "optional": true, "enum": ["chunked_encoding"], "description": "Errors when obtaining POST data: 'chunked_encoding' means that the data was uploaded chunked, which is not currently supported by WebRequest."}, On 2012/07/30 17:54:25, battre wrote: > s/by WebRequest/by the WebRequest API/? Done.
http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:476: const char* bytes, size_t bytes_length) { On 2012/07/31 09:03:18, vabr (Chromium) wrote: > On 2012/07/30 17:54:25, battre wrote: > > nit: next line. > > Sorry, I don't understand. > Did you mean to split bytes and bytes_length in two lines? > I can do that if you want me to, but I wanted to suggest they belong together > (see the style guide, search for "char* str, int str_len"). thanks, I learned something new.
http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/64003/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:440: else if (str == "postData") On 2012/07/30 17:54:25, battre wrote: > I think at this place we need to check whether we are on the dev branch. I think > we don't want to enable this to all branches instantly. Do we? Matt? Good catch. I was thinking we could do it through _permission_features.json, but we can't. Let's just make it an error to specify postData on non-dev.
http://codereview.chromium.org/10694055/diff/63027/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/63027/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:41: key_.replace(0, std::string::npos, str.data(), str.size()); can also do: key_ = str.as_string();
Thanks, Matt and Dominic. One comment and one thing to discuss. No code change since my last e-mail yet. To discuss: Jochen is concerned about the check for the current channel being made through chrome::VersionInfo . His main point was performance. It is also bad to include code in the beta/stable binaries which is not used, they are already big enough. Jochen suggested that this feature be reverted on the beta branch. This seems like a good idea to me, at least as an addition to the run-time check. To the performance itself, even after speaking to Ben I have not a clear picture about the costs incurred by one query to VersionInfo. Looking into the code I saw it uses Windows registry, and it does not seem to do any caching of the result. Because the channel does not change during the lifetime of the browser, should we perhaps cache the channel information ourselves? Any suggestions where, whether on the extensions subsystem level, or only WebRequest level, or elsewhere? Thanks, Vaclav http://codereview.chromium.org/10694055/diff/63027/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/63027/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:41: key_.replace(0, std::string::npos, str.data(), str.size()); On 2012/07/31 09:32:14, Matt Perry wrote: > can also do: key_ = str.as_string(); Yes, but then the bytes would be copied twice, first during making a temporary std::string in as_string(), and then in the assignment to key_. The way I do it now it is only copied once. It is no big deal, but unless you don't really hate the key_.replace, I would leave it with just one pass at copying.
Regarding the version check, I'd say cache the version check locally, but still do it at runtime. We have plenty of code that lives in Chrome but is only enabled for the dev branch (see all the dev-only features in _permission_features.json). Reverting the change on the beta branch is error prone and a pain to do. http://codereview.chromium.org/10694055/diff/63027/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/63027/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:41: key_.replace(0, std::string::npos, str.data(), str.size()); On 2012/07/31 12:17:58, vabr (Chromium) wrote: > On 2012/07/31 09:32:14, Matt Perry wrote: > > can also do: key_ = str.as_string(); > > Yes, but then the bytes would be copied twice, first during making a temporary > std::string in as_string(), and then in the assignment to key_. > The way I do it now it is only copied once. > It is no big deal, but unless you don't really hate the key_.replace, I would > leave it with just one pass at copying. Compilers will likely optimize it as a single copy: http://en.wikipedia.org/wiki/Return_value_optimization
Matt, Dominic, It looks we agree on doing the channel check in run-time, with local caching, and without reverting the code on beta. Local caching means no performance concern, and Matt has a fair point about the unused code being common in branches. Codesearch returns plenty of uses of VersionInfo, but non in c/b/extensions (https://cs.corp.google.com/#search/&q=VersionInfo%20CHANNEL%20package:%5Echro...). Is this the first case of disabling an extension outside the dev channel? Where does it make sense to cache the channel? In ExtensionService? http://codereview.chromium.org/10694055/diff/63027/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/63027/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:41: key_.replace(0, std::string::npos, str.data(), str.size()); On 2012/07/31 12:24:28, Matt Perry wrote: > On 2012/07/31 12:17:58, vabr (Chromium) wrote: > > On 2012/07/31 09:32:14, Matt Perry wrote: > > > can also do: key_ = str.as_string(); > > > > Yes, but then the bytes would be copied twice, first during making a temporary > > std::string in as_string(), and then in the assignment to key_. > > The way I do it now it is only copied once. > > It is no big deal, but unless you don't really hate the key_.replace, I would > > leave it with just one pass at copying. > > Compilers will likely optimize it as a single copy: > http://en.wikipedia.org/wiki/Return_value_optimization Thanks for the interesting link about the copy-con optimization! However, this would be assignment, and I don't see the compiler optimizing that: 1. StringPiece only has a char* pointerand length, and needs to create a separate string, this is certainly one copy. 2. The string returned from StringPiece needs to be assigned to key_ (or value_). This means the target string overwrites (and re-allocates, if necessary) its buffer using the source string data, i.e., another copy. I doubt there is an option to squash constructing a new string with assigning to an old one into a single copy operation. At least this is not the cited optimizing of multiple copy-cons (no copy-cons involved).
On 2012/07/31 12:55:27, vabr (Chromium) wrote: > Matt, Dominic, > > It looks we agree on doing the channel check in run-time, with local caching, > and without reverting the code on beta. Local caching means no performance > concern, and Matt has a fair point about the unused code being common in > branches. > > Codesearch returns plenty of uses of VersionInfo, but non in c/b/extensions > (https://cs.corp.google.com/#search/&q=VersionInfo%2520CHANNEL%2520package:%25...). > Is this the first case of disabling an extension outside the dev channel? > > Where does it make sense to cache the channel? In ExtensionService? I would just do something simple like: static bool IsPostDataEnabled() { static bool result = VersionInfo::Get() <= CHANNEL_DEV; return result; } > > http://codereview.chromium.org/10694055/diff/63027/chrome/browser/extensions/... > File chrome/browser/extensions/api/web_request/post_data_parser.cc (right): > > http://codereview.chromium.org/10694055/diff/63027/chrome/browser/extensions/... > chrome/browser/extensions/api/web_request/post_data_parser.cc:41: > key_.replace(0, std::string::npos, str.data(), str.size()); > On 2012/07/31 12:24:28, Matt Perry wrote: > > On 2012/07/31 12:17:58, vabr (Chromium) wrote: > > > On 2012/07/31 09:32:14, Matt Perry wrote: > > > > can also do: key_ = str.as_string(); > > > > > > Yes, but then the bytes would be copied twice, first during making a > temporary > > > std::string in as_string(), and then in the assignment to key_. > > > The way I do it now it is only copied once. > > > It is no big deal, but unless you don't really hate the key_.replace, I > would > > > leave it with just one pass at copying. > > > > Compilers will likely optimize it as a single copy: > > http://en.wikipedia.org/wiki/Return_value_optimization > > Thanks for the interesting link about the copy-con optimization! > > However, this would be assignment, and I don't see the compiler optimizing that: > 1. StringPiece only has a char* pointerand length, and needs to create a > separate string, this is certainly one copy. > 2. The string returned from StringPiece needs to be assigned to key_ (or > value_). This means the target string overwrites (and re-allocates, if > necessary) its buffer using the source string data, i.e., another copy. > > I doubt there is an option to squash constructing a new string with assigning to > an old one into a single copy operation. At least this is not the cited > optimizing of multiple copy-cons (no copy-cons involved).
Matt, Dominic, Now the channel is checked. I also modified the unit_test so that extraInfoSpec is generated from strings, allowing the channel info to impact that as well. Vaclav http://codereview.chromium.org/10694055/diff/60013/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/60013/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:611: const bool anything_expected = IsPostDataEnabled() && Matt, currently I test that postData is ignored on the current channel. I am not actually sure whether the try-bots generate a channel info (my build is CHANNEL_UNKNOWN). I could make the static variables from IsPostDataEnabled global, and inject different channel values in them here, to test that beta & stable are ignored. This would be very ugly, exposing static variables to anyone. If I would wrap the channel cache into an object, it is still frowned upon by the style guide, even though Singleton could be used. Dominic, what do you think about this?
On 2012/07/31 16:15:41, vabr (Chromium) wrote: > Matt, Dominic, > > Now the channel is checked. I also modified the unit_test so that extraInfoSpec > is generated from strings, allowing the channel info to impact that as well. > > Vaclav > > http://codereview.chromium.org/10694055/diff/60013/chrome/browser/extensions/... > File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc > (right): > > http://codereview.chromium.org/10694055/diff/60013/chrome/browser/extensions/... > chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:611: const > bool anything_expected = IsPostDataEnabled() && > Matt, currently I test that postData is ignored on the current channel. I am not > actually sure whether the try-bots generate a channel info (my build is > CHANNEL_UNKNOWN). I could make the static variables from IsPostDataEnabled > global, and inject different channel values in them here, to test that beta & > stable are ignored. This would be very ugly, exposing static variables to > anyone. If I would wrap the channel cache into an object, it is still frowned > upon by the style guide, even though Singleton could be used. > > Dominic, what do you think about this? Thanks to Dominic who made me aware of a stupid mistake in patch set 29. Corrected in the current set (30).
http://codereview.chromium.org/10694055/diff/60013/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/60013/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:611: const bool anything_expected = IsPostDataEnabled() && On 2012/07/31 16:15:42, vabr (Chromium) wrote: > Matt, currently I test that postData is ignored on the current channel. I am not > actually sure whether the try-bots generate a channel info (my build is > CHANNEL_UNKNOWN). I could make the static variables from IsPostDataEnabled > global, and inject different channel values in them here, to test that beta & > stable are ignored. This would be very ugly, exposing static variables to > anyone. If I would wrap the channel cache into an object, it is still frowned > upon by the style guide, even though Singleton could be used. > > Dominic, what do you think about this? It's not that bad. Other code in Chrome has testing variable exposed through static interfaces. To clarify, I mean something like: web_request_api.h: static void SetChannelForTesting(Channel c); web_request_api.cc: static VersionInfo::Channel g_channel_; static bool g_channel_initialized_ = false; static VersionInfo::Channel GetChannel() { if (!g_channel_initialized_) { g_channel_ = VersionInfo::GetChannel(); g_channel_initialized_ = true; } return g_channel_; } static void SetChannelForTesting(Channel c) { g_channel_initialized_ = true; g_channel_ = c; http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:1867: chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel(); channel is a POD (it's just an enum), so you can make it a static variable. Doing bool IsEnabled() { static chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel(); return channel <= Dev; } works fine, because GetChannel() will only be invoked the first time this method is called. http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.h (right): http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.h:451: bool IsPostDataEnabled(); Use a more descriptive name for a global function like this. (As in, put WebRequest somewhere in there, to avoid name collisions). http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:99: delete value; Prefer using scoped_ptr<Value> for stuff like this. Then the ownership semantics are conveyed in the code, and you don't need to add comments. i.e. return value.release(); (means ownership is passed) return NULL; (means value will be deleted) Foo(value.get()) (means ownership stays with us, not Foo) http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:108: CHECK(message->type() == ExtensionMsg_MessageInvoke::ID); Don't use CHECK in tests. If it fails, you bring down the whole test harness. Use ASSERT_TRUE, EXPECT_TRUE, or maybe if (!foo) ADD_FAILURE. http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:118: DLOG(INFO) << "Failed to parse JSON in the message arguments."; Do these messages represent test failures? If so, make them failures with ADD_FAILURE. If not, don't spam the logs. http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:126: scoped_ptr<const ListValue> list_scoped(list); // To ensure clean-up. comment is unnecessary http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:131: // TODO(vabr): Add const once ListValue getters get corrected. Realistically, this will never happen :). Just remove this TODO.
Review comments on patch set 30:
vabr: sorry about the late review. I didn't have time to
review this on Monday, and I took Tuesday off.
I reviewed everything execept post_data_parser.{h,cc} and
the test-related files. Please let me know if you would
like me to review more.
High-Level Comments
1. postData is the request body of a POST request. Another
HTTP method that has a request body is the PUT method.
If you want to support the PUT method, we should rename
postData to something more generic, such as requestData
or requestBody.
2. postData is only supported if the POST data is a list of
key-value pairs (i.e., form data). The raw POST data is
not available. This limitation was not obvious to me
at first. This is not necessarily a problem.
3. If formData is the only kind of POST data supported,
then it seems that another possible error code is
"not_form".
http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):
http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.cc:166: if
(request->method() != "POST")
If you want to support the PUT method, add
&& request->method() != "PUT"
here.
http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.cc:441:
*extra_info_spec |= (IsPostDataEnabled()) ? POST_DATA : 0;
Nit: you can also write this as
else if (str == "postData" && IsPostDataEnabled())
*extra_info_spec |= POST_DATA;
http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.cc:1867:
chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel();
mpcomplete's suggestion is correct. Note that neither form
is thread-safe, so you need to document that IsPostDataEnabled()
can only be called on a single thread, unless the single-thread
requirement is obvious to people using this function.
http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/web_request_api.h (right):
http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.h:446:
The URL "crbug.com/10694055" is wrong. 10694055 is a code
review, not a bug report.
http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.h:448: // and canary
channels. This function caches the channel info from
Nit: channel => release channel
Should we document that this function returns true for the
Canary and Dev channels only? (I don't know what
CHANNEL_UNKNOWN means.)
http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/web_request_api_constants.h
(right):
http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api_constants.h:32: extern
const char kFormDataKey[];
Nit: does it make sense to name this constant
kPostDataFormDataKey? It'll indicate it is subordinate
to kPostDataKey, but the name is a little long :-)
http://codereview.chromium.org/10694055/diff/62031/chrome/common/extensions/d...
File chrome/common/extensions/docs/extensions/webRequest.html (right):
http://codereview.chromium.org/10694055/diff/62031/chrome/common/extensions/d...
chrome/common/extensions/docs/extensions/webRequest.html:1: <!DOCTYPE html><!--
This page is a placeholder for generated extensions api doc. Note:
If you make the changes I suggested to this file, please make
the same changes to chrome/common/extensions/api/web_request.json
http://codereview.chromium.org/10694055/diff/62031/chrome/common/extensions/d...
chrome/common/extensions/docs/extensions/webRequest.html:1808: <dd>Errors when
obtaining POST data: 'chunked_encoding' means that the data was uploaded
chunked, which is not currently supported by WebRequest.</dd>
Since 'postData' is made available to onBeforeRequest
listeners, the use of past tense "the data was uploaded chunked"
here sounds strange because the data hasn't been uploaded
yet.
Same issue with line 1832 below "the form upload was chunked".
http://codereview.chromium.org/10694055/diff/62031/chrome/common/extensions/d...
chrome/common/extensions/docs/extensions/webRequest.html:1832: <dd>If the POST
data is a sequence of key-value pairs, either in multipart/form-data, or
application/x-www-form-urlencoded, this dictionary is present and for each key
contains the list of all values for that key. If the data is in another
encoding, or if it was malformed, the dictionary is not present. It is also not
present if the form upload was chunked. Example value of this dictionary is
{'key': ['value1', 'value2']}.</dd>
Nit: encoding => media type
(I assume you meant "media type" here.)
because "encoding" can also refer to character encoding,
content encoding, or transfer encoding.
wtc@, many thanks for your comments, both the in-line and the high level ones! Now I have to go home, but I will carefully go through them tomorrow. As for the parsers, it is a bit unfortunate that I had not been more clear -- those were the main part I hoped to get reviewed by you. I believe nobody except me has checked yet that they parse correctly the two supported form transfer encodings. If you found some time to have at least a quick look, that would be great. But I fully understand if you did not, and I would ask somebody else to do the parsers in that case. Matt, Also thanks for your further comments. Please have a look at how I addressed them. Dominic, You may want to check web_request_api_unittest.cc around line 100. Thanks to all, Vaclav http://codereview.chromium.org/10694055/diff/64003/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/64003/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:112: "description": "Experimental feature. Container for representations of data sent by the request via POST method. Only provided if extraInfoSpec contains 'postData'.", On 2012/07/30 17:54:25, battre wrote: > s/Experimental feature/Experimental feature, only available on DEV branch/? Done. http://codereview.chromium.org/10694055/diff/60013/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/60013/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:611: const bool anything_expected = IsPostDataEnabled() && On 2012/08/01 09:46:31, Matt Perry wrote: > On 2012/07/31 16:15:42, vabr (Chromium) wrote: > > Matt, currently I test that postData is ignored on the current channel. I am > not > > actually sure whether the try-bots generate a channel info (my build is > > CHANNEL_UNKNOWN). I could make the static variables from IsPostDataEnabled > > global, and inject different channel values in them here, to test that beta & > > stable are ignored. This would be very ugly, exposing static variables to > > anyone. If I would wrap the channel cache into an object, it is still frowned > > upon by the style guide, even though Singleton could be used. > > > > Dominic, what do you think about this? > > It's not that bad. Other code in Chrome has testing variable exposed through > static interfaces. To clarify, I mean something like: > > web_request_api.h: > static void SetChannelForTesting(Channel c); > > web_request_api.cc: > static VersionInfo::Channel g_channel_; > static bool g_channel_initialized_ = false; > > static VersionInfo::Channel GetChannel() { > if (!g_channel_initialized_) { > g_channel_ = VersionInfo::GetChannel(); > g_channel_initialized_ = true; > } > return g_channel_; > } > > static void SetChannelForTesting(Channel c) { > g_channel_initialized_ = true; > g_channel_ = c; I added the channel switcher, and used it in the test. http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:1867: chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel(); On 2012/08/01 09:46:31, Matt Perry wrote: > channel is a POD (it's just an enum), so you can make it a static variable. > Doing > > bool IsEnabled() { > static chrome::VersionInfo::Channel channel = > chrome::VersionInfo::GetChannel(); > return channel <= Dev; > } > > works fine, because GetChannel() will only be invoked the first time this method > is called. Thanks, I learned something new! :) However, due to adding the channel switcher, those static variables had to go out of the function, where non-constant initialisation is forbidden by the style guide even by POD, so this is no more relevant. http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.h (right): http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.h:451: bool IsPostDataEnabled(); On 2012/08/01 09:46:31, Matt Perry wrote: > Use a more descriptive name for a global function like this. (As in, put > WebRequest somewhere in there, to avoid name collisions). Done. http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:99: delete value; On 2012/08/01 09:46:31, Matt Perry wrote: > Prefer using scoped_ptr<Value> for stuff like this. Then the ownership semantics > are conveyed in the code, and you don't need to add comments. i.e. > return value.release(); (means ownership is passed) > return NULL; (means value will be deleted) > Foo(value.get()) (means ownership stays with us, not Foo) Done. http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:108: CHECK(message->type() == ExtensionMsg_MessageInvoke::ID); On 2012/08/01 09:46:31, Matt Perry wrote: > Don't use CHECK in tests. If it fails, you bring down the whole test harness. > > Use ASSERT_TRUE, EXPECT_TRUE, or maybe if (!foo) ADD_FAILURE. Needed to use EXPECT_*, because ASSERT_* does not work in non-void functions, and the failures are fatal. Dominic, earlier we discussed the error reporting here (right after patch set 10). Are you happy with the current changes? http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:118: DLOG(INFO) << "Failed to parse JSON in the message arguments."; On 2012/08/01 09:46:31, Matt Perry wrote: > Do these messages represent test failures? If so, make them failures with > ADD_FAILURE. If not, don't spam the logs. Unified with the redone CHECKs above. http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:126: scoped_ptr<const ListValue> list_scoped(list); // To ensure clean-up. On 2012/08/01 09:46:31, Matt Perry wrote: > comment is unnecessary Done. http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:131: // TODO(vabr): Add const once ListValue getters get corrected. On 2012/08/01 09:46:31, Matt Perry wrote: > Realistically, this will never happen :). Just remove this TODO. Oh it will happen, that CL (10837044) is done, review pending. I already did the same for DictionaryValue last week ;-).
In this round: Matt, please take a look at my responses. I also responded to most of wtc's comments. Things to do for me yet: * discuss the PUT support with Dominic & Matt, and respond to the general remarks from wtc * make sure that IsWebRequestPostDataEnabled is used only on one thread and add the non-thread safeness in a comment * make the windows compiler compile web_request_api_unittest.cc:96 Vaclav http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:441: *extra_info_spec |= (IsPostDataEnabled()) ? POST_DATA : 0; On 2012/08/01 17:24:41, wtc wrote: > > > Nit: you can also write this as > > else if (str == "postData" && IsPostDataEnabled()) > *extra_info_spec |= POST_DATA; Actually, that would be different. The current code means that "postData" is silently ignored in beta/stable, because InitFromValue returns true. If I did it as suggested, asking for postData in beta/stable would cause InitFromValue to return false, and result in an error during registering the listener. Dominic prefers the silent ignoring, so that extensions experimenting with postData on dev are not broken on beta/stable (of course they will still not be able to use POST data, but at least their listeners will get registered for doing other stuff). Matt, do you have an opinion on this? http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.h (right): http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.h:446: On 2012/08/01 17:24:41, wtc wrote: > > The URL "crbug.com/10694055" is wrong. 10694055 is a code > review, not a bug report. Changed the number to the bug number. Thanks! http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.h:448: // and canary channels. This function caches the channel info from On 2012/08/01 17:24:41, wtc wrote: > > Nit: channel => release channel > > Should we document that this function returns true for the > Canary and Dev channels only? (I don't know what > CHANNEL_UNKNOWN means.) Added "release". As for documenting that it returns true for dev/canary, I changed "available" to "enabled" in the comment, so that it agrees with the name of the function. Is it now clearer? http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_constants.h (right): http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_constants.h:32: extern const char kFormDataKey[]; On 2012/08/01 17:24:41, wtc wrote: > > Nit: does it make sense to name this constant > kPostDataFormDataKey? It'll indicate it is subordinate > to kPostDataKey, but the name is a little long :-) Indeed, they should share the prefix. I removed the redundant "data" in form data to make it shorter. http://codereview.chromium.org/10694055/diff/62031/chrome/common/extensions/d... File chrome/common/extensions/docs/extensions/webRequest.html (right): http://codereview.chromium.org/10694055/diff/62031/chrome/common/extensions/d... chrome/common/extensions/docs/extensions/webRequest.html:1808: <dd>Errors when obtaining POST data: 'chunked_encoding' means that the data was uploaded chunked, which is not currently supported by WebRequest.</dd> On 2012/08/01 17:24:41, wtc wrote: > > Since 'postData' is made available to onBeforeRequest > listeners, the use of past tense "the data was uploaded chunked" > here sounds strange because the data hasn't been uploaded > yet. > > Same issue with line 1832 below "the form upload was chunked". Done. http://codereview.chromium.org/10694055/diff/62031/chrome/common/extensions/d... chrome/common/extensions/docs/extensions/webRequest.html:1832: <dd>If the POST data is a sequence of key-value pairs, either in multipart/form-data, or application/x-www-form-urlencoded, this dictionary is present and for each key contains the list of all values for that key. If the data is in another encoding, or if it was malformed, the dictionary is not present. It is also not present if the form upload was chunked. Example value of this dictionary is {'key': ['value1', 'value2']}.</dd> On 2012/08/01 17:24:41, wtc wrote: > > Nit: encoding => media type > > (I assume you meant "media type" here.) > > because "encoding" can also refer to character encoding, > content encoding, or transfer encoding. Done.
wtc brings up a good point on unstructured POST data. Do we want to support POST data that is not a key/value pair? And PUT data as well? I would say yes. http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:441: *extra_info_spec |= (IsPostDataEnabled()) ? POST_DATA : 0; On 2012/08/02 09:15:15, vabr (Chromium) wrote: > On 2012/08/01 17:24:41, wtc wrote: > > > > > > Nit: you can also write this as > > > > else if (str == "postData" && IsPostDataEnabled()) > > *extra_info_spec |= POST_DATA; > > Actually, that would be different. The current code means that "postData" is > silently ignored in beta/stable, because InitFromValue returns true. If I did it > as suggested, asking for postData in beta/stable would cause InitFromValue to > return false, and result in an error during registering the listener. > > Dominic prefers the silent ignoring, so that extensions experimenting with > postData on dev are not broken on beta/stable (of course they will still not be > able to use POST data, but at least their listeners will get registered for > doing other stuff). > > Matt, do you have an opinion on this? I'm torn, so let's just go with what you have. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:89: g_channel_ == VersionInfo::CHANNEL_DEV; note: the channels go in increasing order of stability so that you can do things like (channel_ <= DEV). http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:97: // Find out the correct channel and flip |g_post_data_enabled_| accordingly. This comment is fairly obvious from the code. Take a look at the "Dont's" section of http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Implementation... . http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:100: g_post_data_enabled_ = PostDataEnabledOnChannel(); no need to cache this imo http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.h (right): http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.h:450: void SetChannelForTestingWebRequest(chrome::VersionInfo::Channel c); nit: I'd put the WebRequest tag as a prefix, not a suffix http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:109: return scoped_ptr<const DictionaryValue>(); Sadly this makes the code a bit hard to read. What do you think of this idea: void GetPartHelper(scoped_ptr<DictionaryValue>* out) { scoped_ptr<DictionaryValue> retval; // use ASSERTs here... (*out)->swap(retval); } scoped_ptr<const DictionaryValue> GetPart() { scoped_ptr<const DictionaryValue> retval; GetPartHelper(args, &retval); return retval; }
http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/62031/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:441: *extra_info_spec |= (IsPostDataEnabled()) ? POST_DATA : 0; On 2012/08/02 09:15:15, vabr (Chromium) wrote: > > Actually, that would be different. The current code means that "postData" is > silently ignored in beta/stable, because InitFromValue returns true. You're right. My suggested change was wrong. Please ignore it.
Review comments on patch set 35:
I reviewed post_data_parser.{h,cc}, but I ran out of steam
when I got to PostDataParserMultipart, so I didn't fully
review that class.
High-level comments:
1. I am not familiar with the two ways to encode form data.
So I am not the best reviewer for post_data_parser.{h,cc}.
I don't know who is the best reviewer for form data parser.
I guess form data are encoded by WebKit? So perhaps one of
the WebKit team members...
2. You don't seem familiar with certain recommendations of
the Style Guide, so I wrote many comments about style nits.
Sorry about that. Most of those comments are marked with "Nit:",
so you can skip them to look at the more important comments
first.
3. I find your automaton for PostDataParserMultipart hard
to understand, even though you wrote good comments to
specify the state transitions. This could be a sign that
the comments could be improved further, or that I am just
too tired to review nontrivial code now :-) I will review
PostDataParserMultipart again when you upload a new patch
set.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/post_data_parser.cc (right):
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:26: }
Nit: add a //namespace comment, like this:
} // namespace
Nit: add a blank line after
namespace {
and before
} // namespace
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:45:
val_.replace(0, std::string::npos, str.data(), str.size());
I think you can use base::CopyToString (declared in
"base/string_piece.h") to implement these two methods.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:54: }
I think these four methods should be named set_key()
and set_value(), to match the naming convention of the
getter methods key() and value().
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:69: const
std::string* content_type_header) {
Nit: this argument is named |content_type| in the header file.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:81: } else if
(content_type == "multipart/form-data") {
Should we do case-insensitive string comparison in this
function?
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:82: const char
kBoundaryString[] = "boundary=";
Nit: add 'static'
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:90: offset,
content_type_header->find(';', offset));
I think we should also return scoped_ptr<PostDataParser>()
if boundary.empty() is true.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:114: if
(parser.get() == NULL) {
Nit: just do
if (!parser) {
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:126: data_valid =
false;
If we simply return scoped_ptr<base::DictionaryValue>() here,
we don't need the data_valid variable.
We ignore all the POST data in a file. Is that intentional?
Is it because form data is unlikely to be in a file?
Why do you consider blobs as invalid data?
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:165: offset_ =
seek;
BUG: if you set offset_ to seek here, it'll cause
AllDataReadOK() to return true, which means the data
was well formed, but the data is actually malformed.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:168: std::string
encoded_key(&(*offset_), seek - offset_); // Safe, see (*).
Is it necessary to do &(*offset_) ? I guess offset_ isn't
of the right type?
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:184: return false;
This allows SetSource() to be called only once. Why?
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:203: {}
Nit: the opening curly brace '{' probably should be on the
previous line.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:254: next_line_ =
0;
Should we also set line_start_ and line_end_ to 0?
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:290: state_ =
kError;
Add a break statement.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:303: return
kEndBoundary;
Nit: I suggest making "final boundary" and "end boundary"
consistent.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:326: next_line_ =
seek + 2;
If the data does not end with a final "\r\n", this will
move next_line_ beyond length_ more than necessary. It
won't break anything, just not necessary.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/post_data_parser.h (right):
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:26: class
PostDataParser {
This class probably should be named FormDataParser.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:32: const
std::string& key() const {
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()?
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:35: const
std::string& val() const {
Nit: it seems better to not abbreviate the getter and
the class member. So name them value() and value_.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:51: static
scoped_ptr<PostDataParser> CreatePostDataParser(
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?
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:55: //
"Content-Type" URLRequest headers. If |content_type| is NULL, it defaults
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
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:62: static
scoped_ptr<base::DictionaryValue> ParseURLRequestData(
This method should be named ParseURLRequestPostData or
ParsePostData.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:65: virtual
~PostDataParser();
Our Style Guide recommends that the destructor be declared
before methods, including static methods.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:73: virtual bool
GetNextPair(Result* result) = 0;
Nit: Pair => KeyValuePair
otherwise it may not be clear what "Pair" refers to.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:75: // Sets the
|source| of the data to be parsed. The ownership is let with the
Nit: "is let with the caller" sounds a little strange.
Did you mean "is left with the caller"?
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:77: // is called
again, whatever comes sooner. Returns true on success.
Nit: whatever => whichever
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:78: virtual bool
SetSource(const std::vector<char>* source) = 0;
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
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:110: //
Implementation of PostDataParser.
Nit: add a blank line before this line.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:117: // This parser
reads the source line by line. There are four types of lines:
This comment says "four types of lines", but there are five
types of lines defined below.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:123: enum LineType
{kBoundary, kEndBoundary, kDisposition, kEmpty, kOther};
Nit: it may be a good idea to add "Line" to all of these
enumerators. Similarly, add "State" to all of the State
enumerators.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:133: enum State
{kInit, kHeadStart, kHead, kHeadRead, kBody, kFinal, kError};
Throughout this class, change "Head" to "Header". (I assume
by "Head" you mean "Header".)
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:136: // This calls
SeekNextLine() and returns it return value.
it => its
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:145: // Extracts
"key" and possibly "val" from a DISP line. Returns success.
Nit: Returns success => Returns true on success ?
What does "DISP line" mean?
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.cc:84: static bool
g_post_data_enabled_ = false;
Nit: these three variables (and also the existing
kWebRequestEvents constant above) are inside an anonymous
namespace, so they don't need to be declared 'static'.
Nit: these three variables are not class members, so they
should not have the trailing underscore '_'.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.cc:86: bool
PostDataEnabledOnChannel() {
Nit: This function operates on the global variable g_channel_.
This is not obvious. I recommend having this function
take the channel as input.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.cc:100:
g_post_data_enabled_ = PostDataEnabledOnChannel();
I agree that the result of PostDataEnabledOnChannel() is
not worth caching.
Is VersionInfo::GetChannel() expensive? If not, we don't
need to cache the result of VersionInfo::GetChannel() either.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.cc:306: void
SetChannelForTestingWebRequest(VersionInfo::Channel channel) {
You name the argument |c| in the header file but name it
|channel| here. The comment on line 305 uses the name |c|.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/web_request_api.h (right):
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.h:452: // Only to be
used in tests. Resets the overriden channel info.
Typo: overriden => overridden
vabr: I searched for files with "form_data" or "FormData" in their names in the Chromium source tree. Indeed form data is handled by WebKit. The best WebKit developer on the Chromium team for form data seems to be jianli. We can ask him to review the form data parsers (especially the multipart/form-data parser) if necessary.
Dominic, Matt and Wan-Teh, Thanks to all of you for careful reviews. I don't think I will manage to finish all work related to you comments today, but I will notify you as soon as I do, probably on the beginning of the coming week. On 2012/08/03 01:17:01, wtc wrote: > (...) The best WebKit developer on the Chromium team for form > data seems to be jianli. (...) Indeed, asking jianli seems like a good idea to me. I will ask him once I finish the current set of improvements suggested by you and others. Regarding the description of the parser, I personally have to draw it each time. Because the image is hard to put into comments, I will put http://www.corp.google.com/~vabr/no_crawl/automaton.png into the next message from me. :) Thanks, Vaclav
Dominic, Matt and Wan-Teh, Summary here, personal notes below: A major change in this patch-set is a different structure of the data passed to onBeforeRequest listeners: All is under a dictionary called "body", which always contains one item. If the request contained a parseable form sent via POST, then it is in "body.parsedForm". Else if the request was POST/PUT with non-parseable data, then the data is in "body.raw" (a string in Base64 encoding). If there was an error, then the only item in "body" is "error", an explanation string. Wan-Teh, 1. I tried to address your comments. Some of them I hesitated to address, and in those cases I gave a detailed explanation, so that you can help me understand why a suggested change would be beneficial. The drawing to better understand the multipart parser is here: http://www.corp.google.com/~vabr/no_crawl/automaton.png 2. Thanks for looking up jianli@ for me. As the parsers are isolated from the rest, I'm thinking of asking him for review once the current reviewers are satisfied. Answering to too many reviewers can be a bit overwhelming. Feel free to not continue with reviewing the parsers, as I will ask jianli@; but of course if you have any comments, I will be very grateful to hear them. Dominic, 1. I know you suggested I postpone the PUT and raw-data support to another CL. But most of the changes involved had to be done in this one as part of refactoring, so I felt I might as well put the complete thing in. If you disagree and want me to split it, please let me know. Matt and Dominic, 1. The new structure of "details" ("body.*") may need re-thinking and re-naming. Please let me know your suggestions. For example, should it be "body", or "requestBody", to match "requestHeaders"? I picked up "body" instead of "postData" as it does not suggest a particular method (POST/PUT). Renaming should also be considered for the extraInfoSpec's "body" and ExtraInfoSpec::BODY. 2. The new files post_data_parser.* need to be renamed, as they currently contain two families of classes, neither of which is called PostDataParser. It is also a question whether the two families (FormDataParser* and *Producer) should be together. As I write this, I realize that unit-tests for *Producer are missing. I will add them later in this CL, unless you talk me out of introducing those classes at all. Vaclav http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:26: } On 2012/08/03 00:57:19, wtc wrote: > > Nit: add a //namespace comment, like this: > > } // namespace > > Nit: add a blank line after > namespace { > and before > } // namespace Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:45: val_.replace(0, std::string::npos, str.data(), str.size()); On 2012/08/03 00:57:19, wtc wrote: > > I think you can use base::CopyToString (declared in > "base/string_piece.h") to implement these two methods. Thanks, that's much more readable! http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:54: } On 2012/08/03 00:57:19, wtc wrote: > > I think these four methods should be named set_key() > and set_value(), to match the naming convention of the > getter methods key() and value(). Done. (For the record, if somebody asks me later: the Google style guide prescribes unix_hacker style for fast inlined functions OR accessors/mutators. In particular, the accessors/mutators should be unix_hacker style even if not inlined.) http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:69: const std::string* content_type_header) { On 2012/08/03 00:57:19, wtc wrote: > > Nit: this argument is named |content_type| in the header file. Done (corrected the header). http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:81: } else if (content_type == "multipart/form-data") { On 2012/08/03 00:57:19, wtc wrote: > > Should we do case-insensitive string comparison in this > function? That's a good point. RFC 2388 does not specify case sensitivity of multipart/form-data (it uses lower-case, but initially mentions Multipart/Form-Data). RFC 2046 about MIME media types also does not specify case sensitivity. I believe it is safer to do case-insensitive comparison, as you suggest. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:82: const char kBoundaryString[] = "boundary="; On 2012/08/03 00:57:19, wtc wrote: > > Nit: add 'static' Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:90: offset, content_type_header->find(';', offset)); On 2012/08/03 00:57:19, wtc wrote: > > I think we should also return scoped_ptr<PostDataParser>() > if boundary.empty() is true. Indeed, thanks for catching this. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:114: if (parser.get() == NULL) { On 2012/08/03 00:57:19, wtc wrote: > > Nit: just do > if (!parser) { Function deleted in the meantime. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:126: data_valid = false; On 2012/08/03 00:57:19, wtc wrote: > > If we simply return scoped_ptr<base::DictionaryValue>() here, > we don't need the data_valid variable. Function, and |data_valid| deleted in the meantime. > > We ignore all the POST data in a file. Is that intentional? > Is it because form data is unlikely to be in a file? Yes, this is intentional. For files we only pass their name, to avoid copying large amounts of data. > > Why do you consider blobs as invalid data? This is due to comment in upload_data.h, above the definition of SetToBlobUrl: "UploadData should not contain any blob reference". http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:165: offset_ = seek; On 2012/08/03 00:57:19, wtc wrote: > > BUG: if you set offset_ to seek here, it'll cause > AllDataReadOK() to return true, which means the data > was well formed, but the data is actually malformed. Indeed, very nicely spotted, thanks! However, this class has been rewritten slightly in the meantime, to also include check for the number of occurrences of '=' and '&'. In this case there would be one '&' too many, and thus AllDataReadOK() would return false. Also, for clarity I added Abort(), which I needed to introduce in GetNextChar anyway. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:168: std::string encoded_key(&(*offset_), seek - offset_); // Safe, see (*). On 2012/08/03 00:57:19, wtc wrote: > > Is it necessary to do &(*offset_) ? I guess offset_ isn't > of the right type? I'm afraid it is necessary. offset_ is a iterator, *offset_ is a char, and &(*offset_) is a pointer to char. There seems to be no implicit conversion from the iterator to char*. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:184: return false; On 2012/08/03 00:57:19, wtc wrote: > > This allows SetSource() to be called only once. Why? I don't think Chrome/WebKit splits URLEncoded forms into more blocks, and it is not clear how would it split them if it did. For example, which of the following splits would be allowed? "name1=value1", "&name2=value2" "name1=value1&", "name2=value2" "name1=value1&nam", "e2=value2" Writing the parser to accept any of the above examples would mean an increased complexity of it. Taking care of the first two cases feels a bit random. So I decided to assume one block only, and fix it when it proves to be a problem. Please let me know if you disagree with my decision on this. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:203: {} On 2012/08/03 00:57:19, wtc wrote: > > Nit: the opening curly brace '{' probably should be on the > previous line. Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:254: next_line_ = 0; On 2012/08/03 00:57:19, wtc wrote: > > Should we also set line_start_ and line_end_ to 0? We do not need to, they will be set accordingly as soon as the first line is requested via SeekNextLine. My preference here was not to set them to 0 here, because it would indicate to the reader that the first line is being considered to be of length 0. I had to step aside and set them to 0 in the constructor, as non-class members should always be initialized. There I marked those values explicitly as dummy. I can do the same here, but in my opinion adding unnecessary lines does not help the readability. Please let me know if you still want me to reset them here. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:290: state_ = kError; On 2012/08/03 00:57:19, wtc wrote: > > Add a break statement. Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:303: return kEndBoundary; On 2012/08/03 00:57:19, wtc wrote: > > Nit: I suggest making "final boundary" and "end boundary" > consistent. Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:326: next_line_ = seek + 2; On 2012/08/03 00:57:19, wtc wrote: > > If the data does not end with a final "\r\n", this will > move next_line_ beyond length_ more than necessary. It > won't break anything, just not necessary. Due to adding a check for garbage at the end, this has been taken care of as well. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.h (right): http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:26: class PostDataParser { On 2012/08/03 00:57:19, wtc wrote: > > This class probably should be named FormDataParser. 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. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:32: const std::string& key() const { On 2012/08/03 00:57:19, wtc wrote: > > 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()? Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:35: const std::string& val() const { On 2012/08/03 00:57:19, wtc wrote: > > Nit: it seems better to not abbreviate the getter and > the class member. So name them value() and value_. Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:51: static scoped_ptr<PostDataParser> CreatePostDataParser( On 2012/08/03 00:57:19, wtc wrote: > > 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? 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. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:55: // "Content-Type" URLRequest headers. If |content_type| is NULL, it defaults On 2012/08/03 00:57:19, wtc wrote: > > 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 > Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:62: static scoped_ptr<base::DictionaryValue> ParseURLRequestData( On 2012/08/03 00:57:19, wtc wrote: > > This method should be named ParseURLRequestPostData or > ParsePostData. Method removed completely in the meantime. Its job is now done by the ParsedDataProducer class. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:65: virtual ~PostDataParser(); On 2012/08/03 00:57:19, wtc wrote: > > Our Style Guide recommends that the destructor be declared > before methods, including static methods. Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:73: virtual bool GetNextPair(Result* result) = 0; On 2012/08/03 00:57:19, wtc wrote: > > Nit: Pair => KeyValuePair > > otherwise it may not be clear what "Pair" refers to. Done as Pair => NameValue, because key is now name, and also leaving the pair there seems redundant and too long. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:75: // Sets the |source| of the data to be parsed. The ownership is let with the On 2012/08/03 00:57:19, wtc wrote: > > Nit: "is let with the caller" sounds a little strange. > Did you mean "is left with the caller"? Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:77: // is called again, whatever comes sooner. Returns true on success. On 2012/08/03 00:57:19, wtc wrote: > > Nit: whatever => whichever Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:78: virtual bool SetSource(const std::vector<char>* source) = 0; On 2012/08/03 00:57:19, wtc wrote: > > Can we use a const reference here? > const std::vector<char>& source 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? > > It seems that we can also represent the source as > const char* source, size_t length 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? http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:110: // Implementation of PostDataParser. On 2012/08/03 00:57:19, wtc wrote: > > Nit: add a blank line before this line. Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:117: // This parser reads the source line by line. There are four types of lines: On 2012/08/03 00:57:19, wtc wrote: > > This comment says "four types of lines", but there are five > types of lines defined below. Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:123: enum LineType {kBoundary, kEndBoundary, kDisposition, kEmpty, kOther}; On 2012/08/03 00:57:19, wtc wrote: > > Nit: it may be a good idea to add "Line" to all of these > enumerators. Similarly, add "State" to all of the State > enumerators. 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. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:133: enum State {kInit, kHeadStart, kHead, kHeadRead, kBody, kFinal, kError}; On 2012/08/03 00:57:19, wtc wrote: > > Throughout this class, change "Head" to "Header". (I assume > by "Head" you mean "Header".) Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:136: // This calls SeekNextLine() and returns it return value. On 2012/08/03 00:57:19, wtc wrote: > > it => its Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:145: // Extracts "key" and possibly "val" from a DISP line. Returns success. On 2012/08/03 00:57:19, wtc wrote: > > Nit: Returns success => Returns true on success ? > > What does "DISP line" mean? 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.) http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:84: static bool g_post_data_enabled_ = false; On 2012/08/03 00:57:19, wtc wrote: > > Nit: these three variables (and also the existing > kWebRequestEvents constant above) are inside an anonymous > namespace, so they don't need to be declared 'static'. > > Nit: these three variables are not class members, so they > should not have the trailing underscore '_'. Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:86: bool PostDataEnabledOnChannel() { On 2012/08/03 00:57:19, wtc wrote: > > Nit: This function operates on the global variable g_channel_. > This is not obvious. I recommend having this function > take the channel as input. The function was removed in the meantime. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:89: g_channel_ == VersionInfo::CHANNEL_DEV; On 2012/08/02 10:11:03, Matt Perry wrote: > note: the channels go in increasing order of stability so that you can do things > like (channel_ <= DEV). Good to know! If that's the convention, then the test is much simpler to write. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:97: // Find out the correct channel and flip |g_post_data_enabled_| accordingly. On 2012/08/02 10:11:03, Matt Perry wrote: > This comment is fairly obvious from the code. Take a look at the "Dont's" > section of > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Implementation... > . Comment removed. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:100: g_post_data_enabled_ = PostDataEnabledOnChannel(); On 2012/08/02 10:11:03, Matt Perry wrote: > no need to cache this imo With switching to channel <= DEV instead of the three equalities my last argument for caching disappeared. In addition, due to absence of caching the "enabled" flag and the simplicity of the channel <= DEV test, I could get rid of PostDataEnabledOnChannel completely. :) http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:306: void SetChannelForTestingWebRequest(VersionInfo::Channel channel) { On 2012/08/03 00:57:19, wtc wrote: > > You name the argument |c| in the header file but name it > |channel| here. The comment on line 305 uses the name |c|. Header synced with implementation. Comment removed in the meantime. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.h (right): http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.h:450: void SetChannelForTestingWebRequest(chrome::VersionInfo::Channel c); On 2012/08/02 10:11:03, Matt Perry wrote: > nit: I'd put the WebRequest tag as a prefix, not a suffix Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.h:452: // Only to be used in tests. Resets the overriden channel info. On 2012/08/03 00:57:19, wtc wrote: > > Typo: overriden => overridden Done. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:109: return scoped_ptr<const DictionaryValue>(); On 2012/08/02 10:11:03, Matt Perry wrote: > Sadly this makes the code a bit hard to read. What do you think of this idea: > > void GetPartHelper(scoped_ptr<DictionaryValue>* out) { > scoped_ptr<DictionaryValue> retval; > // use ASSERTs here... > (*out)->swap(retval); > } > > scoped_ptr<const DictionaryValue> GetPart() { > scoped_ptr<const DictionaryValue> retval; > GetPartHelper(args, &retval); > return retval; > } Good idea. I actually changed the signature of GetPartOfMessageArgument directly, I had no preference for returning the scoped_ptr. http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:443: const char kFileEntryPrefix[] = "FILE_PATH=["; This might not be optimal to distinguish the file name in the (Base64encoded) stream of TYPE_BYTES data.
http://codereview.chromium.org/10694055/diff/62130/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/62130/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:114: "error": {"type": "string", "optional": true, "description": "Errors when obtaining request body data."}, This is a bit abnormal for our APIs. What types of errors would go here? http://codereview.chromium.org/10694055/diff/62130/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:115: "parsedForm": { How about 'formData'?
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:54: } On 2012/08/05 18:54:46, vabr (Chromium) wrote: > On 2012/08/03 00:57:19, wtc wrote: > > > > I think these four methods should be named set_key() > > and set_value(), to match the naming convention of the > > getter methods key() and value(). > > Done. > > (For the record, if somebody asks me later: the Google style guide prescribes > unix_hacker style for fast inlined functions OR accessors/mutators. In > particular, the accessors/mutators should be unix_hacker style even if not > inlined.) Interesting, I didn't know that! Thanks! http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:443: const char kFileEntryPrefix[] = "FILE_PATH=["; On 2012/08/05 18:54:47, vabr (Chromium) wrote: > This might not be optimal to distinguish the file name in the (Base64encoded) > stream of TYPE_BYTES data. So, our raw data is still not quite "raw". Maybe we should just pass down the structured data (arrays of bytes/file/chunk elements). http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:189: extensions::RequestDataRepresentationProducer* const kProducers[] = { just "producers". While "const", this variable doesn't really represent a "constant" value. http://codereview.chromium.org/10694055/diff/62130/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/62130/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:109: "body": { I think we should go with requestBody, because it's otherwise easy to confuse with the response body.
vabr: sorry about the late review. I was at a conference on Tuesday and Wednesday. Let me answer your questions first. In short, I agree with your decisions to not make some of the changes I suggested (or asked about). http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:184: return false; On 2012/08/05 18:54:46, vabr (Chromium) wrote: > > Please let me know if you disagree with my decision on this. Thank you for the explanation. I was wondering if we need to allow a PostDataParser to be reused after having parsed one source. If each PostDataParser object is intended to be used only once, then I agree with your decision. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.cc:254: next_line_ = 0; On 2012/08/05 18:54:46, vabr (Chromium) wrote: > > Please let me know if you still want me to reset them here. No, I don't. My previous suggestion was based on the incorrect assumption that we need to allow a PostDataParser to be reused, so it would seem that SetSource should initialize the data members to the same values as the constructor does. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/post_data_parser.h (right): http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:51: static scoped_ptr<PostDataParser> CreatePostDataParser( On 2012/08/05 18:54:46, vabr (Chromium) wrote: > > 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. If no other reviewers commented on this, it's fine to use scoped_ptr. I am a C programmer :-) http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:78: virtual bool SetSource(const std::vector<char>* source) = 0; On 2012/08/05 18:54:46, vabr (Chromium) wrote: > > Do you still think it is better to have the reference? 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. > Do you still think it will be better as const char* and size_t? No. Thank you for the explanation. http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/post_data_parser.h:123: enum LineType {kBoundary, kEndBoundary, kDisposition, kEmpty, kOther}; On 2012/08/05 18:54:46, vabr (Chromium) wrote: > > 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. 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.)
Patch set 37 LGTM.
I only reviewed post_data_parser.{h,cc} this time. Please
make sure all the files have been reviewed by someone.
Please watch out for reviewer's fatigue. When this CL is
good enough, please check it in and then create new CLs
to improve the code. Right now this CL already has 37 patch
sets.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/post_data_parser.cc (right):
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:146:
last_read_success = GetNextChar(&c);
Nit: this can be a do-while loop:
bool last_read_success;
do {
last_read_success = GetNextChar(&c);
} while (last_read_success && c != '=');
Similarly for lines 159-161 below.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:255: // (*) Now
state_ == kBody, so value_end gets updated below.
Are you sure state_ == kBody here?
At line 249, state_ == kBody. After the DoStep() call
on line 242, state_ may become kHeaderStart or kFinal,
right?
I think this comment is in the wrong place, which is why
it confused me.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:319: const size_t
line_length = line_end_ - line_start_;
If empty lines are common, you can do this optimization:
after this line, add
if (line_length == 0)
return kEmpty;
and then delete lines 327-328.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:346: if (seek + 1
< length_ && strncmp(source_ + seek, "\r\n", 2) == 0) {
Nit: since you have already checked the length, you can
use memcmp instead of strncmp here.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:349: } else if
(seek == length_) {
Nit: don't use "else" after a return statement. Similarly
for line 351 below.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:352: // Neither at
the end, nor ending with a CRLF -- abort.
Can you find out if we need to support a line ending of just
LF?
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:364: const char
kFilenameEquals[] = " filename=\"";
Nit: these two const char arrays can be 'static'.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:368: name_offset
+= strlen(kNameEquals);
Nit: strlen(kNameEquals) can be replaced by
sizeof(kNameEquals) - 1, which is a constant.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:371:
line.find('"', name_offset) - name_offset));
If line.find('"', name_offset) does not find the '"'
character, will this still be correct?
Same question for the line.find('"', value_offset) call
on line 380 below.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:447:
kFileEntryPrefix + arraysize(kFileEntryPrefix));
NOTE: there may be an off-by-one error here.
arraysize(kFileEntryPrefix) includes the terminating null
byte, so I think this will insert the terminating null byte
into data_.
Similarly for line 452 below.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/post_data_parser.h (right):
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:28: // Interface
for parsers for the POST data.
Nit: POST data => form data
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:45: void
set_value(const std::string& str);
Nit: since we define the getters in the header, perhaps
we can also define the setters in the header?
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:177: // Workflow
for objects implementing this interface:
Nit: first define what this interface is or does.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.cc:204: for (size_t n
= 0; !some_succeeded && n < arraysize(kProducers); ++n) {
Nit: this kind of for loop index variable is traditionally
named 'i'. 'n' usually is used for the number of elements.
(This convention probably comes from math and Fortran).
Dear reviewers,
(Those of you no longer involved but still spammed by this long-living CL,
please feel free to remove yourselves. I left everybody in Reviewer/Cc in case
people would like to stay up-to-date.)
Thanks wtc@, mpcomplete@, aa@ and battre@ for your recent comments. I addressed
them, and I'm quite happy with the CL, except for the "multipart" parser which
turns to be incorrect (thanks wtc@ for his CRLF comment).
Summary:
* Action plan
* Changelog
* In-line comments
Action plan
This is the structure of the CL:
A. Adding form data parsers, files: form_data_parser*.
B. Adding upload data presenters, files: upload_data_presenter*.
C. Wiring presenters into WebRequest.
D. Defining the "requestBody" part ('details' and 'extraInfoSpec' stuff in the
JSON file) of the interface.
A depends on nothing. B depends only on the interface of A. C depends only on
the interface of B. In a non-programmatic, only string-replace way, B and C
depend also on D.
Even with changes to the "multipart" parser, parts B-D of the CL can be
finalised, because the parser interface should not change anymore.
wtc@ is right that this is growing too long, so I suggest the following plan:
1. aa@, mpcomplete@, please check C and D. You can also check B if you want to.
I don't plan to change C+D anymore or ask other people to review that, so I
promise it won't change under your hands.
2. battre@, once mpcomplete@ and aa@ are happy, could you please review all
from A and and B except for implementation of A?
3. After that I will hopefully have the "multipart" parser rewritten, and will
contact jianli@ for a review of A.
Obviously, all of you, including people not addressed in the action plan above,
are free to review anything and anytime, the above is just a suggestion to make
this CL converge to landing quicker.
Changelog (patch set 37 to 38)
* Throwing out the static channel caching, and switching to kalman@'s brand new
ScopedCurrentChannel (http://crrev.com/10826199)
* Passing BinaryValue for char[] instead of StringValue, because since the
recent http://crrev.com/10694085 we do not serialize Values before passing to an
extension.
* Renaming *Producers to *Presenters and putting them in a separate file.
Adding a unit test for *Presenters. (This split was after a consultation with
battre@.)
* In the 'details' and 'extraInfoSpec' part of the JavaScript interface "body"
was changed to "requestBody".
Thanks, everyone!
Vaclav
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/post_data_parser.cc (right):
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:184: return false;
On 2012/08/09 22:02:50, wtc wrote:
>
> On 2012/08/05 18:54:46, vabr (Chromium) wrote:
> >
> > Please let me know if you disagree with my decision on this.
>
> Thank you for the explanation.
>
> I was wondering if we need to allow a PostDataParser to be
> reused after having parsed one source. If each PostDataParser
> object is intended to be used only once, then I agree with
> your decision.
>
I can confirm that the each instance of PostDataParser implementations is only
meant for parsing one form.
(The Multipart implementation can be fed multiple parts of the encoded form,
thus multiple calls to SetSource; this is not the case uf the URLencoded
implementation.)
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:254: next_line_ =
0;
On 2012/08/09 22:02:50, wtc wrote:
>
> On 2012/08/05 18:54:46, vabr (Chromium) wrote:
> >
> > Please let me know if you still want me to reset them here.
>
> No, I don't. My previous suggestion was based on the
> incorrect assumption that we need to allow a PostDataParser
> to be reused, so it would seem that SetSource should
> initialize the data members to the same values as the
> constructor does.
Just to be clear, PostDataParserMultipart::SetSource can be called multiple
times during the parser's lifetime. My arguments were actually against
consistency with the constructor, to omit unnecessary code and improve
readability.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/post_data_parser.h (right):
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:78: virtual bool
SetSource(const std::vector<char>* source) = 0;
On 2012/08/09 22:02:50, wtc wrote:
>
> On 2012/08/05 18:54:46, vabr (Chromium) wrote:
> >
> > Do you still think it is better to have the reference?
>
> 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.
>
> > Do you still think it will be better as const char* and size_t?
>
> No. Thank you for the explanation.
Thanks for catching the missing NULL check! I added checking for source==NULL.
http://codereview.chromium.org/10694055/diff/75041/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:123: enum LineType
{kBoundary, kEndBoundary, kDisposition, kEmpty, kOther};
On 2012/08/09 22:02:50, wtc wrote:
>
> On 2012/08/05 18:54:46, vabr (Chromium) wrote:
> >
> > 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.
>
> 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.)
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.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/post_data_parser.cc (right):
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:146:
last_read_success = GetNextChar(&c);
On 2012/08/09 23:39:40, wtc wrote:
>
> Nit: this can be a do-while loop:
>
> bool last_read_success;
> do {
> last_read_success = GetNextChar(&c);
> } while (last_read_success && c != '=');
>
> Similarly for lines 159-161 below.
I prefer the while-loop because:
* it is shorter, and
* allows me to initialise last_read_success immediately.
Because you marked this as nit, I have not changed it; but I'm open to
discussion.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:255: // (*) Now
state_ == kBody, so value_end gets updated below.
On 2012/08/09 23:39:40, wtc wrote:
>
> Are you sure state_ == kBody here?
>
> At line 249, state_ == kBody. After the DoStep() call
> on line 242, state_ may become kHeaderStart or kFinal,
> right?
>
> I think this comment is in the wrong place, which is why
> it confused me.
Thanks for catching this. I was missing a corner case. I corrected it.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:319: const size_t
line_length = line_end_ - line_start_;
On 2012/08/09 23:39:40, wtc wrote:
>
> If empty lines are common, you can do this optimization:
>
> after this line, add
> if (line_length == 0)
> return kEmpty;
>
> and then delete lines 327-328.
Good point, done.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:346: if (seek + 1
< length_ && strncmp(source_ + seek, "\r\n", 2) == 0) {
On 2012/08/09 23:39:40, wtc wrote:
>
> Nit: since you have already checked the length, you can
> use memcmp instead of strncmp here.
Done.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:349: } else if
(seek == length_) {
On 2012/08/09 23:39:40, wtc wrote:
>
> Nit: don't use "else" after a return statement. Similarly
> for line 351 below.
Done. (Not quite sure, though: what is wrong with else after a return?)
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:352: // Neither at
the end, nor ending with a CRLF -- abort.
On 2012/08/09 23:39:40, wtc wrote:
>
> Can you find out if we need to support a line ending of just
> LF?
RFC 2388 (Returning Values from Forms: multipart/form-data) says:
"The media-type multipart/form-data follows the rules of all multipart MIME data
streams as outlined in [RFC 2046]."
RFC 2046 says in section 5.1.1 that boundary lines need to end with CRLF.
But there is nothing being said about the data inside individual parts.
Actually, the parser is wrong as it would refuse to parse data with '\r' not
followed by '\r'.
Although WebKit does not generate '\r' not followed by '\n', it can still appear
in the form through a file upload.
I will rewrite the parser strictly according to RFC 2046, instead of
back-guessing from the usual output.
I am really sorry to have wasted some of the efforts of you reviewers on this
:(.
However, the parser's API will remain, as will the URLEncoded parser.
The question is whether to wait in this CL for the improved parser, or to live
with this one until the correction lands a separate parser. I personally would
prefer the former, and will make sure to write the improved parser very soon
anyway.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:364: const char
kFilenameEquals[] = " filename=\"";
On 2012/08/09 23:39:40, wtc wrote:
>
> Nit: these two const char arrays can be 'static'.
Done.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:368: name_offset
+= strlen(kNameEquals);
On 2012/08/09 23:39:40, wtc wrote:
>
> Nit: strlen(kNameEquals) can be replaced by
> sizeof(kNameEquals) - 1, which is a constant.
Actually, at least in GCC, strlen called on a compile-time constant string is a
compile-time constant itself. This is easily verified by using it, e.g., to
specify size of static arrays. It is also more readable (it says "string length"
and needs no compensation for '\0'), so earlier we agreed with battre@ that I
will use strlen in these cases.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:371:
line.find('"', name_offset) - name_offset));
On 2012/08/09 23:39:40, wtc wrote:
>
> If line.find('"', name_offset) does not find the '"'
> character, will this still be correct?
>
> Same question for the line.find('"', value_offset) call
> on line 380 below.
Check added. Thanks for catching this.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:443: const char
kFileEntryPrefix[] = "FILE_PATH=[";
On 2012/08/06 21:06:45, Matt Perry wrote:
> On 2012/08/05 18:54:47, vabr (Chromium) wrote:
> > This might not be optimal to distinguish the file name in the
(Base64encoded)
> > stream of TYPE_BYTES data.
>
> So, our raw data is still not quite "raw". Maybe we should just pass down the
> structured data (arrays of bytes/file/chunk elements).
Great idea, done.
Now I'm returning an array (ListValue) of the following:
* bytes as ArrayBuffer (BinaryValue)
* files as filename string (StringValue)
A question: Because of filenames with non 7-bit ASCII values (e.g., 電車.txt) I
currently Base64encode the filenames. Is that an overkill? In other words, would
it be better to trade getting '???????.txt' for not having to Base64Decode the
filenames each time in the extension?
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.cc:447:
kFileEntryPrefix + arraysize(kFileEntryPrefix));
On 2012/08/09 23:39:40, wtc wrote:
>
> NOTE: there may be an off-by-one error here.
>
> arraysize(kFileEntryPrefix) includes the terminating null
> byte, so I think this will insert the terminating null byte
> into data_.
>
> Similarly for line 452 below.
Indeed, this was an error. However, the code was removed during work on the
comment from Matt above.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/post_data_parser.h (right):
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:28: // Interface
for parsers for the POST data.
On 2012/08/09 23:39:40, wtc wrote:
>
> Nit: POST data => form data
Done.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:45: void
set_value(const std::string& str);
On 2012/08/09 23:39:40, wtc wrote:
>
> Nit: since we define the getters in the header, perhaps
> we can also define the setters in the header?
Done.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/post_data_parser.h:177: // Workflow
for objects implementing this interface:
On 2012/08/09 23:39:40, wtc wrote:
>
> Nit: first define what this interface is or does.
Done.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.cc:189:
extensions::RequestDataRepresentationProducer* const kProducers[] = {
On 2012/08/06 21:06:45, Matt Perry wrote:
> just "producers". While "const", this variable doesn't really represent a
> "constant" value.
Done.
http://codereview.chromium.org/10694055/diff/62130/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.cc:204: for (size_t n
= 0; !some_succeeded && n < arraysize(kProducers); ++n) {
On 2012/08/09 23:39:40, wtc wrote:
>
> Nit: this kind of for loop index variable is traditionally
> named 'i'. 'n' usually is used for the number of elements.
> (This convention probably comes from math and Fortran).
Done.
I agree, Index versus Number (of).
http://codereview.chromium.org/10694055/diff/62130/chrome/common/extensions/a...
File chrome/common/extensions/api/web_request.json (right):
http://codereview.chromium.org/10694055/diff/62130/chrome/common/extensions/a...
chrome/common/extensions/api/web_request.json:109: "body": {
On 2012/08/06 21:06:45, Matt Perry wrote:
> I think we should go with requestBody, because it's otherwise easy to confuse
> with the response body.
Done, changed both the entry name from 'details', and the corresponding
extraInfoSpec item.
http://codereview.chromium.org/10694055/diff/62130/chrome/common/extensions/a...
chrome/common/extensions/api/web_request.json:114: "error": {"type": "string",
"optional": true, "description": "Errors when obtaining request body data."},
On 2012/08/06 03:56:49, Aaron Boodman wrote:
> This is a bit abnormal for our APIs. What types of errors would go here?
Right now the only errors can be chunked elements in the request body.
Some other might be added, like failed base::Base64Encode.
I'm open to suggestions about how to report an error:
* free-text string as currently, or
* an error code (short enum-string) as before, or
* something more appropriate?
This is not an error-message for an error in WebRequest or event-handling. This
is a part of a normal callback data, and it is used to inform the handler that
the request body could not be passed; but the request itself is still OK and can
be blocked.
http://codereview.chromium.org/10694055/diff/62130/chrome/common/extensions/a...
chrome/common/extensions/api/web_request.json:115: "parsedForm": {
On 2012/08/06 03:56:49, Aaron Boodman wrote:
> How about 'formData'?
I have no preference on parsedForm vs. formData, but I remember that at some
point parsedForm was chosen to include the word "parsed" explicitly.
Does anybody else from the reviewers have a strong preference on the naming? If
nobody then I will rename it to formData in the next update.
I can't review (c) because I'm not that familiar with webrequest, but I'm happy with (d). http://codereview.chromium.org/10694055/diff/62130/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/62130/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:114: "error": {"type": "string", "optional": true, "description": "Errors when obtaining request body data."}, On 2012/08/10 17:12:55, vabr (Chromium) wrote: > On 2012/08/06 03:56:49, Aaron Boodman wrote: > > This is a bit abnormal for our APIs. What types of errors would go here? > > Right now the only errors can be chunked elements in the request body. > Some other might be added, like failed base::Base64Encode. > > I'm open to suggestions about how to report an error: > * free-text string as currently, or > * an error code (short enum-string) as before, or > * something more appropriate? > > This is not an error-message for an error in WebRequest or event-handling. This > is a part of a normal callback data, and it is used to inform the handler that > the request body could not be passed; but the request itself is still OK and can > be blocked. I see. Makes sense.
(except that I still prefer formData rather than parsedForm. I think what irritates me about 'parsedForm' is the double meaning of 'form').
Hi Jian, Do you think you could review my parsers for multipart-encoded and URLencoded HTML forms? The parsers are located in three files in this CL, called form_data_parser* . The parsers are independent of the rest of the CL, and other reviewers are reviewing the rest. Hi Dominic, Could you please review the presenters (upload_data_presenter*) and my changes to web_request_api* ? Hi Aaron, Thanks for your review! I renamed parsedForm to formData, so I guess by that part D is now ready. Thanks to all, Vaclav
On 2012/08/14 19:50:18, vabr (Chromium) wrote: > Hi Jian, > > Do you think you could review my parsers for multipart-encoded and URLencoded > HTML forms? > The parsers are located in three files in this CL, called form_data_parser* . > The parsers are independent of the rest of the CL, and other reviewers are > reviewing the rest. I forgot to add that a drawing of the automaton underlying the "multipart" parser is here [http://www.corp.google.com/~vabr/no_crawl/automaton.jpg], if it is of any help for the review.
C+D LGTM
Thanks, Matt. So it remains to review upload_data_presenter* (Dominic?) and form_data_parser* (Jian Li?). Some more thoughts for the parsers added. Cheers, Vaclav http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/form_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:406: memcmp(dash_boundary_.c_str(), &(*offset_), length) != 0) Doing this each time is unnecessary expensive. It might be a good idea to scan the |source_| in SetSource for dash_boundary occurrences and remember their positions, and then to only check the offset here. I can do this either in a separate CL, so as not to delay this one, or in this one, if the reviewers think it would be wiser. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/form_data_parser.h (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.h:186: // Automaton for "multipart-body": An alternative to hand-writing the automaton would be to encode the "multipart-body" as a regular expression and use, e.g., RE2 library to parse it. Pros: * shorter and clearer code * easily checkable against errors in language specification * very probably faster, although not sure how significantly Cons: * expects the whole input string in a continuous segment of memory, whereas we have several displaced instances of vector<char>; we don't really want to copy them just to have them in one place, they can be big I decided the single "con" was heavier than the three "pros", but feel free to let me know if you think opposite and/or have suggestions at overcoming the "con".
On Thu, Aug 16, 2012 at 1:00 AM, <vabr@chromium.org> wrote: > Thanks, Matt. > > So it remains to review upload_data_presenter* (Dominic?) and > form_data_parser* > (Jian Li?). > Can you find someone who is the expert on parsing to review it? > > Some more thoughts for the parsers added. > > Cheers, > Vaclav > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/form_data_parser.**cc<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/form_data_parser.cc> > File chrome/browser/extensions/api/**web_request/form_data_parser.**cc > (right): > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/form_data_parser.** > cc#newcode406<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/form_data_parser.cc#newcode406> > chrome/browser/extensions/api/**web_request/form_data_parser.**cc:406: > memcmp(dash_boundary_.c_str(), &(*offset_), length) != 0) > Doing this each time is unnecessary expensive. It might be a good idea > to scan the |source_| in SetSource for dash_boundary occurrences and > remember their positions, and then to only check the offset here. I can > do this either in a separate CL, so as not to delay this one, or in this > one, if the reviewers think it would be wiser. > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/form_data_parser.h<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/form_data_parser.h> > File chrome/browser/extensions/api/**web_request/form_data_parser.h > (right): > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/form_data_parser.** > h#newcode186<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/form_data_parser.h#newcode186> > chrome/browser/extensions/api/**web_request/form_data_parser.**h:186: // > Automaton for "multipart-body": > An alternative to hand-writing the automaton would be to encode the > "multipart-body" as a regular expression and use, e.g., RE2 library to > parse it. > Pros: > * shorter and clearer code > * easily checkable against errors in language specification > * very probably faster, although not sure how significantly > Cons: > * expects the whole input string in a continuous segment of memory, > whereas we have several displaced instances of vector<char>; we don't > really want to copy them just to have them in one place, they can be big > > I decided the single "con" was heavier than the three "pros", but feel > free to let me know if you think opposite and/or have suggestions at > overcoming the "con". > > http://codereview.chromium.**org/10694055/<http://codereview.chromium.org/106... >
I did not look into form_data_parser*.* very deeply. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/form_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:318: while (state_ != kPart) nit: {} http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:323: while (state_ != kColonS) nit: {} http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:328: if (state_ == kEnd1 || state_ == kEnd2 || state_ == kEnd3) nit: {} http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:336: while (state_ != kPreData) nit: {} http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:347: if (state_ == kCR4) nit: {} http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:399: return ahead == ' ' || ahead == '\t' ? 1u : 0u; nit: () around condition, also above http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:422: header.data(), kContentDisposition, strlen(kContentDisposition) != 0)) I would move header.data(), kContentDisposition, to the previous line http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/form_data_parser.h (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.h:47: nit: -1 new line http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.h:51: std::string value_; DISALLOW_COPY_AND_ASSIGN(Result); + #include "base/basictypes.h" http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/upload_data_presenter.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:55: net::HttpRequestHeaders::kTransferEncoding, &transfer_encoding)) nit: +4 indent http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:69: return chunks_found_; is this correct? Shouldn't this always return true? http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:100: } // TYPE_BLOB is silently ignored. 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. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:100: } // TYPE_BLOB is silently ignored. 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? http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:127: success_ = base::Base64Encode(filename, &bytes_encoded); Why do you pass the file path as base64 encoded data? Shouldn't this be UTF-8? http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:135: : parser_(FormDataParser::Create(request).release()), is this .release() necessary? http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:172: return dictionary_.PassAs<Value>(); 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. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/upload_data_presenter.h (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.h:31: // UploadDataPresenter is an interface for objects capable to be consume -be http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.h:60: static bool TransferEncodingChunked(const net::URLRequest* request); Put a verb ("Is") to the beginning of method name? http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.h:71: class RawDataPresenter : public UploadDataPresenter { Can you please add a high level description for this class? http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.h:93: class ParsedDataPresenter : public UploadDataPresenter { Can you please add a high level description for this class? http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc:23: virtual void SetUp() OVERRIDE {} if you don't set up anything, you can remove this class entirely. Just use TEST(... instead for TEST_F(... below. The _F stands for fixture. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc:54: ListValue expected_list; How does the consumer of this class know that the first item is a bytes array and the second one is a file name? http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:206: } nit: no {} http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:207: if (presenters[i]->Succeeded()) { This looks wrong to me. Most presenters call Abort() in case they cannot parse an element but can never back out of this state (Abort() sets an error flag that is never cleared). http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:214: } nit: no {} http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:450: scoped_ptr<base::ListValue> list_value(new base::ListValue()); nit: you can use a base::ListValue instead of scoped_ptr<base::ListValue> here http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:577: for (int pass = 0; pass < 2; ++pass) { Can you add a comment, why you have these passes? I don't understand why you use different channels in Part 1 and 2. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:633: // Now send a POST request with body which is not parseable as a form. Why do you do this while no listener is subscribed? http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:634: FireURLRequestWithData(kMethodPost, NULL /*no header*/, plain_1, plain_2); Validate the generated event data? http://codereview.chromium.org/10694055/diff/56038/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/56038/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:118: "description": "If the request method is POST and the body is a sequence of key-value pairs, encoded as either multipart/form-data, or application/x-www-form-urlencoded, this dictionary is present and for each key contains the list of all values for that key. If the data is of another media type, or if it is malformed, the dictionary is not present. It is also not present if the form upload is chunked. Example value of this dictionary is {'key': ['value1', 'value2']}.", An example value http://codereview.chromium.org/10694055/diff/56038/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:129: "description": "If the request method is PUT or POST, and the body is not already parsed in formData then the unparsed request body elements are contained in this array. Data elements appear here as ArrayBuffer, file elements as strings containing the filename." comma after formData http://codereview.chromium.org/10694055/diff/56038/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:129: "description": "If the request method is PUT or POST, and the body is not already parsed in formData then the unparsed request body elements are contained in this array. Data elements appear here as ArrayBuffer, file elements as strings containing the filename." I think filenames are base64 encoded, right? I'd rather have them as plain text. http://codereview.chromium.org/10694055/diff/56038/chrome/test/data/extension... File chrome/test/data/extensions/api_test/webrequest/test_post.js (right): http://codereview.chromium.org/10694055/diff/56038/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/test_post.js:7: function sendPost(formFile, ParseableForm) { nit: parseableForm should start with a lower case letter http://codereview.chromium.org/10694055/diff/56038/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/test_post.js:94: raw: [ { byteLength: 127 } ] why does this contain only the byteLength? I thought you pass this as an array? http://codereview.chromium.org/10694055/diff/56038/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/test_post.js:116: navigateAndWait(getURL(dirName + formFile)); this should be navigateAndWait(getURL(...), function() {close();}); such that close is executed when the navigation is completed.
Hi Jian, To explain: for reviewing the three form_data_parser* files I tried to find a WebKit expert, who is knowledgeable about the way HTML forms are encoded on upload. I had some parts of this CL reviewed by Wan-Teh Chang, who suggested you might be the right WebKit/forms person for the parsers. If you don't want to review this, but know who would be a good match, please do tell me :). In any case, thanks for your reply. Dominic, Thanks for your helpful comments. Please take a look at my answers, at some points I was not sure I understood you properly. Vaclav http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/form_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:318: while (state_ != kPart) On 2012/08/16 19:18:03, battre wrote: > nit: {} Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:323: while (state_ != kColonS) On 2012/08/16 19:18:03, battre wrote: > nit: {} Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:328: if (state_ == kEnd1 || state_ == kEnd2 || state_ == kEnd3) On 2012/08/16 19:18:03, battre wrote: > nit: {} Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:336: while (state_ != kPreData) On 2012/08/16 19:18:03, battre wrote: > nit: {} Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:347: if (state_ == kCR4) On 2012/08/16 19:18:03, battre wrote: > nit: {} Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:399: return ahead == ' ' || ahead == '\t' ? 1u : 0u; On 2012/08/16 19:18:03, battre wrote: > nit: () around condition, also above Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:422: header.data(), kContentDisposition, strlen(kContentDisposition) != 0)) On 2012/08/16 19:18:03, battre wrote: > I would move header.data(), kContentDisposition, to the previous line Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/form_data_parser.h (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.h:47: On 2012/08/16 19:18:03, battre wrote: > nit: -1 new line Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.h:51: std::string value_; On 2012/08/16 19:18:03, battre wrote: > DISALLOW_COPY_AND_ASSIGN(Result); > > + #include "base/basictypes.h" Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/upload_data_presenter.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:55: net::HttpRequestHeaders::kTransferEncoding, &transfer_encoding)) On 2012/08/16 19:18:03, battre wrote: > nit: +4 indent 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. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:69: return chunks_found_; On 2012/08/16 19:18:03, battre wrote: > is this correct? Shouldn't this always return true? 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. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:100: } // TYPE_BLOB is silently ignored. On 2012/08/16 19:18:03, battre wrote: > 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. Good idea. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:100: } // TYPE_BLOB is silently ignored. On 2012/08/16 19:18:03, battre wrote: > 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? 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? http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:127: success_ = base::Base64Encode(filename, &bytes_encoded); On 2012/08/16 19:18:03, battre wrote: > Why do you pass the file path as base64 encoded data? Shouldn't this be UTF-8? 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. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:135: : parser_(FormDataParser::Create(request).release()), On 2012/08/16 19:18:03, battre wrote: > is this .release() necessary? No :). Removed. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:172: return dictionary_.PassAs<Value>(); On 2012/08/16 19:18:03, battre wrote: > 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. 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? http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/upload_data_presenter.h (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.h:31: // UploadDataPresenter is an interface for objects capable to be consume On 2012/08/16 19:18:03, battre wrote: > -be Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.h:60: static bool TransferEncodingChunked(const net::URLRequest* request); On 2012/08/16 19:18:03, battre wrote: > Put a verb ("Is") to the beginning of method name? Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.h:71: class RawDataPresenter : public UploadDataPresenter { On 2012/08/16 19:18:03, battre wrote: > Can you please add a high level description for this class? Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.h:93: class ParsedDataPresenter : public UploadDataPresenter { On 2012/08/16 19:18:03, battre wrote: > Can you please add a high level description for this class? Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc:23: virtual void SetUp() OVERRIDE {} On 2012/08/16 19:18:03, battre wrote: > if you don't set up anything, you can remove this class entirely. Just use > TEST(... instead for TEST_F(... below. The _F stands for fixture. Thanks! I did not know that. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc:54: ListValue expected_list; On 2012/08/16 19:18:03, battre wrote: > How does the consumer of this class know that the first item is a bytes array > and the second one is a file name? On the C++ level this is BinaryValue (bytes array) vs. StringValue (filename). In Javascript they can use "typeof". http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:206: } On 2012/08/16 19:18:03, battre wrote: > nit: no {} Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:207: if (presenters[i]->Succeeded()) { On 2012/08/16 19:18:03, battre wrote: > This looks wrong to me. Most presenters call Abort() in case they cannot parse > an element but can never back out of this state (Abort() sets an error flag that > is never cleared). This is by design, all the elements together represent the input to the parser, so the parser should be able to parse the whole sequence. Let me please know if you feel I misunderstood your comment. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:214: } On 2012/08/16 19:18:03, battre wrote: > nit: no {} Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:450: scoped_ptr<base::ListValue> list_value(new base::ListValue()); On 2012/08/16 19:18:03, battre wrote: > nit: you can use a base::ListValue instead of scoped_ptr<base::ListValue> here Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:577: for (int pass = 0; pass < 2; ++pass) { On 2012/08/16 19:18:03, battre wrote: > Can you add a comment, why you have these passes? I don't understand why you use > different channels in Part 1 and 2. Done. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:614: ExtensionWebRequestEventRouter::GetInstance()->AddEventListener( Here I am adding a listener which stays through parts 4 and 5. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:633: // Now send a POST request with body which is not parseable as a form. On 2012/08/16 19:18:03, battre wrote: > Why do you do this while no listener is subscribed? I believe there is a listener subscribed (marked by a comment above). http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:634: FireURLRequestWithData(kMethodPost, NULL /*no header*/, plain_1, plain_2); On 2012/08/16 19:18:03, battre wrote: > Validate the generated event data? I don't understand your question. If you ask where I validate the generated data then the answer is in the for-loop below, against data in kExpected. http://codereview.chromium.org/10694055/diff/56038/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/56038/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:118: "description": "If the request method is POST and the body is a sequence of key-value pairs, encoded as either multipart/form-data, or application/x-www-form-urlencoded, this dictionary is present and for each key contains the list of all values for that key. If the data is of another media type, or if it is malformed, the dictionary is not present. It is also not present if the form upload is chunked. Example value of this dictionary is {'key': ['value1', 'value2']}.", On 2012/08/16 19:18:03, battre wrote: > An example value Done. http://codereview.chromium.org/10694055/diff/56038/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:129: "description": "If the request method is PUT or POST, and the body is not already parsed in formData then the unparsed request body elements are contained in this array. Data elements appear here as ArrayBuffer, file elements as strings containing the filename." On 2012/08/16 19:18:03, battre wrote: > comma after formData Done. http://codereview.chromium.org/10694055/diff/56038/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:129: "description": "If the request method is PUT or POST, and the body is not already parsed in formData then the unparsed request body elements are contained in this array. Data elements appear here as ArrayBuffer, file elements as strings containing the filename." On 2012/08/16 19:18:03, battre wrote: > I think filenames are base64 encoded, right? I'd rather have them as plain text. Done. http://codereview.chromium.org/10694055/diff/56038/chrome/test/data/extension... File chrome/test/data/extensions/api_test/webrequest/test_post.js (right): http://codereview.chromium.org/10694055/diff/56038/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/test_post.js:7: function sendPost(formFile, ParseableForm) { On 2012/08/16 19:18:03, battre wrote: > nit: parseableForm should start with a lower case letter Done. http://codereview.chromium.org/10694055/diff/56038/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/test_post.js:94: raw: [ { byteLength: 127 } ] On 2012/08/16 19:18:03, battre wrote: > why does this contain only the byteLength? I thought you pass this as an array? I pass it as ArrayBuffer. I don't think its value can be represented in JSON, even in Javascript you need adaptor classes to view it, which specify the number of bytes per elements. The byteLength property is however visible, and while not allowing to test byte-equality, it at least tests that the ArrayBuffer is there. Also, the byte-equality at least on the C++ side is tested in the unit-tests. http://codereview.chromium.org/10694055/diff/56038/chrome/test/data/extension... chrome/test/data/extensions/api_test/webrequest/test_post.js:116: navigateAndWait(getURL(dirName + formFile)); On 2012/08/16 19:18:03, battre wrote: > this should be > navigateAndWait(getURL(...), function() {close();}); such that close is executed > when the navigation is completed. Done. Thanks, I did not know that!
http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/upload_data_presenter.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:55: net::HttpRequestHeaders::kTransferEncoding, &transfer_encoding)) On 2012/08/17 18:29:57, vabr (Chromium) wrote: > On 2012/08/16 19:18:03, battre wrote: > > nit: +4 indent > > 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. 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. ;-) http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:100: } // TYPE_BLOB is silently ignored. On 2012/08/17 18:29:57, vabr (Chromium) wrote: > On 2012/08/16 19:18:03, battre wrote: > > 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? > > 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? Add a NOTREACHED()? http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:172: return dictionary_.PassAs<Value>(); On 2012/08/17 18:29:57, vabr (Chromium) wrote: > On 2012/08/16 19:18:03, battre wrote: > > 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. > > 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? Never mind. I misunderstood your code in web_request_api.cc. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc:54: ListValue expected_list; On 2012/08/17 18:29:57, vabr (Chromium) wrote: > On 2012/08/16 19:18:03, battre wrote: > > How does the consumer of this class know that the first item is a bytes array > > and the second one is a file name? > > On the C++ level this is BinaryValue (bytes array) vs. StringValue (filename). > In Javascript they can use "typeof". I think this should be discussed with the extensions team. We might use Strings for other purposes than filenames in the future. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api.cc:207: if (presenters[i]->Succeeded()) { On 2012/08/17 18:29:57, vabr (Chromium) wrote: > On 2012/08/16 19:18:03, battre wrote: > > This looks wrong to me. Most presenters call Abort() in case they cannot parse > > an element but can never back out of this state (Abort() sets an error flag > that > > is never cleared). > > This is by design, all the elements together represent the input to the parser, > so the parser should be able to parse the whole sequence. Let me please know if > you feel I misunderstood your comment. Nevermind, you are right. I misunderstood the loops. http://codereview.chromium.org/10694055/diff/78005/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/78005/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:592: MessageLoop::current()->RunAllPending(); Please add comment that result of this operation goes into message list of ipc_sender_. http://codereview.chromium.org/10694055/diff/78005/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:604: &profile_, extension_id, kEventName + "/1"); Can you move this to the previous loop? http://codereview.chromium.org/10694055/diff/78005/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:607: filter, extra_info_spec_empty, ipc_sender_factory.GetWeakPtr()); Can you point out that the difference to the previous loop is that you pass an empty extra info spec
tkent might be more appropriate to review HTML forms related stuffs. On Fri, Aug 17, 2012 at 11:29 AM, <vabr@chromium.org> wrote: > Hi Jian, > To explain: for reviewing the three form_data_parser* files I tried to > find a > WebKit expert, who is knowledgeable about the way HTML forms are encoded on > upload. I had some parts of this CL reviewed by Wan-Teh Chang, who > suggested you > might be the right WebKit/forms person for the parsers. > If you don't want to review this, but know who would be a good match, > please do > tell me :). > In any case, thanks for your reply. > > > Dominic, > Thanks for your helpful comments. Please take a look at my answers, at some > points I was not sure I understood you properly. > > Vaclav > > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/form_data_parser.**cc<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/form_data_parser.cc> > File chrome/browser/extensions/api/**web_request/form_data_parser.**cc > (right): > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/form_data_parser.** > cc#newcode318<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/form_data_parser.cc#newcode318> > chrome/browser/extensions/api/**web_request/form_data_parser.**cc:318: > while > (state_ != kPart) > On 2012/08/16 19:18:03, battre wrote: > >> nit: {} >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/form_data_parser.** > cc#newcode323<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/form_data_parser.cc#newcode323> > chrome/browser/extensions/api/**web_request/form_data_parser.**cc:323: > while > (state_ != kColonS) > On 2012/08/16 19:18:03, battre wrote: > >> nit: {} >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/form_data_parser.** > cc#newcode328<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/form_data_parser.cc#newcode328> > chrome/browser/extensions/api/**web_request/form_data_parser.**cc:328: if > (state_ == kEnd1 || state_ == kEnd2 || state_ == kEnd3) > On 2012/08/16 19:18:03, battre wrote: > >> nit: {} >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/form_data_parser.** > cc#newcode336<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/form_data_parser.cc#newcode336> > chrome/browser/extensions/api/**web_request/form_data_parser.**cc:336: > while > (state_ != kPreData) > On 2012/08/16 19:18:03, battre wrote: > >> nit: {} >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/form_data_parser.** > cc#newcode347<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/form_data_parser.cc#newcode347> > chrome/browser/extensions/api/**web_request/form_data_parser.**cc:347: if > (state_ == kCR4) > On 2012/08/16 19:18:03, battre wrote: > >> nit: {} >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/form_data_parser.** > cc#newcode399<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/form_data_parser.cc#newcode399> > chrome/browser/extensions/api/**web_request/form_data_parser.**cc:399: > return ahead == ' ' || ahead == '\t' ? 1u : 0u; > On 2012/08/16 19:18:03, battre wrote: > >> nit: () around condition, also above >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/form_data_parser.** > cc#newcode422<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/form_data_parser.cc#newcode422> > chrome/browser/extensions/api/**web_request/form_data_parser.**cc:422: > header.data(), kContentDisposition, strlen(kContentDisposition) != 0)) > On 2012/08/16 19:18:03, battre wrote: > >> I would move header.data(), kContentDisposition, to the previous line >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/form_data_parser.h<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/form_data_parser.h> > File chrome/browser/extensions/api/**web_request/form_data_parser.h > (right): > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/form_data_parser.**h#newcode47<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/form_data_parser.h#newcode47> > chrome/browser/extensions/api/**web_request/form_data_parser.**h:47: > On 2012/08/16 19:18:03, battre wrote: > >> nit: -1 new line >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/form_data_parser.**h#newcode51<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/form_data_parser.h#newcode51> > chrome/browser/extensions/api/**web_request/form_data_parser.**h:51: > std::string value_; > On 2012/08/16 19:18:03, battre wrote: > >> DISALLOW_COPY_AND_ASSIGN(**Result); >> > > + #include "base/basictypes.h" >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_**presenter.cc<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter.cc> > File chrome/browser/extensions/api/**web_request/upload_data_** > presenter.cc > (right): > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.cc#newcode55<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter.cc#newcode55> > chrome/browser/extensions/api/**web_request/upload_data_**presenter.cc:55: > net::HttpRequestHeaders::**kTransferEncoding, &transfer_encoding)) > On 2012/08/16 19:18:03, battre wrote: > >> nit: +4 indent >> > > 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. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.cc#newcode69<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter.cc#newcode69> > chrome/browser/extensions/api/**web_request/upload_data_**presenter.cc:69: > return chunks_found_; > On 2012/08/16 19:18:03, battre wrote: > >> is this correct? Shouldn't this always return true? >> > > 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. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.cc#newcode100<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter.cc#newcode100> > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.cc:100: > } // TYPE_BLOB is silently ignored. > On 2012/08/16 19:18:03, battre wrote: > >> 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. >> > > Good idea. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.cc#newcode100<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter.cc#newcode100> > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.cc:100: > } // TYPE_BLOB is silently ignored. > On 2012/08/16 19:18:03, battre wrote: > >> 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? > > 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? > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.cc#newcode127<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter.cc#newcode127> > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.cc:127: > success_ = base::Base64Encode(filename, &bytes_encoded); > On 2012/08/16 19:18:03, battre wrote: > >> Why do you pass the file path as base64 encoded data? Shouldn't this >> > be UTF-8? > > 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. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.cc#newcode135<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter.cc#newcode135> > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.cc:135: > : parser_(FormDataParser::**Create(request).release()), > On 2012/08/16 19:18:03, battre wrote: > >> is this .release() necessary? >> > > No :). Removed. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.cc#newcode172<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter.cc#newcode172> > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.cc:172: > return dictionary_.PassAs<Value>(); > On 2012/08/16 19:18:03, battre wrote: > >> 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. > > 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? > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_**presenter.h<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter.h> > File chrome/browser/extensions/api/**web_request/upload_data_**presenter.h > (right): > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.h#newcode31<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter.h#newcode31> > chrome/browser/extensions/api/**web_request/upload_data_**presenter.h:31: > // > UploadDataPresenter is an interface for objects capable to be consume > On 2012/08/16 19:18:03, battre wrote: > >> -be >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.h#newcode60<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter.h#newcode60> > chrome/browser/extensions/api/**web_request/upload_data_**presenter.h:60: > static bool TransferEncodingChunked(const net::URLRequest* request); > On 2012/08/16 19:18:03, battre wrote: > >> Put a verb ("Is") to the beginning of method name? >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.h#newcode71<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter.h#newcode71> > chrome/browser/extensions/api/**web_request/upload_data_**presenter.h:71: > class RawDataPresenter : public UploadDataPresenter { > On 2012/08/16 19:18:03, battre wrote: > >> Can you please add a high level description for this class? >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_** > presenter.h#newcode93<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter.h#newcode93> > chrome/browser/extensions/api/**web_request/upload_data_**presenter.h:93: > class ParsedDataPresenter : public UploadDataPresenter { > On 2012/08/16 19:18:03, battre wrote: > >> Can you please add a high level description for this class? >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_** > presenter_unittest.cc<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc> > File > chrome/browser/extensions/api/**web_request/upload_data_** > presenter_unittest.cc > (right): > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_** > presenter_unittest.cc#**newcode23<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc#newcode23> > chrome/browser/extensions/api/**web_request/upload_data_** > presenter_unittest.cc:23: > virtual void SetUp() OVERRIDE {} > On 2012/08/16 19:18:03, battre wrote: > >> if you don't set up anything, you can remove this class entirely. Just >> > use > >> TEST(... instead for TEST_F(... below. The _F stands for fixture. >> > > Thanks! I did not know that. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/upload_data_** > presenter_unittest.cc#**newcode54<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc#newcode54> > chrome/browser/extensions/api/**web_request/upload_data_** > presenter_unittest.cc:54: > ListValue expected_list; > On 2012/08/16 19:18:03, battre wrote: > >> How does the consumer of this class know that the first item is a >> > bytes array > >> and the second one is a file name? >> > > On the C++ level this is BinaryValue (bytes array) vs. StringValue > (filename). In Javascript they can use "typeof". > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/web_request_api.cc<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/web_request_api.cc> > File chrome/browser/extensions/api/**web_request/web_request_api.cc > (right): > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/web_request_api.** > cc#newcode206<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/web_request_api.cc#newcode206> > chrome/browser/extensions/api/**web_request/web_request_api.**cc:206: } > On 2012/08/16 19:18:03, battre wrote: > >> nit: no {} >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/web_request_api.** > cc#newcode207<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/web_request_api.cc#newcode207> > chrome/browser/extensions/api/**web_request/web_request_api.**cc:207: if > (presenters[i]->Succeeded()) { > On 2012/08/16 19:18:03, battre wrote: > >> This looks wrong to me. Most presenters call Abort() in case they >> > cannot parse > >> an element but can never back out of this state (Abort() sets an error >> > flag that > >> is never cleared). >> > > This is by design, all the elements together represent the input to the > parser, so the parser should be able to parse the whole sequence. Let me > please know if you feel I misunderstood your comment. > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/web_request_api.** > cc#newcode214<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/web_request_api.cc#newcode214> > chrome/browser/extensions/api/**web_request/web_request_api.**cc:214: } > On 2012/08/16 19:18:03, battre wrote: > >> nit: no {} >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/web_request_api_**unittest.cc<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc> > File > chrome/browser/extensions/api/**web_request/web_request_api_**unittest.cc > (right): > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/web_request_api_** > unittest.cc#newcode450<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc#newcode450> > chrome/browser/extensions/api/**web_request/web_request_api_** > unittest.cc:450: > scoped_ptr<base::ListValue> list_value(new base::ListValue()); > On 2012/08/16 19:18:03, battre wrote: > >> nit: you can use a base::ListValue instead of >> > scoped_ptr<base::ListValue> here > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/web_request_api_** > unittest.cc#newcode577<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc#newcode577> > chrome/browser/extensions/api/**web_request/web_request_api_** > unittest.cc:577: > for (int pass = 0; pass < 2; ++pass) { > On 2012/08/16 19:18:03, battre wrote: > >> Can you add a comment, why you have these passes? I don't understand >> > why you use > >> different channels in Part 1 and 2. >> > > Done. > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/web_request_api_** > unittest.cc#newcode614<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc#newcode614> > chrome/browser/extensions/api/**web_request/web_request_api_** > unittest.cc:614: > ExtensionWebRequestEventRouter**::GetInstance()->**AddEventListener( > Here I am adding a listener which stays through parts 4 and 5. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/web_request_api_** > unittest.cc#newcode633<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc#newcode633> > chrome/browser/extensions/api/**web_request/web_request_api_** > unittest.cc:633: > // Now send a POST request with body which is not parseable as a form. > On 2012/08/16 19:18:03, battre wrote: > >> Why do you do this while no listener is subscribed? >> > > I believe there is a listener subscribed (marked by a comment above). > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/browser/extensions/api/**web_request/web_request_api_** > unittest.cc#newcode634<http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc#newcode634> > chrome/browser/extensions/api/**web_request/web_request_api_** > unittest.cc:634: > FireURLRequestWithData(**kMethodPost, NULL /*no header*/, plain_1, > plain_2); > On 2012/08/16 19:18:03, battre wrote: > >> Validate the generated event data? >> > > I don't understand your question. If you ask where I validate the > generated data then the answer is in the for-loop below, against data in > kExpected. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/common/extensions/api/**web_request.json<http://codereview.chromium.org/10694055/diff/56038/chrome/common/extensions/api/web_request.json> > File chrome/common/extensions/api/**web_request.json (right): > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/common/extensions/api/**web_request.json#newcode118<http://codereview.chromium.org/10694055/diff/56038/chrome/common/extensions/api/web_request.json#newcode118> > chrome/common/extensions/api/**web_request.json:118: "description": "If > the request method is POST and the body is a sequence of key-value > pairs, encoded as either multipart/form-data, or > application/x-www-form-**urlencoded, this dictionary is present and for > each key contains the list of all values for that key. If the data is of > another media type, or if it is malformed, the dictionary is not > present. It is also not present if the form upload is chunked. Example > value of this dictionary is {'key': ['value1', 'value2']}.", > On 2012/08/16 19:18:03, battre wrote: > >> An example value >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/common/extensions/api/**web_request.json#newcode129<http://codereview.chromium.org/10694055/diff/56038/chrome/common/extensions/api/web_request.json#newcode129> > chrome/common/extensions/api/**web_request.json:129: "description": "If > the request method is PUT or POST, and the body is not already parsed in > formData then the unparsed request body elements are contained in this > array. Data elements appear here as ArrayBuffer, file elements as > strings containing the filename." > On 2012/08/16 19:18:03, battre wrote: > >> comma after formData >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/common/extensions/api/**web_request.json#newcode129<http://codereview.chromium.org/10694055/diff/56038/chrome/common/extensions/api/web_request.json#newcode129> > chrome/common/extensions/api/**web_request.json:129: "description": "If > the request method is PUT or POST, and the body is not already parsed in > formData then the unparsed request body elements are contained in this > array. Data elements appear here as ArrayBuffer, file elements as > strings containing the filename." > On 2012/08/16 19:18:03, battre wrote: > >> I think filenames are base64 encoded, right? I'd rather have them as >> > plain text. > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/test/data/extensions/**api_test/webrequest/test_post.**js<http://codereview.chromium.org/10694055/diff/56038/chrome/test/data/extensions/api_test/webrequest/test_post.js> > File chrome/test/data/extensions/**api_test/webrequest/test_post.**js > (right): > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/test/data/extensions/**api_test/webrequest/test_post.**js#newcode7<http://codereview.chromium.org/10694055/diff/56038/chrome/test/data/extensions/api_test/webrequest/test_post.js#newcode7> > chrome/test/data/extensions/**api_test/webrequest/test_post.**js:7: > function > sendPost(formFile, ParseableForm) { > On 2012/08/16 19:18:03, battre wrote: > >> nit: parseableForm should start with a lower case letter >> > > Done. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/test/data/extensions/**api_test/webrequest/test_post.**js#newcode94<http://codereview.chromium.org/10694055/diff/56038/chrome/test/data/extensions/api_test/webrequest/test_post.js#newcode94> > chrome/test/data/extensions/**api_test/webrequest/test_post.**js:94: raw: > [ > { byteLength: 127 } ] > On 2012/08/16 19:18:03, battre wrote: > >> why does this contain only the byteLength? I thought you pass this as >> > an array? > > I pass it as ArrayBuffer. I don't think its value can be represented in > JSON, even in Javascript you need adaptor classes to view it, which > specify the number of bytes per elements. The byteLength property is > however visible, and while not allowing to test byte-equality, it at > least tests that the ArrayBuffer is there. Also, the byte-equality at > least on the C++ side is tested in the unit-tests. > > > http://codereview.chromium.**org/10694055/diff/56038/** > chrome/test/data/extensions/**api_test/webrequest/test_post.** > js#newcode116<http://codereview.chromium.org/10694055/diff/56038/chrome/test/data/extensions/api_test/webrequest/test_post.js#newcode116> > chrome/test/data/extensions/**api_test/webrequest/test_post.**js:116: > navigateAndWait(getURL(dirName + formFile)); > On 2012/08/16 19:18:03, battre wrote: > >> this should be >> navigateAndWait(getURL(...), function() {close();}); such that close >> > is executed > >> when the navigation is completed. >> > > Done. > Thanks, I did not know that! > > http://codereview.chromium.**org/10694055/<http://codereview.chromium.org/106... >
Hi Kent, I am seeking somebody knowledgeable in how HTML forms are encoded on uploading to review my two parsers from this CL. Do you think you could take a look at the three files form_data_parser* ? (Other parts of this CL are in review/were reviewed by other people.) The two parsers will be used to parse the encoded HTML forms before making the data accessible to extensions. Hi Dominic, Thanks for your comments. I addressed them, please take another look. Apart of your comments I had to adjust the code where it touches UploadData, because in the meantime people had pulled UploadElement out of it. I like the new changes, they get rid of TYPE_BLOB and also allow forward declaration of UploadElement. Thanks to all, Vaclav http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/upload_data_presenter.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:55: net::HttpRequestHeaders::kTransferEncoding, &transfer_encoding)) On 2012/08/20 12:09:38, battre wrote: > On 2012/08/17 18:29:57, vabr (Chromium) wrote: > > On 2012/08/16 19:18:03, battre wrote: > > > nit: +4 indent > > > > 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. > > 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. ;-) :) 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. http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:100: } // TYPE_BLOB is silently ignored. On 2012/08/20 12:09:38, battre wrote: > On 2012/08/17 18:29:57, vabr (Chromium) wrote: > > On 2012/08/16 19:18:03, battre wrote: > > > 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? > > > > 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? > > Add a NOTREACHED()? Well, CL https://chromiumcodereview.appspot.com/10834289 solved this for me by removing TYPE_BLOB completely. :) http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc (right): http://codereview.chromium.org/10694055/diff/56038/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc:54: ListValue expected_list; On 2012/08/20 12:09:38, battre wrote: > On 2012/08/17 18:29:57, vabr (Chromium) wrote: > > On 2012/08/16 19:18:03, battre wrote: > > > How does the consumer of this class know that the first item is a bytes > array > > > and the second one is a file name? > > > > On the C++ level this is BinaryValue (bytes array) vs. StringValue (filename). > > In Javascript they can use "typeof". > > I think this should be discussed with the extensions team. We might use Strings > for other purposes than filenames in the future. On a second thought, I refined the structure of requestBody.raw elements. They now have properties "bytes" and "file", so any clash with future types is avoided. http://codereview.chromium.org/10694055/diff/78005/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): http://codereview.chromium.org/10694055/diff/78005/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:592: MessageLoop::current()->RunAllPending(); On 2012/08/20 12:09:39, battre wrote: > Please add comment that result of this operation goes into message list of > ipc_sender_. Done. http://codereview.chromium.org/10694055/diff/78005/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:604: &profile_, extension_id, kEventName + "/1"); On 2012/08/20 12:09:39, battre wrote: > Can you move this to the previous loop? Done. (Moved to the previous scope, not loop.) http://codereview.chromium.org/10694055/diff/78005/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:607: filter, extra_info_spec_empty, ipc_sender_factory.GetWeakPtr()); On 2012/08/20 12:09:39, battre wrote: > Can you point out that the difference to the previous loop is that you pass an > empty extra info spec I tried to clarify the comment just below "Part 2." to express this.
Ok. I'll take a look at them on Monday or Tuesday.
satorux might be interested in MIME parsing. http://codereview.chromium.org/10694055/diff/83001/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/form_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/83001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:70: // Other cases are unparseable, including when |content_type| is "text/plain". Why text/plain is not supported? http://codereview.chromium.org/10694055/diff/83001/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/form_data_parser.h (right): http://codereview.chromium.org/10694055/diff/83001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.h:90: class FormDataParserUrlEncoded : public FormDataParser { It seems this class is not referred by files other than form_data_parser.cc. If it is true, we can move the class definition to the top of form_data_parser.cc. http://codereview.chromium.org/10694055/diff/83001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.h:259: class FormDataParserMultipart : public FormDataParser { ditto.
Kent, Thank you very much. It would be great if you could take a look. I addressed your comments so far. Dominic (and possibly other reviewers interested in TYPE_CHUNKS), Because TYPE_CHUNKS and TYPE_BLOB have been removed recently (see http://codereview.chromium.org/10861036/ for chunks and https://chromiumcodereview.appspot.com/10834289/ for blobs), I adjusted this CL as well. It makes things easier, and did not bring too many changes. One change to highlight -- I don't think I currently need the requestBody.error, because the only error to be reported were chunks in the data. Would you prefer if I removed requestBody.error completely, or do you think it might be worth leaving there for future? Thanks, and looking forward for your comments. Vaclav http://codereview.chromium.org/10694055/diff/83001/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/form_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/83001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:70: // Other cases are unparseable, including when |content_type| is "text/plain". On 2012/08/24 14:26:50, Kent Tamura wrote: > Why text/plain is not supported? This encoding is ambiguous. Nice description from http://stackoverflow.com/questions/7628249/method-post-enctype-text-plain-are... has this example: <form method="post" enctype="text/plain" action="..."> <textarea name="input1">abc input2=def</textarea> <input name="input2" value="ghi" /> <input type="submit"> </form> results in input1=abc input2=def input2=ghi But the first "input2" has nothing to do with the field named "input2". http://codereview.chromium.org/10694055/diff/83001/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/form_data_parser.h (right): http://codereview.chromium.org/10694055/diff/83001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.h:90: class FormDataParserUrlEncoded : public FormDataParser { On 2012/08/24 14:26:50, Kent Tamura wrote: > It seems this class is not referred by files other than form_data_parser.cc. > If it is true, we can move the class definition to the top of > form_data_parser.cc. Done, thanks for spotting this. http://codereview.chromium.org/10694055/diff/83001/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.h:259: class FormDataParserMultipart : public FormDataParser { On 2012/08/24 14:26:50, Kent Tamura wrote: > ditto. Done. http://codereview.chromium.org/10694055/diff/83004/chrome/common/extensions/a... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/83004/chrome/common/extensions/a... chrome/common/extensions/api/web_request.json:131: "error": {"type": "string", "optional": true, "description": "Errors when obtaining request body data."}, It looks like this is not needed any more, the only error I reported so far were chunks, which now disappeared. Should I remove "error" completely, or leave it for the future?
http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/form_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:54: // encoding) and 822 (MIME-headers). Please do not refer to RFC 822, which was obsoleted by RFC 2822, which was obsoleted by RFC 5322, which is the latest standard. http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:187: // whole string with the dash--boundary bust be contained in the first source, bust -> must? http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:206: kStart = 0, See http://www.chromium.org/developers/coding-style > Though the Google C++ Style Guide says to use kConstantNaming for enums, Chrome still uses MACRO_STYLE naming for enums for consistency with our existing code. Also, It's hard to understand the difference between kCR1 and kCR. I prefer adding prefixes to them. e.g. STATE_CR TRANSITION_CR http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:276: const char* offset_; The name "offset_" is confusing. It's not an offset. It's a cursor, current-position, or something. http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:374: equality_signs_ == amp_signs_ + 1; Why do we need to check the number of = and & ? equality_signs_ and amp_signs_ make GetNextChar complex. http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:538: FormDataParserMultipart::State FormDataParserMultipart::kNextState[] = { kAvailableTransitions and kNextState should be merged into one. like: {kDashBoundary, kDB1}, {kCR, kCR1}, {kAny, kIgnorePreamble}, .... http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:598: } We had better have a function DoStepsUntil(State). It will improve code readability. if (!DoStepsUntil(kPart)) return false; http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:703: strlen(kContentDisposition) != 0)) strlen is not needed. The length of kContentDisposition is fixed. http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:712: field_offset += strlen(kNameEquals); ditto. http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:716: next_name_.set(header.data() + field_offset, field_end - field_offset); Need to decode the name value. BTW, what's the expected character encoding of next_name_ ? Will it be passed to an extension as JavaScript string? I remember we have no ways to detect character encoding of submitted form data. If a page containing the form is encoded in ISO-8859-1, names and values are encoded in ISO-8859-1, and if a page is encoded in UTF-8, names and values are encoded in UTF-8. http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:722: field_offset += strlen(kFilenameEquals); ditto.
Hi Kent, Thanks for your comments. I realised that the parser code is unnecessary low-level and over-complicated, and decided to using the re2 library (very recently added to chromium tree) to replace the byte-by-byte parsing and look-up tables. I admit this can be annoying, as I know you spent your time reading the previous version. My apologies for that. But I think that right now the code is much more clear and certainly shorter, so hopefully this will gain some of that time back. Your comments were mostly applicable even after the rewrite. Please take a look again, in particular on my answer to your comment about encoding the name of a form field, and on the new parser code in general. Thanks for you patience, Vaclav Hi Dominic, There were a few changes outside of the parser code triggered by Kent's comments. Most of them straightforward, but you may want to check out the sentence about UTF8 added to the JSON documentation. Thanks, Vaclav http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/form_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:54: // encoding) and 822 (MIME-headers). On 2012/08/27 07:09:17, Kent Tamura wrote: > Please do not refer to RFC 822, which was obsoleted by RFC 2822, which was > obsoleted by RFC 5322, which is the latest standard. Done. Thanks for making me aware of this. http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:187: // whole string with the dash--boundary bust be contained in the first source, On 2012/08/27 07:09:17, Kent Tamura wrote: > bust -> must? Rewritten in the meantime. http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:206: kStart = 0, On 2012/08/27 07:09:17, Kent Tamura wrote: > See http://www.chromium.org/developers/coding-style > > Though the Google C++ Style Guide says to use kConstantNaming for enums, > Chrome still uses MACRO_STYLE naming for enums for consistency with our existing > code. > > Also, It's hard to understand the difference between kCR1 and kCR. I prefer > adding prefixes to them. e.g. STATE_CR TRANSITION_CR Added STATE_ prefix to states, transitions disappeared in the meantime. http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:276: const char* offset_; On 2012/08/27 07:09:17, Kent Tamura wrote: > The name "offset_" is confusing. It's not an offset. It's a cursor, > current-position, or something. You're right. This disappeared after rewriting. http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:374: equality_signs_ == amp_signs_ + 1; On 2012/08/27 07:09:17, Kent Tamura wrote: > Why do we need to check the number of = and & ? > equality_signs_ and amp_signs_ make GetNextChar complex. Now the parser uses a regexp which eliminates such bookkeeping. http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:538: FormDataParserMultipart::State FormDataParserMultipart::kNextState[] = { On 2012/08/27 07:09:17, Kent Tamura wrote: > kAvailableTransitions and kNextState should be merged into one. > like: > > {kDashBoundary, kDB1}, {kCR, kCR1}, {kAny, kIgnorePreamble}, > .... Disappeared after rewriting. http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:598: } On 2012/08/27 07:09:17, Kent Tamura wrote: > We had better have a function DoStepsUntil(State). It will improve code > readability. > > if (!DoStepsUntil(kPart)) > return false; > > Disappeared after the rewrite. http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:703: strlen(kContentDisposition) != 0)) On 2012/08/27 07:09:17, Kent Tamura wrote: > strlen is not needed. The length of kContentDisposition is fixed. Correct. Although this particular instance and those pointed below disappeared, I will re-check the whole CL, as there are many instances of this. http://codereview.chromium.org/10694055/diff/83004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:716: next_name_.set(header.data() + field_offset, field_end - field_offset); Thanks very much for bringing this up! On 2012/08/27 07:09:17, Kent Tamura wrote: > I remember we have no ways to detect character encoding of submitted form data. > If a page containing the form is encoded in ISO-8859-1, names and values are > encoded in ISO-8859-1, and if a page is encoded in UTF-8, names and values are > encoded in UTF-8. I am not sure what you mean by "data". The text present in the HTML file itself, i.e., the field names and default values, are, as far as I saw, uploaded without any change. If the HTML file is written in UTF8, the names and default values are uploaded in UTF8, even if the HTML page itself specifies a different content-type encoding via its meta tag "http-equiv". The values entered into the form by the user, on the other hand, seem to follow the declared content-type encoding of the page, not the encoding of the HTML file. > BTW, what's the expected character encoding of next_name_ ? > Will it be passed to an extension as JavaScript string? Yes, it will be passed to the extensions byte-per-byte as a JavaScript string (StringValue). > Need to decode the name value. If both the page encoding and the HTML file are UTF8 then there is no need to decode the name value, it will safely reach the extension. If it is not in UTF8 then parsing will fail, because re2 assumes UTF8 data, and also StringValue checks on construction whether the string is a valid UTF8 string (IsStringUTF8()). In that case the form will not be parsed and the extension gets the unparsed upload data via a different part of code from this CL. If there is no way to detect the character encoding, then I don't see how I could support the non-UTF8 cases. Speaking with battre@, we consider it acceptable to have form parsing only for UTF8 websites. Based on that, I suggest doing no decoding here. What do you think?
> > Need to decode the name value. > > If both the page encoding and the HTML file are UTF8 then there is no need to > decode the name value, it will safely reach the extension. If it is not in UTF8 > then parsing will fail, because re2 assumes UTF8 data, and also StringValue > checks on construction whether the string is a valid UTF8 string > (IsStringUTF8()). In that case the form will not be parsed and the extension > gets the unparsed upload data via a different part of code from this CL. > > If there is no way to detect the character encoding, then I don't see how I > could support the non-UTF8 cases. Speaking with battre@, we consider it > acceptable to have form parsing only for UTF8 websites. Based on that, I suggest > doing no decoding here. What do you think? Ok, Supporting only UTF-8 might be reasonable. If we have many requests to support non-UTF8, we should change WebKit so that it expose raw name-value pairs before constructing multipart messages. We should have tests for names with special characters. e.g. <input type=text name="foo"bar 
" value=value>
http://codereview.chromium.org/10694055/diff/96004/chrome/browser/extensions/... File chrome/browser/extensions/api/web_request/form_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/96004/chrome/browser/extensions/... chrome/browser/extensions/api/web_request/form_data_parser.cc:309: static const RE2 unquote_pattern(escape_closing_quote); Note to myself -- make this a non-static data member. (This instance slipped through during the recent removal of static data-members with non-trivial destructors.)
Warning: do not trust the try job results on patchset 46: many are green but haven't been run. Sorry it's an error on my part.
Kent, I added more tests for non-ASCII characters in field names (and values). At that point I realised that although non-ASCII characters do not get escaped, some ASCII characters do, so I added unescaping, as you suggested. The relevant code is flagged by my comments. I also made a RE2 type variable |unquote_pattern| non-static, and added comment why. How does the unescaping and the rest of the parser code look to you? Marc-Antoine, Thanks for the heads-up. Patchset 46 is not going to be the last one, so I will check the try results for sets after that. Thanks, Vaclav http://codereview.chromium.org/10694055/diff/107001/chrome/browser/extensions... File chrome/browser/extensions/api/web_request/form_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/107001/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:429: result->set_name(unescaped_name); Here is the added name-unescaping. http://codereview.chromium.org/10694055/diff/107001/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/webrequest/test_post.js (right): http://codereview.chromium.org/10694055/diff/107001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/webrequest/test_post.js:14: "text\"1\u011B\u0161\u00FD\u4EBA\r\n \r\n": ["TEST_TEXT_1"], Added control and non-ASCII characters to test against names. Also below for values. Corresponding change in test data is in requestBody/*.html .
lgtm http://codereview.chromium.org/10694055/diff/106003/chrome/browser/extensions... File chrome/browser/extensions/api/web_request/form_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/106003/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:306: std::string FormDataParserMultipart::GetDashBoundaryPattern( nit: The function name doesn't represent what it does. It should be named as "EscapeUnquote", "MakePatternFromLiteral" or something. http://codereview.chromium.org/10694055/diff/106003/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:365: bool FormDataParserMultipart::GetNextNameValueContinue( nit: Again, the function name isn't good. The name should be "GetDataOfCurrentPartAndSkipUntilNextBoudnary()" or something.
Dominic, Now that the parsers are reviewed (thanks Kent!), each part of this CL has been looked at. To complete, could you please do the following? 1. Review the use of LazyInstance I introduced into the parsers following our off-line conversation. 2. Give a final look to chrome/common/extensions/api/web_request.json and check that the API makes sense to you? Once you are satisfied with the CL, I will sort-out rubber-stamps for *.gypi changes, send this to CQ and put some the testing info on the bug page. Thanks! Vaclav http://codereview.chromium.org/10694055/diff/106003/chrome/browser/extensions... File chrome/browser/extensions/api/web_request/form_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/106003/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:306: std::string FormDataParserMultipart::GetDashBoundaryPattern( On 2012/09/04 07:43:53, Kent Tamura wrote: > nit: > The function name doesn't represent what it does. > It should be named as "EscapeUnquote", "MakePatternFromLiteral" or something. Good point. I chose "GetBoundaryPatternFromLiteral" because the function makes one boundary-specific thing: it prepends "--". http://codereview.chromium.org/10694055/diff/106003/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:365: bool FormDataParserMultipart::GetNextNameValueContinue( On 2012/09/04 07:43:53, Kent Tamura wrote: > nit: > Again, the function name isn't good. > The name should be "GetDataOfCurrentPartAndSkipUntilNextBoudnary()" or > something. Changed to "FinishReadingPart", and the argument name from |value| to |data|, to make it clear that both: 1) the body part is read until its end, and 2) the output parameter should contain data, without making the name painfully long. Comment at the declaration also edited to explain properly what the method does.
small nit http://codereview.chromium.org/10694055/diff/107004/chrome/common/extensions/... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/107004/chrome/common/extensions/... chrome/common/extensions/api/web_request.json:96: "type": "any", indent should be +2 not +4
Dominic, This should be short -- based on your suggestion I changed the LazyInstance to a Leaky one. Valgrind tests on Linux pass. How does it look to you? The spacing nits from Matt (thanks!) and you are now corrected. I don't think there is anything else left which has not been looked at and approved. Only the *.gypi change for the added files, but it is straightforward, so I will TBR it. Vaclav http://codereview.chromium.org/10694055/diff/107004/chrome/common/extensions/... File chrome/common/extensions/api/web_request.json (right): http://codereview.chromium.org/10694055/diff/107004/chrome/common/extensions/... chrome/common/extensions/api/web_request.json:96: "type": "any", On 2012/09/05 21:01:55, Matt Perry wrote: > indent should be +2 not +4 Done.
Sorry, a couple of more comments. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... File chrome/browser/extensions/api/web_request/form_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:35: const RE2 transfer_padding_pattern_; no _ at end for structs http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:86: const RE2& pattern() { const http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:87: return g_patterns.Get().url_encoded_pattern_; g_patterns.Get() is relatively expensive. How about you add a variable Patterns* patterns_; to the class and initialize patterns_(g_patterns.Get()) in the constructor? then this function becomes const RE2& pattern() const { return patterns_->url_encoded_pattern; } http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:90: static const size_t args_size_ = 2u; // Auxiliary constant for using RE2. Can you describe what this represents? "Number of arguments $WHAT_THESE_ARGUMENTS_ARE". http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:199: static bool LookAhead(const RE2& pattern, const re2::StringPiece& input); How about naming this "StartsWithPattern" and reverting the parameter order? http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:202: // Content-Disposition, it also extracts |name| from "name=" and possibly what does "it" refer to? From a grammatical POV, it refers to the header, but I think you mean this function. Also below. You can work around this with "we" (If ..., we also extract ...") http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:205: // consumed a header, false if it did not. Sets |value_assigned| to true if it nit: remove ", false if it did not." http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:212: // portion of a body part. It then attempts to read the input until the end of nit: Please get rid of "It". Sentences like "it sets it" are imo very difficult to read. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:375: std::string FormDataParserMultipart::GetBoundaryPatternFromLiteral( nit: I suggest to name functions that don't fetch something but create something "Create..."? http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:376: const std::string& literal) { can you explain what this function does? May be using examples? I don't understand what happens from looking at the code. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:465: } move this into TryReadHeader? There is a code path in TryReadHeader that brings state_ to STATE_ERROR but you continue execution here anyway. Is this intentional or would you want to check state_ here? http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:476: if (value_assigned && source_.size() == 0) // Wait for a new source? I think this becomes clearer if return_value is set in both branches http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:538: if (LookAhead(content_disposition_pattern(), header)) { Inverse the logic here? if (!LookAhead(...)) return true; // Skip headers that don't describe the content-disposition. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:551: RE2::UNANCHORED, groups, 2)) please add {} http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:552: return true; // See (*) for why true. why the asymmetry? Don't you want to set state_ to STATE_ERROR? http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... File chrome/browser/extensions/api/web_request/form_data_parser_unittest.cc (right): http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser_unittest.cc:19: const std::vector<const base::StringPiece* >& bytes, nit: - space before > http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser_unittest.cc:47: virtual void SetUp() OVERRIDE {} please remove empty SetUp methods http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser_unittest.cc:48: }; Please remove the entire class and replace TEST_F(WebRequestFormDataParserTest below with TEST(WebRequestFormDataParserTest the _F indicates a test fixture, which you don't need if the base class is empty. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... File chrome/browser/extensions/api/web_request/upload_data_presenter.cc (right): http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:143: return scoped_ptr<Value>(); Style guide says not to have return in else branch. I would do it as in line 73 (if (!success_) first) http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... File chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc (right): http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc:57: scoped_ptr<Value> result = raw_presenter.Result(); ASSERT_TRUE(result.get()); http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/web_request_api.cc:92: // chrome::VersionInfo. Don't use this outside IO thread. where is this caching? I don't understand why we cannot use it outside the IO thread. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/web_request_api.cc:186: // Get the data presenters, ordered by being interesting. "ordered by being interesting"? grammar and what does that mean? by specificity? http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/web_request_api.cc:188: extensions::ParsedDataPresenter parsed_data_presenter(request); nit: swap previous two lines?
Dominic, Thanks for your never-ending enthusiasm for reading this monster! I really appreciate your comments. Most of them I implemented straightforwardly, but for some of them we might need to talk (I'm free to talk offline whenever it suits you). The latter ones are tagged by "DISCUSS" to help you filter them. Thanks, Vaclav http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... File chrome/browser/extensions/api/web_request/form_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:35: const RE2 transfer_padding_pattern_; On 2012/09/09 22:08:35, battre wrote: > no _ at end for structs Done. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:86: const RE2& pattern() { On 2012/09/09 22:08:35, battre wrote: > const Thanks for pointing that out. I made that a static method instead. It does not only leave the instance unmodified, it does not even read it. (DISCUSS) http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:87: return g_patterns.Get().url_encoded_pattern_; On 2012/09/09 22:08:35, battre wrote: > g_patterns.Get() is relatively expensive. How about you add a variable > Patterns* patterns_; to the class and initialize patterns_(g_patterns.Get()) in > the constructor? then this function becomes > const RE2& pattern() const { > return patterns_->url_encoded_pattern; > } I added the caching. Because I would like to leave the RE2 accessors static, and they are used in two classes, I decided to make the pointer with the cached value as a global. To handle the initialization on the first use, I wrapped it in a class PatternsCache. No instance of that class is ever constructed. The only static global variable is the static pointer PatternsCache::patterns_. (DISCUSS) http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:90: static const size_t args_size_ = 2u; // Auxiliary constant for using RE2. On 2012/09/09 22:08:35, battre wrote: > Can you describe what this represents? "Number of arguments > $WHAT_THESE_ARGUMENTS_ARE". Done. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:199: static bool LookAhead(const RE2& pattern, const re2::StringPiece& input); On 2012/09/09 22:08:35, battre wrote: > How about naming this "StartsWithPattern" and reverting the parameter order? Well, if you insist, I'll do it. I prefer LookAhead because it is 8 characters shorter. In my opinion LookAhead should be clear, as it is a term from parsing theory. The argument names and types should also avoid confusion in their order. (DISCUSS) http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:202: // Content-Disposition, it also extracts |name| from "name=" and possibly On 2012/09/09 22:08:35, battre wrote: > what does "it" refer to? From a grammatical POV, it refers to the header, but I > think you mean this function. Also below. You can work around this with "we" (If > ..., we also extract ...") It indeed refers to the header. I reworded the comment. On some occasions there is still and (implicit) it referring to the function, such as "Returns true", but that should be usual. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:205: // consumed a header, false if it did not. Sets |value_assigned| to true if it On 2012/09/09 22:08:35, battre wrote: > nit: remove ", false if it did not." Done, I replaced those tails with using "iff", so that information value is not dropped. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:212: // portion of a body part. It then attempts to read the input until the end of On 2012/09/09 22:08:35, battre wrote: > nit: Please get rid of "It". Sentences like "it sets it" are imo very difficult > to read. Done. Thanks for pointing that horrible sentence out! http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:375: std::string FormDataParserMultipart::GetBoundaryPatternFromLiteral( On 2012/09/09 22:08:35, battre wrote: > nit: I suggest to name functions that don't fetch something but create something > "Create..."? Done. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:376: const std::string& literal) { On 2012/09/09 22:08:35, battre wrote: > can you explain what this function does? May be using examples? I don't > understand what happens from looking at the code. I expanded the description at the declaration of that function (example included), and tried to make the definition also more readable. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:465: } On 2012/09/09 22:08:35, battre wrote: > move this into TryReadHeader? No, I need to check name.size() == 0 after all TryReadHeader calls were made, not after each one of them. > There is a code path in TryReadHeader that brings state_ to STATE_ERROR but you > continue execution here anyway. Is this intentional or would you want to check > state_ here? Thanks, check added. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:476: if (value_assigned && source_.size() == 0) // Wait for a new source? On 2012/09/09 22:08:35, battre wrote: > I think this becomes clearer if return_value is set in both branches Done. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:538: if (LookAhead(content_disposition_pattern(), header)) { On 2012/09/09 22:08:35, battre wrote: > Inverse the logic here? > if (!LookAhead(...)) > return true; // Skip headers that don't describe the content-disposition. Done, thanks! http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:551: RE2::UNANCHORED, groups, 2)) On 2012/09/09 22:08:35, battre wrote: > please add {} Done. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:552: return true; // See (*) for why true. On 2012/09/09 22:08:35, battre wrote: > why the asymmetry? Don't you want to set state_ to STATE_ERROR? No, it is not an error if the header does not specify "filename" (this is what I feed value with). In fact, for byte-uploads, this is never set. Having no "name" field, on the other hand, is against the input specification. I inverted the second condition to make it look more differently from the previous one, and (by moving the "return" out) to emphasize, that unlike finding no "name", finding no "filename" is not fatal for this function. (DISCUSS)(?) http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... File chrome/browser/extensions/api/web_request/form_data_parser_unittest.cc (right): http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser_unittest.cc:19: const std::vector<const base::StringPiece* >& bytes, On 2012/09/09 22:08:35, battre wrote: > nit: - space before > Done. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser_unittest.cc:47: virtual void SetUp() OVERRIDE {} On 2012/09/09 22:08:35, battre wrote: > please remove empty SetUp methods Done. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser_unittest.cc:48: }; On 2012/09/09 22:08:35, battre wrote: > Please remove the entire class and replace > TEST_F(WebRequestFormDataParserTest > below with > TEST(WebRequestFormDataParserTest > > the _F indicates a test fixture, which you don't need if the base class is > empty. Done. Sorry about that, I remember you had to point that out to me at the presenter unit-tests, and I forgot to update this as well. Hopefully, all new tests I will write will prefer TEST to TEST_F when possible. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... File chrome/browser/extensions/api/web_request/upload_data_presenter.cc (right): http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/upload_data_presenter.cc:143: return scoped_ptr<Value>(); On 2012/09/09 22:08:35, battre wrote: > Style guide says not to have return in else branch. I would do it as in line 73 > (if (!success_) first) Done. Thanks for pointing me to the style guide here, I did not notice that before. (Actually, the style guide says "no else after return", not "no return in else branch". It still applies to this case, though.) http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... File chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc (right): http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc:57: scoped_ptr<Value> result = raw_presenter.Result(); On 2012/09/09 22:08:35, battre wrote: > ASSERT_TRUE(result.get()); Done. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... File chrome/browser/extensions/api/web_request/web_request_api.cc (right): http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/web_request_api.cc:92: // chrome::VersionInfo. Don't use this outside IO thread. On 2012/09/09 22:08:35, battre wrote: > where is this caching? I don't understand why we cannot use it outside the IO > thread. Thanks for catching that. The comment was obsolete, from before I switched to using Feature. I removed the no longer applicable part. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/web_request_api.cc:186: // Get the data presenters, ordered by being interesting. On 2012/09/09 22:08:35, battre wrote: > "ordered by being interesting"? grammar and what does that mean? by specificity? I refined that comment, and also corrected the two in-line comments inside the definition of presenters[]. http://codereview.chromium.org/10694055/diff/108008/chrome/browser/extensions... chrome/browser/extensions/api/web_request/web_request_api.cc:188: extensions::ParsedDataPresenter parsed_data_presenter(request); On 2012/09/09 22:08:35, battre wrote: > nit: swap previous two lines? Done.
Dominic, Thanks for clarifying the rest comments with me. I removed the static class around the cached Patterns*, and changed most(*) of the accessors from static to const. I also renamed LookAhead as required. PTAL. Vaclav (*) The only exception is FormDataParserMultipart::unquote_pattern(), comment should explain it there. The lack of caching should not be a big problem, as that is only called once per each instance.
LGTM, thanks for your patience. http://codereview.chromium.org/10694055/diff/105032/chrome/browser/extensions... File chrome/browser/extensions/api/web_request/form_data_parser.cc (right): http://codereview.chromium.org/10694055/diff/105032/chrome/browser/extensions... chrome/browser/extensions/api/web_request/form_data_parser.cc:278: // Caching the pointer to g_patterns.Get() . nit: - space before .
Thanks Dominic, (and all other brave reviewers of this CL! *) Nit addressed, sending to the CQ. jhawkins@ in TBR for chrome/*.gypi, where I only added the new files for WebRequest. Thanks! Vaclav * I promise I will try hard to split my future work into nice small CLs. :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/10694055/120002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/10694055/120002
Retried try job too often for step(s)
On 2012/09/13 11:00:22, I haz the power (commit-bot) wrote: > Retried try job too often for step(s) I'll try to add CRLF support soon.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/10694055/108014
On 2012/09/13 12:48:26, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/vabr%40chromium.org/10694055/108014 To explain the last patch-set: git cl upload falsely detected submit.js as a fork of some unrelated JS file. So I uploaded a patch generated from the same code, but with copy-detection disabled. (Following maruel's pointer to http://code.google.com/p/chromium/issues/detail?id=125983 this can currently be achieved by adjusting upload.py in depot_tools). Vaclav
Change committed as 156547 |
