Rewrite PhishingClassifierDelegateTest using ChromeRenderViewTest to run with multi-process

Rewrite PhishingClassifierDelegateTest by using RenderViewTest, such that it can run with
--site-per-process flag.

Also resolved the flakiness as a side product.

BUG=373155, 568710
Committed: https://crrev.com/69b3ebd9e244ae0211b3621f52ab4a620294d2b9
Cr-Commit-Position: refs/heads/master@{#372223}

[Jialiu Lin, 2016-01-22 18:56:45]

Description was changed from

==========
first attemp

BUG=
==========

to

==========
Rewrite PhishingClassifierDelegateTest by using RenderViewTest, such that it can
run with
--site-per-process flag.

Also resolved the flakiness as a side product.  

BUG=373155,568710
==========

[Jialiu Lin, 2016-01-22 23:22:14]

Patchset #7 (id:70005) has been deleted

[Jialiu Lin, 2016-01-26 00:11:24]

jialiul@chromium.org changed reviewers:
+ nparker@chromium.org

[Jialiu Lin, 2016-01-26 00:11:25]

Hi nparker, 
could you take a look at this CL? Thanks!

[Nathan Parker, 2016-01-26 16:49:20]

lgtm

mostly questions and nits... Looks good overall.

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
(right):

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:75: //
Return ture anyway, since we don't want to block other IPC.
s/ture/true

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:80:
scoped_ptr<ClientPhishingRequest> verdict(new ClientPhishingRequest);
nit: Does this need to be a ptr?  Could be just an object on the stack.

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:81: if
(verdict->ParseFromString(verdict_str) && verdict->IsInitialized()) {
Do you want to catch if the msg is not parsable?

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:89:
class PhishingClassifierDelegateTest : public ChromeRenderViewTest {
Q: How does this fit with rv->rf refactoring?

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:153:
Mock::VerifyAndClearExpectations(classifier_);
nit: newline before VerifyAndClearExpectations (or after, as long as its
consistent), so we can see which block it goes with

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:169:
content::PageState back_state = GetCurrentPageState();
What does this do?

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:249:
// Inp page navigation.
"In-page"?

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:264:
// For this test, we'll create the delegate with no scorer available yet.
Do you know when this could happen?  Is if we haven't received a model yet?

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:376:
// a
nit: paragraph flow.

[Jialiu Lin, 2016-01-28 19:55:24]

Thanks, nparker! Your comments are all addressed.

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
(right):

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:75: //
Return ture anyway, since we don't want to block other IPC.
On 2016/01/26 16:49:19, nparker wrote:
> s/ture/true

Done.

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:80:
scoped_ptr<ClientPhishingRequest> verdict(new ClientPhishingRequest);
On 2016/01/26 16:49:19, nparker wrote:
> nit: Does this need to be a ptr?  Could be just an object on the stack.
regular point will do too.

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:81: if
(verdict->ParseFromString(verdict_str) && verdict->IsInitialized()) {
On 2016/01/26 16:49:19, nparker wrote:
> Do you want to catch if the msg is not parsable?

yes, it makes more sense. Done

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:89:
class PhishingClassifierDelegateTest : public ChromeRenderViewTest {
On 2016/01/26 16:49:19, nparker wrote:
> Q: How does this fit with rv->rf refactoring?
ah, this class name (chromeRenderViewTest) is indeed confusing. I asked nasko
the same question before. Basically we don't have a equivalent
ChromeRenderFrameTest fixture yet. But actually,  ChromeRenderViewTest provides
a testing environment that only involves render process, and it provide APIs to
access both render frames and render view (some APIs were added recently for
OOPIF to make render frame test easier). So yes, we can use this one to test
render frame related classes, and I guess this class might be renamed to
something more general and less confusing very soon.

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:153:
Mock::VerifyAndClearExpectations(classifier_);
On 2016/01/26 16:49:19, nparker wrote:
> nit: newline before VerifyAndClearExpectations (or after, as long as its
> consistent), so we can see which block it goes with

Done.

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:169:
content::PageState back_state = GetCurrentPageState();
On 2016/01/26 16:49:19, nparker wrote:
> What does this do?

oops, forgot to delete this line...

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:249:
// Inp page navigation.
On 2016/01/26 16:49:19, nparker wrote:
> "In-page"?

Done.

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:264:
// For this test, we'll create the delegate with no scorer available yet.
On 2016/01/26 16:49:19, nparker wrote:
> Do you know when this could happen?  Is if we haven't received a model yet?

yes, this tests if we haven't received the model yet(or somehow model cannot be
loaded) when we load a page.

https://codereview.chromium.org/1585003002/diff/150001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:376:
// a
On 2016/01/26 16:49:19, nparker wrote:
> nit: paragraph flow.

Done.

[Jialiu Lin, 2016-01-28 23:38:32]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2016-01-28 23:38:33]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/1585003002/#ps210001
(title: "nit")

[commit-bot: I haz the power, 2016-01-28 23:39:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1585003002/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1585003002/210001

[commit-bot: I haz the power, 2016-01-29 00:38:42]

Description was changed from

==========
Rewrite PhishingClassifierDelegateTest by using RenderViewTest, such that it can
run with
--site-per-process flag.

Also resolved the flakiness as a side product.  

BUG=373155,568710
==========

to

==========
Rewrite PhishingClassifierDelegateTest by using RenderViewTest, such that it can
run with
--site-per-process flag.

Also resolved the flakiness as a side product.  

BUG=373155,568710
==========

[commit-bot: I haz the power, 2016-01-29 00:38:42]

Committed patchset #11 (id:210001)

[commit-bot: I haz the power, 2016-01-29 00:40:19]

Description was changed from

==========
Rewrite PhishingClassifierDelegateTest by using RenderViewTest, such that it can
run with
--site-per-process flag.

Also resolved the flakiness as a side product.  

BUG=373155,568710
==========

to

==========
Rewrite PhishingClassifierDelegateTest by using RenderViewTest, such that it can
run with
--site-per-process flag.

Also resolved the flakiness as a side product.

BUG=373155,568710

Committed: https://crrev.com/69b3ebd9e244ae0211b3621f52ab4a620294d2b9
Cr-Commit-Position: refs/heads/master@{#372223}
==========

[commit-bot: I haz the power, 2016-01-29 00:40:19]

Patchset 11 (id:??) landed as
https://crrev.com/69b3ebd9e244ae0211b3621f52ab4a620294d2b9
Cr-Commit-Position: refs/heads/master@{#372223}