Add a Node message in the malware details protocol buffer. This allows us to ...

Add a unique ID for each URL in the malware details protocol buffer. This allows us to save some space in the protocol buffer by not repeating the urls of parents and children. Also, change some string fields to bytes to avoid the implicit utf-8 conversion when not applicable.
BUG=60831
TEST=relevant unit_tests, browser_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71384

[noelutz, 2011-01-11 22:28:26]

Mostly LG.  Just two minor comments.

noe.

http://codereview.chromium.org/6208003/diff/1/chrome/browser/safe_browsing/ma...
File chrome/browser/safe_browsing/malware_details.cc (right):

http://codereview.chromium.org/6208003/diff/1/chrome/browser/safe_browsing/ma...
chrome/browser/safe_browsing/malware_details.cc:46: *node =
node_it->second.get();
The release binary will crash here if the DCHECK condition is false.  Either
convert the DCHECK to a check or test that node_it != nodes_.end() before you
access the iterator.

http://codereview.chromium.org/6208003/diff/1/chrome/browser/safe_browsing/ma...
chrome/browser/safe_browsing/malware_details.cc:84:
url_node->set_parent_id(parent_node->id());
same as above.

[panayiotis, 2011-01-11 22:45:38]

Good points, turned to CHECKs.

On Tue, Jan 11, 2011 at 2:28 PM,  <noelutz@google.com> wrote:
> Mostly LG.  Just two minor comments.
>
> noe.
>
>
>
http://codereview.chromium.org/6208003/diff/1/chrome/browser/safe_browsing/ma...
> File chrome/browser/safe_browsing/malware_details.cc (right):
>
>
http://codereview.chromium.org/6208003/diff/1/chrome/browser/safe_browsing/ma...
> chrome/browser/safe_browsing/malware_details.cc:46: *node =
> node_it->second.get();
> The release binary will crash here if the DCHECK condition is false.
> Either convert the DCHECK to a check or test that node_it !=
> nodes_.end() before you access the iterator.
>
>
http://codereview.chromium.org/6208003/diff/1/chrome/browser/safe_browsing/ma...
> chrome/browser/safe_browsing/malware_details.cc:84:
> url_node->set_parent_id(parent_node->id());
> same as above.
>
> http://codereview.chromium.org/6208003/
>

[lzheng, 2011-01-12 22:30:15]

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/malware_details.cc (right):

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/malware_details.cc:73: FindOrCreateResource(url,
&url_resource, &url_node);
Can we break this function to to? Find() and Create()?

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/malware_details.h (right):

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/malware_details.h:40: typedef base::hash_map<
How about a little bit more verbose like "Map url to Resource."? Same below for
id -> Node?

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/malware_details.h:78: // a (Url, id) pair.
Resource also contains http response and http request, right?

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/report.proto (right):

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/report.proto:65: message Resource {
Reading the code, it seems to me that we can merge Resource and Node into one
message. That way, we don't need to have a NodeMap and ResourceMap in
MalwareDetails and the code will be simpler. Is it right or might I miss
something?

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/report.proto:79: repeated int32 child_ids = 3;
I don't see this is used?

[panayiotis, 2011-01-13 02:25:00]

Thanks Lei, please have another look.

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/malware_details.cc (right):

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/malware_details.cc:73: FindOrCreateResource(url,
&url_resource, &url_node);
On 2011/01/12 22:30:15, lzheng wrote:
> Can we break this function to to? Find() and Create()?

It's now quite shorter, do you think it still needs to be broken into two?

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/malware_details.h (right):

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/malware_details.h:40: typedef base::hash_map<
On 2011/01/12 22:30:15, lzheng wrote:
> How about a little bit more verbose like "Map url to Resource."? Same below
for
> id -> Node?

Done.

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/malware_details.h:78: // a (Url, id) pair.
On 2011/01/12 22:30:15, lzheng wrote:
> Resource also contains http response and http request, right?

Done.

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/report.proto (right):

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/report.proto:65: message Resource {
You are right, it is much simpler like this. 

On 2011/01/12 22:30:15, lzheng wrote:
> Reading the code, it seems to me that we can merge Resource and Node into one
> message. That way, we don't need to have a NodeMap and ResourceMap in
> MalwareDetails and the code will be simpler. Is it right or might I miss
> something?

http://codereview.chromium.org/6208003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/report.proto:79: repeated int32 child_ids = 3;
On 2011/01/12 22:30:15, lzheng wrote:
> I don't see this is used?

This is not used yet, like many other fields in the protocol, but as soon as we
get the DOM parser we'll use it.

[lzheng, 2011-01-13 20:18:33]

http://codereview.chromium.org/6208003/diff/25001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/malware_details.cc (right):

http://codereview.chromium.org/6208003/diff/25001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/malware_details.cc:59: CHECK(url_resource != NULL);
nit: Do you still feel the CHECK here and below is necessary?

[panayiotis, 2011-01-13 21:08:39]

No, removed.

On Thu, Jan 13, 2011 at 12:18 PM,  <lzheng@chromium.org> wrote:
>
>
http://codereview.chromium.org/6208003/diff/25001/chrome/browser/safe_browsin...
> File chrome/browser/safe_browsing/malware_details.cc (right):
>
>
http://codereview.chromium.org/6208003/diff/25001/chrome/browser/safe_browsin...
> chrome/browser/safe_browsing/malware_details.cc:59: CHECK(url_resource
> != NULL);
> nit: Do you still feel the CHECK here and below is necessary?
>
> http://codereview.chromium.org/6208003/
>

[lzheng, 2011-01-13 22:18:25]

LGTM