Framework for indexing of subresource filtering rules.

Framework for indexing of subresource filtering rules.

Make the RulesetService capable of receiving and indexing Safe Browsing
subresource filtering rules coming in through the component updater. The wire
format is still TBD, but for now consists of a single rule.

Also, change the directory structure so that both indexed and unindexed rules
are stored under a BASE directory at "$USER_DATA_DIR/Subresource Filter/".
Unindexed rules are now stored under "$BASE/Unindexed Rules/$CONTENT_VERSION/";
and indexed rules under "$BASE/Indexed Rules/$FORMAT_VERSION/$CONTENT_VERSION",
instead of "$BASE/$FORMAT_VERSION_$CONTENT_VERSION".

Furthermore, refactor TestRulesetCreator to facilitate testing by supplying
both the indexed/unindexed representation of the testing rules, and making them
available as files, file paths or blobs.

BUG=609747
Committed: https://crrev.com/6cfa34f00f9631c99f8540188e9a5aab7502eebe
Cr-Commit-Position: refs/heads/master@{#408615}

[engedy, 2016-07-28 11:41:21]

Description was changed from

==========
Implement indexing of subresource filtering rules.

Make the RulesetService capable of receiving and indexing Safe Browsing
subresource filtering rules coming in through the component updater.

Also, change the directory structure so that both indexed and unindexed rules
are stored under $USER_DATA_DIR/Subresource Filtering Rules/, and store indexed
rulesets under "$BASE_DIR/Indexed/$FORMAT_VERSION/$CONTENT_VERSION" instead of
"$BASE_DIR/$FORMAT_VERSION_$CONTENT_VERSION".

Furthermore, refactor TestRulesetCreator to facilitate tests with either the
indexed/unindexed representation of the rules.

BUG=609747
==========

to

==========
Implement indexing of subresource filtering rules.

Make the RulesetService capable of receiving and indexing Safe Browsing
subresource filtering rules coming in through the component updater.

Also, change the directory structure so that both indexed and unindexed rules
are stored under the BASE_DIR at "$USER_DATA_DIR/Subresource Filtering Rules/".
Unindexed rulesets are now stored under "$BASE_DIR/Unindexed/$CONTENT_VERSION/";
and indexed rulesets under "$BASE_DIR/Indexed/$FORMAT_VERSION/$CONTENT_VERSION",
instead of "$BASE_DIR/$FORMAT_VERSION_$CONTENT_VERSION".

Furthermore, refactor TestRulesetCreator to facilitate tests with either the
indexed/unindexed representation of the rules.

BUG=609747
==========

[engedy, 2016-07-28 13:33:56]

Patchset #1 (id:1) has been deleted

[engedy, 2016-07-28 13:38:20]

Description was changed from

==========
Implement indexing of subresource filtering rules.

Make the RulesetService capable of receiving and indexing Safe Browsing
subresource filtering rules coming in through the component updater.

Also, change the directory structure so that both indexed and unindexed rules
are stored under the BASE_DIR at "$USER_DATA_DIR/Subresource Filtering Rules/".
Unindexed rulesets are now stored under "$BASE_DIR/Unindexed/$CONTENT_VERSION/";
and indexed rulesets under "$BASE_DIR/Indexed/$FORMAT_VERSION/$CONTENT_VERSION",
instead of "$BASE_DIR/$FORMAT_VERSION_$CONTENT_VERSION".

Furthermore, refactor TestRulesetCreator to facilitate tests with either the
indexed/unindexed representation of the rules.

BUG=609747
==========

to

==========
Implement indexing of subresource filtering rules.

Make the RulesetService capable of receiving and indexing Safe Browsing
subresource filtering rules coming in through the component updater.

Also, change the directory structure so that both indexed and unindexed rules
are stored under a BASE directory at "$USER_DATA_DIR/Subresource Filter/".

Unindexed rules are now stored under "$BASE/Unindexed Rules/$CONTENT_VERSION/";
and indexed rules under "$BASE/Indexed Rules/$FORMAT_VERSION/$CONTENT_VERSION",
instead of "$BASE/$FORMAT_VERSION_$CONTENT_VERSION".

Furthermore, refactor TestRulesetCreator to facilitate testing by supplying 
both the indexed/unindexed representation of the testing rules, and making them
available as files, file paths or blobs

BUG=609747
==========

[engedy, 2016-07-28 13:38:40]

Description was changed from

==========
Implement indexing of subresource filtering rules.

Make the RulesetService capable of receiving and indexing Safe Browsing
subresource filtering rules coming in through the component updater.

Also, change the directory structure so that both indexed and unindexed rules
are stored under a BASE directory at "$USER_DATA_DIR/Subresource Filter/".

Unindexed rules are now stored under "$BASE/Unindexed Rules/$CONTENT_VERSION/";
and indexed rules under "$BASE/Indexed Rules/$FORMAT_VERSION/$CONTENT_VERSION",
instead of "$BASE/$FORMAT_VERSION_$CONTENT_VERSION".

Furthermore, refactor TestRulesetCreator to facilitate testing by supplying 
both the indexed/unindexed representation of the testing rules, and making them
available as files, file paths or blobs

BUG=609747
==========

to

==========
Implement indexing of subresource filtering rules.

Make the RulesetService capable of receiving and indexing Safe Browsing
subresource filtering rules coming in through the component updater.

Also, change the directory structure so that both indexed and unindexed rules
are stored under a BASE directory at "$USER_DATA_DIR/Subresource Filter/".

Unindexed rules are now stored under "$BASE/Unindexed Rules/$CONTENT_VERSION/";
and indexed rules under "$BASE/Indexed Rules/$FORMAT_VERSION/$CONTENT_VERSION",
instead of "$BASE/$FORMAT_VERSION_$CONTENT_VERSION".

Furthermore, refactor TestRulesetCreator to facilitate testing by supplying 
both the indexed/unindexed representation of the testing rules, and making them
available as files, file paths or blobs.

BUG=609747
==========

[engedy, 2016-07-28 13:38:40]

engedy@chromium.org changed reviewers:
+ melandory@chromium.org, pkalinnikov@chromium.org

[engedy, 2016-07-28 13:40:32]

Description was changed from

==========
Implement indexing of subresource filtering rules.

Make the RulesetService capable of receiving and indexing Safe Browsing
subresource filtering rules coming in through the component updater.

Also, change the directory structure so that both indexed and unindexed rules
are stored under a BASE directory at "$USER_DATA_DIR/Subresource Filter/".

Unindexed rules are now stored under "$BASE/Unindexed Rules/$CONTENT_VERSION/";
and indexed rules under "$BASE/Indexed Rules/$FORMAT_VERSION/$CONTENT_VERSION",
instead of "$BASE/$FORMAT_VERSION_$CONTENT_VERSION".

Furthermore, refactor TestRulesetCreator to facilitate testing by supplying 
both the indexed/unindexed representation of the testing rules, and making them
available as files, file paths or blobs.

BUG=609747
==========

to

==========
Implement indexing of subresource filtering rules.

Make the RulesetService capable of receiving and indexing Safe Browsing
subresource filtering rules coming in through the component updater. The wire
format is still TBD, but for now consists of a single rule.

Also, change the directory structure so that both indexed and unindexed rules
are stored under a BASE directory at "$USER_DATA_DIR/Subresource Filter/".
Unindexed rules are now stored under "$BASE/Unindexed Rules/$CONTENT_VERSION/";
and indexed rules under "$BASE/Indexed Rules/$FORMAT_VERSION/$CONTENT_VERSION",
instead of "$BASE/$FORMAT_VERSION_$CONTENT_VERSION".

Furthermore, refactor TestRulesetCreator to facilitate testing by supplying 
both the indexed/unindexed representation of the testing rules, and making them
available as files, file paths or blobs.

BUG=609747
==========

[engedy, 2016-07-28 13:41:12]

Description was changed from

==========
Implement indexing of subresource filtering rules.

Make the RulesetService capable of receiving and indexing Safe Browsing
subresource filtering rules coming in through the component updater. The wire
format is still TBD, but for now consists of a single rule.

Also, change the directory structure so that both indexed and unindexed rules
are stored under a BASE directory at "$USER_DATA_DIR/Subresource Filter/".
Unindexed rules are now stored under "$BASE/Unindexed Rules/$CONTENT_VERSION/";
and indexed rules under "$BASE/Indexed Rules/$FORMAT_VERSION/$CONTENT_VERSION",
instead of "$BASE/$FORMAT_VERSION_$CONTENT_VERSION".

Furthermore, refactor TestRulesetCreator to facilitate testing by supplying 
both the indexed/unindexed representation of the testing rules, and making them
available as files, file paths or blobs.

BUG=609747
==========

to

==========
Framework for indexing of subresource filtering rules.

Make the RulesetService capable of receiving and indexing Safe Browsing
subresource filtering rules coming in through the component updater. The wire
format is still TBD, but for now consists of a single rule.

Also, change the directory structure so that both indexed and unindexed rules
are stored under a BASE directory at "$USER_DATA_DIR/Subresource Filter/".
Unindexed rules are now stored under "$BASE/Unindexed Rules/$CONTENT_VERSION/";
and indexed rules under "$BASE/Indexed Rules/$FORMAT_VERSION/$CONTENT_VERSION",
instead of "$BASE/$FORMAT_VERSION_$CONTENT_VERSION".

Furthermore, refactor TestRulesetCreator to facilitate testing by supplying 
both the indexed/unindexed representation of the testing rules, and making them
available as files, file paths or blobs.

BUG=609747
==========

[engedy, 2016-07-28 13:44:44]

@Pavel, please review the entire CL.
@Tanja, please review component_updater parts.

[melandory, 2016-07-28 13:53:26]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 13:53:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 13:56:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-28 13:56:44]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[engedy, 2016-07-28 14:02:31]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 14:02:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2016-07-28 14:40:07]

