Fix UnindexedRulesetWriter finishing.

components/subresource_filter/core/common/unindexed_ruleset_unittest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/subresource_filter/core/common/unindexed_ruleset.h"
 
 #include <memory>
 #include <string>
 #include <vector>
 
@@ skipping 100 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(UnindexedRulesetTestBuilder);
 };
 
 bool IsRulesetValid(const std::string& ruleset_contents,
                     const std::vector<proto::UrlRule>& expected_url_rules) {
   google::protobuf::io::ArrayInputStream array_input(ruleset_contents.data(),
                                                      ruleset_contents.size());
   UnindexedRulesetReader reader(&array_input);
   proto::FilteringRules chunk;
   std::vector<proto::UrlRule> read_rules;
   while (reader.ReadNextChunk(&chunk)) {

[engedy, 2016/08/26 15:43:17]

The test below only ensures that either of these statements is true:
  -- both Finish and the dtor call Trim, or
  -- neither calls Trim.

The ensure the former, could you please expose CodedInputStream::CurrentPosition
as UnindexedRulesetReader::GetBytesRead or num_bytes_read (whichever you
prefer), and ensure that it is equal to ruleset_contents.size()?

And once we have that, could you please update the check in
RulesetService::IndexRuleset to use that?

[pkalinnikov1, 2016/08/26 16:29:47]

Done.

     read_rules.insert(read_rules.end(), chunk.url_rules().begin(),
                       chunk.url_rules().end());
   }
 
   if (expected_url_rules.size() != read_rules.size())
     return false;
   for (size_t i = 0, size = read_rules.size(); i != size; ++i) {
     if (!IsEqual(expected_url_rules[i], read_rules[i]))
       return false;
   }
   return true;
 }
 
 }  // namespace
 
 TEST(UnindexedRulesetTest, EmptyRuleset) {
   UnindexedRulesetTestBuilder builder;
   EXPECT_TRUE(builder.Finish());
   EXPECT_TRUE(IsRulesetValid(builder.ruleset_contents(), builder.url_rules()));
 }
 
+// The following test addresses a bug, that the UnindexedRulesetWriter didn't

[engedy, 2016/08/26 15:43:17]

Let's just call a spade a spade + phrasing suggestion.

// Regression test for a bug where the ... resulting in the output having ...

[pkalinnikov1, 2016/08/26 16:29:47]

Done.

+// trim the output coded stream on Finish, causing the output had some extra
+// uninitialized bytes at the end.
+TEST(UnindexedRulesetTest, EmptyRulesetRegression) {
+  size_t contents_size_after_finish = 0;
+  std::string contents;
+
+  {
+    google::protobuf::io::StringOutputStream output(&contents);
+    UnindexedRulesetWriter ruleset_writer(&output);
+    EXPECT_TRUE(ruleset_writer.Finish());

[engedy, 2016/08/26 15:43:17]

Hang on, is this going to allocate a single byte of buffer if we don't write
anything?

[pkalinnikov1, 2016/08/26 16:29:47]

No, the size of the contents will be 0. I could add EXPECT_EQ(0, size), but this
would make the test depend on the wire format. E.g., if we add a fixed header at
the beginning of UnindexedRuleset, the size will become sizeof(Header). Should I
leave this code as is?

+    contents_size_after_finish = contents.size();
+  }
+
+  // The |contents| shouldn't be resized after writer/stream destruction.
+  EXPECT_EQ(contents_size_after_finish, contents.size());
+}
+
 TEST(UnindexedRulesetTest, OneUrlRule) {
   UnindexedRulesetTestBuilder builder;
   EXPECT_TRUE(builder.AddUrlRule(UrlPattern("example.com"),
                                  proto::SOURCE_TYPE_THIRD_PARTY, false));
   EXPECT_TRUE(builder.Finish());
   EXPECT_TRUE(IsRulesetValid(builder.ruleset_contents(), builder.url_rules()));
 }
 
 TEST(UnindexedRulesetTest, ManyUrlRules) {
   UnindexedRulesetTestBuilder builder;
@@ skipping 36 matching lines @@
 
   {
     std::string ruleset_contents = builder.ruleset_contents();
     ASSERT_GT(ruleset_contents.size(), static_cast<size_t>(100));
     ruleset_contents.resize(100);
     EXPECT_FALSE(IsRulesetValid(ruleset_contents, builder.url_rules()));
   }
 }
 
 }  // namespace subresource_filter