Only whitelist messages.json files in _locales for content verification

Only whitelist messages.json files in _locales for content verification

The messages.json files used for i18n/l10n get transcoded during the
extension install process, so we need to avoid doing content
verification checks on them because we expect their contents to be
modified. The code we had for testing whether a path should be skipped
because of this was incorrect, skipping more files than it should.

Through the courtesy of Antony: https://crrev.com/2572833004

The CL also renames some path variables in content verification
code to stress that they contain unix style '/' separators.

BUG=672008
Review-Url: https://codereview.chromium.org/2834873002
Cr-Commit-Position: refs/heads/master@{#466769}
Committed: https://chromium.googlesource.com/chromium/src/+/d05d1e844e7d376400a7437989c4c50a5875b36f

[lazyboy, 2017-04-21 18:34:53]

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

[commit-bot: I haz the power, 2017-04-21 18:35:20]

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, 2017-04-21 20:03:57]

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

[commit-bot: I haz the power, 2017-04-21 20:03:57]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lazyboy, 2017-04-22 00:44:30]

Description was changed from

==========
Try job test for CL 2572833004.

BUG=
==========

to

==========
Only whitelist messages.json files in _locales for content verification

The messages.json files used for i18n/l10n get transcoded during the
extension install process, so we need to avoid doing content
verification checks on them because we expect their contents to be
modified. The code we had for testing whether a path should be skipped
because of this was incorrect, skipping more files than it should.

Through the courtesy of Antony: https://crrev.com/2572833004

The CL also renames some path variables in content verification
code to stress that they contain unix style '/' separators.

BUG=672008
==========

[lazyboy, 2017-04-22 00:49:42]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[lazyboy, 2017-04-22 00:49:42]

Reviving Antony's patch https://crrev.com/2572833004 with fix.

The fix is primarily in two chunks in extensions/browser/content_verifier.cc.
 The issue was that verified_contents.json keeps relative paths as dictionary
keys and those paths contain unix style / separators. I've renamed our relevant
content verification code variables to contain "unix" substring to make sure we
are talking about unix style paths.

Patchset #4 -> Patchset #2 diff will show what I've changed.

https://codereview.chromium.org/2834873002/diff/60001/extensions/browser/cont...
File extensions/browser/content_verifier.cc (right):

https://codereview.chromium.org/2834873002/diff/60001/extensions/browser/cont...
extensions/browser/content_verifier.cc:274: base::FilePath full_path =
This is the change from the original CL that makes sure we create the right path
in windows.

Should fix the test failure.

https://codereview.chromium.org/2834873002/diff/60001/extensions/browser/cont...
extensions/browser/content_verifier.cc:289: if (full_path.BaseName() ==
base::FilePath(kMessagesFilename) &&
Another change: compare the filename first with "messages.json" first, instead
of constructing and checking the entire path. Not necessary to fix the tests.

[lazyboy, 2017-04-22 00:50:19]

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

[commit-bot: I haz the power, 2017-04-22 00:50:40]

Dry run: CQ is trying da patch.

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

[lazyboy, 2017-04-22 01:08:28]

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

[commit-bot: I haz the power, 2017-04-22 01:08:31]

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, 2017-04-22 01:58:23]

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

[commit-bot: I haz the power, 2017-04-22 01:58:23]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-04-24 15:06:56]

lgtm

https://codereview.chromium.org/2834873002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/content_verifier_browsertest.cc (right):

https://codereview.chromium.org/2834873002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/content_verifier_browsertest.cc:523:
ASSERT_EQ(extension->id(), id);
nit: expected, actual so ASSERT_EQ(id, extension->id())

https://codereview.chromium.org/2834873002/diff/60001/extensions/browser/cont...
File extensions/browser/content_hash_fetcher.cc (right):

https://codereview.chromium.org/2834873002/diff/60001/extensions/browser/cont...
extensions/browser/content_hash_fetcher.cc:84: // Returns the set of paths (with
unix style / separators) that had a hash
I read this as "unix style/separators" rather than "unix style '/' separators". 
Can we put '/' in quotes?

https://codereview.chromium.org/2834873002/diff/60001/extensions/browser/cont...
File extensions/browser/content_verifier.cc (right):