On 2016/07/28 13:44:44, engedy wrote:
> @Pavel, please review the entire CL.
> @Tanja, please review component_updater parts.

lgtm (you should add sorin@ or waffles@ for owners review, they did initial code
reviews for our installer)

[commit-bot: I haz the power, 2016-07-28 14:40:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-28 14:40:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pkalinnikov, 2016-07-28 18:08:51]

LGTM % comments.

One generic question. Even though the unindexed ruleset format is supposed to be
relatively stable, don't we need some future-proof protection against its rare
changes?

https://codereview.chromium.org/2182493009/diff/60001/chrome/browser/subresou...
File chrome/browser/subresource_filter/subresource_filter_browsertest.cc
(right):

https://codereview.chromium.org/2182493009/diff/60001/chrome/browser/subresou...
chrome/browser/subresource_filter/subresource_filter_browsertest.cc:91: // For
simplicity, use the |suffix| itself as the uniquely identifying
I am bit confused, because |suffix| is not a number :)
Suggestion: For simplicity, use the |suffix| as the( unique)? content version
id(entifier)?

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
File
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc
(right):

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc:10:
#include "base/files/file.h"
This is not used any more, is it?

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
File components/subresource_filter/core/browser/ruleset_service.cc (right):

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/browser/ruleset_service.cc:95:
.AppendASCII(content_version);
What is a typical |content_version|? Is it a hex representation of a hash or
something?

