Component updater for FileTypePolicies.

Component updater for FileTypePolicies.

BUG=596555
Committed: https://crrev.com/070d6316bbea3e3ed648c84648391c47439e2c9c
Cr-Commit-Position: refs/heads/master@{#396542}

[Nathan Parker, 2016-05-17 23:09:22]

nparker@chromium.org changed reviewers:
+ waffles@chromium.org

[waffles, 2016-05-18 17:31:51]

lgtm

https://codereview.chromium.org/1986403002/diff/40001/chrome/browser/componen...
File chrome/browser/component_updater/file_type_policies_component_installer.cc
(right):

https://codereview.chromium.org/1986403002/diff/40001/chrome/browser/componen...
chrome/browser/component_updater/file_type_policies_component_installer.cc:64:
return false;  // TODO(nparker): Figure if this is correct.
That's correct. The update will always be secure, but it may not be confidential
(e.g. MITM can read but not tamper). Some components require confidentiality,
but since this component is always present, there's no privacy concern with
confidentiality.

https://codereview.chromium.org/1986403002/diff/40001/chrome/browser/componen...
chrome/browser/component_updater/file_type_policies_component_installer.cc:78:
.Append(FILE_PATH_LITERAL("all"))
_platform_specific/all is a little weird: I don't really understand why it ever
existed, much less why other components use it. Anyways, you might find it
simpler to just put file_type_policies.pb in the root of the CRX, unless you're
getting some benefit from this that I don't see.

https://codereview.chromium.org/1986403002/diff/40001/chrome/browser/componen...
chrome/browser/component_updater/file_type_policies_component_installer.cc:101:
// determine if there's value in checking that here.
Doing so would be beneficial in the case that the file is corrupted on disk, or
something like that, but is a big uglier to code and will result in more file
I/O. Judgment call, I think.

https://codereview.chromium.org/1986403002/diff/40001/chrome/browser/componen...
File chrome/browser/component_updater/file_type_policies_component_installer.h
(right):

https://codereview.chromium.org/1986403002/diff/40001/chrome/browser/componen...
chrome/browser/component_updater/file_type_policies_component_installer.h:25:
class FileTypePoliciesComponentInstallerTraits
It looks like nobody besides the .cc needs this type, so consider moving it into
the .cc.

[waffles, 2016-05-18 17:31:55]

lgtm

[Nathan Parker, 2016-05-18 19:11:36]

I'll wait on submitting this CL till there are files published for this to read,
and I can test it.

https://codereview.chromium.org/1986403002/diff/40001/chrome/browser/componen...
File chrome/browser/component_updater/file_type_policies_component_installer.cc
(right):

https://codereview.chromium.org/1986403002/diff/40001/chrome/browser/componen...
chrome/browser/component_updater/file_type_policies_component_installer.cc:64:
return false;  // TODO(nparker): Figure if this is correct.
On 2016/05/18 17:31:51, waffles wrote:
> That's correct. The update will always be secure, but it may not be
confidential
> (e.g. MITM can read but not tamper). Some components require confidentiality,
> but since this component is always present, there's no privacy concern with
> confidentiality.

Acknowledged.

https://codereview.chromium.org/1986403002/diff/40001/chrome/browser/componen...
chrome/browser/component_updater/file_type_policies_component_installer.cc:78:
.Append(FILE_PATH_LITERAL("all"))
On 2016/05/18 17:31:51, waffles wrote:
> _platform_specific/all is a little weird: I don't really understand why it
ever
> existed, much less why other components use it. Anyways, you might find it
> simpler to just put file_type_policies.pb in the root of the CRX, unless
you're
> getting some benefit from this that I don't see.

Yea I wasn't sure if this was a standard practice enforced by the component
updater.  I'll put it in the root.

https://codereview.chromium.org/1986403002/diff/40001/chrome/browser/componen...
chrome/browser/component_updater/file_type_policies_component_installer.cc:101:
// determine if there's value in checking that here.
On 2016/05/18 17:31:51, waffles wrote:
> Doing so would be beneficial in the case that the file is corrupted on disk,
or
> something like that, but is a big uglier to code and will result in more file
> I/O. Judgment call, I think.

I'll leave it out for now, since it would involve reading it twice I think.  If
UMA results from PopulateFromDynamicUpdate() suggest that corruption is common,
I can add checking here.  Though, if the result is the same (it remains
corrupt), then it doesn't add value.

[Nathan Parker, 2016-05-19 00:36:08]

nparker@chromium.org changed reviewers:
+ thestig@chromium.org

[Nathan Parker, 2016-05-19 00:36:08]

thestig@chromium.org: Please review changes in 
  chrome/browser/chrome_browser_main.cc

Thanks!

[Lei Zhang, 2016-05-19 00:57:56]

lgtm

[Nathan Parker, 2016-05-27 17:45:50]

The CQ bit was checked by nparker@chromium.org

[Nathan Parker, 2016-05-27 17:45:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
waffles@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1986403002/#ps120001
(title: "Fix component file name, and add a new UMA value")

[commit-bot: I haz the power, 2016-05-27 17:46:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1986403002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1986403002/120001

[commit-bot: I haz the power, 2016-05-27 18:12:18]

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

[commit-bot: I haz the power, 2016-05-27 18:12:24]

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

[Nathan Parker, 2016-05-27 18:35:26]

The CQ bit was checked by nparker@chromium.org

[Nathan Parker, 2016-05-27 18:35:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
waffles@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1986403002/#ps140001
(title: "Fix test's enum check")

[commit-bot: I haz the power, 2016-05-27 18:35:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1986403002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1986403002/140001

[commit-bot: I haz the power, 2016-05-27 19:57:47]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-27 19:59:47]

Description was changed from

==========
Component updater for FileTypePolicies.

BUG=596555
==========

to

==========
Component updater for FileTypePolicies.

BUG=596555

Committed: https://crrev.com/070d6316bbea3e3ed648c84648391c47439e2c9c
Cr-Commit-Position: refs/heads/master@{#396542}
==========

[commit-bot: I haz the power, 2016-05-27 19:59:49]

Patchset 8 (id:??) landed as
https://crrev.com/070d6316bbea3e3ed648c84648391c47439e2c9c
Cr-Commit-Position: refs/heads/master@{#396542}