Use FileTypePolicies for is_archive and is_supported classifications.

Use FileTypePolicies as authoritative source for is_archive and is_supported download classifications.

  * Replace all usage of download_protection_util.cc functions except for
    GetDownloadType().
  * Enable it for Android since it will be used for chrome-controlled
    downloads in the future.
  * Make FileTypePolicies thread safe via locking. It's created
    and updated from several threads (between the ZipAnalyzer sandbox,
    SafeBrowsingService, and unit/browser tests). It'll only be accessed
    during downloads and incident reports, and will be updated less than
    once per few hours.

Coming in following CL:
  * Replace all usage of download_extensions.h and FileDangerLevel
BUG=596555
Committed: https://crrev.com/ac9da06e6e139a110e031dc484757561e9fdeb43
Cr-Commit-Position: refs/heads/master@{#394879}

[Nathan Parker, 2016-05-16 18:33:04]

Description was changed from

==========
Use FileTypePolicies as authoritative source for download classification.

  * Replace all uses of download_protection_util.cc functions except for
    GetDownloadType().
  * Enable it for Android since it will be used for chrome-controlled
    downloads in the near future.
  * Make FileTypePolicies thread safe via locking. It's created
    and updated from several threads (between the ZipAnalyzer sandbox,
    SafeBrowsingService, and unit/browser tests). It'll only be accessed
    during downloads and incident reports, and will be updated less than
    once per few hours.

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

to

==========
Use FileTypePolicies as authoritative source for is_archive and is_supported

  * Replace all usage of download_protection_util.cc functions except for
    GetDownloadType().
  * Enable it for Android since it will be used for chrome-controlled
    downloads in the future.
  * Make FileTypePolicies thread safe via locking. It's created
    and updated from several threads (between the ZipAnalyzer sandbox,
    SafeBrowsingService, and unit/browser tests). It'll only be accessed
    during downloads and incident reports, and will be updated less than
    once per few hours.

Coming in following CL:
  * Replace all usage of download_extensions.h and FileDangerLevel
BUG=596555
==========

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

Description was changed from

==========
Use FileTypePolicies as authoritative source for is_archive and is_supported

  * Replace all usage of download_protection_util.cc functions except for
    GetDownloadType().
  * Enable it for Android since it will be used for chrome-controlled
    downloads in the future.
  * Make FileTypePolicies thread safe via locking. It's created
    and updated from several threads (between the ZipAnalyzer sandbox,
    SafeBrowsingService, and unit/browser tests). It'll only be accessed
    during downloads and incident reports, and will be updated less than
    once per few hours.

Coming in following CL:
  * Replace all usage of download_extensions.h and FileDangerLevel
BUG=596555
==========

to

==========
Use FileTypePolicies as authoritative source for is_archive and is_supported
download classifications.

  * Replace all usage of download_protection_util.cc functions except for
    GetDownloadType().
  * Enable it for Android since it will be used for chrome-controlled
    downloads in the future.
  * Make FileTypePolicies thread safe via locking. It's created
    and updated from several threads (between the ZipAnalyzer sandbox,
    SafeBrowsingService, and unit/browser tests). It'll only be accessed
    during downloads and incident reports, and will be updated less than
    once per few hours.

Coming in following CL:
  * Replace all usage of download_extensions.h and FileDangerLevel
BUG=596555
==========

[Nathan Parker, 2016-05-16 18:36:20]

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

[asanka, 2016-05-18 18:47:54]

Almost there!

https://codereview.chromium.org/1979153002/diff/40001/chrome/browser/download...
File chrome/browser/download/download_danger_prompt.cc (right):

https://codereview.chromium.org/1979153002/diff/40001/chrome/browser/download...
chrome/browser/download/download_danger_prompt.cc:15: #include
"chrome/grit/generated_resources.h"
Thanks for these.

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
File chrome/common/safe_browsing/file_type_policies.cc (right):

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.cc:29:
FileTypePolicies::SetGlobalInstance(nullptr);
This one is a bit iffy. I'd lean towards leaking the FileTypePolicies object
because the pointer returned by GetGlobalInstance() is supposed to be thread
safe and has no lifetime guarantees.

Or, alternatively, we could use a singleton here. Those get cleaned up at AtExit
time, which should be safe.

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
File chrome/common/safe_browsing/file_type_policies.h (right):

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:32: FileTypePolicies();
If the lifetime of this class is going to be managed by a designated class, it
should probably not have a public constructor / desctructor.

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:37: static FileTypePolicies*
GlobalInstance();
Can this return a const* ?

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:55: static const
base::FilePath::StringType GetFileExtension(
const unnecessary for return type since it's being returned by value.

Also, how is this different from FilePath::GetFinalExtension()?

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:61: DownloadFileType
PolicyForFile(const base::FilePath& file);
All of these should be 'const' no?

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:118: base::Lock lock_;
Locks are often mutable, and should explain what it's protecting (I guess
everything?).

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:129: // If the global object
already exists, this does nothing.
Shall we DCHECK if folks try to use more than one instance? This class isn't
thread safe.

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:137: class
FileTypePoliciesManager {
Should mention how it's to be used (or not to be used) and the fact that it's
not thread safe.

[Nathan Parker, 2016-05-18 23:53:29]

Thanks for the review.  I dropped a number of files from this CL that no longer
need to be edited. PTAL.

thestig@: I need OWNERS approval for the following:
  chrome/browser/BUILD.gn
  chrome/browser/browser_resources.grd
  chrome/common/BUILD.gn

Thanks!

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
File chrome/common/safe_browsing/file_type_policies.cc (right):

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.cc:29:
FileTypePolicies::SetGlobalInstance(nullptr);
On 2016/05/18 18:47:54, asanka wrote:
> This one is a bit iffy. I'd lean towards leaking the FileTypePolicies object
> because the pointer returned by GetGlobalInstance() is supposed to be thread
> safe and has no lifetime guarantees.
> 
> Or, alternatively, we could use a singleton here. Those get cleaned up at
AtExit
> time, which should be safe.

I've switched to Singleton.  Actually, this in combination with the fact that
I've made all access/update thread-safe means it can be constructed on any
thread, so lifetime management is a lot simpler.  Nice!

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
File chrome/common/safe_browsing/file_type_policies.h (right):

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:32: FileTypePolicies();
On 2016/05/18 18:47:54, asanka wrote:
> If the lifetime of this class is going to be managed by a designated class, it
> should probably not have a public constructor / desctructor.

Done.

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:37: static FileTypePolicies*
GlobalInstance();
On 2016/05/18 18:47:54, asanka wrote:
> Can this return a const* ?

Nope, can't, because the PopulateFromDynanmicUpdate() will modify it.  That's
getting used in https://codereview.chromium.org/1986403002/ for
component-update.  I could make a GetInstanceMutable that's private and
friendify the component-updater class, but that seems to tangle things more than
is worth it.

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:55: static const
base::FilePath::StringType GetFileExtension(
On 2016/05/18 18:47:54, asanka wrote:
> const unnecessary for return type since it's being returned by value.

Done.

> Also, how is this different from FilePath::GetFinalExtension()?

It strips trailing [. ]+ before calling FilePath::FinalExtension().  Maybe we
should add this to GetFinalExtension() and just call that?

I had moved this from download_protection_util.cc, since android needs this but
doesn't build that file.

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:61: DownloadFileType
PolicyForFile(const base::FilePath& file);
On 2016/05/18 18:47:54, asanka wrote:
> All of these should be 'const' no?

Done, with the mutable lock_.

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:118: base::Lock lock_;
On 2016/05/18 18:47:54, asanka wrote:
> Locks are often mutable, and should explain what it's protecting (I guess
> everything?).

I've got "protected by lock_" on the members above.  Would it be better to have
those here?

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:129: // If the global object
already exists, this does nothing.
On 2016/05/18 18:47:54, asanka wrote:
> Shall we DCHECK if folks try to use more than one instance? This class isn't
> thread safe.

I've removed this class.

https://codereview.chromium.org/1979153002/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:137: class
FileTypePoliciesManager {
On 2016/05/18 18:47:54, asanka wrote:
> Should mention how it's to be used (or not to be used) and the fact that it's
> not thread safe.

I've removed this class.

[Nathan Parker, 2016-05-19 00:38:46]

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

[Nathan Parker, 2016-05-19 00:38:46]

thestig@ for OWNERS approval for the following:
  chrome/browser/BUILD.gn
  chrome/browser/browser_resources.grd
  chrome/common/BUILD.gn

(This is the first in a three part CL)

Thanks!

[Lei Zhang, 2016-05-19 01:04:35]

lgtm if asanka approves.

I only looked at the GN/GYP/GRD files.

[asanka, 2016-05-19 20:25:58]

lgtm. thanks!

https://codereview.chromium.org/1979153002/diff/60001/chrome/common/safe_brow...
File chrome/common/safe_browsing/file_type_policies.h (right):

https://codereview.chromium.org/1979153002/diff/60001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:44: // Returns the file
extension, with the dot.
Nit: Should explain that this strips trailing characters that are commonly
ignored. I.e. it's different from FilePath::Extension() or
FilePath::FinalExtension(). Should also clarify that it returns only the final
extension where there are multiple extensions (e.g. "foo.tar.gz " returns ".gz")

[Nathan Parker, 2016-05-19 20:45:19]

Thanks!

https://codereview.chromium.org/1979153002/diff/60001/chrome/common/safe_brow...
File chrome/common/safe_browsing/file_type_policies.h (right):

https://codereview.chromium.org/1979153002/diff/60001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.h:44: // Returns the file
extension, with the dot.
On 2016/05/19 20:25:58, asanka wrote:
> Nit: Should explain that this strips trailing characters that are commonly
> ignored. I.e. it's different from FilePath::Extension() or
> FilePath::FinalExtension(). Should also clarify that it returns only the final
> extension where there are multiple extensions (e.g. "foo.tar.gz " returns
".gz")

Done.

[Nathan Parker, 2016-05-19 20:45:59]

The CQ bit was checked by nparker@chromium.org

[Nathan Parker, 2016-05-19 20:45:59]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1979153002/#ps80001
(title: "Fix comment, per asanka's review")

[commit-bot: I haz the power, 2016-05-19 20:46:51]

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

[commit-bot: I haz the power, 2016-05-19 21:47:34]

Description was changed from

==========
Use FileTypePolicies as authoritative source for is_archive and is_supported
download classifications.

  * Replace all usage of download_protection_util.cc functions except for
    GetDownloadType().
  * Enable it for Android since it will be used for chrome-controlled
    downloads in the future.
  * Make FileTypePolicies thread safe via locking. It's created
    and updated from several threads (between the ZipAnalyzer sandbox,
    SafeBrowsingService, and unit/browser tests). It'll only be accessed
    during downloads and incident reports, and will be updated less than
    once per few hours.

Coming in following CL:
  * Replace all usage of download_extensions.h and FileDangerLevel
BUG=596555
==========

to

==========
Use FileTypePolicies as authoritative source for is_archive and is_supported
download classifications.

  * Replace all usage of download_protection_util.cc functions except for
    GetDownloadType().
  * Enable it for Android since it will be used for chrome-controlled
    downloads in the future.
  * Make FileTypePolicies thread safe via locking. It's created
    and updated from several threads (between the ZipAnalyzer sandbox,
    SafeBrowsingService, and unit/browser tests). It'll only be accessed
    during downloads and incident reports, and will be updated less than
    once per few hours.

Coming in following CL:
  * Replace all usage of download_extensions.h and FileDangerLevel
BUG=596555
==========

[commit-bot: I haz the power, 2016-05-19 21:47:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-19 21:48:37]

Description was changed from

==========
Use FileTypePolicies as authoritative source for is_archive and is_supported
download classifications.

  * Replace all usage of download_protection_util.cc functions except for
    GetDownloadType().
  * Enable it for Android since it will be used for chrome-controlled
    downloads in the future.
  * Make FileTypePolicies thread safe via locking. It's created
    and updated from several threads (between the ZipAnalyzer sandbox,
    SafeBrowsingService, and unit/browser tests). It'll only be accessed
    during downloads and incident reports, and will be updated less than
    once per few hours.

Coming in following CL:
  * Replace all usage of download_extensions.h and FileDangerLevel
BUG=596555
==========

to

==========
Use FileTypePolicies as authoritative source for is_archive and is_supported
download classifications.

  * Replace all usage of download_protection_util.cc functions except for
    GetDownloadType().
  * Enable it for Android since it will be used for chrome-controlled
    downloads in the future.
  * Make FileTypePolicies thread safe via locking. It's created
    and updated from several threads (between the ZipAnalyzer sandbox,
    SafeBrowsingService, and unit/browser tests). It'll only be accessed
    during downloads and incident reports, and will be updated less than
    once per few hours.

Coming in following CL:
  * Replace all usage of download_extensions.h and FileDangerLevel
BUG=596555

Committed: https://crrev.com/ac9da06e6e139a110e031dc484757561e9fdeb43
Cr-Commit-Position: refs/heads/master@{#394879}
==========

[commit-bot: I haz the power, 2016-05-19 21:48:38]

Patchset 5 (id:??) landed as
https://crrev.com/ac9da06e6e139a110e031dc484757561e9fdeb43
Cr-Commit-Position: refs/heads/master@{#394879}