Intergration of the client-side phishing detection.

Intergration of the client-side phishing detection.

If the client-side phishing detection classifies a page as phishing it will
send back a ping to Google to verify whether or not the page is really phishing.

If the server also classifies the site as phishing we may show a phishing
interstitial if it is enabled.

BUG=
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75299

[noelutz, 2011-01-15 00:32:34]

I'm not sure why this CL didn't get sent out earlier.  Anyways.  I merged in
Brian's changes which removed the thumbnail everywhere.

Please take a look at this CL.
Thanks,
noe.

[Brian Ryner, 2011-01-20 23:36:40]

Looks pretty good to me overall, just some minor comments.

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_view_host_delegate.h:759: virtual void
DetectedPhishingSite(const GURL& phishing_url,
Add a comment for this method?

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:49: delete this;
This looks a little odd at first glance, can you add a comment?

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:338: LOG(INFO) <<
"Start sending csd request";
Make this a VLOG?

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:379: LOG(INFO) <<
"Stop sending csd request";
Same here.

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_service.h (right):

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.h:55: virtual void
OnUrlCheckResult(const GURL& url,
Comment that these methods are from SafeBrowsingService::Client?

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.h:76: };
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.cc:3162: // if there isn't any service
class in the browser.
Checking this is still a good idea though, since we can't trust that the
renderer is well-behaved.

[lzheng, 2011-01-25 00:43:43]

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:47: void
CsdClient::OnUrlCheckResult(const GURL& url,
I don't think this is actually used? The interface for SafeBrowsing::Client
client is changed and if you don't actually call safebrowsing service to check
url, you don't need to implement this.

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_service.h (right):

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.h:50: class CsdClient
: public SafeBrowsingService::Client,
Can add comments regarding which thread will call this class, and add DCHECKS at
proper places?
It will be helpful if there is comment about the lifetime of an instance of this
class too.

[noelutz, 2011-02-10 01:16:23]

Hi there,
I refactored the code to use the new tab contents delegate class.  Please take
another look.

Thanks.
noe.

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:47: void
CsdClient::OnUrlCheckResult(const GURL& url,
On 2011/01/25 00:43:43, lzheng wrote:
> I don't think this is actually used? The interface for SafeBrowsing::Client
> client is changed and if you don't actually call safebrowsing service to check
> url, you don't need to implement this. 

Removed.

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:49: delete this;
On 2011/01/20 23:36:40, Brian Ryner wrote:
> This looks a little odd at first glance, can you add a comment?

Done.

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:338: LOG(INFO) <<
"Start sending csd request";
On 2011/01/20 23:36:40, Brian Ryner wrote:
> Make this a VLOG?

Removed.  Needed only for debugging.

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:379: LOG(INFO) <<
"Stop sending csd request";
On 2011/01/20 23:36:40, Brian Ryner wrote:
> Same here.

Removed.  Needed only for debugging.

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_service.h (right):

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.h:50: class CsdClient
: public SafeBrowsingService::Client,
On 2011/01/25 00:43:43, lzheng wrote:
> Can add comments regarding which thread will call this class, and add DCHECKS
at
> proper places?
> It will be helpful if there is comment about the lifetime of an instance of
this
> class too.

Done.

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.h:55: virtual void
OnUrlCheckResult(const GURL& url,
On 2011/01/20 23:36:40, Brian Ryner wrote:
> Comment that these methods are from SafeBrowsingService::Client?

Done.

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.h:76: };
On 2011/01/20 23:36:40, Brian Ryner wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6014003/diff/7001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.cc:3162: // if there isn't any service
class in the browser.
On 2011/01/20 23:36:40, Brian Ryner wrote:
> Checking this is still a good idea though, since we can't trust that the
> renderer is well-behaved.

Yeah. Good point.

[Brian Ryner, 2011-02-11 01:30:39]

Looks nice overall, I like the refactoring.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host.cc (left):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host.cc:1672: void
RenderViewHost::OnDetectedPhishingSite(const GURL& phishing_url,
Also remove the declaration of this method in render_view_host.h.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:60: //
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Just so I understand, is the issue that the tests are actually calling this from
the wrong thread, or are they just not instantiating a BrowserThread for the UI
thread?

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:67: if
(cb_factory_.HasPendingCallbacks()) {
Is there a reason why you're wrapping this in HasPendingCallback, or could you
just always call RevokeAll?

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:75: // this method is
called.  The renderer should not start phishing detection
Would this ever happen during shutdown? i.e. could the
ClientSideDetectionService class be destroyed while there's still a pending
DetectedPhishingSite IPC?

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:98: // user browses
away before the interstitial is shown.  Maybe we should
It would be kind of nice if we could change
SafeBrowsingService::ShowBlockingPage to handle this for us.  For example, if we
could pass in a URL or a page id and it would verify that it's still showing
before it shows the interstitial.  This could be a future change though.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:40: // From
TabContentsObserver.  If we navigate way we cancel all pending
way -> away

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:71:
ClientSideDetectionService* service_;  // might be NULL.
Here and on line 73 - can you explain briefly when these would be null?

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:81: // TODO(noelutz):
might want to move this class to the CC file.
Seems reasonable to me to move it.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:84: explicit
CsdClient();
"explicit" is only needed for 1-argument constructors.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:96: ~CsdClient();
This should be declared virtual since it's virtual in the base class.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:33:
void(const GURL& phishing_url,
Hm, I usually see these written without the parameter names, since they aren't
needed, e.g.

MOCK_METHOD7(SendClientReportPhishingRequest,
                                void(const GURL&, double,
ClientReportPhishingRequestCallback*));

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:68:
io_thread_.reset(new BrowserThread(BrowserThread::IO, &message_loop_));
Unless there's a reason not to, you may want to actually spin this up as a real
thread.  That way it will exercise your DCHECKs about events happening on the
correct thread.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:93: 
Extra blank line?

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:106:
MessageLoop::current()->Run();
This is kind of a nit, but you're using different logic for when you expect the
interstitial to be shown vs. not shown.

For example, suppose the interstitial is shown after a task gets posted back to
the message loop and runs.  Your current code will work fine in that case.  But,
if it does two asynchronous operations, ExpectNotShowInterstitial will now
always succeed, because it only runs through the message loop once.

I'm not sure if there's a better solution here, but it seems like calling
MessageLoop::RunAllPending the same number of times for both cases would help
ensure that the test is working correctly.  Thoughts?

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_service.cc:60: :
model_path_(),
As I mentioned on another CL, I'm kind of concerned about duplicating the
initialization code here, since the two versions might get out of sync.  Can
your mock just call the regular constructor, passing in an empty model_path and
null request_context_getter?

[lzheng, 2011-02-11 19:39:42]

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:102: // the
SafeBrowsing service class.
I will be happy if the DisplayBlockingPage could be refactored so
malware/phishing/csd/download could share.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:29: // TODO(noelutz):
move all client-side detection IPCs to this class.
What are other IPC messages that would be moved here? 
Looking at Brian's change http://codereview.chromium.org/6398001, 
does it make sense to move the ClientSideDetectionService::Observe here?

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:66: void
set_safe_browsing_service(SafeBrowsingService* service);
set_client_side_detection_service and set_safe_browsing_service could be removed
if you check the comments I left in .cc.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:75:
csd_host_->set_safe_browsing_service(sb_service_.get());
You can get rid of these two set_xx by using
SafeBrowsingService::RegisterFactory() and also create something similar for csd
service. You can use safe_browsing_blocking_page_test.cc as an example. The
basic idea is to create a factory that will use your mock as the implementation.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_service.cc:25: #include
"chrome/common/chrome_switches.h"
Is this used somewhere? You might want to double check the new header files just
added too.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_service.h (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_service.h:36: #include
"chrome/browser/tab_contents/navigation_controller.h"
From the changes, it is not obvious to me hy these headers are added either.
Double check?

http://codereview.chromium.org/6014003/diff/21001/chrome/common/safebrowsing_...
File chrome/common/safebrowsing_messages.cc (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/common/safebrowsing_...
chrome/common/safebrowsing_messages.cc:4: 
Do we need all these header files below? e.g.: I wonder if file_path.h is
needed.

[jam, 2011-02-15 18:54:53]

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:50: // Handles the IPC
that is sent from the renderer the a URL is
nit: no need to comment IPC message handlers, since the message itself is
commented

http://codereview.chromium.org/6014003/diff/21001/chrome/common/safebrowsing_...
File chrome/common/safebrowsing_messages.h (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/common/safebrowsing_...
chrome/common/safebrowsing_messages.h:24: // renderer.  TODO(noelutz): move
other IPC messages here.
thanks for creating this new file.  the sooner this TODO done, the better

[noelutz, 2011-02-15 23:00:55]

Thanks for your comments.  Please take another look.

noe.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host.cc (left):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host.cc:1672: void
RenderViewHost::OnDetectedPhishingSite(const GURL& phishing_url,
On 2011/02/11 01:30:39, Brian Ryner wrote:
> Also remove the declaration of this method in render_view_host.h.

Done.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:60: //
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2011/02/11 01:30:39, Brian Ryner wrote:
> Just so I understand, is the issue that the tests are actually calling this
from
> the wrong thread, or are they just not instantiating a BrowserThread for the
UI
> thread?

Tests just don't create the UI browser thread object.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:67: if
(cb_factory_.HasPendingCallbacks()) {
On 2011/02/11 01:30:39, Brian Ryner wrote:
> Is there a reason why you're wrapping this in HasPendingCallback, or could you
> just always call RevokeAll?

You're right.  It's not needed.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:75: // this method is
called.  The renderer should not start phishing detection
On 2011/02/11 01:30:39, Brian Ryner wrote:
> Would this ever happen during shutdown? i.e. could the
> ClientSideDetectionService class be destroyed while there's still a pending
> DetectedPhishingSite IPC?

Good question.  The ClientSideDetectionService class only gets destroyed once
the browser destructor is called.  I'm not sure if this object will still be
around at this point.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:98: // user browses
away before the interstitial is shown.  Maybe we should
On 2011/02/11 01:30:39, Brian Ryner wrote:
> It would be kind of nice if we could change
> SafeBrowsingService::ShowBlockingPage to handle this for us.  For example, if
we
> could pass in a URL or a page id and it would verify that it's still showing
> before it shows the interstitial.  This could be a future change though.

Page id might work. We can't simply check the phishing url because it is
canonicalized.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:102: // the
SafeBrowsing service class.
On 2011/02/11 19:39:42, lzheng wrote:
> I will be happy if the DisplayBlockingPage could be refactored so
> malware/phishing/csd/download could share.

what does download do right now?  We do share malware/phishing/csd but the csd
usage of it is a bit hacky.  See comment above.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:29: // TODO(noelutz):
move all client-side detection IPCs to this class.
On 2011/02/11 19:39:42, lzheng wrote:
> What are other IPC messages that would be moved here? 
> Looking at Brian's change http://codereview.chromium.org/6398001, 
> does it make sense to move the ClientSideDetectionService::Observe here? 

I guess we have three IPCs?

Browser -> Renderer:
  - send model file to the renderer
  - tell the renderer to start classification

Renderer -> Browser:
  - renderer tells the browser that a page is phishing.

I was thinking we could move the other two IPCs here but definitely in a
separate CL.  Thoughts?

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:40: // From
TabContentsObserver.  If we navigate way we cancel all pending
On 2011/02/11 01:30:39, Brian Ryner wrote:
> way -> away

Done.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:50: // Handles the IPC
that is sent from the renderer the a URL is
On 2011/02/15 18:54:53, John Abd-El-Malek wrote:
> nit: no need to comment IPC message handlers, since the message itself is
> commented

Done.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:66: void
set_safe_browsing_service(SafeBrowsingService* service);
On 2011/02/11 19:39:42, lzheng wrote:
> set_client_side_detection_service and set_safe_browsing_service could be
removed
> if you check the comments I left in .cc.

see comment in the cc file.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:71:
ClientSideDetectionService* service_;  // might be NULL.
On 2011/02/11 01:30:39, Brian Ryner wrote:
> Here and on line 73 - can you explain briefly when these would be null?

Done.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:81: // TODO(noelutz):
might want to move this class to the CC file.
On 2011/02/11 01:30:39, Brian Ryner wrote:
> Seems reasonable to me to move it.

Done.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:84: explicit
CsdClient();
On 2011/02/11 01:30:39, Brian Ryner wrote:
> "explicit" is only needed for 1-argument constructors.

Sure. Leftover from previous version of the code.  Removed.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:96: ~CsdClient();
On 2011/02/11 01:30:39, Brian Ryner wrote:
> This should be declared virtual since it's virtual in the base class.

Nice catch.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:33:
void(const GURL& phishing_url,
On 2011/02/11 01:30:39, Brian Ryner wrote:
> Hm, I usually see these written without the parameter names, since they aren't
> needed, e.g.
> 
> MOCK_METHOD7(SendClientReportPhishingRequest,
>                                 void(const GURL&, double,
> ClientReportPhishingRequestCallback*));

Done.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:68:
io_thread_.reset(new BrowserThread(BrowserThread::IO, &message_loop_));
On 2011/02/11 01:30:39, Brian Ryner wrote:
> Unless there's a reason not to, you may want to actually spin this up as a
real
> thread.  That way it will exercise your DCHECKs about events happening on the
> correct thread.

Done.  This was much harder than I thought but it improves the test I think.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:75:
csd_host_->set_safe_browsing_service(sb_service_.get());
On 2011/02/11 19:39:42, lzheng wrote:
> You can get rid of these two set_xx by using
> SafeBrowsingService::RegisterFactory() and also create something similar for
csd
> service. You can use safe_browsing_blocking_page_test.cc as an example. The
> basic idea is to create a factory that will use your mock as the
implementation.

I don't think that would work.  Another reason I inject the service objects like
this is because there is no resource dispatcher host object in my unit test. 
Even if I register a special safebrowsing service factory sb_service_ will be
NULL in my test. if I don't use these setters.

An alternative would be to create a factory class for the new client-side
detection host class.  TabContents would use the factory to create an instance. 
In the test I could provide a test factory instance that would create the csd
host object with the two mock service instances.  Would you like that better
than the current solution?

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:93: 
On 2011/02/11 01:30:39, Brian Ryner wrote:
> Extra blank line?

Done.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:106:
MessageLoop::current()->Run();
On 2011/02/11 01:30:39, Brian Ryner wrote:
> This is kind of a nit, but you're using different logic for when you expect
the
> interstitial to be shown vs. not shown.
> 
> For example, suppose the interstitial is shown after a task gets posted back
to
> the message loop and runs.  Your current code will work fine in that case. 
But,
> if it does two asynchronous operations, ExpectNotShowInterstitial will now
> always succeed, because it only runs through the message loop once.
> 
> I'm not sure if there's a better solution here, but it seems like calling
> MessageLoop::RunAllPending the same number of times for both cases would help
> ensure that the test is working correctly.  Thoughts?

Agreed.  It makes more sense to use the same logic in both cases.  Changed.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_service.cc:25: #include
"chrome/common/chrome_switches.h"
On 2011/02/11 19:39:42, lzheng wrote:
> Is this used somewhere? You might want to double check the new header files
just
> added too.

Done.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_service.cc:60: :
model_path_(),
On 2011/02/11 01:30:39, Brian Ryner wrote:
> As I mentioned on another CL, I'm kind of concerned about duplicating the
> initialization code here, since the two versions might get out of sync.  Can
> your mock just call the regular constructor, passing in an empty model_path
and
> null request_context_getter?

Done.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_service.h (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_service.h:36: #include
"chrome/browser/tab_contents/navigation_controller.h"
On 2011/02/11 19:39:42, lzheng wrote:
> From the changes, it is not obvious to me hy these headers are added either.
> Double check?

Done.

http://codereview.chromium.org/6014003/diff/21001/chrome/common/safebrowsing_...
File chrome/common/safebrowsing_messages.cc (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/common/safebrowsing_...
chrome/common/safebrowsing_messages.cc:4: 
On 2011/02/11 19:39:42, lzheng wrote:
> Do we need all these header files below? e.g.: I wonder if file_path.h is
> needed.

Done.

http://codereview.chromium.org/6014003/diff/21001/chrome/common/safebrowsing_...
File chrome/common/safebrowsing_messages.h (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/common/safebrowsing_...
chrome/common/safebrowsing_messages.h:24: // renderer.  TODO(noelutz): move
other IPC messages here.
On 2011/02/15 18:54:53, John Abd-El-Malek wrote:
> thanks for creating this new file.  the sooner this TODO done, the better

np.

[jam, 2011-02-15 23:34:42]

lgtm with the one nit

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:50: // Handles the IPC
that is sent from the renderer the a URL is
On 2011/02/15 23:00:55, noelutz wrote:
> On 2011/02/15 18:54:53, John Abd-El-Malek wrote:
> > nit: no need to comment IPC message handlers, since the message itself is
> > commented
> 
> Done.

nit: there's still a comment, what i mean is nothing is needed at all, i.e. to
say that a function handles message X.  we normally don't comment that, since
it's something that's apparent from the function name

[lzheng, 2011-02-16 19:09:31]

LGTM

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:102: // the
SafeBrowsing service class.
Download doesn't use the interstitial page(I should not include that in the
comments). What I meant is that we should avoid creating a new way of showing
warning pages for csd, we should share code with phishing/malware.
 
On 2011/02/15 23:00:55, noelutz wrote:
> On 2011/02/11 19:39:42, lzheng wrote:
> > I will be happy if the DisplayBlockingPage could be refactored so
> > malware/phishing/csd/download could share.
> 
> what does download do right now?  We do share malware/phishing/csd but the csd
> usage of it is a bit hacky.  See comment above.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:29: // TODO(noelutz):
move all client-side detection IPCs to this class.
I agree.
On 2011/02/15 23:00:55, noelutz wrote:
> On 2011/02/11 19:39:42, lzheng wrote:
> > What are other IPC messages that would be moved here? 
> > Looking at Brian's change http://codereview.chromium.org/6398001, 
> > does it make sense to move the ClientSideDetectionService::Observe here? 
> 
> I guess we have three IPCs?
> 
> Browser -> Renderer:
>   - send model file to the renderer
>   - tell the renderer to start classification
> 
> Renderer -> Browser:
>   - renderer tells the browser that a page is phishing.
> 
> I was thinking we could move the other two IPCs here but definitely in a
> separate CL.  Thoughts?

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:75:
csd_host_->set_safe_browsing_service(sb_service_.get());
Hm, let's stay with what you have for now. It is not too bad. In case we need to
mock out the csd_host someday, I think we could use the factory approach.


On 2011/02/15 23:00:55, noelutz wrote:
> On 2011/02/11 19:39:42, lzheng wrote:
> > You can get rid of these two set_xx by using
> > SafeBrowsingService::RegisterFactory() and also create something similar for
> csd
> > service. You can use safe_browsing_blocking_page_test.cc as an example. The
> > basic idea is to create a factory that will use your mock as the
> implementation.
> 
> I don't think that would work.  Another reason I inject the service objects
like
> this is because there is no resource dispatcher host object in my unit test. 
> Even if I register a special safebrowsing service factory sb_service_ will be
> NULL in my test. if I don't use these setters.
> 
> An alternative would be to create a factory class for the new client-side
> detection host class.  TabContents would use the factory to create an
instance. 
> In the test I could provide a test factory instance that would create the csd
> host object with the two mock service instances.  Would you like that better
> than the current solution?

[noelutz, 2011-02-16 21:24:43]

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:102: // the
SafeBrowsing service class.
On 2011/02/16 19:09:32, lzheng wrote:
> Download doesn't use the interstitial page(I should not include that in the
> comments). What I meant is that we should avoid creating a new way of showing
> warning pages for csd, we should share code with phishing/malware.
>  
> On 2011/02/15 23:00:55, noelutz wrote:
> > On 2011/02/11 19:39:42, lzheng wrote:
> > > I will be happy if the DisplayBlockingPage could be refactored so
> > > malware/phishing/csd/download could share.
> > 
> > what does download do right now?  We do share malware/phishing/csd but the
csd
> > usage of it is a bit hacky.  See comment above.
> 

I'm not sure I fully understand your comment.  This code does reuse the way we
show the phishing warning.  My point in the TODO is that the only reason we use
the SafeBrowsingService class and go through the IO thread is because the
service class keeps a whitelist around that contains sites where the client has
clicked through the warning.  I wouldn't want a phishing warning to show
followed by a csd warning.  Other than that we reuse all the code to show an
interstitial (we couldn't reuse more code I don't think).  

My comment basically suggests that we may want to move the whitelist logic that
is currently in the safebrowsing service class to the safebrowsing blocking page
class so that we don't need to use the service class here (and so that we don't
need to go through the IO thread).

Does that make sense?  I think this can be done in a separate CL though.

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:50: // Handles the IPC
that is sent from the renderer the a URL is
On 2011/02/15 23:34:42, John Abd-El-Malek wrote:
> On 2011/02/15 23:00:55, noelutz wrote:
> > On 2011/02/15 18:54:53, John Abd-El-Malek wrote:
> > > nit: no need to comment IPC message handlers, since the message itself is
> > > commented
> > 
> > Done.
> 
> nit: there's still a comment, what i mean is nothing is needed at all, i.e. to
> say that a function handles message X.  we normally don't comment that, since
> it's something that's apparent from the function name

Done.

[lzheng1, 2011-02-16 21:59:03]

On Wed, Feb 16, 2011 at 1:24 PM, <noelutz@google.com> wrote:

>
>
>
http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
> File chrome/browser/safe_browsing/client_side_detection_host.cc (right):
>
>
>
http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
> chrome/browser/safe_browsing/client_side_detection_host.cc:102: // the
> SafeBrowsing service class.
> On 2011/02/16 19:09:32, lzheng wrote:
>
>> Download doesn't use the interstitial page(I should not include that
>>
> in the
>
>> comments). What I meant is that we should avoid creating a new way of
>>
> showing
>
>> warning pages for csd, we should share code with phishing/malware.
>>
>
>  On 2011/02/15 23:00:55, noelutz wrote:
>> > On 2011/02/11 19:39:42, lzheng wrote:
>> > > I will be happy if the DisplayBlockingPage could be refactored so
>> > > malware/phishing/csd/download could share.
>> >
>> > what does download do right now?  We do share malware/phishing/csd
>>
> but the csd
>
>> > usage of it is a bit hacky.  See comment above.
>>
>
>
> I'm not sure I fully understand your comment.  This code does reuse the
> way we show the phishing warning.  My point in the TODO is that the only
> reason we use the SafeBrowsingService class and go through the IO thread
> is because the service class keeps a whitelist around that contains
> sites where the client has clicked through the warning.  I wouldn't want
> a phishing warning to show followed by a csd warning.  Other than that
> we reuse all the code to show an interstitial (we couldn't reuse more
> code I don't think).
>
> My comment basically suggests that we may want to move the whitelist
> logic that is currently in the safebrowsing service class to the
> safebrowsing blocking page class so that we don't need to use the
> service class here (and so that we don't need to go through the IO
> thread).
>
> Does that make sense?  I think this can be done in a separate CL though.
>
>
We are thinking about the same thing. I was a little bit confused by the
todo's wording because I thought you are going to do something on the
ClientSideDetectionHost
side. Maybe the TODO should in safebrowsing service rather than here. Can
you either drop this todo comment or make it clear where the refactor should
be done?

Thanks!

Lei


>
>
>
http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
> File chrome/browser/safe_browsing/client_side_detection_host.h (right):
>
>
>
http://codereview.chromium.org/6014003/diff/21001/chrome/browser/safe_browsin...
> chrome/browser/safe_browsing/client_side_detection_host.h:50: // Handles
> the IPC that is sent from the renderer the a URL is
> On 2011/02/15 23:34:42, John Abd-El-Malek wrote:
>
>> On 2011/02/15 23:00:55, noelutz wrote:
>> > On 2011/02/15 18:54:53, John Abd-El-Malek wrote:
>> > > nit: no need to comment IPC message handlers, since the message
>>
> itself is
>
>> > > commented
>> >
>> > Done.
>>
>
>  nit: there's still a comment, what i mean is nothing is needed at all,
>>
> i.e. to
>
>> say that a function handles message X.  we normally don't comment
>>
> that, since
>
>> it's something that's apparent from the function name
>>
>
> Done.
>
>
> http://codereview.chromium.org/6014003/
>

[Brian Ryner, 2011-02-16 22:37:10]

Looks nice, just a few minor comments

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:10: #include
"base/gtest_prod_util.h"
I don't think this one is needed anymore.

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:82:
CHECK(io_thread_->Start());
I've been told in the past to prefer ASSERT_TRUE over CHECK in unittest code, so
that if the assertion fails it doesn't bring down the entire test run on the
bots.

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:97:
io_thread_->Stop();
I think the thread stops automatically when it is destroyed.

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:164: // If
there was a message posted on the IO thread to display the
You could factor this out into a helper method, that way you don't have to worry
about duplicating the comment.

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:259: // We
expect that the interstitial is shown for the second phishing URL.
Do you want to add an EXPECT_CALL for the first url with .Times(0) ?

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.h:157: virtual void
DisplayBlockingPage(const GURL& url,
This method has a lot of parameters, as a follow-up it might make sense to group
them into a struct.

[noelutz, 2011-02-16 23:22:35]

Thanks.  Please take a final look.

noe.

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.h:10: #include
"base/gtest_prod_util.h"
On 2011/02/16 22:37:10, Brian Ryner wrote:
> I don't think this one is needed anymore.

Done.

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:82:
CHECK(io_thread_->Start());
On 2011/02/16 22:37:10, Brian Ryner wrote:
> I've been told in the past to prefer ASSERT_TRUE over CHECK in unittest code,
so
> that if the assertion fails it doesn't bring down the entire test run on the
> bots.

Done.

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:97:
io_thread_->Stop();
On 2011/02/16 22:37:10, Brian Ryner wrote:
> I think the thread stops automatically when it is destroyed.

Done.

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:164: // If
there was a message posted on the IO thread to display the
On 2011/02/16 22:37:10, Brian Ryner wrote:
> You could factor this out into a helper method, that way you don't have to
worry
> about duplicating the comment.

Done.

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:164: // If
there was a message posted on the IO thread to display the
On 2011/02/16 22:37:10, Brian Ryner wrote:
> You could factor this out into a helper method, that way you don't have to
worry
> about duplicating the comment.

Done.

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/6014003/diff/42001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.h:157: virtual void
DisplayBlockingPage(const GURL& url,
On 2011/02/16 22:37:10, Brian Ryner wrote:
> This method has a lot of parameters, as a follow-up it might make sense to
group
> them into a struct.

Agreed.

[Brian Ryner, 2011-02-17 04:51:29]

LGTM