Fetch extension blacklist states from SafeBrowsing server

BUG=267514
Design doc: https://docs.google.com/a/google.com/document/d/1oDS8nT2hj6tOAnhK-sZpK5sIhfc3RPaapzRw1Fb0sng

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

[not at google - send to devlin, 2013-11-18 17:03:49]

Sorry it took me so long to get to this.

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
File chrome/browser/extensions/blacklist.cc (right):

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist.cc:158: Blacklist::Blacklist(ExtensionPrefs*
prefs) : fetcher_() {
fetcher_() is implicit?

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist.cc:259: }
preferred the way it was

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist.cc:268: const std::set<std::string> ids,
base::Callback<void()> callback) {
Both of these should have been const&; in the latter case I wasn't aware, but in
reading the callback.h documentation I saw this:
https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&sq...

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist.cc:272: }
no {} around single-line if bodies

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist.cc:284: void
Blacklist::OnBlacklistStateReceived(std::string id, BlacklistState state) {
const std::string&

base::Bind can handle it.

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist.cc:288: StateRequestsList::iterator
requests_it = state_requests_.begin();
I'm not sure you need state_requests_ at all. BlacklistFetcher knows how to
coalesce fetches. You do caching. If you pass around the result map with a
callback and a request count, then run the callback when size(result map) ==
request count, you'd save the effort of looping over all state requests every
time any response comes back... and this class would be carrying around less
state, which is nice.

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist.cc:297: blacklist_state_cache_.end()) {
since you're not holding onto the iterator result of find(), you could use
blacklist_state_cache_.count(*ids_it) here.

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
File chrome/browser/extensions/blacklist.h (right):

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist.h:108: void
SetBlacklistFetcherForTest(BlacklistFetcher* fetcher);
could you make this part of the ScopedDatabaseManagerForTest class? Perhaps
rename that TestScope, then make it take 2 arguments, the database manager and
the fetcher?

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist.h:142: scoped_ptr<BlacklistFetcher>
fetcher_;
state_fetcher? Otherwise it may imply that this is responsible for fetching the
actual whole blacklist.

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist.h:148: StateRequestsList state_requests_;
The usage of |state_requests_| is non-trivial so a comment would be nice.

I suppose the same could be said for |blacklist_state_cache_|.

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
File chrome/browser/extensions/blacklist_fetcher.cc (right):

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist_fetcher.cc:34: FROM_HERE,
base::Bind(callback, Blacklist::NOT_BLACKLISTED));
are you sure you shouldn't wait until its initialized before responding?

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist_fetcher.cc:39: bool request_already_sent =
callbacks_.find(id) != callbacks_.end();
likewise since you're not using the iterator result of callbacks_.find you could
do callbacks._count(id) > 0 here.

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist_fetcher.cc:50: GURL request_url =
RequestUrl();
inline this?

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
File chrome/browser/extensions/blacklist_fetcher.h (right):

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist_fetcher.h:19: class BlacklistFetcher :
public net::URLFetcherDelegate {
comment

also I made this comment earlier, but should this be BlacklistStateFetcher,
since the actual blacklist fetching happens elsewhere?

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist_fetcher.h:47: // Callbacks by
extension ID?

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
File chrome/browser/extensions/blacklist_fetcher_unittest.cc (right):

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist_fetcher_unittest.cc:23: }
doesn't seem much point in defining an empty SetUp?

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
File chrome/browser/extensions/blacklist_unittest.cc (right):

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist_unittest.cc:81: if (states_.find(id) !=
states_.end()) {
likewise you could use count here. but since you do actually access states_[id]
you could hold onto the result of find.

also nit, single-line if.

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist_unittest.cc:96: bool delayed_;
not used

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist_unittest.cc:104: const
Blacklist::BlacklistStateMap& from) {
nit: indent

though could you templatise Assign rather than making two methods?

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist_unittest.cc:236: EXPECT_EQ(states_abc.end(),
states_abc.find(e));
colud use EXPECT_EQ(0, states_abc.count(e)) here.

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist_unittest.cc:237: EXPECT_EQ(states_bcd.end(),
states_bcd.find(e));
could you also test a purely cached response?

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist_unittest.cc:266: // Two fetchers should be
created.
indent

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist_unittest.cc:272: EXPECT_EQ(states.end(),
states.find(c));
likewise caching

[not at google - send to devlin, 2013-11-18 18:21:28]

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
File chrome/browser/extensions/blacklist_fetcher.h (right):

https://codereview.chromium.org/49253005/diff/200001/chrome/browser/extension...
chrome/browser/extensions/blacklist_fetcher.h:43: std::map<const
net::URLFetcher*, std::string> requests_;
also, I think you need to delete all of the unresolved net::URLFetchers when the
BlacklistFetcher is destroyed.

[Oleg Eterevsky, 2013-11-20 12:58:44]

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
File chrome/browser/extensions/blacklist.cc (right):

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:158: Blacklist::Blacklist(ExtensionPrefs*
prefs) : fetcher_() {
On 2013/11/18 17:03:49, kalman wrote:
> fetcher_() is implicit?

Removed.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:259: }
On 2013/11/18 17:03:49, kalman wrote:
> preferred the way it was

Must have added something and then removed. Changed back.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:268: const std::set<std::string> ids,
base::Callback<void()> callback) {
On 2013/11/18 17:03:49, kalman wrote:
> Both of these should have been const&; in the latter case I wasn't aware, but
in
> reading the callback.h documentation I saw this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&sq...

Done.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:272: }
On 2013/11/18 17:03:49, kalman wrote:
> no {} around single-line if bodies

