Store list information in ListInfo (was: StoreIdAndFIleName)

1. Store list information in ListInfo (was: StoreIdAndFIleName). This includes the list identifier, list filename on disk, the SBThreatType for the list, and whether to fetch updates for this list.

2. Fixed potential leaks by inserting the client into pending_clients_ before calling GetFullHashes
(based on shess@'s feedback in 2349603003)

BUG=543161, 608075
Committed: https://crrev.com/ed61c9fcb2a0de50f04bbf62f9cab4de5db37a81
Cr-Commit-Position: refs/heads/master@{#420515}

[vakh (use Gerrit instead), 2016-09-21 17:40:53]

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

[commit-bot: I haz the power, 2016-09-21 17:41:28]

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

[vakh (use Gerrit instead), 2016-09-21 17:42:53]

vakh@chromium.org changed reviewers:
+ nparker@chromium.org, shess@chromium.org

[Nathan Parker, 2016-09-21 19:02:58]

https://codereview.chromium.org/2353413002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_protocol_manager_util.cc (right):

https://codereview.chromium.org/2353413002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_protocol_manager_util.cc:130: threat_type_ ==
other.threat_type_;
Why not compare SBThreatType as well?  And in the hash below.

https://codereview.chromium.org/2353413002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/2353413002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_protocol_manager_util.h:112:
ListIdentifier(PlatformType, ThreatEntryType, ThreatType);
When would a ListIdentifier not have an SBThreatType? Is that a valid state?

[Scott Hess - ex-Googler, 2016-09-21 19:13:49]

LGTM, except you need the CL description to say what you did, not be an
easter-egg hunt!

WRT the SBThreadType field, I was actually thinking in terms of having a query
function which internally switched on threat_type_ to generate the SBThreadType.
 Storing a preloaded item also is reasonable, but as Nathan's points maybe
suggest, then you need to figure out if it can be half-initialized or not. 
Probably not, as I think you need threat_type_ and sb_threat_type_ to have an
invariant relationship, in which case it doesn't need to participate in hashing
and equality.

[commit-bot: I haz the power, 2016-09-21 19:48:28]

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

[commit-bot: I haz the power, 2016-09-21 19:48:29]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[vakh (use Gerrit instead), 2016-09-22 00:00:42]

s/StoreIdAndFileName/ListInfo. Move SBThreatType into ListInfo.

[vakh (use Gerrit instead), 2016-09-22 00:00:44]

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

[commit-bot: I haz the power, 2016-09-22 00:01:08]

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

[vakh (use Gerrit instead), 2016-09-22 00:05:41]

Minor: Update comments

[vakh (use Gerrit instead), 2016-09-22 00:05:43]

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

[vakh (use Gerrit instead), 2016-09-22 00:05:58]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-09-22 00:06:05]

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

[vakh (use Gerrit instead), 2016-09-22 00:07:32]

Description was changed from

==========
Address shess@'s feedback in 2349603003

BUG=543161
==========

to

==========
1. Store list information in ListInfo (was: StoreIdAndFIleName). This includes
the list identifier, list filename on disk, the SBThreatType for the list, and
whether to fetch updates for this list.

2. Address shess@'s feedback in 2349603003

BUG=543161, 608075
==========

[vakh (use Gerrit instead), 2016-09-22 00:10:25]

On 2016/09/21 19:13:49, Scott Hess wrote:
> LGTM, except you need the CL description to say what you did, not be an
> easter-egg hunt!
Done

> 
> WRT the SBThreadType field, I was actually thinking in terms of having a query
> function which internally switched on threat_type_ to generate the
SBThreadType.
>  Storing a preloaded item also is reasonable, but as Nathan's points maybe
> suggest, then you need to figure out if it can be half-initialized or not. 
> Probably not, as I think you need threat_type_ and sb_threat_type_ to have an
> invariant relationship, in which case it doesn't need to participate in
hashing
> and equality.

I've changed the implementation now.
1. Renamed StoreIdAndFileName to ListInfo.
2. Included the SBThreatType in ListInfo.
3. Included a bool to represent whether to fetch updates for that list.

PTAL.

[vakh (use Gerrit instead), 2016-09-22 00:10:36]

https://codereview.chromium.org/2353413002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_protocol_manager_util.cc (right):

https://codereview.chromium.org/2353413002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_protocol_manager_util.cc:130: threat_type_ ==
other.threat_type_;
On 2016/09/21 19:02:57, Nathan Parker wrote:
> Why not compare SBThreatType as well?  And in the hash below.

This triad is sufficient to identify a list.
Anyway, I've reverted the change to ListIdentifier.

https://codereview.chromium.org/2353413002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/2353413002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_protocol_manager_util.h:112:
ListIdentifier(PlatformType, ThreatEntryType, ThreatType);
On 2016/09/21 19:02:57, Nathan Parker wrote:
> When would a ListIdentifier not have an SBThreatType? Is that a valid state?

Reverted this change.

[commit-bot: I haz the power, 2016-09-22 01:30:53]

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

[commit-bot: I haz the power, 2016-09-22 01:30:55]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nathan Parker, 2016-09-22 18:35:13]

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.cc:200: DCHECK(!fetch_updates_ ||
!filename_.empty());
DCHECK(fetch_update_ != filename_.empty())

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.h:33: struct ListInfo {
Add a comment for this struct (associates various metadata with each list_id. 
list_id is expected to be unique...)

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.h:43: bool fetch_updates() const {
return fetch_updates_; }
(At first I read this wrong and I started to write out a possible solution,
ending up with exactly what you have here. so, l-g-t-m!)

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.h:128: SBThreatType
sb_threat_type_;
You don't need sb_threat_type_ here if it's in ListInfo now, ya?

[vakh (use Gerrit instead), 2016-09-22 18:50:53]

nparker@ feedback

[vakh (use Gerrit instead), 2016-09-22 18:50:55]

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

[commit-bot: I haz the power, 2016-09-22 18:51:31]

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

[vakh (use Gerrit instead), 2016-09-22 20:33:41]

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.cc:200: DCHECK(!fetch_updates_ ||
!filename_.empty());
On 2016/09/22 18:35:13, Nathan Parker wrote:
> DCHECK(fetch_update_ != filename_.empty())

I find the current form easier to understand.
It is in DCHECK so I'd prefer readability over conciseness even more.
WDYT?

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.h:33: struct ListInfo {
On 2016/09/22 18:35:13, Nathan Parker wrote:
> Add a comment for this struct (associates various metadata with each list_id. 
> list_id is expected to be unique...)

Added a single line that explains the purpose of the struct.
Comments next to member variables explain their purpose in detail.

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.h:43: bool fetch_updates() const {
return fetch_updates_; }
On 2016/09/22 18:35:13, Nathan Parker wrote:
> (At first I read this wrong and I started to write out a possible solution,
> ending up with exactly what you have here. so, l-g-t-m!)

Acknowledged.

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.h:128: SBThreatType
sb_threat_type_;
On 2016/09/22 18:35:13, Nathan Parker wrote:
> You don't need sb_threat_type_ here if it's in ListInfo now, ya?

Done.

[commit-bot: I haz the power, 2016-09-22 20:39:49]

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

[commit-bot: I haz the power, 2016-09-22 20:39:50]

Dry run: This issue passed the CQ dry run.

[Nathan Parker, 2016-09-22 22:00:59]

lgtm

Great!

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.cc:200: DCHECK(!fetch_updates_ ||
!filename_.empty());
On 2016/09/22 20:33:41, vakh (Varun Khaneja) wrote:
> On 2016/09/22 18:35:13, Nathan Parker wrote:
> > DCHECK(fetch_update_ != filename_.empty())
> 
> I find the current form easier to understand.
> It is in DCHECK so I'd prefer readability over conciseness even more.
> WDYT?

Sure, either way is fine.  (My line has a tighter constraint, since it'll fail
if fetch_updates_==false and filename_.empty()==true.)

[vakh (use Gerrit instead), 2016-09-22 22:03:19]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2016-09-22 22:03:19]

The patchset sent to the CQ was uploaded after l-g-t-m from shess@chromium.org
Link to the patchset: https://codereview.chromium.org/2353413002/#ps60001
(title: "nparker@ feedback")

[commit-bot: I haz the power, 2016-09-22 22:04:04]

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

[vakh (use Gerrit instead), 2016-09-22 22:04:23]

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/2353413002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.cc:200: DCHECK(!fetch_updates_ ||
!filename_.empty());
On 2016/09/22 22:00:59, Nathan Parker wrote:
> On 2016/09/22 20:33:41, vakh (Varun Khaneja) wrote:
> > On 2016/09/22 18:35:13, Nathan Parker wrote:
> > > DCHECK(fetch_update_ != filename_.empty())
> > 
> > I find the current form easier to understand.
> > It is in DCHECK so I'd prefer readability over conciseness even more.
> > WDYT?
> 
> Sure, either way is fine.  (My line has a tighter constraint, since it'll fail
> if fetch_updates_==false and filename_.empty()==true.)

My check is intentional since that condition is a DONTCARE. If we are not going
to fetch updates, then the filename is irrelevant.

[commit-bot: I haz the power, 2016-09-22 22:15:24]

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

[commit-bot: I haz the power, 2016-09-22 22:15:25]

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

[vakh (use Gerrit instead), 2016-09-22 22:23:41]

vakh@chromium.org changed reviewers:
+ benwells@chromium.org

[vakh (use Gerrit instead), 2016-09-22 22:23:42]

benwells@chromium.org:
Please review the changes in chrome/browser/extensions/BUILD.gn

[Scott Hess - ex-Googler, 2016-09-22 23:11:29]

lgtm.

WRT the CL description - I think you can inline more!  If someone notices a
crash happening, and it's on a line coming from this CL, and the description
says "Fix the stuff shess mentioned in some other CL", that's kind of a "Maybe,
maybe not" response.  The fact that I had comments there really isn't very
relevant to some future reader, but something like "Fixed potential leaks in
pending_clients_" might be.

https://codereview.chromium.org/2353413002/diff/60001/components/safe_browsin...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/2353413002/diff/60001/components/safe_browsin...
components/safe_browsing_db/v4_database.h:60: bool fetch_updates_;
As long as you're in here ... you should either order these alphabetically, or
in a padding-appropriate fashion.

[I mean like {string, enum, enum, bool}, or maybe {string, bool, enum, enum}.  I
don't mean that as an actual goal, mostly I'm saying if the order is random,
then maybe make it a plausibly-useful random.  Or be alphabetical :-).]

