Small modifications to safebrowsing code to make it simpler to add the extension

Small modifications to safebrowsing code to make it simpler to add the extension
blacklist, and clean up a few things.


BUG=154149
TBR=cbentzel@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177719

[not at google - send to devlin, 2012-12-18 00:48:46]

Hi Scott, here is the non-extension specific part of that code review pulled
out.

https://codereview.chromium.org/11615011/diff/2001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/database_manager.h (right):

https://codereview.chromium.org/11615011/diff/2001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/database_manager.h:69: std::map<SBFullHash,
SBThreatType> full_hash_threats;
Needed because extensions will be checking multiple hashes.

https://codereview.chromium.org/11615011/diff/2001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/database_manager.h:74: safe_browsing_util::ListType
check_type;  // See comment in constructor.
Needed because it's not just URL or download anymore; it's URL or download or
extension, and this was getting in the way. I'm not 100% happy with the way I've
done this.

https://codereview.chromium.org/11615011/diff/2001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/database_manager.h:377: base::TimeDelta
download_url_check_timeout_;
Cleanup.

https://codereview.chromium.org/11615011/diff/2001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/11615011/diff/2001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_database.cc:601: return
MatchAddPrefixes(download_store_.get(),
Extension store will call into this function.

https://codereview.chromium.org/11615011/diff/2001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/11615011/diff/2001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_database.h:372:
std::set<safe_browsing_util::ListType> changes_detected_;
Not sure about this change. It means that the Update*Store calls can be a bit
targeted, but the original motivation for the change (making the notifications
targeted) I have removed at your request. Not sure it's worthwhile sticking with
it, but it was easier to leave in than revert.

[not at google - send to devlin, 2012-12-18 17:20:22]

https://codereview.chromium.org/11615011/diff/2001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/11615011/diff/2001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_database.cc:1026: if
(changes_detected_.count(safe_browsing_util::BINURL) ||
Turns out this stuff actually breaks a test
(SafeBrowsingDatabaseTest.Whitelists). Oops.

Another test is failing too, though for a different reason
(DownloadProtectionServiceTest.TestCheckDownloadUrl) I'll look into it in a sec.

[not at google - send to devlin, 2013-01-10 01:02:41]

Coming back to this work post-holiday. Fixed tests. PTAL.

[Scott Hess - ex-Googler, 2013-01-11 23:44:04]

Sorry for the long delay.  I'm been dreading looking at safe-browsing code.

I'm still not completely 100% on the "track what list you're checking for"
change, probably because it's starting from a horrible place from the get-go. 
It feels like the type of check should be implicit in the client making the
check, rather than manually dispatching this way.  Maybe I should go rewrite it
all in terms of callbacks or something.

https://codereview.chromium.org/11615011/diff/2001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/database_manager.h (right):

https://codereview.chromium.org/11615011/diff/2001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/database_manager.h:69: std::map<SBFullHash,
SBThreatType> full_hash_threats;
On 2012/12/18 00:48:46, kalman wrote:
> Needed because extensions will be checking multiple hashes.

I haven't gotten far enough to see why this would be, but couldn't it just be a
vector in parallel with the source, rather than a map?

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:98:
GetMaxThreatType(check.url_threats));
If url_threats were a parallel vector pre-filled with SB_THREAT_TYPE_SAFE, then
this would just be url_threats[0].

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:102:
GetMaxThreatType(check.url_threats));
And this would be *max_element(url_threats.begin(), url_threads.end()).

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:113:
GetMaxThreatType(check.full_hash_threats));
Likewise here, just a different vector.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:137:
base::TimeDelta::FromMilliseconds(kCheckTimeoutMs)) {
If they're going to be the same, do they need to be distinct settings?  AFAICT,
the only reason the instance variables are needed is so tests can reach in and
mess with them, but it looks like those tests won't care if these are a single
timeout or separate timeouts.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/database_manager.h (right):

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.h:59: explicit
SafeBrowsingCheck(safe_browsing_util::ListType check_type);
Have you talked to the server people about this?  AFAICT, this only matters if
there is a full-hash collision, which should be very unlikely.  If it's deemed
likely enough to be worth protecting against, then we need to allow per-list
queries.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.h:68: std::map<SBFullHash,
SBThreatType> full_hash_threats;
Having these maps seems over-engineered.  I think you could either:
 - use parallel vectors<SBThreadType> pre-filled with SB_THREAT_TYPE_SAFE.
 - make the vectors above be maps, with default value SB_THREAD_TYPE_SAFE.
 - make the above vectors of pairs, like vector<pair<GURL,SBThreatType>>.

Of course, the latter is exactly like the map version, just more efficient for
storage, and less general for searching.  But it's not clear if searching is
very important for these structures, and they're short.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_protection_service.cc:302:
CHECK(database_manager_);
Are you certain?  The code in DownloadProtectionService clearly expects that
NULL database_manager_ is a possibility.  It would be NULL if FULL_SAFE_BROWSING
were not defined.  AFAICT, that applies to iOS and Android, but I don't know
whether in those cases the code would never arrive here, or if the short-circuit
in StartCheck() is expected to take care of it.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/download_protection_service_unittest.cc
(right):

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:290:
result_.Pass();
I think this is like 57% too elaborate, given that you're just storing an enum. 
I think something like this might be cleaner on the eyes:

if (!saved_result)
  return testing::AssertionFailure() << "No result";
bool ret = expected != *saved_result;
saved_result.reset();
if (!ret) {
  return testing::AssertionFailure() <<
            "Expected " << expected << ", got " << *saved_result;
}
return testing::AssertionSuccess();

but, honestly, I think converting result_ to a bool to indicate "is set" plus
the value set would be an improvement.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.cc:147:
prefix_hits->clear();
I don't think having to test prefix_hits and having to track found_match is a
material improvement over having the caller make a fake prefix_hits.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.h:272: // or
csd_whitelist_store_ based on list_id.
Egregious formatting change is egregious.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.h:275: // Deletes the files
on disk.
Though this one isn't.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.h:372:
std::set<safe_browsing_util::ListType> changes_detected_;
As with the earlier CL we were discussing, I still don't agree with this change.
 The fact that the database is split across multiple stores is not something the
caller should be aware of, and I'd prefer not to start making it aware of such
things unless it's really necessary.

From what I recall, you wanted that information so that someone could skip
rebuilding a data structure.  Note that all of the other data doesn't have an
external data structure which gets out of sync, because those data structures
are internal to the database structure.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_util.h (right):

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_util.h:12: #include <set>
Is set used in this header?

[not at google - send to devlin, 2013-01-14 23:00:54]

https://codereview.chromium.org/11615011/diff/2001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/database_manager.h (right):

https://codereview.chromium.org/11615011/diff/2001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/database_manager.h:69: std::map<SBFullHash,
SBThreatType> full_hash_threats;
On 2013/01/11 23:44:05, shess wrote:
> On 2012/12/18 00:48:46, kalman wrote:
> > Needed because extensions will be checking multiple hashes.
> 
> I haven't gotten far enough to see why this would be, but couldn't it just be
a
> vector in parallel with the source, rather than a map?

Done.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:98:
GetMaxThreatType(check.url_threats));
On 2013/01/11 23:44:05, shess wrote:
> If url_threats were a parallel vector pre-filled with SB_THREAT_TYPE_SAFE,
then
> this would just be url_threats[0].