Done.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:284: void
Blacklist::OnBlacklistStateReceived(std::string id, BlacklistState state) {
On 2013/11/18 17:03:49, kalman wrote:
> const std::string&
> 
> base::Bind can handle it.

Done.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:288: StateRequestsList::iterator
requests_it = state_requests_.begin();
I'm a bit reluctant about it. To do this we'll have to pass BlacklistStateMap&
to this function and I am not entirely sure, what will happen to non-const value
like that in the callback. Who will free it once it is not use? Do you propose
to make it scoped_refptr? I feel safer when I have some state in the class as
opposed to some curried callback.

Also in most cases there sholdn't be more than 1 simultaneous request, so the
linear search over requests shouldn't be inefficient.

On 2013/11/18 17:03:49, kalman wrote:
> I'm not sure you need state_requests_ at all. BlacklistFetcher knows how to
> coalesce fetches. You do caching. If you pass around the result map with a
> callback and a request count, then run the callback when size(result map) ==
> request count, you'd save the effort of looping over all state requests every
> time any response comes back... and this class would be carrying around less
> state, which is nice.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:297: blacklist_state_cache_.end()) {
On 2013/11/18 17:03:49, kalman wrote:
> since you're not holding onto the iterator result of find(), you could use
> blacklist_state_cache_.count(*ids_it) here.

I just googled "find vs count stl" and people tend to find find() more readable.
It is also more compatible with other types of containers.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
File chrome/browser/extensions/blacklist.h (right):

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.h:108: void
SetBlacklistFetcherForTest(BlacklistFetcher* fetcher);
On 2013/11/18 17:03:49, kalman wrote:
> could you make this part of the ScopedDatabaseManagerForTest class? Perhaps
> rename that TestScope, then make it take 2 arguments, the database manager and
> the fetcher?

I am not sure, but I don't think it'll make the code simpler. The
DatabaseManager is a separate global object (g_database_manager), and if I
understand correctly, ScopedDatabaseManager is created to manage the temporary
stub. On the other hand, BlacklistFetcher always belongs to Blacklist and is
deleted when Blacklist object is destroyed in any case, so just replacing it
like this seems much more straightforward to me.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.h:142: scoped_ptr<BlacklistFetcher>
fetcher_;
On 2013/11/18 17:03:49, kalman wrote:
> state_fetcher? Otherwise it may imply that this is responsible for fetching
the
> actual whole blacklist.

Done.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.h:148: StateRequestsList state_requests_;
On 2013/11/18 17:03:49, kalman wrote:
> The usage of |state_requests_| is non-trivial so a comment would be nice.
> 
> I suppose the same could be said for |blacklist_state_cache_|.

Done.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
File chrome/browser/extensions/blacklist_fetcher.cc (right):

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_fetcher.cc:34: FROM_HERE,
base::Bind(callback, Blacklist::NOT_BLACKLISTED));
On 2013/11/18 17:03:49, kalman wrote:
> are you sure you shouldn't wait until its initialized before responding?

I was under impression that if safe_browsing_service() returns NULL, then
SafeBrowsing is turned off completely. If SafeBrowsingService was not created by
the time safe_browsing_service() is called, it is created synchronously and
returned
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...)
unless SafeBrowsing is turned off.

That said, I think, I should return BLACKLISTED_UNKNOWN here.

Also added a comment.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_fetcher.cc:39: bool request_already_sent =
callbacks_.find(id) != callbacks_.end();
On 2013/11/18 17:03:49, kalman wrote:
> likewise since you're not using the iterator result of callbacks_.find you
could
> do callbacks._count(id) > 0 here.

Ditto.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_fetcher.cc:50: GURL request_url =
RequestUrl();
On 2013/11/18 17:03:49, kalman wrote:
> inline this?

It looks more readable like this, and the compiler will inline it anyway.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
File chrome/browser/extensions/blacklist_fetcher.h (right):

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_fetcher.h:19: class BlacklistFetcher :
public net::URLFetcherDelegate {
On 2013/11/18 17:03:49, kalman wrote:
> comment
> 
> also I made this comment earlier, but should this be BlacklistStateFetcher,
> since the actual blacklist fetching happens elsewhere?

Done.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_fetcher.h:43: std::map<const
net::URLFetcher*, std::string> requests_;
On 2013/11/18 18:21:28, kalman wrote:
> also, I think you need to delete all of the unresolved net::URLFetchers when
the
> BlacklistFetcher is destroyed.

Done. Nice catch.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_fetcher.h:47: // Callbacks by
On 2013/11/18 17:03:49, kalman wrote:
> extension ID?

Done.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
File chrome/browser/extensions/blacklist_fetcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_fetcher_unittest.cc:23: }
On 2013/11/18 17:03:49, kalman wrote:
> doesn't seem much point in defining an empty SetUp?