https://codereview.chromium.org/2834873002/diff/60001/extensions/browser/cont...
extensions/browser/content_verifier.cc:268: if (relative_unix_path ==
base::FilePath(kManifestFilename))
drive-by: this reparses kManifestFilename as a filepath on each iteration; can
we instead do:
const base::FilePath manifest_file(kManifestFilename);
for (...) {
  if (relative_unix_path == manifest_file)
?

https://codereview.chromium.org/2834873002/diff/60001/extensions/browser/cont...
extensions/browser/content_verifier.cc:289: if (full_path.BaseName() ==
base::FilePath(kMessagesFilename) &&
ditto re the use of a const FilePath.

[lazyboy, 2017-04-24 18:29:16]

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

[lazyboy, 2017-04-24 18:29:59]

https://codereview.chromium.org/2834873002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/content_verifier_browsertest.cc (right):

https://codereview.chromium.org/2834873002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/content_verifier_browsertest.cc:523:
ASSERT_EQ(extension->id(), id);
On 2017/04/24 15:06:56, Devlin wrote:
> nit: expected, actual so ASSERT_EQ(id, extension->id())

Done.

https://codereview.chromium.org/2834873002/diff/60001/extensions/browser/cont...
File extensions/browser/content_hash_fetcher.cc (right):

https://codereview.chromium.org/2834873002/diff/60001/extensions/browser/cont...
extensions/browser/content_hash_fetcher.cc:84: // Returns the set of paths (with
unix style / separators) that had a hash
On 2017/04/24 15:06:56, Devlin wrote:
> I read this as "unix style/separators" rather than "unix style '/'
separators". 
> Can we put '/' in quotes?

Done.

https://codereview.chromium.org/2834873002/diff/60001/extensions/browser/cont...
File extensions/browser/content_verifier.cc (right):

https://codereview.chromium.org/2834873002/diff/60001/extensions/browser/cont...
extensions/browser/content_verifier.cc:268: if (relative_unix_path ==
base::FilePath(kManifestFilename))
On 2017/04/24 15:06:56, Devlin wrote:
> drive-by: this reparses kManifestFilename as a filepath on each iteration; can
> we instead do:
> const base::FilePath manifest_file(kManifestFilename);
> for (...) {
>   if (relative_unix_path == manifest_file)
> ?

Thanks, Done.

[commit-bot: I haz the power, 2017-04-24 18:30:07]

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, 2017-04-24 21:11:27]

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

[commit-bot: I haz the power, 2017-04-24 21:11:28]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-04-24 21:15:11]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2017-04-24 21:15:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2834873002/#ps80001
(title: "address comments")

[commit-bot: I haz the power, 2017-04-24 21:15:44]

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, 2017-04-24 21:20:45]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493068511214810,
"parent_rev": "e66f2ff1ce2c15bb342ad8dd7579a0578af1d6bc", "commit_rev":
"d05d1e844e7d376400a7437989c4c50a5875b36f"}

[commit-bot: I haz the power, 2017-04-24 21:20:56]

Description was changed from

==========
Only whitelist messages.json files in _locales for content verification

The messages.json files used for i18n/l10n get transcoded during the
extension install process, so we need to avoid doing content
verification checks on them because we expect their contents to be
modified. The code we had for testing whether a path should be skipped
because of this was incorrect, skipping more files than it should.

Through the courtesy of Antony: https://crrev.com/2572833004

The CL also renames some path variables in content verification
code to stress that they contain unix style '/' separators.

BUG=672008
==========

to

==========
Only whitelist messages.json files in _locales for content verification

The messages.json files used for i18n/l10n get transcoded during the
extension install process, so we need to avoid doing content
verification checks on them because we expect their contents to be
modified. The code we had for testing whether a path should be skipped
because of this was incorrect, skipping more files than it should.

Through the courtesy of Antony: https://crrev.com/2572833004

The CL also renames some path variables in content verification
code to stress that they contain unix style '/' separators.

BUG=672008

Review-Url: https://codereview.chromium.org/2834873002
Cr-Commit-Position: refs/heads/master@{#466769}
Committed:
https://chromium.googlesource.com/chromium/src/+/d05d1e844e7d376400a7437989c4...
==========

[commit-bot: I haz the power, 2017-04-24 21:20:57]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/d05d1e844e7d376400a7437989c4...