Make RenderView not have to know about how PhishingClassifierDelegate.

Make RenderView not have to know about how PhishingClassifierDelegate.


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

[Brian Ryner, 2011-02-07 20:08:19]

Looks nice overall, just a few comments.

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
File chrome/renderer/safe_browsing/phishing_classifier_delegate.cc (right):

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:87:
SetPhishingScorer(g_phishing_scorer.Get().get());
I think this needs to be moved down to below where classifier_ is set on line
89.  Otherwise, classifier_ will still be NULL when SetPhishingScorer tries to
access it on line 102.  I'm surprised none of the test cases caught this, I'll
make sure to add one as a follow-up (or feel free to add it yourself if you'd
like)[

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:143: GURL
url_without_ref = StripToplevelUrl();
Would it ever be possible for this notification to happen after the WebView has
been destroyed?  If so, we would crash trying to access the WebView in
StripToplevelUrl().

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:153:
classifier_page_text_ = page_text;
As I mentioned to you offline, I'd set this up as a swap() to avoid copying a
potentially large string (up to 64KB).  It still seems preferable to avoid
this... what do you think?

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:176: /*
Did you mean to leave this commented-out code here?

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right):

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:149:
RunCallback(false);  // The WebView is going away.
We're not using it currently, but there is a similar NOTREACHED in
phishing_thumbnailer.cc.  Could you fix that one as well?

[jam, 2011-02-07 20:24:49]

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
File chrome/renderer/safe_browsing/phishing_classifier_delegate.cc (right):

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:87:
SetPhishingScorer(g_phishing_scorer.Get().get());
On 2011/02/07 20:08:19, Brian Ryner wrote:
> I think this needs to be moved down to below where classifier_ is set on line
> 89.  Otherwise, classifier_ will still be NULL when SetPhishingScorer tries to
> access it on line 102.  I'm surprised none of the test cases caught this, I'll
> make sure to add one as a follow-up (or feel free to add it yourself if you'd
> like)[

Done.

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:143: GURL
url_without_ref = StripToplevelUrl();
On 2011/02/07 20:08:19, Brian Ryner wrote:
> Would it ever be possible for this notification to happen after the WebView
has
> been destroyed?  If so, we would crash trying to access the WebView in
> StripToplevelUrl().

I don't think this would happen.  But if I'm wrong, then it'll be easy to see
from the crash dumps.

I don't think I changed the logic here from before, so it should be the same.

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:153:
classifier_page_text_ = page_text;
On 2011/02/07 20:08:19, Brian Ryner wrote:
> As I mentioned to you offline, I'd set this up as a swap() to avoid copying a
> potentially large string (up to 64KB).  It still seems preferable to avoid
> this... what do you think?

Yeah, I thought about it.  But I decided to skip this optimization because it
breaks the abstraction between RenderView and its observers.  It's preferable to
not add constraints on the ordering of observers.  I think if profiling shows
that this is needed, we can reconsider, but until then best to keep the API
simple.

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:176: /*
On 2011/02/07 20:08:19, Brian Ryner wrote:
> Did you mean to leave this commented-out code here?

yep.  I can't uncomment it until a message is added, which is awaiting
http://codereview.chromium.org/6398001/

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right):

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:149:
RunCallback(false);  // The WebView is going away.
On 2011/02/07 20:08:19, Brian Ryner wrote:
> We're not using it currently, but there is a similar NOTREACHED in
> phishing_thumbnailer.cc.  Could you fix that one as well?

Done.

[Brian Ryner, 2011-02-07 21:39:41]

LGTM

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
File chrome/renderer/safe_browsing/phishing_classifier_delegate.cc (right):

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:153:
classifier_page_text_ = page_text;
On 2011/02/07 20:24:49, John Abd-El-Malek wrote:
> On 2011/02/07 20:08:19, Brian Ryner wrote:
> > As I mentioned to you offline, I'd set this up as a swap() to avoid copying
a
> > potentially large string (up to 64KB).  It still seems preferable to avoid
> > this... what do you think?
> 
> Yeah, I thought about it.  But I decided to skip this optimization because it
> breaks the abstraction between RenderView and its observers.  It's preferable
to
> not add constraints on the ordering of observers.  I think if profiling shows
> that this is needed, we can reconsider, but until then best to keep the API
> simple.

Ok, sounds reasonable.

http://codereview.chromium.org/6250176/diff/7004/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:176: /*
On 2011/02/07 20:24:49, John Abd-El-Malek wrote:
> On 2011/02/07 20:08:19, Brian Ryner wrote:
> > Did you mean to leave this commented-out code here?
> 
> yep.  I can't uncomment it until a message is added, which is awaiting
> http://codereview.chromium.org/6398001/

Ok.  If you end up submitting this first, I'll merge it into my change.