Done.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
File chrome/browser/extensions/blacklist_unittest.cc (right):

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_unittest.cc:81: if (states_.find(id) !=
states_.end()) {
On 2013/11/18 17:03:49, kalman wrote:
> likewise you could use count here. but since you do actually access
states_[id]
> you could hold onto the result of find.

If we were using C++11, I'd write
  (auto it = state_.find(it)) != states_.end()

> also nit, single-line if.

Done.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_unittest.cc:96: bool delayed_;
On 2013/11/18 17:03:49, kalman wrote:
> not used

Done.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_unittest.cc:104: const
Blacklist::BlacklistStateMap& from) {
On 2013/11/18 17:03:49, kalman wrote:
> nit: indent
> 
> though could you templatise Assign rather than making two methods?

Done.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_unittest.cc:236: EXPECT_EQ(states_abc.end(),
states_abc.find(e));
On 2013/11/18 17:03:49, kalman wrote:
> colud use EXPECT_EQ(0, states_abc.count(e)) here.

Same find vs count as before.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_unittest.cc:237: EXPECT_EQ(states_bcd.end(),
states_bcd.find(e));
On 2013/11/18 17:03:49, kalman wrote:
> could you also test a purely cached response?

Done.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_unittest.cc:266: // Two fetchers should be
created.
On 2013/11/18 17:03:49, kalman wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_unittest.cc:272: EXPECT_EQ(states.end(),
states.find(c));
On 2013/11/18 17:03:49, kalman wrote:
> likewise caching

Done.

[not at google - send to devlin, 2013-11-20 23:44:47]

Just the question about the base::Owned thing. I do find
Blacklist::OnBlacklistStateReceived rather hard to read and I feel it'd be nicer
without blacklist_state_cache_, but I won't push on it any further.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
File chrome/browser/extensions/blacklist.cc (right):

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:288: StateRequestsList::iterator
requests_it = state_requests_.begin();
On 2013/11/20 12:58:45, Oleg wrote:
> I'm a bit reluctant about it. To do this we'll have to pass BlacklistStateMap&
> to this function and I am not entirely sure, what will happen to non-const
value
> like that in the callback. Who will free it once it is not use? Do you propose
> to make it scoped_refptr? I feel safer when I have some state in the class as
> opposed to some curried callback.
> 
> Also in most cases there sholdn't be more than 1 simultaneous request, so the
> linear search over requests shouldn't be inefficient.
> 
> On 2013/11/18 17:03:49, kalman wrote:
> > I'm not sure you need state_requests_ at all. BlacklistFetcher knows how to
> > coalesce fetches. You do caching. If you pass around the result map with a
> > callback and a request count, then run the callback when size(result map) ==
> > request count, you'd save the effort of looping over all state requests
every
> > time any response comes back... and this class would be carrying around less
> > state, which is nice.
> 

You would pass around a pointer with base::Owned(). Fair point about probably
only being called once, though I prefer it if the class didn't maintain
transient state like that, IMO it's nicer to curry transient state where
possible. I think the code would be simpler overall as well.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:297: blacklist_state_cache_.end()) {
On 2013/11/20 12:58:45, Oleg wrote:
> On 2013/11/18 17:03:49, kalman wrote:
> > since you're not holding onto the iterator result of find(), you could use
> > blacklist_state_cache_.count(*ids_it) here.
> 
> I just googled "find vs count stl" and people tend to find find() more
readable.
> It is also more compatible with other types of containers.

I'm told you should use ContainsKey:

https://code.google.com/p/chromium/codesearch#chromium/src/base/stl_util.h&q=...

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
File chrome/browser/extensions/blacklist.h (right):

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.h:108: void
SetBlacklistFetcherForTest(BlacklistFetcher* fetcher);
On 2013/11/20 12:58:45, Oleg wrote:
> On 2013/11/18 17:03:49, kalman wrote:
> > could you make this part of the ScopedDatabaseManagerForTest class? Perhaps
> > rename that TestScope, then make it take 2 arguments, the database manager
and
> > the fetcher?
> 
> I am not sure, but I don't think it'll make the code simpler. The
> DatabaseManager is a separate global object (g_database_manager), and if I
> understand correctly, ScopedDatabaseManager is created to manage the temporary
> stub. On the other hand, BlacklistFetcher always belongs to Blacklist and is
> deleted when Blacklist object is destroyed in any case, so just replacing it
> like this seems much more straightforward to me.

Right, good point.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
File chrome/browser/extensions/blacklist_fetcher.cc (right):

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_fetcher.cc:34: FROM_HERE,
base::Bind(callback, Blacklist::NOT_BLACKLISTED));
On 2013/11/20 12:58:45, Oleg wrote:
> On 2013/11/18 17:03:49, kalman wrote:
> > are you sure you shouldn't wait until its initialized before responding?
> 
> I was under impression that if safe_browsing_service() returns NULL, then
> SafeBrowsing is turned off completely. If SafeBrowsingService was not created
by
> the time safe_browsing_service() is called, it is created synchronously and
> returned
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...)
> unless SafeBrowsing is turned off.
> 
> That said, I think, I should return BLACKLISTED_UNKNOWN here.
> 
> Also added a comment.

Yes you're right, I misread this a little bit, sorry.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist_fetcher.cc:50: GURL request_url =
RequestUrl();
On 2013/11/20 12:58:45, Oleg wrote:
> On 2013/11/18 17:03:49, kalman wrote:
> > inline this?
> 
> It looks more readable like this, and the compiler will inline it anyway.

It's not about inlining it's about readability, so I guess we disagree on that.

[mattm, 2013-11-21 00:04:01]

I feel like I'm missing some background here.  Can you set a commit description
on the issue?  Is there a bug for this? A design doc?

[Oleg Eterevsky, 2013-11-21 16:53:13]

Matt, sorry for not linking the issue, thought it was in the description. Add
the link to it, and also to the design doc.

I've just notices that some tests are failing in ExtensionService, so expect
some further (hopefully) small fixes.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
File chrome/browser/extensions/blacklist.cc (right):

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:288: StateRequestsList::iterator
requests_it = state_requests_.begin();
On 2013/11/20 23:44:48, kalman wrote:
> On 2013/11/20 12:58:45, Oleg wrote:
> > I'm a bit reluctant about it. To do this we'll have to pass
BlacklistStateMap&
> > to this function and I am not entirely sure, what will happen to non-const
> value
> > like that in the callback. Who will free it once it is not use? Do you
propose
> > to make it scoped_refptr? I feel safer when I have some state in the class
as
> > opposed to some curried callback.
> > 
> > Also in most cases there sholdn't be more than 1 simultaneous request, so
the
> > linear search over requests shouldn't be inefficient.
> > 
> > On 2013/11/18 17:03:49, kalman wrote:
> > > I'm not sure you need state_requests_ at all. BlacklistFetcher knows how
to
> > > coalesce fetches. You do caching. If you pass around the result map with a
> > > callback and a request count, then run the callback when size(result map)
==
> > > request count, you'd save the effort of looping over all state requests
> every
> > > time any response comes back... and this class would be carrying around
less
> > > state, which is nice.
> > 
> 
> You would pass around a pointer with base::Owned(). Fair point about probably
> only being called once, though I prefer it if the class didn't maintain
> transient state like that, IMO it's nicer to curry transient state where
> possible. I think the code would be simpler overall as well.

base::Owned won't work here, since OnBlacklistStateReceived will be called
several times if several blacklist statuses are requested, and the instances of
base::Callback will have to share this map.

I also thought of passing to OnBlacklistStateReceived the set of ids and a final
callback, but this can fail also, since if there are two concurrent requests for
the same 2 ids, the callback will be called twice.