[vakh (use Gerrit instead), 2016-09-22 23:27:18]

Description was changed from

==========
1. Store list information in ListInfo (was: StoreIdAndFIleName). This includes
the list identifier, list filename on disk, the SBThreatType for the list, and
whether to fetch updates for this list.

2. Address shess@'s feedback in 2349603003

BUG=543161, 608075
==========

to

==========
1. Store list information in ListInfo (was: StoreIdAndFIleName). This includes
the list identifier, list filename on disk, the SBThreatType for the list, and
whether to fetch updates for this list.

2. Fixed potential leaks by inserting the client into pending_clients_ before
calling GetFullHashes
(based on shess@'s feedback in 2349603003)

BUG=543161, 608075
==========

[vakh (use Gerrit instead), 2016-09-22 23:28:07]

sort the members of ListInfo by member name

[vakh (use Gerrit instead), 2016-09-22 23:28:10]

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

[vakh (use Gerrit instead), 2016-09-22 23:28:21]

https://codereview.chromium.org/2353413002/diff/60001/components/safe_browsin...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/2353413002/diff/60001/components/safe_browsin...
components/safe_browsing_db/v4_database.h:60: bool fetch_updates_;
On 2016/09/22 23:11:29, Scott Hess wrote:
> As long as you're in here ... you should either order these alphabetically, or
> in a padding-appropriate fashion.
> 
> [I mean like {string, enum, enum, bool}, or maybe {string, bool, enum, enum}. 
I
> don't mean that as an actual goal, mostly I'm saying if the order is random,
> then maybe make it a plausibly-useful random.  Or be alphabetical :-).]

