Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2279803002:
Fix UnindexedRulesetWriter finishing. (Closed)
|
Created:
4 years, 3 months ago by pkalinnikov Modified:
4 years, 3 months ago Reviewers:
engedy CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix UnindexedRulesetWriter finishing.
This CL fixes the bug where UnindexedRulesetWriter doesn't trim the
CodedOutputStream on Finish, resulting in the output having some extra
uninitialized bytes at the end.
BUG=609747
Committed: https://crrev.com/1ffab702f55fb26416fd782e0f86f692db39f346
Cr-Commit-Position: refs/heads/master@{#414762}
Patch Set 1 #Patch Set 2 : Add regression test. #
Total comments: 8
Patch Set 3 : Address comments of engedy@ #Patch Set 4 : Remove redundant test. #
Messages
Total messages: 26 (15 generated)
Description was changed from ========== Fixed UnindexedRulesetWriter finishing. BUG=609747 ========== to ========== Fixed UnindexedRulesetWriter finishing. BUG=609747 ==========
pkalinnikov@chromium.org changed reviewers: + engedy@chromium.org
PTAL.
Looks good, but can I haz regression tests plz?
Sure, here you are.
LGTM % comments, thanks! https://codereview.chromium.org/2279803002/diff/20001/components/subresource_... File components/subresource_filter/core/common/unindexed_ruleset.cc (right): https://codereview.chromium.org/2279803002/diff/20001/components/subresource_... components/subresource_filter/core/common/unindexed_ruleset.cc:54: const bool success = !pending_chunk_.url_rules_size() || WritePendingChunk(); nit: Same here as above, feel free to return false without calling Trim(). https://codereview.chromium.org/2279803002/diff/20001/components/subresource_... File components/subresource_filter/core/common/unindexed_ruleset_unittest.cc (right): https://codereview.chromium.org/2279803002/diff/20001/components/subresource_... components/subresource_filter/core/common/unindexed_ruleset_unittest.cc:121: while (reader.ReadNextChunk(&chunk)) { 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? https://codereview.chromium.org/2279803002/diff/20001/components/subresource_... components/subresource_filter/core/common/unindexed_ruleset_unittest.cc:143: // The following test addresses a bug, that the UnindexedRulesetWriter didn't Let's just call a spade a spade + phrasing suggestion. // Regression test for a bug where the ... resulting in the output having ... https://codereview.chromium.org/2279803002/diff/20001/components/subresource_... components/subresource_filter/core/common/unindexed_ruleset_unittest.cc:153: EXPECT_TRUE(ruleset_writer.Finish()); Hang on, is this going to allocate a single byte of buffer if we don't write anything?
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pkalinnikov@google.com changed reviewers: + pkalinnikov@google.com
Done. One question. https://codereview.chromium.org/2279803002/diff/20001/components/subresource_... File components/subresource_filter/core/common/unindexed_ruleset.cc (right): https://codereview.chromium.org/2279803002/diff/20001/components/subresource_... components/subresource_filter/core/common/unindexed_ruleset.cc:54: const bool success = !pending_chunk_.url_rules_size() || WritePendingChunk(); On 2016/08/26 15:43:17, engedy wrote: > nit: Same here as above, feel free to return false without calling Trim(). Done. https://codereview.chromium.org/2279803002/diff/20001/components/subresource_... File components/subresource_filter/core/common/unindexed_ruleset_unittest.cc (right): https://codereview.chromium.org/2279803002/diff/20001/components/subresource_... components/subresource_filter/core/common/unindexed_ruleset_unittest.cc:121: while (reader.ReadNextChunk(&chunk)) { On 2016/08/26 15:43:17, engedy wrote: > 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? Done. https://codereview.chromium.org/2279803002/diff/20001/components/subresource_... components/subresource_filter/core/common/unindexed_ruleset_unittest.cc:143: // The following test addresses a bug, that the UnindexedRulesetWriter didn't On 2016/08/26 15:43:17, engedy wrote: > Let's just call a spade a spade + phrasing suggestion. > > // Regression test for a bug where the ... resulting in the output having ... Done. https://codereview.chromium.org/2279803002/diff/20001/components/subresource_... components/subresource_filter/core/common/unindexed_ruleset_unittest.cc:153: EXPECT_TRUE(ruleset_writer.Finish()); On 2016/08/26 15:43:17, engedy wrote: > Hang on, is this going to allocate a single byte of buffer if we don't write > anything? 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?
Removed the test, because it is covered by the introduced "num_of_bytes" check.
pkalinnikov@google.com changed reviewers: - pkalinnikov@google.com
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
ACK, still LGTM.
On 2016/08/26 16:39:21, engedy wrote: > ACK, still LGTM. Could you please describe in a sentence in the CL description what the issue was?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fixed UnindexedRulesetWriter finishing. BUG=609747 ========== to ========== Fixed UnindexedRulesetWriter finishing. This CL fixes the bug where UnindexedRulesetWriter doesn't trim the CodedOutputStream on Finish, resulting in the output having some extra uninitialized bytes at the end. BUG=609747 ==========
Description was changed from ========== Fixed UnindexedRulesetWriter finishing. This CL fixes the bug where UnindexedRulesetWriter doesn't trim the CodedOutputStream on Finish, resulting in the output having some extra uninitialized bytes at the end. BUG=609747 ========== to ========== Fix UnindexedRulesetWriter finishing. This CL fixes the bug where UnindexedRulesetWriter doesn't trim the CodedOutputStream on Finish, resulting in the output having some extra uninitialized bytes at the end. BUG=609747 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pkalinnikov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix UnindexedRulesetWriter finishing. This CL fixes the bug where UnindexedRulesetWriter doesn't trim the CodedOutputStream on Finish, resulting in the output having some extra uninitialized bytes at the end. BUG=609747 ========== to ========== Fix UnindexedRulesetWriter finishing. This CL fixes the bug where UnindexedRulesetWriter doesn't trim the CodedOutputStream on Finish, resulting in the output having some extra uninitialized bytes at the end. BUG=609747 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix UnindexedRulesetWriter finishing. This CL fixes the bug where UnindexedRulesetWriter doesn't trim the CodedOutputStream on Finish, resulting in the output having some extra uninitialized bytes at the end. BUG=609747 ========== to ========== Fix UnindexedRulesetWriter finishing. This CL fixes the bug where UnindexedRulesetWriter doesn't trim the CodedOutputStream on Finish, resulting in the output having some extra uninitialized bytes at the end. BUG=609747 Committed: https://crrev.com/1ffab702f55fb26416fd782e0f86f692db39f346 Cr-Commit-Position: refs/heads/master@{#414762} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1ffab702f55fb26416fd782e0f86f692db39f346 Cr-Commit-Position: refs/heads/master@{#414762} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