Can this concatenation give an ambiguous id? E.g., if the |content_version|'s
size is not fixed. Even if not, wouldn't it be nicer to put some delimiter
between the |format_version| and |content_version|?

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
File components/subresource_filter/core/browser/ruleset_service.h (right):

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/browser/ruleset_service.h:40: // versions.
Isn't the last sentence redundant?

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/browser/ruleset_service.h:119: void
IndexAndStoreRuleset(const base::FilePath& unindexed_ruleset_path,
I don't see much difference between "Store" and "Write", so this function and
the one below are not very distinguishable. How about
"IndexAndStoreRuleset(And|Then)(CallBack|Notify)"?

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
File components/subresource_filter/core/common/indexed_ruleset.h (right):

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/common/indexed_ruleset.h:59: static const int
kIndexedFormatVersion;
nit: This line should be above the constructor (cpp style guide).

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
File components/subresource_filter/core/common/test_ruleset_creator.cc (right):

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/common/test_ruleset_creator.cc:35:
static_assert(CHAR_BIT == 8, "Assumed char was 8 bits.");
There is the same static_assert below. Can't we just put one instance to the
beginning of this file?

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
File components/subresource_filter/core/common/test_ruleset_creator.h (right):

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/common/test_ruleset_creator.h:23: // data
blob, or a file handle (through the Open method).
How about: ... as a file |path|, a |data| blob, or ...

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/common/test_ruleset_creator.h:23: // data
blob, or a file handle (through the Open method).
Is it really OR or AND?

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/common/test_ruleset_creator.h:30:
std::vector<uint8_t> contents;
... and s/contents/data/ ?

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/common/test_ruleset_creator.h:63: void
CreateTestRulesetFromContents(std::vector<uint8_t> ruleset_contents,
... and here, correspondingly, s/ruleset_contents/ruleset_data/g.

[pkalinnikov, 2016-07-28 18:13:07]

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
File components/subresource_filter/core/common/test_ruleset_creator.h (right):

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/common/test_ruleset_creator.h:30:
std::vector<uint8_t> contents;
On 2016/07/28 18:08:52, pkalinnikov wrote:
> ... and s/contents/data/ ?

Actually, I see that you use the term "ruleset contents" consistently, so this
comment and the one below can be ignored.

[engedy, 2016-07-28 19:46:45]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 19:47:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2016-07-28 19:51:54]

So currently, if the wire format changes in backward-incompatible ways, the
proto parsing will just fail. We could also just restrict updates to new-enough
client versions.

Neither of those solutions are particularly nice, so I have been wondering
myself I wonder if we should have a proto in the beginning, containing a version
number. Or maybe store a version number in the manifest file.

https://codereview.chromium.org/2182493009/diff/60001/chrome/browser/subresou...
File chrome/browser/subresource_filter/subresource_filter_browsertest.cc
(right):

https://codereview.chromium.org/2182493009/diff/60001/chrome/browser/subresou...
chrome/browser/subresource_filter/subresource_filter_browsertest.cc:91: // For
simplicity, use the |suffix| itself as the uniquely identifying
On 2016/07/28 18:08:51, pkalinnikov wrote:
> I am bit confused, because |suffix| is not a number :)
> Suggestion: For simplicity, use the |suffix| as the( unique)? content version
> id(entifier)?