Done.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:102:
GetMaxThreatType(check.url_threats));
On 2013/01/11 23:44:05, shess wrote:
> And this would be *max_element(url_threats.begin(), url_threads.end()).

Done.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:113:
GetMaxThreatType(check.full_hash_threats));
On 2013/01/11 23:44:05, shess wrote:
> Likewise here, just a different vector.

Done.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:137:
base::TimeDelta::FromMilliseconds(kCheckTimeoutMs)) {
On 2013/01/11 23:44:05, shess wrote:
> If they're going to be the same, do they need to be distinct settings? 
AFAICT,
> the only reason the instance variables are needed is so tests can reach in and
> mess with them, but it looks like those tests won't care if these are a single
> timeout or separate timeouts.

No idea, I assumed it was this way for some good reason but I'll change it.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/database_manager.h (right):

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.h:59: explicit
SafeBrowsingCheck(safe_browsing_util::ListType check_type);
On 2013/01/11 23:44:05, shess wrote:
> Have you talked to the server people about this?  AFAICT, this only matters if
> there is a full-hash collision, which should be very unlikely.  If it's deemed
> likely enough to be worth protecting against, then we need to allow per-list
> queries.

As discussed (err, I think) this is just for knowing which of Client's callback
methods to run. If we just combined all the callbacks into a single ohe, this
would be moot (though you'd go back to needing that "is_download" thing for
accounting in ProtocolManager). I'm going to leave it the way it is here but if
you would prefer me to change it (e.g. combine the callbacks, make it a callback
itself, etc) then I'm happy to change it. In the meantime I re-wrote the
comment.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.h:68: std::map<SBFullHash,
SBThreatType> full_hash_threats;
On 2013/01/11 23:44:05, shess wrote:
> Having these maps seems over-engineered.  I think you could either:
>  - use parallel vectors<SBThreadType> pre-filled with SB_THREAT_TYPE_SAFE.
>  - make the vectors above be maps, with default value SB_THREAD_TYPE_SAFE.
>  - make the above vectors of pairs, like vector<pair<GURL,SBThreatType>>.
> 
> Of course, the latter is exactly like the map version, just more efficient for
> storage, and less general for searching.  But it's not clear if searching is
> very important for these structures, and they're short.

I like the vector<pair<>> one, but it's much simpler to implement this stuff
with parallel vectors so I'll just do it that way.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_protection_service.cc:302:
CHECK(database_manager_);
On 2013/01/11 23:44:05, shess wrote:
> Are you certain?  The code in DownloadProtectionService clearly expects that
> NULL database_manager_ is a possibility.  It would be NULL if
FULL_SAFE_BROWSING
> were not defined.  AFAICT, that applies to iOS and Android, but I don't know
> whether in those cases the code would never arrive here, or if the
short-circuit
> in StartCheck() is expected to take care of it.

Yeah, don't know why I made this change.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/download_protection_service_unittest.cc
(right):

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:290:
result_.Pass();
On 2013/01/11 23:44:05, shess wrote:
> I think this is like 57% too elaborate, given that you're just storing an
enum. 
> I think something like this might be cleaner on the eyes:
> 
> if (!saved_result)
>   return testing::AssertionFailure() << "No result";
> bool ret = expected != *saved_result;
> saved_result.reset();
> if (!ret) {
>   return testing::AssertionFailure() <<
>             "Expected " << expected << ", got " << *saved_result;
> }
> return testing::AssertionSuccess();
> 
> but, honestly, I think converting result_ to a bool to indicate "is set" plus
> the value set would be an improvement.

I'll make it use a bool flag.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.cc:147:
prefix_hits->clear();
On 2013/01/11 23:44:05, shess wrote:
> I don't think having to test prefix_hits and having to track found_match is a
> material improvement over having the caller make a fake prefix_hits.

Done.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.h:272: // or
csd_whitelist_store_ based on list_id.
On 2013/01/11 23:44:05, shess wrote:
> Egregious formatting change is egregious.

Done.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.h:372:
std::set<safe_browsing_util::ListType> changes_detected_;
On 2013/01/11 23:44:05, shess wrote:
> As with the earlier CL we were discussing, I still don't agree with this
change.
>  The fact that the database is split across multiple stores is not something
the
> caller should be aware of, and I'd prefer not to start making it aware of such
> things unless it's really necessary.
> 
> From what I recall, you wanted that information so that someone could skip
> rebuilding a data structure.  Note that all of the other data doesn't have an
> external data structure which gets out of sync, because those data structures
> are internal to the database structure.

The "external data structure" for extensions is the set of extensions for that
profile (ExtensionService::extensions()) so it's not really a question of
maintaining that data structure in here, if that's what you mean.

I'd just like to avoid having to loop over all installed extensions every time
some other list changes. Admittedly I can't predict how often the list is going
to be updated compared to others, but instinctively it's going to be much less
frequently.

So - if it's exposing the list type - another option would be to add a very
specific event like OnExtensionsBlacklistUpdated, and not need to expose the
actual ListType enum values to callers.

Either way, sounds like I should pull this change out of this patch. It's only
here because I was trying to make my next patch pure cargo cult as requested.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_util.h (right):

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_util.h:12: #include <set>
On 2013/01/11 23:44:05, shess wrote:
> Is set used in this header?

Done.

[not at google - send to devlin, 2013-01-17 16:30:42]

ping

[Scott Hess - ex-Googler, 2013-01-17 19:24:42]

LGTM, with the changes to download_protection_service.cc reverted.

The database_manager.cc comment comment you can take or leave, just a
suggestion.

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/11615011/diff/30001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_database.h:372:
std::set<safe_browsing_util::ListType> changes_detected_;
On 2013/01/14 23:00:55, kalman wrote:
> On 2013/01/11 23:44:05, shess wrote:
> > As with the earlier CL we were discussing, I still don't agree with this
> change.
> >  The fact that the database is split across multiple stores is not something
> the
> > caller should be aware of, and I'd prefer not to start making it aware of
such
> > things unless it's really necessary.
> > 
> > From what I recall, you wanted that information so that someone could skip
> > rebuilding a data structure.  Note that all of the other data doesn't have
an
> > external data structure which gets out of sync, because those data
structures
> > are internal to the database structure.
> 
> The "external data structure" for extensions is the set of extensions for that
> profile (ExtensionService::extensions()) so it's not really a question of
> maintaining that data structure in here, if that's what you mean.
> 
> I'd just like to avoid having to loop over all installed extensions every time
> some other list changes. Admittedly I can't predict how often the list is
going
> to be updated compared to others, but instinctively it's going to be much less
> frequently.

The update frequency is once every 45 minutes, and something like half the
updates are empty.  Non-empty updates already involve a somewhat substantial
operation to update the database, which is likely to very much dominate the
costs.  Furthermore, the iterate-over-extensions code already has to be correct
and should be reasonably fast (like if the state of matching didn't change,
there should be no work for that extension).  So mostly I don't want to add
additional API knobs to preemptively optimize against something which shouldn't
anyhow be a notable fraction of performance costs.

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:78: DCHECK(!urls.empty() ||
!full_hashes.empty());
I don't think the meaning of the check is obvious, maybe a comment is in order.

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_protection_service.cc:306: if
(database_manager_->CheckDownloadUrl(info_.download_url_chain, this)) {
Per earlier discussion, I think you need to put back the NULL test for
database_manager_.

Actually, at that point the whitespace change becomes egregious, so maybe just
revert this entire file.

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/download_protection_service_unittest.cc
(right):

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:305: bool
has_result_;
Yeah, I think that improved it.  A pointer to an enum seems silly.  I suppose
the enum could have gotten a NULL/unset value added, but probably not
worthwhile.

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:77:
check.url_results[0] = badurls[gurl.spec()];
I see that this is the same as before, just wanted to double-check that SAFE is
0?

[not at google - send to devlin, 2013-01-18 01:02:42]

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/database_manager.cc:78: DCHECK(!urls.empty() ||
!full_hashes.empty());
On 2013/01/17 19:24:42, shess wrote:
> I don't think the meaning of the check is obvious, maybe a comment is in
order.

Done.

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_protection_service.cc:306: if
(database_manager_->CheckDownloadUrl(info_.download_url_chain, this)) {
On 2013/01/17 19:24:42, shess wrote:
> Per earlier discussion, I think you need to put back the NULL test for
> database_manager_.
> 
> Actually, at that point the whitespace change becomes egregious, so maybe just
> revert this entire file.

Done.

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:77:
check.url_results[0] = badurls[gurl.spec()];
On 2013/01/17 19:24:42, shess wrote:
> I see that this is the same as before, just wanted to double-check that SAFE
is
> 0?

I don't parse comment, sorry...

[commit-bot: I haz the power, 2013-01-18 01:08:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/11615011/47002

[Scott Hess - ex-Googler, 2013-01-18 01:27:52]

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

https://codereview.chromium.org/11615011/diff/33002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:77:
check.url_results[0] = badurls[gurl.spec()];
On 2013/01/18 01:02:42, kalman wrote:
> On 2013/01/17 19:24:42, shess wrote:
> > I see that this is the same as before, just wanted to double-check that SAFE
> is
> > 0?
> 
> I don't parse comment, sorry...

badurls[gurl.spec()] will be 0 for anything that isn't specifically set, so this
won't work if the SAFE setting isn't 0.  Now that I think about it, though, the
tests will fail, so someone will look at it, so it will fail safely, and
handling the non-issue would make the code grosser.

[commit-bot: I haz the power, 2013-01-18 02:47:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/11615011/47002

[commit-bot: I haz the power, 2013-01-18 08:25:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/11615011/49013

[commit-bot: I haz the power, 2013-01-18 09:09:35]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-18 16:45:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/11615011/59004

[commit-bot: I haz the power, 2013-01-18 18:48:09]

Change committed as 177719