Done.

[commit-bot: I haz the power, 2016-09-22 23:28:53]

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

[benwells, 2016-09-22 23:31:56]

lgtm

[vakh (use Gerrit instead), 2016-09-22 23:34:15]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2016-09-22 23:34:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, shess@chromium.org
Link to the patchset: https://codereview.chromium.org/2353413002/#ps80001
(title: "sort the members of ListInfo by member name")

[commit-bot: I haz the power, 2016-09-22 23:34:47]

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-09-23 00:57:39]

Description was changed from

==========
1. Store list information in ListInfo (was: StoreIdAndFIleName). This includes
the list identifier, list filename on disk, the SBThreatType for the list, and
whether to fetch updates for this list.

2. Fixed potential leaks by inserting the client into pending_clients_ before
calling GetFullHashes
(based on shess@'s feedback in 2349603003)

BUG=543161, 608075
==========

to

==========
1. Store list information in ListInfo (was: StoreIdAndFIleName). This includes
the list identifier, list filename on disk, the SBThreatType for the list, and
whether to fetch updates for this list.

2. Fixed potential leaks by inserting the client into pending_clients_ before
calling GetFullHashes
(based on shess@'s feedback in 2349603003)

BUG=543161, 608075
==========

[commit-bot: I haz the power, 2016-09-23 00:57:45]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-09-23 01:00:21]

Description was changed from

==========
1. Store list information in ListInfo (was: StoreIdAndFIleName). This includes
the list identifier, list filename on disk, the SBThreatType for the list, and
whether to fetch updates for this list.

2. Fixed potential leaks by inserting the client into pending_clients_ before
calling GetFullHashes
(based on shess@'s feedback in 2349603003)

BUG=543161, 608075
==========

to

==========
1. Store list information in ListInfo (was: StoreIdAndFIleName). This includes
the list identifier, list filename on disk, the SBThreatType for the list, and
whether to fetch updates for this list.

2. Fixed potential leaks by inserting the client into pending_clients_ before
calling GetFullHashes
(based on shess@'s feedback in 2349603003)

BUG=543161, 608075

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

[commit-bot: I haz the power, 2016-09-23 01:00:22]

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