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

Issue 3277008: add safebrowsing testserver (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 7 months ago by lzheng
Modified:
2 years, 11 months ago
CC:
chromium-reviews_chromium.org, ben+cc_chromium.org, Paul Godavari, gcasto
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) Lint 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 0 errors 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 0 errors Download
Trybot results:
Commit:

Messages

Total messages: 25
lzheng
Add the full end to end test. The code is ready for review. However, before ...
3 years, 7 months ago #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": ...
3 years, 7 months ago #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 ...
3 years, 7 months ago #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 ...
3 years, 7 months ago #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"\"" ...
3 years, 7 months ago #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: > ...
3 years, 7 months ago #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 ...
3 years, 7 months ago #7
shess
codereview site is giving me issues, so getting this posted even though I'm not really ...
3 years, 7 months ago #8
lzheng
Scott: I updated the code and your comments are addressed. Please take another look. Thanks! ...
3 years, 7 months ago #9
shess
Starting to LGTM. Actually, looks like my comments are all completely advisory. So LGTM whether ...
3 years, 7 months ago #10
lzheng
Scott: a. I switched CHECK to either ASSERT_XX or EXPECT_XXX. b. As you suspected, ASSERT_xxx ...
3 years, 7 months ago #11
shess
still lgtm. On 2010/09/17 17:36:47, lzheng wrote: > Scott: > > a. I switched CHECK ...
3 years, 7 months ago #12
Paweł Hajdan Jr.
Sorry, I still have some comments. I'm not going to complain about the inner loop ...
3 years, 7 months ago #13
lzheng1
I think I addressed all the comments. Another look? The net/test/python_utils.cc change was intended, since ...
3 years, 7 months ago #14
Paweł Hajdan Jr.
By "get rid of it" I have really meant it. :) It's also less code ...
3 years, 7 months ago #15
lzheng
I will find a time to chat with you sometime today, so we don't need ...
3 years, 7 months ago #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 ...
3 years, 7 months ago #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 ...
3 years, 7 months ago #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 ...
3 years, 7 months ago #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 ...
3 years, 6 months ago #20
lzheng
Eric: Thanks for the review. Your comments are addressed. Please see comments inline. http://codereview.chromium.org/3277008/diff/104001/100008 File ...
3 years, 6 months ago #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 ...
3 years, 6 months ago #22
eroman
lgtm. please run it a number of times locally to make sure it runs non-flakily. ...
3 years, 6 months ago #23
lzheng
Eric: All comments addressed. I was meant to wait for the bot timeout change to ...
3 years, 6 months ago #24
eroman
3 years, 6 months ago #25
lgtm
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6