The only possible way to deal with it is to have a reference-counted
BlacklistStateMap, and I dislike this solution.

Adding a comment on what's happening here.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:297: blacklist_state_cache_.end()) {
Replaced it here and in other places.

On 2013/11/20 23:44:48, kalman wrote:
> On 2013/11/20 12:58:45, Oleg wrote:
> > On 2013/11/18 17:03:49, kalman wrote:
> > > since you're not holding onto the iterator result of find(), you could use
> > > blacklist_state_cache_.count(*ids_it) here.
> > 
> > I just googled "find vs count stl" and people tend to find find() more
> readable.
> > It is also more compatible with other types of containers.
> 
> I'm told you should use ContainsKey:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/stl_util.h&q=...

[not at google - send to devlin, 2013-11-21 18:42:57]

Responding to comments; ping when the ExtensionServiceTests are fixed. If I
haven't convinced you at this point that taking the state out of Blacklist and
into a targeted scope is better/safer/readable... well I suppose I never will.
That's fine.

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
File chrome/browser/extensions/blacklist.cc (right):

https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:288: StateRequestsList::iterator
requests_it = state_requests_.begin();
On 2013/11/21 16:53:13, Oleg wrote:
> On 2013/11/20 23:44:48, kalman wrote:
> > On 2013/11/20 12:58:45, Oleg wrote:
> > > I'm a bit reluctant about it. To do this we'll have to pass
> BlacklistStateMap&
> > > to this function and I am not entirely sure, what will happen to non-const
> > value
> > > like that in the callback. Who will free it once it is not use? Do you
> propose
> > > to make it scoped_refptr? I feel safer when I have some state in the class
> as
> > > opposed to some curried callback.
> > > 
> > > Also in most cases there sholdn't be more than 1 simultaneous request, so
> the
> > > linear search over requests shouldn't be inefficient.
> > > 
> > > On 2013/11/18 17:03:49, kalman wrote:
> > > > I'm not sure you need state_requests_ at all. BlacklistFetcher knows how
> to
> > > > coalesce fetches. You do caching. If you pass around the result map with
a
> > > > callback and a request count, then run the callback when size(result
map)
> ==
> > > > request count, you'd save the effort of looping over all state requests
> > every
> > > > time any response comes back... and this class would be carrying around
> less
> > > > state, which is nice.
> > > 
> > 
> > You would pass around a pointer with base::Owned(). Fair point about
probably
> > only being called once, though I prefer it if the class didn't maintain
> > transient state like that, IMO it's nicer to curry transient state where
> > possible. I think the code would be simpler overall as well.
> 
> base::Owned won't work here, since OnBlacklistStateReceived will be called
> several times if several blacklist statuses are requested, and the instances
of
> base::Callback will have to share this map.
> 
> I also thought of passing to OnBlacklistStateReceived the set of ids and a
final
> callback, but this can fail also, since if there are two concurrent requests
for
> the same 2 ids, the callback will be called twice.
> 
> The only possible way to deal with it is to have a reference-counted
> BlacklistStateMap, and I dislike this solution.
> 
> Adding a comment on what's happening here.

You're right, and now that my memory is working correctly, I suggested passing
around a counter I think, which was just a crappy version of refcounting.

Honestly I don't think refcounting is that big a deal, it's a not-uncommon idiom
of these things to create a refcounted wrapper class with the map as a member
variable, populate it as callbacks come in, then send the response on
destruction of that wrapper class. Using that idiom is safe with respect to the
message loop and/or the state fetcher from going away.

[Oleg Eterevsky, 2013-11-22 16:20:25]

On 2013/11/21 18:42:57, kalman wrote:
> Responding to comments; ping when the ExtensionServiceTests are fixed. If I
> haven't convinced you at this point that taking the state out of Blacklist and
> into a targeted scope is better/safer/readable... well I suppose I never will.
> That's fine.
> 
>
https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
> File chrome/browser/extensions/blacklist.cc (right):
> 
>
https://chromiumcodereview.appspot.com/49253005/diff/200001/chrome/browser/ex...
> chrome/browser/extensions/blacklist.cc:288: StateRequestsList::iterator
> requests_it = state_requests_.begin();
> On 2013/11/21 16:53:13, Oleg wrote:
> > On 2013/11/20 23:44:48, kalman wrote:
> > > On 2013/11/20 12:58:45, Oleg wrote:
> > > > I'm a bit reluctant about it. To do this we'll have to pass
> > BlacklistStateMap&
> > > > to this function and I am not entirely sure, what will happen to
non-const
> > > value
> > > > like that in the callback. Who will free it once it is not use? Do you
> > propose
> > > > to make it scoped_refptr? I feel safer when I have some state in the
class
> > as
> > > > opposed to some curried callback.
> > > > 
> > > > Also in most cases there sholdn't be more than 1 simultaneous request,
so
> > the
> > > > linear search over requests shouldn't be inefficient.
> > > > 
> > > > On 2013/11/18 17:03:49, kalman wrote:
> > > > > I'm not sure you need state_requests_ at all. BlacklistFetcher knows
how
> > to
> > > > > coalesce fetches. You do caching. If you pass around the result map
with
> a
> > > > > callback and a request count, then run the callback when size(result
> map)
> > ==
> > > > > request count, you'd save the effort of looping over all state
requests
> > > every
> > > > > time any response comes back... and this class would be carrying
around
> > less
> > > > > state, which is nice.
> > > > 
> > > 
> > > You would pass around a pointer with base::Owned(). Fair point about
> probably
> > > only being called once, though I prefer it if the class didn't maintain
> > > transient state like that, IMO it's nicer to curry transient state where
> > > possible. I think the code would be simpler overall as well.
> > 
> > base::Owned won't work here, since OnBlacklistStateReceived will be called
> > several times if several blacklist statuses are requested, and the instances
> of
> > base::Callback will have to share this map.
> > 
> > I also thought of passing to OnBlacklistStateReceived the set of ids and a
> final
> > callback, but this can fail also, since if there are two concurrent requests
> for
> > the same 2 ids, the callback will be called twice.
> > 
> > The only possible way to deal with it is to have a reference-counted
> > BlacklistStateMap, and I dislike this solution.
> > 
> > Adding a comment on what's happening here.
> 
> You're right, and now that my memory is working correctly, I suggested passing
> around a counter I think, which was just a crappy version of refcounting.
> 
> Honestly I don't think refcounting is that big a deal, it's a not-uncommon
idiom
> of these things to create a refcounted wrapper class with the map as a member
> variable, populate it as callbacks come in, then send the response on
> destruction of that wrapper class. Using that idiom is safe with respect to
the
> message loop and/or the state fetcher from going away.

