Chromium Code Reviews
Help | Chromium Project | Sign in
(1)

Issue 3277008: add safebrowsing testserver (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 10 months ago by lzheng
Modified:
4 years, 1 month ago
CC:
chromium-reviews, ben+cc_chromium.org, Paul Godavari, gcasto (DO NOT USE)
Visibility:
Public.

Description

Add the dependency with safebrowsing's test suite server and make safebrowsing browsertest run end to end test with the test server. BUG=47318 TEST=this is the test Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=63021

Patch Set 1 : '' #

Total comments: 21

Patch Set 2 : '' #

Total comments: 28

Patch Set 3 : '' #

Total comments: 45

Patch Set 4 : address shess's comments #

Total comments: 7

Patch Set 5 : Get rid of pinger, address more comments from shess #

Total comments: 7

Patch Set 6 : '' #

Total comments: 12

Patch Set 7 : '' #

Total comments: 9

Patch Set 8 : '' #

Patch Set 9 : 'python_util #

Total comments: 26

Patch Set 10 : '' #

Patch Set 11 : '' #

Patch Set 12 : '' #

Patch Set 13 : '' #

Total comments: 31

Patch Set 14 : address Eric's comments #

Patch Set 15 : resync before submitting #

Patch Set 16 : resync before submitting #

Unified diffs Side-by-side diffs Delta from patch set Stats (+471 lines, -47 lines) Patch
M DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/safe_browsing/safe_browsing_test.cc View 6 7 8 9 10 11 12 13 14 15 8 chunks +470 lines, -46 lines 0 comments Download
Trybot results:
Commit: CQ not working?

Messages

Total messages: 25 (0 generated)
lzheng
Add the full end to end test. The code is ready for review. However, before ...
4 years, 9 months ago (2010-09-10 17:00:38 UTC) #1
Paweł Hajdan Jr.
Please do not commit without my explicit "LGTM". http://codereview.chromium.org/3277008/diff/13001/14001 File DEPS (right): http://codereview.chromium.org/3277008/diff/13001/14001#newcode52 DEPS:52: "src/third_party/safe_browsing": ...
4 years, 9 months ago (2010-09-10 17:53:48 UTC) #2
lzheng
Pawel: Thanks for your good comments. Please see comments inline. http://codereview.chromium.org/3277008/diff/13001/14001 File DEPS (right): http://codereview.chromium.org/3277008/diff/13001/14001#newcode52 ...
4 years, 9 months ago (2010-09-10 22:52:59 UTC) #3
Paweł Hajdan Jr.
http://codereview.chromium.org/3277008/diff/13001/14001 File DEPS (right): http://codereview.chromium.org/3277008/diff/13001/14001#newcode52 DEPS:52: "src/third_party/safe_browsing": On 2010/09/10 22:52:59, lzheng wrote: > On 2010/09/10 ...
4 years, 9 months ago (2010-09-10 23:11:26 UTC) #4
lzheng
Updated with latest change. Please take another look. http://codereview.chromium.org/3277008/diff/13001/14002 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/3277008/diff/13001/14002#newcode88 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:88: L"\"" ...
4 years, 9 months ago (2010-09-14 06:39:19 UTC) #5
Paweł Hajdan Jr.
http://codereview.chromium.org/3277008/diff/18001/17003 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/3277008/diff/18001/17003#newcode48 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:48: class SafeBrowsingTestServer { On 2010/09/14 06:39:20, lzheng wrote: > ...
4 years, 9 months ago (2010-09-14 17:30:07 UTC) #6
lzheng1
On Tue, Sep 14, 2010 at 10:30 AM, <phajdan.jr@chromium.org> wrote: > > http://codereview.chromium.org/3277008/diff/18001/17003 > File ...
4 years, 9 months ago (2010-09-14 23:11:02 UTC) #7
Scott Hess (OOO 6.25-7.5)
codereview site is giving me issues, so getting this posted even though I'm not really ...
4 years, 9 months ago (2010-09-15 01:09:17 UTC) #8
lzheng
Scott: I updated the code and your comments are addressed. Please take another look. Thanks! ...
4 years, 9 months ago (2010-09-15 19:21:49 UTC) #9
Scott Hess (OOO 6.25-7.5)
Starting to LGTM. Actually, looks like my comments are all completely advisory. So LGTM whether ...
4 years, 9 months ago (2010-09-16 23:57:49 UTC) #10
lzheng
Scott: a. I switched CHECK to either ASSERT_XX or EXPECT_XXX. b. As you suspected, ASSERT_xxx ...
4 years, 9 months ago (2010-09-17 17:36:47 UTC) #11
Scott Hess (OOO 6.25-7.5)
still lgtm. On 2010/09/17 17:36:47, lzheng wrote: > Scott: > > a. I switched CHECK ...
4 years, 9 months ago (2010-09-17 21:34:35 UTC) #12
Paweł Hajdan Jr.
Sorry, I still have some comments. I'm not going to complain about the inner loop ...
4 years, 9 months ago (2010-09-20 10:16:49 UTC) #13
lzheng1
I think I addressed all the comments. Another look? The net/test/python_utils.cc change was intended, since ...
4 years, 9 months ago (2010-09-21 00:34:33 UTC) #14
Paweł Hajdan Jr.
By "get rid of it" I have really meant it. :) It's also less code ...
4 years, 9 months ago (2010-09-21 08:42:07 UTC) #15
lzheng
I will find a time to chat with you sometime today, so we don't need ...
4 years, 9 months ago (2010-09-21 16:49:38 UTC) #16
lzheng
http://codereview.chromium.org/3277008/diff/71002/59006 File chrome/browser/safe_browsing/safe_browsing_test.cc (right): http://codereview.chromium.org/3277008/diff/71002/59006#newcode441 chrome/browser/safe_browsing/safe_browsing_test.cc:441: // TODO(lzheng): We should have a way to reliablely ...
4 years, 9 months ago (2010-09-21 17:25:37 UTC) #17
Paweł Hajdan Jr.
Code I commented in the drive-by lg with some comments. http://codereview.chromium.org/3277008/diff/71002/59006 File chrome/browser/safe_browsing/safe_browsing_test.cc (right): http://codereview.chromium.org/3277008/diff/71002/59006#newcode185 ...
4 years, 9 months ago (2010-09-21 17:35:07 UTC) #18
lzheng
http://codereview.chromium.org/3277008/diff/71002/59006 File chrome/browser/safe_browsing/safe_browsing_test.cc (right): http://codereview.chromium.org/3277008/diff/71002/59006#newcode185 chrome/browser/safe_browsing/safe_browsing_test.cc:185: static const char k_host_[]; On 2010/09/21 17:35:07, Paweł Hajdan ...
4 years, 9 months ago (2010-09-21 18:05:38 UTC) #19
eroman
http://codereview.chromium.org/3277008/diff/104001/100008 File chrome/browser/safe_browsing/safe_browsing_test.cc (right): http://codereview.chromium.org/3277008/diff/104001/100008#newcode5 chrome/browser/safe_browsing/safe_browsing_test.cc:5: // This test uses safebrowsing test server published at ...
4 years, 9 months ago (2010-09-29 01:32:11 UTC) #20
lzheng
Eric: Thanks for the review. Your comments are addressed. Please see comments inline. http://codereview.chromium.org/3277008/diff/104001/100008 File ...
4 years, 9 months ago (2010-09-29 18:31:06 UTC) #21
eroman
http://codereview.chromium.org/3277008/diff/104001/100008 File chrome/browser/safe_browsing/safe_browsing_test.cc (right): http://codereview.chromium.org/3277008/diff/104001/100008#newcode220 chrome/browser/safe_browsing/safe_browsing_test.cc:220: ASSERT_TRUE(safe_browsing_service_); On 2010/09/29 18:31:06, lzheng wrote: > On 2010/09/29 ...
4 years, 9 months ago (2010-10-06 16:51:28 UTC) #22
eroman
lgtm. please run it a number of times locally to make sure it runs non-flakily. ...
4 years, 8 months ago (2010-10-08 22:28:07 UTC) #23
lzheng
Eric: All comments addressed. I was meant to wait for the bot timeout change to ...
4 years, 8 months ago (2010-10-14 18:29:31 UTC) #24
eroman
4 years, 8 months ago (2010-10-14 20:52:19 UTC) #25
lgtm
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1f9106d