Yeah, poor choice of words. Replaced "number" with "tag".

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
File
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc
(right):

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc:10:
#include "base/files/file.h"
On 2016/07/28 18:08:51, pkalinnikov wrote:
> This is not used any more, is it?

Good question! So the complete type is needed because of, e.g., a call to
testing::TestRuleset::Open() which returns an instance of base::File. Both
ruleset_dealer.h and test_ruleset_creator.h could just fwd-declare it. In
practice, even though TestRulesetCreate happens to actually include file.h, I
usually like to not rely on such fragile things.

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
File components/subresource_filter/core/browser/ruleset_service.cc (right):

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/browser/ruleset_service.cc:95:
.AppendASCII(content_version);
On 2016/07/28 18:08:52, pkalinnikov wrote:
> What is a typical |content_version|? Is it a hex representation of a hash or
> something?
> 
> Can this concatenation give an ambiguous id? E.g., if the |content_version|'s
> size is not fixed. Even if not, wouldn't it be nicer to put some delimiter
> between the |format_version| and |content_version|?

Agreed this would be a problem, but AppendASCII takes care of adding the
delimiter in-between.

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
File components/subresource_filter/core/browser/ruleset_service.h (right):

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/browser/ruleset_service.h:40: // versions.
On 2016/07/28 18:08:52, pkalinnikov wrote:
> Isn't the last sentence redundant?