ExtensionServiceTests are fixed. The problem was that they do not initialized
the BlacklistFetcher stub, and incorrectly handled BLACKLISTED_UNKNOWN. I
changed the behavior to treat UNKNOWN as MALICIOUS in some cases.

[not at google - send to devlin, 2013-11-25 18:48:24]

lgtm

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
File chrome/browser/extensions/blacklist.cc (right):

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:309: // is returned, so no need to
increment it.
a lot of words to explain some C++ library behaviour; could you replace this
with just

requests_it = state_requests_.erase(requests_it);  // returns next element

[mattm, 2013-11-25 21:48:55]

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
File chrome/browser/extensions/blacklist.cc (right):

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:281: state_fetcher_->Request(
Have you tested this in a debug build? This function DCHECKS for UI thread and
BlacklistStateFetcher::Request DCHECKS for IO thread.

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
File chrome/browser/extensions/blacklist_state_fetcher.cc (right):

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_state_fetcher.cc:26:
BlacklistStateFetcher::~BlacklistStateFetcher() {
DCHECK the thread here too

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_state_fetcher.cc:40: // completely and there
is not point retrying.
Should there be a NOTREACHED here? Seems like it would be impossible to get here
if safebrowsing is disabled.

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_state_fetcher.cc:62: this);
Shouldn't this be using the safebrowsing URLRequestContext?

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_state_fetcher.cc:96: return;
seems like this should be a DCHECK

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_state_fetcher.cc:113: state =
Blacklist::NOT_BLACKLISTED;
shouldn't this be UNKNOWN?

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
File chrome/browser/extensions/blacklist_state_fetcher.h (right):

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_state_fetcher.h:6: #define
CHROME_BROWSER_EXTENSIONS_BLACKLIST_STATE_FETCHER_H_
kinda seems like the fetcher should live in the safe_browsing directory

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_state_fetcher.h:29: void
SetSafeBrowsingConfig(const SafeBrowsingProtocolConfig& config);
Seems cleaner to just pass the config into the constructor

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
File chrome/browser/extensions/blacklist_unittest.cc (right):

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_unittest.cc:80: virtual void Request(const
std::string& id, const RequestCallback& callback) {
OVERRIDE

[Oleg Eterevsky, 2013-11-27 14:07:37]

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
File chrome/browser/extensions/blacklist.cc (right):

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:281: state_fetcher_->Request(
On 2013/11/25 21:48:55, mattm wrote:
> Have you tested this in a debug build? This function DCHECKS for UI thread and
> BlacklistStateFetcher::Request DCHECKS for IO thread.

Done.

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist.cc:309: // is returned, so no need to
increment it.
On 2013/11/25 18:48:24, kalman wrote:
> a lot of words to explain some C++ library behaviour; could you replace this
> with just
> 
> requests_it = state_requests_.erase(requests_it);  // returns next element

Done.

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
File chrome/browser/extensions/blacklist_state_fetcher.cc (right):

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_state_fetcher.cc:26:
BlacklistStateFetcher::~BlacklistStateFetcher() {
On 2013/11/25 21:48:55, mattm wrote:
> DCHECK the thread here too

Done.

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_state_fetcher.cc:40: // completely and there
is not point retrying.
On 2013/11/25 21:48:55, mattm wrote:
> Should there be a NOTREACHED here? Seems like it would be impossible to get
here
> if safebrowsing is disabled.

Done. Left returning BLACKLISTED_UNKNOWN just in case.

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_state_fetcher.cc:62: this);
On 2013/11/25 21:48:55, mattm wrote:
> Shouldn't this be using the safebrowsing URLRequestContext?

Done.

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_state_fetcher.cc:96: return;
On 2013/11/25 21:48:55, mattm wrote:
> seems like this should be a DCHECK

Done.

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_state_fetcher.cc:113: state =
Blacklist::NOT_BLACKLISTED;
On 2013/11/25 21:48:55, mattm wrote:
> shouldn't this be UNKNOWN?

Done.

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
File chrome/browser/extensions/blacklist_state_fetcher.h (right):

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_state_fetcher.h:6: #define
CHROME_BROWSER_EXTENSIONS_BLACKLIST_STATE_FETCHER_H_
On 2013/11/25 21:48:55, mattm wrote:
> kinda seems like the fetcher should live in the safe_browsing directory

I thought about it and it seems strange to have blacklist and blacklist_fetcher
in different directories.

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_state_fetcher.h:29: void
SetSafeBrowsingConfig(const SafeBrowsingProtocolConfig& config);
On 2013/11/25 21:48:55, mattm wrote:
> Seems cleaner to just pass the config into the constructor

The config is either passed here (usually in case of unit tests), or taken from
g_browser_process->safe_browsing_service().

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
File chrome/browser/extensions/blacklist_unittest.cc (right):

https://chromiumcodereview.appspot.com/49253005/diff/560001/chrome/browser/ex...
chrome/browser/extensions/blacklist_unittest.cc:80: virtual void Request(const
std::string& id, const RequestCallback& callback) {
On 2013/11/25 21:48:55, mattm wrote:
> OVERRIDE

Done.

[mattm, 2013-12-03 02:55:45]

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
File chrome/browser/extensions/blacklist.h (right):

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
chrome/browser/extensions/blacklist.h:62: NOT_BLACKLISTED = 0,
It's pretty confusing that 0 is UNKNOWN in the proto but NOT_BLACKLISTED here,
and further there is a different value of UNKNOWN below.

It would be better to just use the enum from the proto everywhere rather than
have a duplicate definition here. (You could just put the extra
"unknown"/uninitialized value in the proto with a comment that the server should
never set that value or something).

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
File chrome/browser/extensions/blacklist_state_fetcher.h (right):

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
chrome/browser/extensions/blacklist_state_fetcher.h:6: #define
CHROME_BROWSER_EXTENSIONS_BLACKLIST_STATE_FETCHER_H_
On 2013/11/27 14:07:38, Oleg wrote:
> On 2013/11/25 21:48:55, mattm wrote:
> > kinda seems like the fetcher should live in the safe_browsing directory
> 
> I thought about it and it seems strange to have blacklist and
blacklist_fetcher
> in different directories.

Well, it's also weird that BlacklistStateFetcher depends on the safebrowsing
config and context and connects to safebrowsing servers, but isn't under the
safebrowsing directory. Also the proto is in chrome/common/safe_browsing but
only used from extensions.
I think it makes sense to keep the stuff that interacts with safebrowsing
services in safe_browsing/, that doesn't mean their users need to be in the same
directory. This is how existing cases work (ex: DownloadProtectionService &
DownloadFeedbackService live in safe_browsing but are used from downloads/)

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
chrome/browser/extensions/blacklist_state_fetcher.h:13: #include
"chrome/browser/extensions/blacklist.h"
This is just for BlacklistState? This is somewhat of a circular dep, cleaner to
remove and use the enum from the proto instead.

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
chrome/browser/extensions/blacklist_state_fetcher.h:29: void
SetSafeBrowsingConfig(const SafeBrowsingProtocolConfig& config);
On 2013/11/27 14:07:38, Oleg wrote:
> On 2013/11/25 21:48:55, mattm wrote:
> > Seems cleaner to just pass the config into the constructor
> 
> The config is either passed here (usually in case of unit tests), or taken
from
> g_browser_process->safe_browsing_service().

Right, but with the constructor the unittests can pass in their own test
versions when they create the fetcher, and in the real chrome, whoever creates
the BlacklistStateFetcher can get the values from SafeBrowsingService to pass
in.  It is cleaner to have things passed in the constructor rather than having
otherwise independent classes like this calling into globals in order to
initialize themselves.

https://codereview.chromium.org/49253005/diff/610001/chrome/browser/extension...
File chrome/browser/extensions/blacklist.cc (right):

https://codereview.chromium.org/49253005/diff/610001/chrome/browser/extension...
chrome/browser/extensions/blacklist.cc:3: // found in the LICENSE file.
It seems that nothing checks whether safebrowsing is actually enabled for the
current profile.  SafeBrowsingService, SafeBrowsingDatabaseManager, etc are
global objects.  Each profile has its own pref for whether it should use
safebrowsing: prefs::kSafeBrowsingEnabled.

[Oleg Eterevsky, 2013-12-04 10:45:10]

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
File chrome/browser/extensions/blacklist.h (right):

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
chrome/browser/extensions/blacklist.h:62: NOT_BLACKLISTED = 0,
On 2013/12/03 02:55:45, mattm wrote:
> It's pretty confusing that 0 is UNKNOWN in the proto but NOT_BLACKLISTED here,
> and further there is a different value of UNKNOWN below.
> 
> It would be better to just use the enum from the proto everywhere rather than
> have a duplicate definition here. (You could just put the extra
> "unknown"/uninitialized value in the proto with a comment that the server
should
> never set that value or something).

I understand your concern. The reason for different unknown's is as follows: the
proto was first added on server side. 'unknown' there basically mean that the
extension in question is not in the blacklist. Then in Chrome we had to use some
state when we failed to connect to server, and I added another UNKNOWN.

How about I write to Nav, who wrote the server-side, and ask him to change proto
there, and once it's done, I'll change it here?

Also, I do not like the idea of directly referring everywhere to an enum from
some proto, since it should be visible and used only by the protocol. Still I
agree that it would be nice to have this enum and proto synced.

WDYT?

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
File chrome/browser/extensions/blacklist_state_fetcher.h (right):

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
chrome/browser/extensions/blacklist_state_fetcher.h:13: #include
"chrome/browser/extensions/blacklist.h"
On 2013/12/03 02:55:45, mattm wrote:
> This is just for BlacklistState? This is somewhat of a circular dep, cleaner
to
> remove and use the enum from the proto instead.

As I said, I don't like the idea of some arbitrary extensions code depending on
crx_info.pb.h. I see two choices (besides leaving it as it is):
- Convert from proto enum to Blacklist enum in Blacklist instead of
BlacklistStateFetcher.
- Make a separate header just for this enum to avoid circular dependence. 

Also, strictly speaking, it's a circular dep since I only include blacklist.h in
blacklist_fetcher.cc, not blacklist_fether.h.

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
chrome/browser/extensions/blacklist_state_fetcher.h:29: void
SetSafeBrowsingConfig(const SafeBrowsingProtocolConfig& config);
Ok, makes sense, in case this moves to SafeBrowsing. Just to check that I
understand correctly your idea:

- BlacklistFetcher is created by and managed by SafeBrowsing service,
- Blacklist gets it from g_browser_process->safe_browsing_service()

Right?

https://codereview.chromium.org/49253005/diff/610001/chrome/browser/extension...
File chrome/browser/extensions/blacklist.cc (right):

https://codereview.chromium.org/49253005/diff/610001/chrome/browser/extension...
chrome/browser/extensions/blacklist.cc:3: // found in the LICENSE file.
On 2013/12/03 02:55:45, mattm wrote:
> It seems that nothing checks whether safebrowsing is actually enabled for the
> current profile.  SafeBrowsingService, SafeBrowsingDatabaseManager, etc are
> global objects.  Each profile has its own pref for whether it should use
> safebrowsing: prefs::kSafeBrowsingEnabled.

I followed the way how it now works with just checking if extension is disabled.
Maybe it isn't supposed to depend upon this property?

[not at google - send to devlin, 2013-12-04 17:59:52]

This code (BlacklistStateFetcher etc) should be in extensions not safebrowsing.
Safebrowsing happens to be the infrastructure we use for efficiently propagating
and storing the blacklist, but the extension blacklisting is higher-level than
that. The state fetcher is part of this higher-level interpretation of the data
that safebrowsing gives us, so should be an extension not a safebrowsing
concern.

https://codereview.chromium.org/49253005/diff/610001/chrome/browser/extension...
File chrome/browser/extensions/blacklist.cc (right):

https://codereview.chromium.org/49253005/diff/610001/chrome/browser/extension...
chrome/browser/extensions/blacklist.cc:3: // found in the LICENSE file.
On 2013/12/04 10:45:11, Oleg wrote:
> On 2013/12/03 02:55:45, mattm wrote:
> > It seems that nothing checks whether safebrowsing is actually enabled for
the
> > current profile.  SafeBrowsingService, SafeBrowsingDatabaseManager, etc are
> > global objects.  Each profile has its own pref for whether it should use
> > safebrowsing: prefs::kSafeBrowsingEnabled.
> 
> I followed the way how it now works with just checking if extension is
disabled.
> Maybe it isn't supposed to depend upon this property?

For us, safebrowsing is the infrastructure but not the product. We always want
the extension blacklist enabled if possible.

[mattm, 2013-12-10 11:09:40]

On 2013/12/04 17:59:52, kalman wrote:
> This code (BlacklistStateFetcher etc) should be in extensions not
safebrowsing.
> Safebrowsing happens to be the infrastructure we use for efficiently
propagating
> and storing the blacklist, but the extension blacklisting is higher-level than
> that. The state fetcher is part of this higher-level interpretation of the
data
> that safebrowsing gives us, so should be an extension not a safebrowsing
> concern.
Ok. I think there are arguments for both ways, but I guess this doesn't bother
me that much.

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
File chrome/browser/extensions/blacklist.h (right):

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
chrome/browser/extensions/blacklist.h:62: NOT_BLACKLISTED = 0,
On 2013/12/04 10:45:10, Oleg wrote:
> On 2013/12/03 02:55:45, mattm wrote:
> > It's pretty confusing that 0 is UNKNOWN in the proto but NOT_BLACKLISTED
here,
> > and further there is a different value of UNKNOWN below.
> > 
> > It would be better to just use the enum from the proto everywhere rather
than
> > have a duplicate definition here. (You could just put the extra
> > "unknown"/uninitialized value in the proto with a comment that the server
> should
> > never set that value or something).
> 
> I understand your concern. The reason for different unknown's is as follows:
the
> proto was first added on server side. 'unknown' there basically mean that the
> extension in question is not in the blacklist. Then in Chrome we had to use
some
> state when we failed to connect to server, and I added another UNKNOWN.
> 
> How about I write to Nav, who wrote the server-side, and ask him to change
proto
> there, and once it's done, I'll change it here?
> 
> Also, I do not like the idea of directly referring everywhere to an enum from
> some proto, since it should be visible and used only by the protocol. Still I
> agree that it would be nice to have this enum and proto synced.
> 
> WDYT?

sounds good

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
File chrome/browser/extensions/blacklist_state_fetcher.h (right):

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
chrome/browser/extensions/blacklist_state_fetcher.h:13: #include
"chrome/browser/extensions/blacklist.h"
On 2013/12/04 10:45:11, Oleg wrote:
> On 2013/12/03 02:55:45, mattm wrote:
> > This is just for BlacklistState? This is somewhat of a circular dep, cleaner
> to
> > remove and use the enum from the proto instead.
> 
> As I said, I don't like the idea of some arbitrary extensions code depending
on
> crx_info.pb.h. I see two choices (besides leaving it as it is):
> - Convert from proto enum to Blacklist enum in Blacklist instead of
> BlacklistStateFetcher.
> - Make a separate header just for this enum to avoid circular dependence. 

I guess either is fine.

> Also, strictly speaking, it's a circular dep since I only include blacklist.h
in
> blacklist_fetcher.cc, not blacklist_fether.h.

https://codereview.chromium.org/49253005/diff/610001/chrome/browser/extension...
File chrome/browser/extensions/blacklist.cc (right):

https://codereview.chromium.org/49253005/diff/610001/chrome/browser/extension...
chrome/browser/extensions/blacklist.cc:3: // found in the LICENSE file.
On 2013/12/04 17:59:53, kalman wrote:
> On 2013/12/04 10:45:11, Oleg wrote:
> > On 2013/12/03 02:55:45, mattm wrote:
> > > It seems that nothing checks whether safebrowsing is actually enabled for
> the
> > > current profile.  SafeBrowsingService, SafeBrowsingDatabaseManager, etc
are
> > > global objects.  Each profile has its own pref for whether it should use
> > > safebrowsing: prefs::kSafeBrowsingEnabled.
> > 
> > I followed the way how it now works with just checking if extension is
> disabled.
> > Maybe it isn't supposed to depend upon this property?
> 
> For us, safebrowsing is the infrastructure but not the product. We always want
> the extension blacklist enabled if possible.

Okay. Thanks for the explanation.

https://codereview.chromium.org/49253005/diff/610001/chrome/browser/extension...
File chrome/browser/extensions/blacklist_state_fetcher.cc (right):

https://codereview.chromium.org/49253005/diff/610001/chrome/browser/extension...
chrome/browser/extensions/blacklist_state_fetcher.cc:70:
scoped_refptr<net::URLRequestContextGetter> request_context(
I don't think the intermediate storing in a scoped_refptr is necessary.

[Oleg Eterevsky, 2013-12-10 17:10:43]

Based of Ben's advice I decided to to move Fetcher code to SafeBrowsing.

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
File chrome/browser/extensions/blacklist.h (right):

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
chrome/browser/extensions/blacklist.h:62: NOT_BLACKLISTED = 0,
On 2013/12/10 11:09:40, mattm wrote:
> On 2013/12/04 10:45:10, Oleg wrote:
> > On 2013/12/03 02:55:45, mattm wrote:
> > > It's pretty confusing that 0 is UNKNOWN in the proto but NOT_BLACKLISTED
> here,
> > > and further there is a different value of UNKNOWN below.
> > > 
> > > It would be better to just use the enum from the proto everywhere rather
> than
> > > have a duplicate definition here. (You could just put the extra
> > > "unknown"/uninitialized value in the proto with a comment that the server
> > should
> > > never set that value or something).
> > 
> > I understand your concern. The reason for different unknown's is as follows:
> the
> > proto was first added on server side. 'unknown' there basically mean that
the
> > extension in question is not in the blacklist. Then in Chrome we had to use
> some
> > state when we failed to connect to server, and I added another UNKNOWN.
> > 
> > How about I write to Nav, who wrote the server-side, and ask him to change
> proto
> > there, and once it's done, I'll change it here?
> > 
> > Also, I do not like the idea of directly referring everywhere to an enum
from
> > some proto, since it should be visible and used only by the protocol. Still
I
> > agree that it would be nice to have this enum and proto synced.
> > 
> > WDYT?
> 
> sounds good

Done.

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
File chrome/browser/extensions/blacklist_state_fetcher.h (right):

https://codereview.chromium.org/49253005/diff/560001/chrome/browser/extension...
chrome/browser/extensions/blacklist_state_fetcher.h:13: #include
"chrome/browser/extensions/blacklist.h"
On 2013/12/10 11:09:40, mattm wrote:
> On 2013/12/04 10:45:11, Oleg wrote:
> > On 2013/12/03 02:55:45, mattm wrote:
> > > This is just for BlacklistState? This is somewhat of a circular dep,
cleaner
> > to
> > > remove and use the enum from the proto instead.
> > 
> > As I said, I don't like the idea of some arbitrary extensions code depending
> on
> > crx_info.pb.h. I see two choices (besides leaving it as it is):
> > - Convert from proto enum to Blacklist enum in Blacklist instead of
> > BlacklistStateFetcher.
> > - Make a separate header just for this enum to avoid circular dependence. 
> 
> I guess either is fine.
> 
> > Also, strictly speaking, it's a circular dep since I only include
blacklist.h
> in
> > blacklist_fetcher.cc, not blacklist_fether.h.
> 

Done.

https://codereview.chromium.org/49253005/diff/610001/chrome/browser/extension...
File chrome/browser/extensions/blacklist_state_fetcher.cc (right):

https://codereview.chromium.org/49253005/diff/610001/chrome/browser/extension...
chrome/browser/extensions/blacklist_state_fetcher.cc:70:
scoped_refptr<net::URLRequestContextGetter> request_context(
On 2013/12/10 11:09:41, mattm wrote:
> I don't think the intermediate storing in a scoped_refptr is necessary.

Done.

[not at google - send to devlin, 2013-12-10 17:16:01]

On 2013/12/10 17:10:43, Oleg wrote:
> Based of Ben's advice I decided to to move Fetcher code to SafeBrowsing.
> 

I meant to say the opposite, that it should stay in extensions?

[olege, 2013-12-10 17:16:10]

On Tue, Dec 10, 2013 at 9:10 PM,  <oleg@chromium.org> wrote:
> Based of Ben's advice I decided to to move Fetcher code to SafeBrowsing.

* decided NOT to move

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mattm, 2013-12-10 22:58:49]

lgtm

https://codereview.chromium.org/49253005/diff/650001/chrome/browser/extension...
File chrome/browser/extensions/blacklist_state_fetcher.h (right):

https://codereview.chromium.org/49253005/diff/650001/chrome/browser/extension...
chrome/browser/extensions/blacklist_state_fetcher.h:15: #include
"chrome/common/safe_browsing/crx_info.pb.h"
Does this need to be in the header now?

[Oleg Eterevsky, 2013-12-11 13:02:53]

Thanks a lot for review!

https://codereview.chromium.org/49253005/diff/650001/chrome/browser/extension...
File chrome/browser/extensions/blacklist_state_fetcher.h (right):

https://codereview.chromium.org/49253005/diff/650001/chrome/browser/extension...
chrome/browser/extensions/blacklist_state_fetcher.h:15: #include
"chrome/common/safe_browsing/crx_info.pb.h"
On 2013/12/10 22:58:50, mattm wrote:
> Does this need to be in the header now?

Done.

[commit-bot: I haz the power, 2013-12-11 13:03:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oleg@chromium.org/49253005/670001

[commit-bot: I haz the power, 2013-12-11 13:40:35]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-11 13:57:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oleg@chromium.org/49253005/690001

[commit-bot: I haz the power, 2013-12-11 14:32:58]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-12-11 16:01:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oleg@chromium.org/49253005/710001

[commit-bot: I haz the power, 2013-12-11 16:23:04]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Oleg Eterevsky, 2013-12-11 16:45:20]

Hi!

Could you please review the minor changes in the following files?

chrome/browser/sync/test/integration/sync_extension_helper.ccchrome/browser/ui/panels/base_panel_browser_test.cc

Thanks.

[Nicolas Zea, 2013-12-11 23:06:12]

sync LGTM

[jennb, 2013-12-12 02:11:43]

LGTM for panels.

[commit-bot: I haz the power, 2013-12-12 14:37:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oleg@chromium.org/49253005/710001

[commit-bot: I haz the power, 2013-12-12 15:12:58]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-15 12:56:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oleg@chromium.org/49253005/760001

[commit-bot: I haz the power, 2013-12-15 13:08:18]

Retried try job too often on linux_rel for step(s) browser_tests, net_unittests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-16 13:03:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oleg@chromium.org/49253005/800001

[commit-bot: I haz the power, 2013-12-16 13:31:47]

Retried try job too often on linux_rel for step(s) browser_tests, net_unittests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-17 13:03:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oleg@chromium.org/49253005/820001

[commit-bot: I haz the power, 2013-12-17 17:49:16]

Change committed as 241316