Move code in components/safe_browsing_db and chrome/browser/s_b/ under the safe_browsing namespace.

Move code in components/safe_browsing_db and chrome/browser/safe_browsing/ under the safe_browsing namespace.

BUG=543161
Committed: https://crrev.com/9a474d832c1362b280d46dcad9de01b58903a00f
Cr-Commit-Position: refs/heads/master@{#359462}

[vakh (old account. dont use), 2015-10-29 07:24:50]

vakh@google.com changed reviewers:
+ nparker@chromium.org

[Nathan Parker, 2015-11-05 22:00:53]

Lots of files!  Looks pretty good.  This is a great cleanup.


I have the same comment in many places, should be easy to fix:
Anything in a non-test .cc file that is only used there should normally go in an
anonymous namespace:

namespace safe_browsing {
namespace {
stuff...
}

other stuff..
}

I think tests don't really matter as much, though I see some people putting
everything inside both.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/blacklist.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/blacklist.cc:47:
scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> get() {
My opinion: If there are more than 2 or 3 uses in a .cc, I'd put a 
 using safe_browsing::TheClass..;
up top.  Makes the code more readable.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc
(right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:59:
const scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>&
using safe_browsing::SafeBrowsingDatabaseManager

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager_helper.h (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager_helper.h:15: 
rm extra line

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_parser.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_parser.cc:24: // Helper class for scanning
a buffer.
If this is used only in this file, you could put it in an anonymous namespace
withing the safe_browsing namespace, to avoid polluting the SB namespace.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/remote_database_manager.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/remote_database_manager.cc:21: // Android field
trial
These should be in anon-namespace.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_api_handler.h (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_api_handler.h:20: class
SafeBrowsingApiHandler {
We'll need to modify the impl in //clank private tree

I've just crafted https://chrome-internal-review.googlesource.com/237829 to do
that (to save you the trouble of checkingout/building //clank).  I'll test that
with this CL once it's finalized and has passing bots.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:55: // For malware
interstitial pages, we link the problematic URL to Google's
Keep these in anon-namespace.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.cc:40: // Filename suffix
for the bloom filter.
Keep these in anon-namespace.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.cc:111: DCHECK_NE(list_id,
INVALID);
(This is a terribly generically named enum val.  We should rename it to
LIST_INVALID or some such.  Later.)

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.cc:70: // Filename suffix for
the cookie database.
anon-namespace

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:73: namespace
{
This should be "namespace safe_browsing" since the tests use a ton of stuff from
there.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_store.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_store.cc:13: // Return |true| if the
range is sorted by the given comparator.
anon namespace

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_store_file.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_store_file.cc:16: // NOTE(shess):
kFileMagic should not be a byte-wise palindrome, so
anon-namespace

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_test.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_test.cc:62: namespace {
namespace safe_browsing

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/threat_details.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/threat_details.cc:29: class
ClientMalwareReportRequest;
Should be able to remove this, since it must have a concrete definition from one
of the header files already.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/threat_details_unittest.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/threat_details_unittest.cc:65: class
ClientMalwareReportRequest;
Shouldn't need this.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.cc:40: 
anon-namespace

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ui_manager_unittest.cc (left):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager_unittest.cc:5: #ifndef
CHROME_BROWSER_SAFE_BROWSING_UI_MANAGER_UNITTEST_CC_
Haha!  That's pretty weird.

https://codereview.chromium.org/1420053005/diff/20001/components/safe_browsin...
File components/safe_browsing_db/prefix_set.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/components/safe_browsin...
components/safe_browsing_db/prefix_set.cc:17: 
anon-namespace

https://codereview.chromium.org/1420053005/diff/20001/components/safe_browsin...
File components/safe_browsing_db/util.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/components/safe_browsin...
components/safe_browsing_db/util.cc:17: for (size_t i = 0; i <
arraysize(safe_browsing::kAllLists); ++i) {
Can put this anon-namespace inside safe_browsing, then remove safe_browsing::.

[Nathan Parker, 2015-11-05 22:02:16]

FYI this will likely collide with jialiul's
https://codereview.chromium.org/1414343007/, but will be just a handful of lines
to fix (renaming *MalwareDetails* to *ThreadDetails*).

[vakh (old account. dont use), 2015-11-06 23:01:48]

Description was changed from

==========
Move everything inside [1] and [2] under the safe_browsing namespace.

[1]: components/safe_browsing_db/
[2]: chrome/browser/safe_browsing/

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

to

==========
Move everything inside comp/s_b_db and ch/br/s_b/ under the safe_browsing
namespace.

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

[vakh (old account. dont use), 2015-11-06 23:02:37]

On 2015/11/05 22:00:53, nparker wrote:
> Lots of files!  Looks pretty good.  This is a great cleanup.
> 
> 
> I have the same comment in many places, should be easy to fix:
> Anything in a non-test .cc file that is only used there should normally go in
an
> anonymous namespace:
> 
> namespace safe_browsing {
> namespace {
> stuff...
> }
> 
> other stuff..
> }
> 
> I think tests don't really matter as much, though I see some people putting
> everything inside both.
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/extensio...
> File chrome/browser/extensions/blacklist.cc (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/extensio...
> chrome/browser/extensions/blacklist.cc:47:
> scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> get() {
> My opinion: If there are more than 2 or 3 uses in a .cc, I'd put a 
>  using safe_browsing::TheClass..;
> up top.  Makes the code more readable.
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File
>
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc
> (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
>
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:59:
> const scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>&
> using safe_browsing::SafeBrowsingDatabaseManager
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/protocol_manager_helper.h (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/protocol_manager_helper.h:15: 
> rm extra line
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/protocol_parser.cc (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/protocol_parser.cc:24: // Helper class for
scanning
> a buffer.
> If this is used only in this file, you could put it in an anonymous namespace
> withing the safe_browsing namespace, to avoid polluting the SB namespace.
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/remote_database_manager.cc (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/remote_database_manager.cc:21: // Android field
> trial
> These should be in anon-namespace.
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/safe_browsing_api_handler.h (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/safe_browsing_api_handler.h:20: class
> SafeBrowsingApiHandler {
> We'll need to modify the impl in //clank private tree
> 
> I've just crafted https://chrome-internal-review.googlesource.com/237829 to do
> that (to save you the trouble of checkingout/building //clank).  I'll test
that
> with this CL once it's finalized and has passing bots.
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:55: // For malware
> interstitial pages, we link the problematic URL to Google's
> Keep these in anon-namespace.
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/safe_browsing_database.cc (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/safe_browsing_database.cc:40: // Filename suffix
> for the bloom filter.
> Keep these in anon-namespace.
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/safe_browsing_database.cc:111: DCHECK_NE(list_id,
> INVALID);
> (This is a terribly generically named enum val.  We should rename it to
> LIST_INVALID or some such.  Later.)
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/safe_browsing_service.cc (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/safe_browsing_service.cc:70: // Filename suffix
for
> the cookie database.
> anon-namespace
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
(right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:73:
namespace
> {
> This should be "namespace safe_browsing" since the tests use a ton of stuff
from
> there.
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/safe_browsing_store.cc (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/safe_browsing_store.cc:13: // Return |true| if
the
> range is sorted by the given comparator.
> anon namespace
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/safe_browsing_store_file.cc (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/safe_browsing_store_file.cc:16: // NOTE(shess):
> kFileMagic should not be a byte-wise palindrome, so
> anon-namespace
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/safe_browsing_test.cc (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/safe_browsing_test.cc:62: namespace {
> namespace safe_browsing
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/threat_details.cc (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/threat_details.cc:29: class
> ClientMalwareReportRequest;
> Should be able to remove this, since it must have a concrete definition from
one
> of the header files already.
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/threat_details_unittest.cc (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/threat_details_unittest.cc:65: class
> ClientMalwareReportRequest;
> Shouldn't need this.
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/ui_manager.cc (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/ui_manager.cc:40: 
> anon-namespace
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/ui_manager_unittest.cc (left):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/ui_manager_unittest.cc:5: #ifndef
> CHROME_BROWSER_SAFE_BROWSING_UI_MANAGER_UNITTEST_CC_
> Haha!  That's pretty weird.
> 
>
https://codereview.chromium.org/1420053005/diff/20001/components/safe_browsin...
> File components/safe_browsing_db/prefix_set.cc (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/components/safe_browsin...
> components/safe_browsing_db/prefix_set.cc:17: 
> anon-namespace
> 
>
https://codereview.chromium.org/1420053005/diff/20001/components/safe_browsin...
> File components/safe_browsing_db/util.cc (right):
> 
>
https://codereview.chromium.org/1420053005/diff/20001/components/safe_browsin...
> components/safe_browsing_db/util.cc:17: for (size_t i = 0; i <
> arraysize(safe_browsing::kAllLists); ++i) {
> Can put this anon-namespace inside safe_browsing, then remove safe_browsing::.

Thanks for the review.
All comments addressed.

[vakh (old account. dont use), 2015-11-06 23:05:49]

On 2015/11/05 22:02:16, nparker wrote:
> FYI this will likely collide with jialiul's
> https://codereview.chromium.org/1414343007/, but will be just a handful of
lines
> to fix (renaming *MalwareDetails* to *ThreadDetails*).

Ack. Will do that once it lands.

[Nathan Parker, 2015-11-07 01:15:13]

lgtm

cc: mattm, jailiu for heads up.

LGTM for /safe_browsing/ parts and the namespace change.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/local_database_manager.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/local_database_manager.cc:178: namespace
safe_browsing {
If you put this up top, and the anonymous namespace within it, then you won't
need the "usings" or the safe_browsing::'s in that anon-namespace.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_database.cc:311: namespace
safe_browsing {
Similar here: this can encompase the whole file, w/ a anon-namespace within it.

[Nathan Parker, 2015-11-07 01:15:52]

oh, and need to update the title+desc.

[vakh (old account. dont use), 2015-11-07 01:21:54]

Description was changed from

==========
Move everything inside comp/s_b_db and ch/br/s_b/ under the safe_browsing
namespace.

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

to

==========
Move code in components/safe_browsing_db and chrome/browser/safe_browsing/ under
the safe_browsing namespace.

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

[vakh (old account. dont use), 2015-11-07 01:22:12]

On 2015/11/07 01:15:52, nparker wrote:
> oh, and need to update the title+desc.

Done.

[vakh (old account. dont use), 2015-11-07 01:22:57]

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/blacklist.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/blacklist.cc:47:
scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> get() {
On 2015/11/05 22:00:52, nparker wrote:
> My opinion: If there are more than 2 or 3 uses in a .cc, I'd put a 
>  using safe_browsing::TheClass..;
> up top.  Makes the code more readable.

Done.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc
(right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:59:
const scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>&
On 2015/11/05 22:00:52, nparker wrote:
> using safe_browsing::SafeBrowsingDatabaseManager

Done.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager_helper.h (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager_helper.h:15: 
On 2015/11/05 22:00:52, nparker wrote:
> rm extra line

Done.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_parser.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_parser.cc:24: // Helper class for scanning
a buffer.
On 2015/11/05 22:00:52, nparker wrote:
> If this is used only in this file, you could put it in an anonymous namespace
> withing the safe_browsing namespace, to avoid polluting the SB namespace.

Done.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/remote_database_manager.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/remote_database_manager.cc:21: // Android field
trial
On 2015/11/05 22:00:52, nparker wrote:
> These should be in anon-namespace.

Done.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_api_handler.h (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_api_handler.h:20: class
SafeBrowsingApiHandler {
On 2015/11/05 22:00:52, nparker wrote:
> We'll need to modify the impl in //clank private tree
> 
> I've just crafted https://chrome-internal-review.googlesource.com/237829 to do
> that (to save you the trouble of checkingout/building //clank).  I'll test
that
> with this CL once it's finalized and has passing bots.

Acknowledged.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:55: // For malware
interstitial pages, we link the problematic URL to Google's
On 2015/11/05 22:00:53, nparker wrote:
> Keep these in anon-namespace.

Done.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.cc:40: // Filename suffix
for the bloom filter.
On 2015/11/05 22:00:53, nparker wrote:
> Keep these in anon-namespace.

Done.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.cc:70: // Filename suffix for
the cookie database.
On 2015/11/05 22:00:53, nparker wrote:
> anon-namespace

Done.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:73: namespace
{
On 2015/11/05 22:00:53, nparker wrote:
> This should be "namespace safe_browsing" since the tests use a ton of stuff
from
> there.

Done.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_store.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_store.cc:13: // Return |true| if the
range is sorted by the given comparator.
On 2015/11/05 22:00:53, nparker wrote:
> anon namespace

Done.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_store_file.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_store_file.cc:16: // NOTE(shess):
kFileMagic should not be a byte-wise palindrome, so
On 2015/11/05 22:00:53, nparker wrote:
> anon-namespace

Done.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_test.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_test.cc:62: namespace {
On 2015/11/05 22:00:53, nparker wrote:
> namespace safe_browsing

As best as I can tell, these definitions are only being used in this file so to
be consistent with the rest of the changes in this CL, I think we should leave
these in the anon namespace.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/threat_details.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/threat_details.cc:29: class
ClientMalwareReportRequest;
On 2015/11/05 22:00:53, nparker wrote:
> Should be able to remove this, since it must have a concrete definition from
one
> of the header files already.

Done.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/threat_details_unittest.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/threat_details_unittest.cc:65: class
ClientMalwareReportRequest;
On 2015/11/05 22:00:53, nparker wrote:
> Shouldn't need this.

Done.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.cc:40: 
On 2015/11/05 22:00:53, nparker wrote:
> anon-namespace

Done.

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ui_manager_unittest.cc (left):

https://codereview.chromium.org/1420053005/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager_unittest.cc:5: #ifndef
CHROME_BROWSER_SAFE_BROWSING_UI_MANAGER_UNITTEST_CC_
On 2015/11/05 22:00:53, nparker wrote:
> Haha!  That's pretty weird.

Acknowledged. Yes, I was surprised to find it there.

https://codereview.chromium.org/1420053005/diff/20001/components/safe_browsin...
File components/safe_browsing_db/prefix_set.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/components/safe_browsin...
components/safe_browsing_db/prefix_set.cc:17: 
On 2015/11/05 22:00:53, nparker wrote:
> anon-namespace

Done.

https://codereview.chromium.org/1420053005/diff/20001/components/safe_browsin...
File components/safe_browsing_db/util.cc (right):

https://codereview.chromium.org/1420053005/diff/20001/components/safe_browsin...
components/safe_browsing_db/util.cc:17: for (size_t i = 0; i <
arraysize(safe_browsing::kAllLists); ++i) {
On 2015/11/05 22:00:53, nparker wrote:
> Can put this anon-namespace inside safe_browsing, then remove safe_browsing::.

The function is only used in this file so to be consistent with the other
changes in this CL, I think it is appropriate to keep this function in the anon
namespace.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/local_database_manager.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/local_database_manager.cc:178: namespace
safe_browsing {
On 2015/11/07 01:15:13, nparker wrote:
> If you put this up top, and the anonymous namespace within it, then you won't
> need the "usings" or the safe_browsing::'s in that anon-namespace.

You mean like this:

namespace safe_browsing {
namespace {
...
}  // namespace
...
}  // namespace safe_browsing


It doesn't seem to work. I get:

../../components/safe_browsing_db/prefix_set.cc:64:7: error: unknown type name
'SBPrefix'; did you mean 'safe_browsing::SBPrefix'?

[Nathan Parker, 2015-11-10 23:53:34]

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/blacklist.h (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/blacklist.h:24: using
safe_browsing::SafeBrowsingDatabaseManager;
"using" should only be used in .cc files, since otherwise it pollutes the top
level namespace for anyone who includes it. So "using" is good in .cc and
certainly in _unittest.cc's, but not in .h.  Sorry if I wasn't clear on that
one. 

Oh, and a c++ quirk: "using" doesn't work on enums.  Not much we can do about
that.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/local_database_manager.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/local_database_manager.cc:178: namespace
safe_browsing {
On 2015/11/07 01:22:57, Varun Khaneja wrote:
> On 2015/11/07 01:15:13, nparker wrote:
> > If you put this up top, and the anonymous namespace within it, then you
won't
> > need the "usings" or the safe_browsing::'s in that anon-namespace.
> 
> You mean like this:
> 
> namespace safe_browsing {
> namespace {
> ...
> }  // namespace
> ...
> }  // namespace safe_browsing
> 
> 
> It doesn't seem to work. I get:
> 
> ../../components/safe_browsing_db/prefix_set.cc:64:7: error: unknown type name
> 'SBPrefix'; did you mean 'safe_browsing::SBPrefix'?

Really? Hrm, then I've lead you astray. Carry on.

I wonder if this is due to a clang-warning flag of some type, since this works
with plain gcc.  OR, maybe the inner anon-namespace has access only to things in
the same file.  This works w/ gcc:


#include <stdio.h>

namespace foo {
  void a() { printf("a: Hello\n"); }
}

namespace foo {
  namespace {
    void b() { a(); }
  }
  void c() { b(); }
}

int main() {
  foo::c();
  return 0;
}

[mattm, 2015-11-11 01:10:15]

mattm@chromium.org changed reviewers:
+ mattm@chromium.org

[mattm, 2015-11-11 01:10:17]

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/local_database_manager.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/local_database_manager.cc:178: namespace
safe_browsing {
On 2015/11/10 23:53:34, nparker wrote:
> On 2015/11/07 01:22:57, Varun Khaneja wrote:
> > On 2015/11/07 01:15:13, nparker wrote:
> > > If you put this up top, and the anonymous namespace within it, then you
> won't
> > > need the "usings" or the safe_browsing::'s in that anon-namespace.
> > 
> > You mean like this:
> > 
> > namespace safe_browsing {
> > namespace {
> > ...
> > }  // namespace
> > ...
> > }  // namespace safe_browsing
> > 
> > 
> > It doesn't seem to work. I get:
> > 
> > ../../components/safe_browsing_db/prefix_set.cc:64:7: error: unknown type
name
> > 'SBPrefix'; did you mean 'safe_browsing::SBPrefix'?
> 
> Really? Hrm, then I've lead you astray. Carry on.
> 
> I wonder if this is due to a clang-warning flag of some type, since this works
> with plain gcc.  OR, maybe the inner anon-namespace has access only to things
in
> the same file.  This works w/ gcc:
> 
> 
> #include <stdio.h>
> 
> namespace foo {
>   void a() { printf("a: Hello\n"); }
> }
> 
> namespace foo {
>   namespace {
>     void b() { a(); }
>   }
>   void c() { b(); }
> }
> 
> int main() {
>   foo::c();
>   return 0;
> }

It definitely should work. I noticed prefix_set.cc isn't part of this CL, which
could certainly explain the error. If you try that and continue having trouble,
could you upload the patchset with the problem so we can look at it?

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/local_database_manager_unittest.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/local_database_manager_unittest.cc:39: }  //
namespace
Keep the anon namespace inside the new namespace.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/protocol_parser_unittest.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/protocol_parser_unittest.cc:16: const char
kDefaultMalwareList[] = "goog-malware-shavar";
keep the constants in anon namespace

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/remote_database_manager_unittest.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager_unittest.cc:22: };
keep in anon namespace

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:357: }  //
namespace
Keep the anon namespace inside the new namespace.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
(left):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc:84: }  //
namespace
Keep the anon namespace inside the new namespace.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:252: }  //
namespace
Keep the anon namespace inside the new namespace.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:414: }  //
namespace
Keep the anon namespace inside the new namespace.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_store_file.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_store_file.cc:24: using
safe_browsing::SBSubPrefixes;
Put this namespace inside the namespace safe_browsing to avoid this

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc:40: }  //
namespace
Keep the anon namespace inside the new namespace.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_store_unittest.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_store_unittest.cc:55: }
Keep the constants and helpers inside anon namespace.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/threat_details_unittest.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/threat_details_unittest.cc:177: 
Keep the anon namespace inside the new namespace.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/two_phase_uploader_unittest.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/two_phase_uploader_unittest.cc:57: }  // namespace
Keep the anon namespace inside the new namespace.

https://codereview.chromium.org/1420053005/diff/100001/components/safe_browsi...
File components/safe_browsing_db/prefix_set_unittest.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/components/safe_browsi...
components/safe_browsing_db/prefix_set_unittest.cc:31: }  // namespace
Keep the anon namespace inside the new namespace.

https://codereview.chromium.org/1420053005/diff/100001/components/safe_browsi...
File components/safe_browsing_db/util_unittest.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/components/safe_browsi...
components/safe_browsing_db/util_unittest.cc:17: }
Keep helpers inside anon namespace

[vakh (old account. dont use), 2015-11-11 18:59:53]

Thanks for the review Nathan and Matt.
Please take another look.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/blacklist.h (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/blacklist.h:24: using
safe_browsing::SafeBrowsingDatabaseManager;
On 2015/11/10 23:53:34, nparker wrote:
> "using" should only be used in .cc files, since otherwise it pollutes the top
> level namespace for anyone who includes it. So "using" is good in .cc and
> certainly in _unittest.cc's, but not in .h.  Sorry if I wasn't clear on that
> one. 
> 
> Oh, and a c++ quirk: "using" doesn't work on enums.  Not much we can do about
> that.

Done.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/local_database_manager.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/local_database_manager.cc:178: namespace
safe_browsing {
On 2015/11/11 01:10:16, mattm wrote:
> On 2015/11/10 23:53:34, nparker wrote:
> > On 2015/11/07 01:22:57, Varun Khaneja wrote:
> > > On 2015/11/07 01:15:13, nparker wrote:
> > > > If you put this up top, and the anonymous namespace within it, then you
> > won't
> > > > need the "usings" or the safe_browsing::'s in that anon-namespace.
> > > 
> > > You mean like this:
> > > 
> > > namespace safe_browsing {
> > > namespace {
> > > ...
> > > }  // namespace
> > > ...
> > > }  // namespace safe_browsing
> > > 
> > > 
> > > It doesn't seem to work. I get:
> > > 
> > > ../../components/safe_browsing_db/prefix_set.cc:64:7: error: unknown type
> name
> > > 'SBPrefix'; did you mean 'safe_browsing::SBPrefix'?
> > 
> > Really? Hrm, then I've lead you astray. Carry on.
> > 
> > I wonder if this is due to a clang-warning flag of some type, since this
works
> > with plain gcc.  OR, maybe the inner anon-namespace has access only to
things
> in
> > the same file.  This works w/ gcc:
> > 
> > 
> > #include <stdio.h>
> > 
> > namespace foo {
> >   void a() { printf("a: Hello\n"); }
> > }
> > 
> > namespace foo {
> >   namespace {
> >     void b() { a(); }
> >   }
> >   void c() { b(); }
> > }
> > 
> > int main() {
> >   foo::c();
> >   return 0;
> > }
> 
> It definitely should work. I noticed prefix_set.cc isn't part of this CL,
which
> could certainly explain the error. If you try that and continue having
trouble,
> could you upload the patchset with the problem so we can look at it?

You're right. I have fixed prefix_set.cc and it works now.
Thanks for pointing that out.
Sorry for the confusion.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/local_database_manager_unittest.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/local_database_manager_unittest.cc:39: }  //
namespace
On 2015/11/11 01:10:16, mattm wrote:
> Keep the anon namespace inside the new namespace.

Done.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/protocol_parser_unittest.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/protocol_parser_unittest.cc:16: const char
kDefaultMalwareList[] = "goog-malware-shavar";
On 2015/11/11 01:10:16, mattm wrote:
> keep the constants in anon namespace

Done.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/remote_database_manager_unittest.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager_unittest.cc:22: };
On 2015/11/11 01:10:16, mattm wrote:
> keep in anon namespace

Done.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:357: }  //
namespace
On 2015/11/11 01:10:16, mattm wrote:
> Keep the anon namespace inside the new namespace.

Done.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_database.cc:311: namespace
safe_browsing {
On 2015/11/07 01:15:13, nparker wrote:
> Similar here: this can encompase the whole file, w/ a anon-namespace within
it.

Done.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:252: }  //
namespace
On 2015/11/11 01:10:16, mattm wrote:
> Keep the anon namespace inside the new namespace.

Done.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:414: }  //
namespace
On 2015/11/11 01:10:16, mattm wrote:
> Keep the anon namespace inside the new namespace.

Done.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_store_file.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_store_file.cc:24: using
safe_browsing::SBSubPrefixes;
On 2015/11/11 01:10:16, mattm wrote:
> Put this namespace inside the namespace safe_browsing to avoid this

Done.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc:40: }  //
namespace
On 2015/11/11 01:10:16, mattm wrote:
> Keep the anon namespace inside the new namespace.

Done.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_store_unittest.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_store_unittest.cc:55: }
On 2015/11/11 01:10:16, mattm wrote:
> Keep the constants and helpers inside anon namespace.

Done.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/threat_details_unittest.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/threat_details_unittest.cc:177: 
On 2015/11/11 01:10:16, mattm wrote:
> Keep the anon namespace inside the new namespace.

Done.

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/two_phase_uploader_unittest.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/two_phase_uploader_unittest.cc:57: }  // namespace
On 2015/11/11 01:10:16, mattm wrote:
> Keep the anon namespace inside the new namespace.

Done.

https://codereview.chromium.org/1420053005/diff/100001/components/safe_browsi...
File components/safe_browsing_db/prefix_set_unittest.cc (left):

https://codereview.chromium.org/1420053005/diff/100001/components/safe_browsi...
components/safe_browsing_db/prefix_set_unittest.cc:31: }  // namespace
On 2015/11/11 01:10:16, mattm wrote:
> Keep the anon namespace inside the new namespace.

Done.

https://codereview.chromium.org/1420053005/diff/100001/components/safe_browsi...
File components/safe_browsing_db/util_unittest.cc (right):

https://codereview.chromium.org/1420053005/diff/100001/components/safe_browsi...
components/safe_browsing_db/util_unittest.cc:17: }
On 2015/11/11 01:10:17, mattm wrote:
> Keep helpers inside anon namespace

Done.

[Nathan Parker, 2015-11-11 21:23:36]

lgtm

A few minor tweaks.

This is a big clean up!  I think it's great to have it all consistent.  Thanks,
Varun.

https://codereview.chromium.org/1420053005/diff/120001/chrome/browser/rendere...
File chrome/browser/renderer_host/safe_browsing_resource_throttle.h (right):

https://codereview.chromium.org/1420053005/diff/120001/chrome/browser/rendere...
chrome/browser/renderer_host/safe_browsing_resource_throttle.h:21: using
safe_browsing::SafeBrowsingDatabaseManager;
shouldn't use "using" in .h's.

https://codereview.chromium.org/1420053005/diff/120001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h
(right):

https://codereview.chromium.org/1420053005/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h:75:
safe_browsing::SafeBrowsingService* safe_browsing_service,
Can rm safe_browsing:: here and below.

https://codereview.chromium.org/1420053005/diff/120001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/threat_details_cache.cc (right):

https://codereview.chromium.org/1420053005/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/threat_details_cache.cc:29: class
ClientMalwareReportRequest;
Shouldn't need this pre-decl, since it must be in a header anyway.

[vakh (old account. dont use), 2015-11-11 23:22:46]

https://codereview.chromium.org/1420053005/diff/120001/chrome/browser/rendere...
File chrome/browser/renderer_host/safe_browsing_resource_throttle.h (right):

https://codereview.chromium.org/1420053005/diff/120001/chrome/browser/rendere...
chrome/browser/renderer_host/safe_browsing_resource_throttle.h:21: using
safe_browsing::SafeBrowsingDatabaseManager;
On 2015/11/11 21:23:36, nparker wrote:
> shouldn't use "using" in .h's.

Done.

https://codereview.chromium.org/1420053005/diff/120001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h
(right):

https://codereview.chromium.org/1420053005/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h:75:
safe_browsing::SafeBrowsingService* safe_browsing_service,
On 2015/11/11 21:23:36, nparker wrote:
> Can rm safe_browsing:: here and below.

Done.

https://codereview.chromium.org/1420053005/diff/120001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/threat_details_cache.cc (right):

https://codereview.chromium.org/1420053005/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/threat_details_cache.cc:29: class
ClientMalwareReportRequest;
On 2015/11/11 21:23:36, nparker wrote:
> Shouldn't need this pre-decl, since it must be in a header anyway.

Done.

[mattm, 2015-11-12 00:36:06]

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/blacklist.h (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/blacklist.h:123: GetDatabaseManager();
formatting

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/local_database_manager.cc (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/local_database_manager.cc:156: ListType list_type)
{
formatting

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:95:
base::LazyInstance<safe_browsing::SafeBrowsingBlockingPage::UnsafeResourceMap>
unnecessary safe_browsing::

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:238: } //
namespace
two spaces before //

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.h:118:
safe_browsing_detection_service() const {
Will this fit on a single line?

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.h:126:
download_protection_service() const {
Will this fit on a single line?

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.h:152: const
DelayedAnalysisCallback& callback);
Will this fit on a single line?

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.h:183:
CreateIncidentReportingService();
This should fit on a single line

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:718:
&uws_full_hash);
formatting

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:787:
&malware_full_hash);
formatting

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:902:
&full_hash_result);
formatting

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:923: bad_url,
UNWANTEDURL, &full_hash_result);
formatting

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:945: bad_url,
MALWARE, &full_hash_result);
formatting

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:965: bad_url,
MALWARE, &full_hash_result);
formatting

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:988: bad_url,
UNWANTEDURL, &full_hash_result);
formatting

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1013:
&full_hash_result);
formatting

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1036:
&full_hash_result);
formatting

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/srt_global_error_win.cc (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/srt_global_error_win.cc:110:
safe_browsing::RecordSRTPromptHistogram(
bunch of safe_browsing:: in this file could be removed

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/threat_details.h (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/threat_details.h:94:
scoped_ptr<safe_browsing::ClientSafeBrowsingReportRequest> report_;
more safe_browsing:: that could be removed in this file

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/threat_details_cache.h (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/threat_details_cache.h:87:
safe_browsing::ClientSafeBrowsingReportRequest::Resource* GetResource(
a few more safe_browsing:: that could be removed here

https://codereview.chromium.org/1420053005/diff/160001/components/safe_browsi...
File components/safe_browsing_db/util.cc (right):

https://codereview.chromium.org/1420053005/diff/160001/components/safe_browsi...
components/safe_browsing_db/util.cc:18: if (!strcmp(safe_browsing::kAllLists[i],
name.c_str())) {
move anon namespace inside safe_browsing namespace

https://codereview.chromium.org/1420053005/diff/160001/components/safe_browsi...
File components/safe_browsing_db/util_unittest.cc (right):

https://codereview.chromium.org/1420053005/diff/160001/components/safe_browsi...
components/safe_browsing_db/util_unittest.cc:287: &list_name));
formatting (and elsewhere in file)

[mattm, 2015-11-12 00:37:17]

Also it's generally appreciated to upload the merge/rebase in a separate
patchset from any changes of your own, makes it easier to do inter-patchset
diffs.

[vakh (old account. dont use), 2015-11-12 09:27:44]

Thanks for patiently reviewing this enormous CL.
I have tried to fix all the tests now (except one/few Windows tests).

I have also tried to make the formatting consistent across all files. There may
still be some discrepancies but hopefully not too many.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/blacklist.h (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/blacklist.h:123: GetDatabaseManager();
On 2015/11/12 00:36:05, mattm wrote:
> formatting

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/local_database_manager.cc (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/local_database_manager.cc:156: ListType list_type)
{
On 2015/11/12 00:36:05, mattm wrote:
> formatting

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:95:
base::LazyInstance<safe_browsing::SafeBrowsingBlockingPage::UnsafeResourceMap>
On 2015/11/12 00:36:05, mattm wrote:
> unnecessary safe_browsing::

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:238: } //
namespace
On 2015/11/12 00:36:05, mattm wrote:
> two spaces before //

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.h:118:
safe_browsing_detection_service() const {
On 2015/11/12 00:36:05, mattm wrote:
> Will this fit on a single line?

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.h:126:
download_protection_service() const {
On 2015/11/12 00:36:05, mattm wrote:
> Will this fit on a single line?

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.h:152: const
DelayedAnalysisCallback& callback);
On 2015/11/12 00:36:05, mattm wrote:
> Will this fit on a single line?

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.h:183:
CreateIncidentReportingService();
On 2015/11/12 00:36:05, mattm wrote:
> This should fit on a single line

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:718:
&uws_full_hash);
On 2015/11/12 00:36:05, mattm wrote:
> formatting

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:787:
&malware_full_hash);
On 2015/11/12 00:36:06, mattm wrote:
> formatting

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:902:
&full_hash_result);
On 2015/11/12 00:36:06, mattm wrote:
> formatting

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:923: bad_url,
UNWANTEDURL, &full_hash_result);
On 2015/11/12 00:36:05, mattm wrote:
> formatting

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:945: bad_url,
MALWARE, &full_hash_result);
On 2015/11/12 00:36:06, mattm wrote:
> formatting

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:965: bad_url,
MALWARE, &full_hash_result);
On 2015/11/12 00:36:05, mattm wrote:
> formatting

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:988: bad_url,
UNWANTEDURL, &full_hash_result);
On 2015/11/12 00:36:06, mattm wrote:
> formatting

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1013:
&full_hash_result);
On 2015/11/12 00:36:06, mattm wrote:
> formatting

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1036:
&full_hash_result);
On 2015/11/12 00:36:05, mattm wrote:
> formatting

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/srt_global_error_win.cc (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/srt_global_error_win.cc:110:
safe_browsing::RecordSRTPromptHistogram(
On 2015/11/12 00:36:06, mattm wrote:
> bunch of safe_browsing:: in this file could be removed

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/threat_details.h (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/threat_details.h:94:
scoped_ptr<safe_browsing::ClientSafeBrowsingReportRequest> report_;
On 2015/11/12 00:36:06, mattm wrote:
> more safe_browsing:: that could be removed in this file

Done.

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/threat_details_cache.h (right):

https://codereview.chromium.org/1420053005/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/threat_details_cache.h:87:
safe_browsing::ClientSafeBrowsingReportRequest::Resource* GetResource(
On 2015/11/12 00:36:06, mattm wrote:
> a few more safe_browsing:: that could be removed here

Done.

https://codereview.chromium.org/1420053005/diff/160001/components/safe_browsi...
File components/safe_browsing_db/util.cc (right):

https://codereview.chromium.org/1420053005/diff/160001/components/safe_browsi...
components/safe_browsing_db/util.cc:18: if (!strcmp(safe_browsing::kAllLists[i],
name.c_str())) {
On 2015/11/12 00:36:06, mattm wrote:
> move anon namespace inside safe_browsing namespace

Done.

https://codereview.chromium.org/1420053005/diff/160001/components/safe_browsi...
File components/safe_browsing_db/util_unittest.cc (right):

https://codereview.chromium.org/1420053005/diff/160001/components/safe_browsi...
components/safe_browsing_db/util_unittest.cc:287: &list_name));
On 2015/11/12 00:36:06, mattm wrote:
> formatting (and elsewhere in file)

Done.

[mattm, 2015-11-12 17:17:56]

Looks like a few compile failures still.

https://codereview.chromium.org/1420053005/diff/160001/components/safe_browsi...
File components/safe_browsing_db/util.cc (right):

https://codereview.chromium.org/1420053005/diff/160001/components/safe_browsi...
components/safe_browsing_db/util.cc:18: if (!strcmp(safe_browsing::kAllLists[i],
name.c_str())) {
On 2015/11/12 09:27:43, Varun Khaneja wrote:
> On 2015/11/12 00:36:06, mattm wrote:
> > move anon namespace inside safe_browsing namespace
> 
> Done.

can remove this safe_browsing:: too

[vakh (old account. dont use), 2015-11-12 19:03:59]

Remove safe_browsing:: from util.cc, fix Android compilation errors.

[vakh (old account. dont use), 2015-11-12 21:03:43]

On 2015/11/12 17:17:56, mattm wrote:
> Looks like a few compile failures still.
Should be fixed now.

> 
>
https://codereview.chromium.org/1420053005/diff/160001/components/safe_browsi...
> File components/safe_browsing_db/util.cc (right):
> 
>
https://codereview.chromium.org/1420053005/diff/160001/components/safe_browsi...
> components/safe_browsing_db/util.cc:18: if
(!strcmp(safe_browsing::kAllLists[i],
> name.c_str())) {
> On 2015/11/12 09:27:43, Varun Khaneja wrote:
> > On 2015/11/12 00:36:06, mattm wrote:
> > > move anon namespace inside safe_browsing namespace
> > 
> > Done.
> 
> can remove this safe_browsing:: too
Done.

[vakh (old account. dont use), 2015-11-12 22:21:03]

vakh@google.com changed reviewers:
+ thestig@chromium.org

[vakh (old account. dont use), 2015-11-12 22:21:04]

thestig@, please review the changes outside of:
components/safe_browsing_db/...
chrome/browser/safe_browsing/...

nparker@ and mattm@ have already looked at those two directories but you are
welcome to look there yourself.

[Lei Zhang, 2015-11-12 22:56:46]

lgtm

https://codereview.chromium.org/1420053005/diff/300001/chrome/app/android/chr...
File chrome/app/android/chrome_main_delegate_android.cc (right):

https://codereview.chromium.org/1420053005/diff/300001/chrome/app/android/chr...
chrome/app/android/chrome_main_delegate_android.cc:25: using
safe_browsing::SafeBrowsingApiHandler;
There's only a few users. Just add safe_browsing:: where needed?

https://codereview.chromium.org/1420053005/diff/300001/chrome/app/android/chr...
File chrome/app/android/chrome_main_delegate_android.h (right):

https://codereview.chromium.org/1420053005/diff/300001/chrome/app/android/chr...
chrome/app/android/chrome_main_delegate_android.h:14: }  // namespace
safe_browsing
I prefer no trailing // namespace foo when the entire namespace is only 1 line
long on the inside.

[vakh (old account. dont use), 2015-11-12 23:03:06]

Remove '// namespace safe_browsing' for a small fwd decl block.

[vakh (old account. dont use), 2015-11-12 23:04:10]

Thanks for the review, nparker@, mattm@ and thestig@.

https://codereview.chromium.org/1420053005/diff/300001/chrome/app/android/chr...
File chrome/app/android/chrome_main_delegate_android.cc (right):

https://codereview.chromium.org/1420053005/diff/300001/chrome/app/android/chr...
chrome/app/android/chrome_main_delegate_android.cc:25: using
safe_browsing::SafeBrowsingApiHandler;
On 2015/11/12 22:56:45, Lei Zhang wrote:
> There's only a few users. Just add safe_browsing:: where needed?

There are 3 locations.
We decided to have a threshold of 3 or more locations for using "using" for this
CL so keeping it as-is to remain consistent with the rest of the files in the
CL.

https://codereview.chromium.org/1420053005/diff/300001/chrome/app/android/chr...
File chrome/app/android/chrome_main_delegate_android.h (right):

https://codereview.chromium.org/1420053005/diff/300001/chrome/app/android/chr...
chrome/app/android/chrome_main_delegate_android.h:14: }  // namespace
safe_browsing
On 2015/11/12 22:56:45, Lei Zhang wrote:
> I prefer no trailing // namespace foo when the entire namespace is only 1 line
> long on the inside.

Done.

[vakh (old account. dont use), 2015-11-12 23:04:43]

The CQ bit was checked by vakh@google.com

[vakh (old account. dont use), 2015-11-12 23:04:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1420053005/#ps320001
(title: "Remove '// namespace safe_browsing' for a small fwd decl block.")

[vakh (old account. dont use), 2015-11-12 23:05:03]

The CQ bit was unchecked by vakh@google.com

[vakh (old account. dont use), 2015-11-12 23:36:06]

The CQ bit was checked by vakh@google.com

[commit-bot: I haz the power, 2015-11-12 23:39:54]

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

[commit-bot: I haz the power, 2015-11-13 01:43:21]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-11-13 01:44:09]

Patchset 17 (id:??) landed as
https://crrev.com/9a474d832c1362b280d46dcad9de01b58903a00f
Cr-Commit-Position: refs/heads/master@{#359462}