Done. Removed.

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/browser/ruleset_service.h:119: void
IndexAndStoreRuleset(const base::FilePath& unindexed_ruleset_path,
On 2016/07/28 18:08:52, pkalinnikov wrote:
> I don't see much difference between "Store" and "Write", so this function and
> the one below are not very distinguishable. How about
> "IndexAndStoreRuleset(And|Then)(CallBack|Notify)"?

Clarified. Done.

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
File components/subresource_filter/core/common/indexed_ruleset.h (right):

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/common/indexed_ruleset.h:59: static const int
kIndexedFormatVersion;
On 2016/07/28 18:08:52, pkalinnikov wrote:
> nit: This line should be above the constructor (cpp style guide).

Done.

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
File components/subresource_filter/core/common/test_ruleset_creator.cc (right):

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/common/test_ruleset_creator.cc:35:
static_assert(CHAR_BIT == 8, "Assumed char was 8 bits.");
On 2016/07/28 18:08:52, pkalinnikov wrote:
> There is the same static_assert below. Can't we just put one instance to the
> beginning of this file?

Done.

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
File components/subresource_filter/core/common/test_ruleset_creator.h (right):

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/common/test_ruleset_creator.h:23: // data
blob, or a file handle (through the Open method).
On 2016/07/28 18:08:52, pkalinnikov wrote:
> Is it really OR or AND?

Yep.

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/common/test_ruleset_creator.h:23: // data
blob, or a file handle (through the Open method).
On 2016/07/28 18:08:52, pkalinnikov wrote:
> How about: ... as a file |path|, a |data| blob, or ...

Done. Rephrased.

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/common/test_ruleset_creator.h:30:
std::vector<uint8_t> contents;
On 2016/07/28 18:13:07, pkalinnikov wrote:
> On 2016/07/28 18:08:52, pkalinnikov wrote:
> > ... and s/contents/data/ ?
> 
> Actually, I see that you use the term "ruleset contents" consistently, so this
> comment and the one below can be ignored.

Okay, I'm fine either way but kept contents.

https://codereview.chromium.org/2182493009/diff/60001/components/subresource_...
components/subresource_filter/core/common/test_ruleset_creator.h:63: void
CreateTestRulesetFromContents(std::vector<uint8_t> ruleset_contents,
On 2016/07/28 18:08:52, pkalinnikov wrote:
> ... and here, correspondingly, s/ruleset_contents/ruleset_data/g.

Kept contents as discussed above.

[engedy, 2016-07-28 19:59:05]

engedy@chromium.org changed reviewers:
+ thestig@chromium.org, waffles@chromium.org

[engedy, 2016-07-28 19:59:06]

@Joshua, could you please take a look at:

  c/b/component_updater/

@Lei, could you please take a look at (mechanical changes in):

  chrome/browser/browser_process_impl.cc
  chrome/browser/prefs/browser_prefs.cc

[Lei Zhang, 2016-07-28 20:51:56]

lgtm

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
File chrome/browser/component_updater/subresource_filter_component_installer.cc
(right):

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/subresource_filter_component_installer.cc:78:
install_dir.Append(FILE_PATH_LITERAL("subresource_filter_rules.blob"));
Should subresource_filter_rules.blob be a constant that lives near
subresource_filter::kUnindexedRulesetBaseDirectoryName? It's only used in 2
places. Here and the unit test. *shrug*

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
File
chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc
(right):

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc:50:
const base::FilePath& ruleset_path() { return ruleset_path_; }
method should be const too.

[waffles, 2016-07-28 21:58:57]

component_updater lgtm

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
File chrome/browser/component_updater/subresource_filter_component_installer.cc
(right):

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/subresource_filter_component_installer.cc:74:
return;
In what circumstance do you expect this to happen?

[engedy, 2016-07-28 22:17:21]

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
File chrome/browser/component_updater/subresource_filter_component_installer.cc
(right):

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/subresource_filter_component_installer.cc:74:
return;
On 2016/07/28 21:58:56, waffles wrote:
> In what circumstance do you expect this to happen?

I just refactored this from line 104. Happy to remove if this will never happen.

[waffles, 2016-07-28 22:28:52]

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
File chrome/browser/component_updater/subresource_filter_component_installer.cc
(right):

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/subresource_filter_component_installer.cc:74:
return;
On 2016/07/28 22:17:21, engedy wrote:
> On 2016/07/28 21:58:56, waffles wrote:
> > In what circumstance do you expect this to happen?
> 
> I just refactored this from line 104. Happy to remove if this will never
happen.
> 

Yeah, either DCHECK or remove it altogether.

[Lei Zhang, 2016-07-28 22:42:26]

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
File chrome/browser/component_updater/subresource_filter_component_installer.cc
(right):

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/subresource_filter_component_installer.cc:74:
return;
On 2016/07/28 22:28:52, waffles wrote:
> On 2016/07/28 22:17:21, engedy wrote:
> > On 2016/07/28 21:58:56, waffles wrote:
> > > In what circumstance do you expect this to happen?
> > 
> > I just refactored this from line 104. Happy to remove if this will never
> happen.
> > 
> 
> Yeah, either DCHECK or remove it altogether.

Vote for DCHECK. Don't want it to ever fail and start trying to read/write files
at the wrong place.

[engedy, 2016-07-29 10:02:10]

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
File chrome/browser/component_updater/subresource_filter_component_installer.cc
(right):

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/subresource_filter_component_installer.cc:74:
return;
On 2016/07/28 22:42:26, Lei Zhang wrote:
> On 2016/07/28 22:28:52, waffles wrote:
> > On 2016/07/28 22:17:21, engedy wrote:
> > > On 2016/07/28 21:58:56, waffles wrote:
> > > > In what circumstance do you expect this to happen?
> > > 
> > > I just refactored this from line 104. Happy to remove if this will never
> > happen.
> > > 
> > 
> > Yeah, either DCHECK or remove it altogether.
> 
> Vote for DCHECK. Don't want it to ever fail and start trying to read/write
files
> at the wrong place.

Done. Made this a DHECK.

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/subresource_filter_component_installer.cc:78:
install_dir.Append(FILE_PATH_LITERAL("subresource_filter_rules.blob"));
On 2016/07/28 20:51:56, Lei Zhang wrote:
> Should subresource_filter_rules.blob be a constant that lives near
> subresource_filter::kUnindexedRulesetBaseDirectoryName? It's only used in 2
> places. Here and the unit test. *shrug*

I think I'd actually prefer to restrict this to within the component updater
itself. I will leave this as it is for now, and will revisit in an up-coming CL
that will add one more file to the CRX.

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
File
chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc
(right):

https://codereview.chromium.org/2182493009/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc:50:
const base::FilePath& ruleset_path() { return ruleset_path_; }
On 2016/07/28 20:51:56, Lei Zhang wrote:
> method should be const too.

Done. Here and the line below.

[engedy, 2016-07-29 10:12:31]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 10:12:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 11:12:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-29 11:12:40]

Dry run: This issue passed the CQ dry run.

[engedy, 2016-07-29 11:14:17]

The CQ bit was checked by engedy@chromium.org

[engedy, 2016-07-29 11:14:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
melandory@chromium.org, pkalinnikov@chromium.org, thestig@chromium.org,
waffles@chromium.org
Link to the patchset: https://codereview.chromium.org/2182493009/#ps140001
(title: "Attempt #2 to fix tests on Windows.")

[commit-bot: I haz the power, 2016-07-29 11:14:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 11:18:22]

Description was changed from

==========
Framework for indexing of subresource filtering rules.

Make the RulesetService capable of receiving and indexing Safe Browsing
subresource filtering rules coming in through the component updater. The wire
format is still TBD, but for now consists of a single rule.

Also, change the directory structure so that both indexed and unindexed rules
are stored under a BASE directory at "$USER_DATA_DIR/Subresource Filter/".
Unindexed rules are now stored under "$BASE/Unindexed Rules/$CONTENT_VERSION/";
and indexed rules under "$BASE/Indexed Rules/$FORMAT_VERSION/$CONTENT_VERSION",
instead of "$BASE/$FORMAT_VERSION_$CONTENT_VERSION".

Furthermore, refactor TestRulesetCreator to facilitate testing by supplying 
both the indexed/unindexed representation of the testing rules, and making them
available as files, file paths or blobs.

BUG=609747
==========

to

==========
Framework for indexing of subresource filtering rules.

Make the RulesetService capable of receiving and indexing Safe Browsing
subresource filtering rules coming in through the component updater. The wire
format is still TBD, but for now consists of a single rule.

Also, change the directory structure so that both indexed and unindexed rules
are stored under a BASE directory at "$USER_DATA_DIR/Subresource Filter/".
Unindexed rules are now stored under "$BASE/Unindexed Rules/$CONTENT_VERSION/";
and indexed rules under "$BASE/Indexed Rules/$FORMAT_VERSION/$CONTENT_VERSION",
instead of "$BASE/$FORMAT_VERSION_$CONTENT_VERSION".

Furthermore, refactor TestRulesetCreator to facilitate testing by supplying 
both the indexed/unindexed representation of the testing rules, and making them
available as files, file paths or blobs.

BUG=609747
==========

[commit-bot: I haz the power, 2016-07-29 11:18:24]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-07-29 11:20:12]

Description was changed from

==========
Framework for indexing of subresource filtering rules.

Make the RulesetService capable of receiving and indexing Safe Browsing
subresource filtering rules coming in through the component updater. The wire
format is still TBD, but for now consists of a single rule.

Also, change the directory structure so that both indexed and unindexed rules
are stored under a BASE directory at "$USER_DATA_DIR/Subresource Filter/".
Unindexed rules are now stored under "$BASE/Unindexed Rules/$CONTENT_VERSION/";
and indexed rules under "$BASE/Indexed Rules/$FORMAT_VERSION/$CONTENT_VERSION",
instead of "$BASE/$FORMAT_VERSION_$CONTENT_VERSION".

Furthermore, refactor TestRulesetCreator to facilitate testing by supplying 
both the indexed/unindexed representation of the testing rules, and making them
available as files, file paths or blobs.

BUG=609747
==========

to

==========
Framework for indexing of subresource filtering rules.

Make the RulesetService capable of receiving and indexing Safe Browsing
subresource filtering rules coming in through the component updater. The wire
format is still TBD, but for now consists of a single rule.

Also, change the directory structure so that both indexed and unindexed rules
are stored under a BASE directory at "$USER_DATA_DIR/Subresource Filter/".
Unindexed rules are now stored under "$BASE/Unindexed Rules/$CONTENT_VERSION/";
and indexed rules under "$BASE/Indexed Rules/$FORMAT_VERSION/$CONTENT_VERSION",
instead of "$BASE/$FORMAT_VERSION_$CONTENT_VERSION".

Furthermore, refactor TestRulesetCreator to facilitate testing by supplying
both the indexed/unindexed representation of the testing rules, and making them
available as files, file paths or blobs.

BUG=609747

Committed: https://crrev.com/6cfa34f00f9631c99f8540188e9a5aab7502eebe
Cr-Commit-Position: refs/heads/master@{#408615}
==========

[commit-bot: I haz the power, 2016-07-29 11:20:13]

Patchset 7 (id:??) landed as
https://crrev.com/6cfa34f00f9631c99f8540188e9a5aab7502eebe
Cr-Commit-Position: refs/heads/master@{#408615}