|
|
Created:
10 years, 5 months ago by lzheng Modified:
9 years, 7 months ago Reviewers:
eroman CC:
chromium-reviews, Paweł Hajdan Jr., Paul Godavari, ben+cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionFirst change to add safebrowsing service test.
Right now, the test only launch the browser and makes sure no
auto update is scheduled.
Later, the test suite server will be launched within the test
and requests will be issued,
BUG=47318
TEST=this is the test.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52719
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 13
Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #
Total comments: 30
Patch Set 11 : '' #Patch Set 12 : '' #
Messages
Total messages: 11 (0 generated)
More will come. This change is mainly to add a file to the browser test. I check with Nicolas Sylvain, he is okay to add this test to browser test as long as it won't run too long. When I later add changes to launch the save browsing test server, I will cc him the cl too.
http://codereview.chromium.org/2845035/diff/20001/21001 File chrome/browser/safe_browsing/protocol_manager.h (right): http://codereview.chromium.org/2845035/diff/20001/21001#newcode57 chrome/browser/safe_browsing/protocol_manager.h:57: friend class SafeBrowsingServiceTest; Can you not use FRIEND_TEST ? http://codereview.chromium.org/2845035/diff/20001/21002 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/2845035/diff/20001/21002#newcode45 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:45: bool is_update_in_progress() { I don't really understand how these variables are being used. The locking seems strange. How about doing all the test assertions on the IO thread instead, so you can test values directly in SafeBrowsingService / ProtocolManager? http://codereview.chromium.org/2845035/diff/20001/21002#newcode91 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:91: explicit SafeBrowsingServiceTestHelper( no need for explicit when there are more than 1 parameter. http://codereview.chromium.org/2845035/diff/20001/21002#newcode93 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:93: SafeBrowsingServiceTest* safe_browsing_test) This division feels strange. Can the code from SafeBrowsingServiceTest simply be merged into this class? http://codereview.chromium.org/2845035/diff/20001/21002#newcode158 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:158: // happending. typo. http://codereview.chromium.org/2845035/diff/20001/21003 File chrome/browser/safe_browsing/safe_browsing_service.h (right): http://codereview.chromium.org/2845035/diff/20001/21003#newcode183 chrome/browser/safe_browsing/safe_browsing_service.h:183: friend class SafeBrowsingServiceTest; can you not use friend test?
+ Scott Hey, Scott: Eric suggested that we should let you take a quick look at this change to provide us some high level suggestions, since neither of us are familiar with browser tests. The areas we need some suggestions are: 1. Do we want a mutex to synchronize between IO thread and UI thread in the test. e.g.: in http://codereview.chromium.org/2845035/diff/20001/21002#newcode93, there are several status variable that is updated in IO thread. They are updated in IO thread in UpdateSafeBrowsingStatus, and later checked using EXPECT_XXX in SafeBrowsingSystemTest under a mutex protection. Is this a common practice or we usually do the check in IO thread too so we don't need the mutex? e.g.: we could post a task to IO thread where we do all EXPECT_XXX. However, my concern is that if EXPECT_XXX failed, it is hard to tell which test case fails. 2. Is there any problem to implement thread safe ref count in InProcessBrowserTest? In in_process_browser_test.h, we have: static bool ImplementsThreadSafeReferenceCounting() { return false; } So, when I want to send a method of SafeBrowsingServiceTest from UI thread to IO thread using ChromeThread::PostTask(xxx, ..., NewRunnableMethod...), I trigger the DCHECK in src\base\task.h(232): DCHECK(T::ImplementsThreadSafeReferenceCounting()); I created a separate helper class (SafeBrowsingServiceTestHelper) to handle the call backs, which solves to problem, but if InProcessBrowserTest supports safereferencecounting, I don't need the helper class. Let me know if the question is not clear, I could certainly drop by and chat. Thanks! Lei theory, we could check the status variables in IO thread again and put EXPECT On Wed, Jul 7, 2010 at 4:02 PM, <eroman@chromium.org> wrote: > > http://codereview.chromium.org/2845035/diff/20001/21001 > File chrome/browser/safe_browsing/protocol_manager.h (right): > > http://codereview.chromium.org/2845035/diff/20001/21001#newcode57 > chrome/browser/safe_browsing/protocol_manager.h:57: friend class > SafeBrowsingServiceTest; > Can you not use FRIEND_TEST ? > > http://codereview.chromium.org/2845035/diff/20001/21002 > File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): > > http://codereview.chromium.org/2845035/diff/20001/21002#newcode45 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:45: bool > is_update_in_progress() { > I don't really understand how these variables are being used. The > locking seems strange. > > How about doing all the test assertions on the IO thread instead, so you > can test values directly in SafeBrowsingService / ProtocolManager? > > http://codereview.chromium.org/2845035/diff/20001/21002#newcode91 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:91: explicit > SafeBrowsingServiceTestHelper( > no need for explicit when there are more than 1 parameter. > > http://codereview.chromium.org/2845035/diff/20001/21002#newcode93 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:93: > SafeBrowsingServiceTest* safe_browsing_test) > This division feels strange. > > Can the code from SafeBrowsingServiceTest simply be merged into this > class? > > http://codereview.chromium.org/2845035/diff/20001/21002#newcode158 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:158: // > happending. > typo. > > http://codereview.chromium.org/2845035/diff/20001/21003 > File chrome/browser/safe_browsing/safe_browsing_service.h (right): > > http://codereview.chromium.org/2845035/diff/20001/21003#newcode183 > chrome/browser/safe_browsing/safe_browsing_service.h:183: friend class > SafeBrowsingServiceTest; > can you not use friend test? > > > http://codereview.chromium.org/2845035/show >
On Wed, Jul 7, 2010 at 5:54 PM, Lei Zheng <lzheng@google.com> wrote: > + Scott > Hey, Scott: > Eric suggested that we should let you take a quick look at this change to > provide us some high level suggestions, since neither of us are familiar > with browser tests. > The areas we need some suggestions are: > 1. Do we want a mutex to synchronize between IO thread and UI thread in the > test. > e.g.: in http://codereview.chromium.org/2845035/diff/20001/21002#newcode93, > there are several status variable that is updated in IO thread. They are > updated in IO thread in UpdateSafeBrowsingStatus, and later checked using > EXPECT_XXX in SafeBrowsingSystemTest under a mutex protection. Is this a > common practice or we usually do the check in IO thread too so we don't need > the mutex? e.g.: we could post a task to IO thread where we do all > EXPECT_XXX. However, my concern is that if EXPECT_XXX failed, it is hard to > tell which test case fails. I'm not sure how thread safe gtest is. http://code.google.com/p/googletest/wiki/GoogleTestAdvancedGuide#Catching_Fai... discusses thread safety, but I'm not sure if the primitives have been ported yet. Either way I would recommend structuring the test in such a way that all your assertions run on the ui thread. This is especially crucial if you're using ASSERT_XXX as they trigger a method to early return, so that if you're on a random thread and your ASSERT evaluates to false chances are an early return is going to make the test time out, rather than fail. Tests where a portion of the test runs on a different thread are typically structured like: 1. schedule the work to be done on the background thread. 2. Use MessageLoop::current()->Run() on the main thread so that it blocks. 3. when the background thread is done processing have it quit the main threads message loop. 4. do your assertions on the main thread after 2 completes. > 2. Is there any problem to implement thread safe ref count in > InProcessBrowserTest? In in_process_browser_test.h, we have: > static bool ImplementsThreadSafeReferenceCounting() { return false; } > So, when I want to send a method of SafeBrowsingServiceTest from UI thread > to IO thread using ChromeThread::PostTask(xxx, ..., NewRunnableMethod...), I > trigger the DCHECK in > src\base\task.h(232): > DCHECK(T::ImplementsThreadSafeReferenceCounting()); > I created a separate helper class (SafeBrowsingServiceTestHelper) to handle > the call backs, which solves to problem, but if InProcessBrowserTest > supports safereferencecounting, I don't need the helper class. > Let me know if the question is not clear, I could certainly drop by and > chat. InProcessBrowserTests lifetime is defined by gtest, so it doesn't correctly implement ref counting (AddRef/Release do nothing). This is a bit misleading. Given this, in theory it should be fine to make ImplementsThreadSafeReferenceCounting return true, although it's probably better if InProcessBrowserTest didn't implement Add/Release at all. -Scott > Thanks! > > Lei > > > theory, we could check the status variables in IO thread again and put > EXPECT > > On Wed, Jul 7, 2010 at 4:02 PM, <eroman@chromium.org> wrote: >> >> http://codereview.chromium.org/2845035/diff/20001/21001 >> File chrome/browser/safe_browsing/protocol_manager.h (right): >> >> http://codereview.chromium.org/2845035/diff/20001/21001#newcode57 >> chrome/browser/safe_browsing/protocol_manager.h:57: friend class >> SafeBrowsingServiceTest; >> Can you not use FRIEND_TEST ? >> >> http://codereview.chromium.org/2845035/diff/20001/21002 >> File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): >> >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode45 >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:45: bool >> is_update_in_progress() { >> I don't really understand how these variables are being used. The >> locking seems strange. >> >> How about doing all the test assertions on the IO thread instead, so you >> can test values directly in SafeBrowsingService / ProtocolManager? >> >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode91 >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:91: explicit >> SafeBrowsingServiceTestHelper( >> no need for explicit when there are more than 1 parameter. >> >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode93 >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:93: >> SafeBrowsingServiceTest* safe_browsing_test) >> This division feels strange. >> >> Can the code from SafeBrowsingServiceTest simply be merged into this >> class? >> >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode158 >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:158: // >> happending. >> typo. >> >> http://codereview.chromium.org/2845035/diff/20001/21003 >> File chrome/browser/safe_browsing/safe_browsing_service.h (right): >> >> http://codereview.chromium.org/2845035/diff/20001/21003#newcode183 >> chrome/browser/safe_browsing/safe_browsing_service.h:183: friend class >> SafeBrowsingServiceTest; >> can you not use friend test? >> >> http://codereview.chromium.org/2845035/show > >
Scott: Thanks for the response. A couple of comments inline: On Thu, Jul 8, 2010 at 9:13 AM, Scott Violet <sky@chromium.org> wrote: > On Wed, Jul 7, 2010 at 5:54 PM, Lei Zheng <lzheng@google.com> wrote: > > + Scott > > Hey, Scott: > > Eric suggested that we should let you take a quick look at this change to > > provide us some high level suggestions, since neither of us are familiar > > with browser tests. > > The areas we need some suggestions are: > > 1. Do we want a mutex to synchronize between IO thread and UI thread in > the > > test. > > e.g.: in > http://codereview.chromium.org/2845035/diff/20001/21002#newcode93, > > there are several status variable that is updated in IO thread. They are > > updated in IO thread in UpdateSafeBrowsingStatus, and later checked using > > EXPECT_XXX in SafeBrowsingSystemTest under a mutex protection. Is this a > > common practice or we usually do the check in IO thread too so we don't > need > > the mutex? e.g.: we could post a task to IO thread where we do all > > EXPECT_XXX. However, my concern is that if EXPECT_XXX failed, it is hard > to > > tell which test case fails. > > I'm not sure how thread safe gtest is. > > http://code.google.com/p/googletest/wiki/GoogleTestAdvancedGuide#Catching_Fai... > discusses thread safety, but I'm not sure if the primitives have been > ported yet. > > Either way I would recommend structuring the test in such a way that > all your assertions run on the ui thread. This is especially crucial > if you're using ASSERT_XXX as they trigger a method to early return, > so that if you're on a random thread and your ASSERT evaluates to > false chances are an early return is going to make the test time out, > rather than fail. Tests where a portion of the test runs on a > different thread are typically structured like: > > 1. schedule the work to be done on the background thread. > 2. Use MessageLoop::current()->Run() on the main thread so that it blocks. > 3. when the background thread is done processing have it quit the main > threads message loop. > 4. do your assertions on the main thread after 2 completes. > > Cool. This is essentially what I am doing now. I will chat with Eric to see if I could make the logics more clear/easy to follow. > > 2. Is there any problem to implement thread safe ref count in > > InProcessBrowserTest? In in_process_browser_test.h, we have: > > static bool ImplementsThreadSafeReferenceCounting() { return false; } > > So, when I want to send a method of SafeBrowsingServiceTest from UI > thread > > to IO thread using ChromeThread::PostTask(xxx, ..., > NewRunnableMethod...), I > > trigger the DCHECK in > > src\base\task.h(232): > > DCHECK(T::ImplementsThreadSafeReferenceCounting()); > > I created a separate helper class (SafeBrowsingServiceTestHelper) to > handle > > the call backs, which solves to problem, but if InProcessBrowserTest > > supports safereferencecounting, I don't need the helper class. > > Let me know if the question is not clear, I could certainly drop by and > > chat. > > InProcessBrowserTests lifetime is defined by gtest, so it doesn't > correctly implement ref counting (AddRef/Release do nothing). This is > a bit misleading. Given this, in theory it should be fine to make > ImplementsThreadSafeReferenceCounting return true, although it's > probably better if InProcessBrowserTest didn't implement Add/Release > at all. > Can you explain more why you think it is better not to implement Add/Release in InProcessBrowserTest? Thanks! Lei > -Scott > > > Thanks! > > > > Lei > > > > > > theory, we could check the status variables in IO thread again and put > > EXPECT > > > > On Wed, Jul 7, 2010 at 4:02 PM, <eroman@chromium.org> wrote: > >> > >> http://codereview.chromium.org/2845035/diff/20001/21001 > >> File chrome/browser/safe_browsing/protocol_manager.h (right): > >> > >> http://codereview.chromium.org/2845035/diff/20001/21001#newcode57 > >> chrome/browser/safe_browsing/protocol_manager.h:57: friend class > >> SafeBrowsingServiceTest; > >> Can you not use FRIEND_TEST ? > >> > >> http://codereview.chromium.org/2845035/diff/20001/21002 > >> File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): > >> > >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode45 > >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:45: bool > >> is_update_in_progress() { > >> I don't really understand how these variables are being used. The > >> locking seems strange. > >> > >> How about doing all the test assertions on the IO thread instead, so you > >> can test values directly in SafeBrowsingService / ProtocolManager? > >> > >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode91 > >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:91: explicit > >> SafeBrowsingServiceTestHelper( > >> no need for explicit when there are more than 1 parameter. > >> > >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode93 > >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:93: > >> SafeBrowsingServiceTest* safe_browsing_test) > >> This division feels strange. > >> > >> Can the code from SafeBrowsingServiceTest simply be merged into this > >> class? > >> > >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode158 > >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:158: // > >> happending. > >> typo. > >> > >> http://codereview.chromium.org/2845035/diff/20001/21003 > >> File chrome/browser/safe_browsing/safe_browsing_service.h (right): > >> > >> http://codereview.chromium.org/2845035/diff/20001/21003#newcode183 > >> chrome/browser/safe_browsing/safe_browsing_service.h:183: friend class > >> SafeBrowsingServiceTest; > >> can you not use friend test? > >> > >> http://codereview.chromium.org/2845035/show > > > > >
Thu, Jul 8, 2010 at 9:22 AM, Lei Zheng <lzheng@google.com> wrote: > Scott: > Thanks for the response. A couple of comments inline: > > On Thu, Jul 8, 2010 at 9:13 AM, Scott Violet <sky@chromium.org> wrote: >> >> On Wed, Jul 7, 2010 at 5:54 PM, Lei Zheng <lzheng@google.com> wrote: >> > + Scott >> > Hey, Scott: >> > Eric suggested that we should let you take a quick look at this change >> > to >> > provide us some high level suggestions, since neither of us are familiar >> > with browser tests. >> > The areas we need some suggestions are: >> > 1. Do we want a mutex to synchronize between IO thread and UI thread in >> > the >> > test. >> > e.g.: >> > in http://codereview.chromium.org/2845035/diff/20001/21002#newcode93, >> > there are several status variable that is updated in IO thread. They are >> > updated in IO thread in UpdateSafeBrowsingStatus, and later checked >> > using >> > EXPECT_XXX in SafeBrowsingSystemTest under a mutex protection. Is this a >> > common practice or we usually do the check in IO thread too so we don't >> > need >> > the mutex? e.g.: we could post a task to IO thread where we do all >> > EXPECT_XXX. However, my concern is that if EXPECT_XXX failed, it is hard >> > to >> > tell which test case fails. >> >> I'm not sure how thread safe gtest is. >> >> http://code.google.com/p/googletest/wiki/GoogleTestAdvancedGuide#Catching_Fai... >> discusses thread safety, but I'm not sure if the primitives have been >> ported yet. >> >> Either way I would recommend structuring the test in such a way that >> all your assertions run on the ui thread. This is especially crucial >> if you're using ASSERT_XXX as they trigger a method to early return, >> so that if you're on a random thread and your ASSERT evaluates to >> false chances are an early return is going to make the test time out, >> rather than fail. Tests where a portion of the test runs on a >> different thread are typically structured like: >> >> 1. schedule the work to be done on the background thread. >> 2. Use MessageLoop::current()->Run() on the main thread so that it blocks. >> 3. when the background thread is done processing have it quit the main >> threads message loop. >> 4. do your assertions on the main thread after 2 completes. >> > > Cool. This is essentially what I am doing now. I will chat with Eric to see > if I could make the logics more clear/easy to follow. > >> >> > 2. Is there any problem to implement thread safe ref count in >> > InProcessBrowserTest? In in_process_browser_test.h, we have: >> > static bool ImplementsThreadSafeReferenceCounting() { return false; } >> > So, when I want to send a method of SafeBrowsingServiceTest from UI >> > thread >> > to IO thread using ChromeThread::PostTask(xxx, ..., >> > NewRunnableMethod...), I >> > trigger the DCHECK in >> > src\base\task.h(232): >> > DCHECK(T::ImplementsThreadSafeReferenceCounting()); >> > I created a separate helper class (SafeBrowsingServiceTestHelper) to >> > handle >> > the call backs, which solves to problem, but if InProcessBrowserTest >> > supports safereferencecounting, I don't need the helper class. >> > Let me know if the question is not clear, I could certainly drop by and >> > chat. >> >> InProcessBrowserTests lifetime is defined by gtest, so it doesn't >> correctly implement ref counting (AddRef/Release do nothing). This is >> a bit misleading. Given this, in theory it should be fine to make >> ImplementsThreadSafeReferenceCounting return true, although it's >> probably better if InProcessBrowserTest didn't implement Add/Release >> at all. > > Can you explain more why you think it is better not to implement Add/Release > in InProcessBrowserTest? A ref counted object is destroyed when the ref count goes to 0. InProcessBrowserTest behaves like a ref counted object because it implements AddRef/Release, but it is destroyed by gtest when the test completes and so doesn't act like a typical ref counted object. I say it probably shouldn't implement the AddRef/Release so that it's clear InProcessBrowserTest isn't truly ref counted. OTOH we could make AddRef/Release really work and when the test is done EXPECT the ref count is 0. So, InProcessBrowserTest would implement AddRef/Release to track the ref count, but gtest would still end up deleting it. -Scott > > Thanks! > > Lei >> >> -Scott >> >> > Thanks! >> > >> > Lei >> > >> > >> > theory, we could check the status variables in IO thread again and put >> > EXPECT >> > >> > On Wed, Jul 7, 2010 at 4:02 PM, <eroman@chromium.org> wrote: >> >> >> >> http://codereview.chromium.org/2845035/diff/20001/21001 >> >> File chrome/browser/safe_browsing/protocol_manager.h (right): >> >> >> >> http://codereview.chromium.org/2845035/diff/20001/21001#newcode57 >> >> chrome/browser/safe_browsing/protocol_manager.h:57: friend class >> >> SafeBrowsingServiceTest; >> >> Can you not use FRIEND_TEST ? >> >> >> >> http://codereview.chromium.org/2845035/diff/20001/21002 >> >> File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): >> >> >> >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode45 >> >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:45: bool >> >> is_update_in_progress() { >> >> I don't really understand how these variables are being used. The >> >> locking seems strange. >> >> >> >> How about doing all the test assertions on the IO thread instead, so >> >> you >> >> can test values directly in SafeBrowsingService / ProtocolManager? >> >> >> >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode91 >> >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:91: explicit >> >> SafeBrowsingServiceTestHelper( >> >> no need for explicit when there are more than 1 parameter. >> >> >> >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode93 >> >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:93: >> >> SafeBrowsingServiceTest* safe_browsing_test) >> >> This division feels strange. >> >> >> >> Can the code from SafeBrowsingServiceTest simply be merged into this >> >> class? >> >> >> >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode158 >> >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:158: // >> >> happending. >> >> typo. >> >> >> >> http://codereview.chromium.org/2845035/diff/20001/21003 >> >> File chrome/browser/safe_browsing/safe_browsing_service.h (right): >> >> >> >> http://codereview.chromium.org/2845035/diff/20001/21003#newcode183 >> >> chrome/browser/safe_browsing/safe_browsing_service.h:183: friend class >> >> SafeBrowsingServiceTest; >> >> can you not use friend test? >> >> >> >> http://codereview.chromium.org/2845035/show >> > >> > > >
On Thu, Jul 8, 2010 at 9:38 AM, Scott Violet <sky@chromium.org> wrote: > Thu, Jul 8, 2010 at 9:22 AM, Lei Zheng <lzheng@google.com> wrote: > > Scott: > > Thanks for the response. A couple of comments inline: > > > > On Thu, Jul 8, 2010 at 9:13 AM, Scott Violet <sky@chromium.org> wrote: > >> > >> On Wed, Jul 7, 2010 at 5:54 PM, Lei Zheng <lzheng@google.com> wrote: > >> > + Scott > >> > Hey, Scott: > >> > Eric suggested that we should let you take a quick look at this change > >> > to > >> > provide us some high level suggestions, since neither of us are > familiar > >> > with browser tests. > >> > The areas we need some suggestions are: > >> > 1. Do we want a mutex to synchronize between IO thread and UI thread > in > >> > the > >> > test. > >> > e.g.: > >> > in http://codereview.chromium.org/2845035/diff/20001/21002#newcode93, > >> > there are several status variable that is updated in IO thread. They > are > >> > updated in IO thread in UpdateSafeBrowsingStatus, and later checked > >> > using > >> > EXPECT_XXX in SafeBrowsingSystemTest under a mutex protection. Is this > a > >> > common practice or we usually do the check in IO thread too so we > don't > >> > need > >> > the mutex? e.g.: we could post a task to IO thread where we do all > >> > EXPECT_XXX. However, my concern is that if EXPECT_XXX failed, it is > hard > >> > to > >> > tell which test case fails. > >> > >> I'm not sure how thread safe gtest is. > >> > >> > http://code.google.com/p/googletest/wiki/GoogleTestAdvancedGuide#Catching_Fai... > >> discusses thread safety, but I'm not sure if the primitives have been > >> ported yet. > >> > >> Either way I would recommend structuring the test in such a way that > >> all your assertions run on the ui thread. This is especially crucial > >> if you're using ASSERT_XXX as they trigger a method to early return, > >> so that if you're on a random thread and your ASSERT evaluates to > >> false chances are an early return is going to make the test time out, > >> rather than fail. Tests where a portion of the test runs on a > >> different thread are typically structured like: > >> > >> 1. schedule the work to be done on the background thread. > >> 2. Use MessageLoop::current()->Run() on the main thread so that it > blocks. > >> 3. when the background thread is done processing have it quit the main > >> threads message loop. > >> 4. do your assertions on the main thread after 2 completes. > >> > > > > Cool. This is essentially what I am doing now. I will chat with Eric to > see > > if I could make the logics more clear/easy to follow. > > > >> > >> > 2. Is there any problem to implement thread safe ref count in > >> > InProcessBrowserTest? In in_process_browser_test.h, we have: > >> > static bool ImplementsThreadSafeReferenceCounting() { return false; } > >> > So, when I want to send a method of SafeBrowsingServiceTest from UI > >> > thread > >> > to IO thread using ChromeThread::PostTask(xxx, ..., > >> > NewRunnableMethod...), I > >> > trigger the DCHECK in > >> > src\base\task.h(232): > >> > DCHECK(T::ImplementsThreadSafeReferenceCounting()); > >> > I created a separate helper class (SafeBrowsingServiceTestHelper) to > >> > handle > >> > the call backs, which solves to problem, but if InProcessBrowserTest > >> > supports safereferencecounting, I don't need the helper class. > >> > Let me know if the question is not clear, I could certainly drop by > and > >> > chat. > >> > >> InProcessBrowserTests lifetime is defined by gtest, so it doesn't > >> correctly implement ref counting (AddRef/Release do nothing). This is > >> a bit misleading. Given this, in theory it should be fine to make > >> ImplementsThreadSafeReferenceCounting return true, although it's > >> probably better if InProcessBrowserTest didn't implement Add/Release > >> at all. > > > > Can you explain more why you think it is better not to implement > Add/Release > > in InProcessBrowserTest? > > A ref counted object is destroyed when the ref count goes to 0. > InProcessBrowserTest behaves like a ref counted object because it > implements AddRef/Release, but it is destroyed by gtest when the test > completes and so doesn't act like a typical ref counted object. I say > it probably shouldn't implement the AddRef/Release so that it's clear > InProcessBrowserTest isn't truly ref counted. OTOH we could make > AddRef/Release really work and when the test is done EXPECT the ref > count is 0. So, InProcessBrowserTest would implement AddRef/Release to > track the ref count, but gtest would still end up deleting it. > > I like the way to consider that InProcessBrowserTest is the actual browser that it will only be destroyed after the test is finished. So, it might be better to keep it as is in that sense. Eric: I refactored the code so it should be more clear that the helper class only handles callbacks and the SafeBrowsingServiceTest keeps the desired status. Hope this is makes the flow easier to follow. I will address your review comments in a separate thread by replying from the codereview site. Please feel free to take another look when you get a chance. Thanks! Lei > -Scott > > > > > Thanks! > > > > Lei > >> > >> -Scott > >> > >> > Thanks! > >> > > >> > Lei > >> > > >> > > >> > theory, we could check the status variables in IO thread again and > put > >> > EXPECT > >> > > >> > On Wed, Jul 7, 2010 at 4:02 PM, <eroman@chromium.org> wrote: > >> >> > >> >> http://codereview.chromium.org/2845035/diff/20001/21001 > >> >> File chrome/browser/safe_browsing/protocol_manager.h (right): > >> >> > >> >> http://codereview.chromium.org/2845035/diff/20001/21001#newcode57 > >> >> chrome/browser/safe_browsing/protocol_manager.h:57: friend class > >> >> SafeBrowsingServiceTest; > >> >> Can you not use FRIEND_TEST ? > >> >> > >> >> http://codereview.chromium.org/2845035/diff/20001/21002 > >> >> File chrome/browser/safe_browsing/safe_browsing_browsertest.cc > (right): > >> >> > >> >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode45 > >> >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:45: bool > >> >> is_update_in_progress() { > >> >> I don't really understand how these variables are being used. The > >> >> locking seems strange. > >> >> > >> >> How about doing all the test assertions on the IO thread instead, so > >> >> you > >> >> can test values directly in SafeBrowsingService / ProtocolManager? > >> >> > >> >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode91 > >> >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:91: > explicit > >> >> SafeBrowsingServiceTestHelper( > >> >> no need for explicit when there are more than 1 parameter. > >> >> > >> >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode93 > >> >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:93: > >> >> SafeBrowsingServiceTest* safe_browsing_test) > >> >> This division feels strange. > >> >> > >> >> Can the code from SafeBrowsingServiceTest simply be merged into this > >> >> class? > >> >> > >> >> http://codereview.chromium.org/2845035/diff/20001/21002#newcode158 > >> >> chrome/browser/safe_browsing/safe_browsing_browsertest.cc:158: // > >> >> happending. > >> >> typo. > >> >> > >> >> http://codereview.chromium.org/2845035/diff/20001/21003 > >> >> File chrome/browser/safe_browsing/safe_browsing_service.h (right): > >> >> > >> >> http://codereview.chromium.org/2845035/diff/20001/21003#newcode183 > >> >> chrome/browser/safe_browsing/safe_browsing_service.h:183: friend > class > >> >> SafeBrowsingServiceTest; > >> >> can you not use friend test? > >> >> > >> >> http://codereview.chromium.org/2845035/show > >> > > >> > > > > > >
http://codereview.chromium.org/2845035/diff/20001/21001 File chrome/browser/safe_browsing/protocol_manager.h (right): http://codereview.chromium.org/2845035/diff/20001/21001#newcode57 chrome/browser/safe_browsing/protocol_manager.h:57: friend class SafeBrowsingServiceTest; On 2010/07/07 23:02:06, eroman wrote: > Can you not use FRIEND_TEST ? The access of protocol_manager memebers are wrapped in SafeBrowsingServiceTest, so, I only need to declare "SafeBrowsingServiceTest" as friend. For FRIEND_TEST(test_case, test_methodX), it means the test_method is accessing the private members. The benefit for the first is that we only need to declare one class as friend in case we need to access protocol buffers in test_methodY, test_methodZ... http://codereview.chromium.org/2845035/diff/20001/21002 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/2845035/diff/20001/21002#newcode45 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:45: bool is_update_in_progress() { On 2010/07/07 23:02:06, eroman wrote: > I don't really understand how these variables are being used. The locking seems > strange. > > How about doing all the test assertions on the IO thread instead, so you can > test values directly in SafeBrowsingService / ProtocolManager? As we discussed in this thread, the lock is to synchronize when we access these variables in different threads. http://codereview.chromium.org/2845035/diff/20001/21002#newcode91 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:91: explicit SafeBrowsingServiceTestHelper( On 2010/07/07 23:02:06, eroman wrote: > no need for explicit when there are more than 1 parameter. Done. http://codereview.chromium.org/2845035/diff/20001/21002#newcode93 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:93: SafeBrowsingServiceTest* safe_browsing_test) On 2010/07/07 23:02:06, eroman wrote: > This division feels strange. > > Can the code from SafeBrowsingServiceTest simply be merged into this class? I refactored the code a bit so helper only handles callbacks and SafeBrowsingServiceTest handles states. http://codereview.chromium.org/2845035/diff/20001/21002#newcode158 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:158: // happending. On 2010/07/07 23:02:06, eroman wrote: > typo. Done. http://codereview.chromium.org/2845035/diff/20001/21002#newcode158 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:158: // happending. On 2010/07/07 23:02:06, eroman wrote: > typo. Done. http://codereview.chromium.org/2845035/diff/20001/21003 File chrome/browser/safe_browsing/safe_browsing_service.h (right): http://codereview.chromium.org/2845035/diff/20001/21003#newcode183 chrome/browser/safe_browsing/safe_browsing_service.h:183: friend class SafeBrowsingServiceTest; On 2010/07/07 23:02:06, eroman wrote: > can you not use friend test? Same as the friend in protocolmanager.
Overall LGTM, some nits: http://codereview.chromium.org/2845035/diff/39002/46002 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/2845035/diff/39002/46002#newcode37 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:37: } nit: in some cases you have no newline between functions, in others you do. please keep it consistent -- always have a line separating them. http://codereview.chromium.org/2845035/diff/39002/46002#newcode39 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:39: const GURL& url) { nit: does this fit on the previous line? http://codereview.chromium.org/2845035/diff/39002/46002#newcode43 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:43: safe_browsing_service_->CancelCheck(helper); Don't you want to handle the async completion case as well? http://codereview.chromium.org/2845035/diff/39002/46002#newcode81 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:81: Lock update_status_mutex_; Can you add a comment explaining what members this protects? http://codereview.chromium.org/2845035/diff/39002/46002#newcode97 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:97: : public base::RefCountedThreadSafe<SafeBrowsingServiceTestHelper>, indent by 4 spaces http://codereview.chromium.org/2845035/diff/39002/46002#newcode101 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:101: SafeBrowsingServiceTest* safe_browsing_test) Can this go on the previous line? http://codereview.chromium.org/2845035/diff/39002/46002#newcode107 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:107: SafeBrowsingService::UrlCheckResult result) { indentation http://codereview.chromium.org/2845035/diff/39002/46002#newcode117 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:117: ChromeThread::PostTask(ChromeThread::IO, Move this parameter to the next line http://codereview.chromium.org/2845035/diff/39002/46002#newcode122 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:122: void CheckUrlInIOThread(const GURL& url) { nit: "On" instead of "In" for consistency with our other code which tends to use "on thread". http://codereview.chromium.org/2845035/diff/39002/46002#newcode128 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:128: void OnCheckUrlInIOThreadDone() { same comment. please prefer "on thread" http://codereview.chromium.org/2845035/diff/39002/46002#newcode135 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:135: ChromeThread::PostTask(ChromeThread::IO, please break move first parameter down a line. http://codereview.chromium.org/2845035/diff/39002/46002#newcode140 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:140: void CheckStatusInIOThread() { "In" --> "On" http://codereview.chromium.org/2845035/diff/39002/46002#newcode150 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:150: check_update_timer_.Start( I don't think you need a timer for this, as it isn't a recurring task. How about using PostDelayed ? http://codereview.chromium.org/2845035/diff/39002/46002#newcode175 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:175: ui_test_utils::RunMessageLoop(); Please add a comment here that explains we will break out once the "CheckStatus" has occurred. http://codereview.chromium.org/2845035/diff/39002/46002#newcode182 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:182: ui_test_utils::RunMessageLoop(); Same thing -- please comment the condition when we break out of this (notably once CheckUrl completes).
Another look? http://codereview.chromium.org/2845035/diff/39002/46002 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/2845035/diff/39002/46002#newcode37 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:37: } On 2010/07/09 18:23:23, eroman wrote: > nit: in some cases you have no newline between functions, in others you do. > please keep it consistent -- always have a line separating them. I use blank lines to separate most of the functions now. Except that in SafeBrowsingServiceTestHelper, in order to group some functions together to indicate they are callbacks that handle the same request. Let me what you think. http://codereview.chromium.org/2845035/diff/39002/46002#newcode39 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:39: const GURL& url) { On 2010/07/09 18:23:23, eroman wrote: > nit: does this fit on the previous line? Done. http://codereview.chromium.org/2845035/diff/39002/46002#newcode43 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:43: safe_browsing_service_->CancelCheck(helper); On 2010/07/09 18:23:23, eroman wrote: > Don't you want to handle the async completion case as well? I renamed the variable to is_url_match_db_ so when it is true, it means this url's prefix has a match in safebrowsing db. Later, we will need to handle async case. I need to implement the "OnUrlCheckResult" below. http://codereview.chromium.org/2845035/diff/39002/46002#newcode81 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:81: Lock update_status_mutex_; On 2010/07/09 18:23:23, eroman wrote: > Can you add a comment explaining what members this protects? Done. http://codereview.chromium.org/2845035/diff/39002/46002#newcode97 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:97: : public base::RefCountedThreadSafe<SafeBrowsingServiceTestHelper>, On 2010/07/09 18:23:23, eroman wrote: > indent by 4 spaces Done. http://codereview.chromium.org/2845035/diff/39002/46002#newcode101 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:101: SafeBrowsingServiceTest* safe_browsing_test) On 2010/07/09 18:23:23, eroman wrote: > Can this go on the previous line? Done. http://codereview.chromium.org/2845035/diff/39002/46002#newcode107 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:107: SafeBrowsingService::UrlCheckResult result) { On 2010/07/09 18:23:23, eroman wrote: > indentation Done. http://codereview.chromium.org/2845035/diff/39002/46002#newcode117 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:117: ChromeThread::PostTask(ChromeThread::IO, On 2010/07/09 18:23:23, eroman wrote: > Move this parameter to the next line Moved most of them into one line. Let me know if that is okay. http://codereview.chromium.org/2845035/diff/39002/46002#newcode122 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:122: void CheckUrlInIOThread(const GURL& url) { On 2010/07/09 18:23:23, eroman wrote: > nit: "On" instead of "In" for consistency with our other code which tends to use > "on thread". Done. http://codereview.chromium.org/2845035/diff/39002/46002#newcode128 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:128: void OnCheckUrlInIOThreadDone() { On 2010/07/09 18:23:23, eroman wrote: > same comment. please prefer "on thread" Done. http://codereview.chromium.org/2845035/diff/39002/46002#newcode135 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:135: ChromeThread::PostTask(ChromeThread::IO, On 2010/07/09 18:23:23, eroman wrote: > please break move first parameter down a line. Moved most of them into one line. http://codereview.chromium.org/2845035/diff/39002/46002#newcode140 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:140: void CheckStatusInIOThread() { On 2010/07/09 18:23:23, eroman wrote: > "In" --> "On" Done. http://codereview.chromium.org/2845035/diff/39002/46002#newcode150 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:150: check_update_timer_.Start( On 2010/07/09 18:23:23, eroman wrote: > I don't think you need a timer for this, as it isn't a recurring task. > > How about using PostDelayed ? That's what I want. Thanks. http://codereview.chromium.org/2845035/diff/39002/46002#newcode175 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:175: ui_test_utils::RunMessageLoop(); On 2010/07/09 18:23:23, eroman wrote: > Please add a comment here that explains we will break out once the "CheckStatus" > has occurred. Done. http://codereview.chromium.org/2845035/diff/39002/46002#newcode182 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:182: ui_test_utils::RunMessageLoop(); On 2010/07/09 18:23:23, eroman wrote: > Same thing -- please comment the condition when we break out of this (notably > once CheckUrl completes). Done.
LGTM |