Moved ios popup opening code from Tab to BlockedPopupTabHelper.

Moved ios popup opening code from Tab to BlockedPopupTabHelper.

BUG=692117
Review-Url: https://codereview.chromium.org/2717613006
Cr-Commit-Position: refs/heads/master@{#454338}
Committed: https://chromium.googlesource.com/chromium/src/+/29ed2a2bcc95599c41501cbf5a1918735bb96d7b

[Eugene But (OOO till 7-30), 2017-02-24 18:38:22]

The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 18:38:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 18:56:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 18:56:27]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-02-24 19:36:53]

eugenebut@chromium.org changed reviewers:
+ rohitrao@chromium.org

[Eugene But (OOO till 7-30), 2017-02-24 19:36:54]

PTAL. All internal tests passed

https://codereview.chromium.org/2717613006/diff/20001/ios/web/web_state/block...
File ios/web/web_state/blocked_popup_info.h (right):

https://codereview.chromium.org/2717613006/diff/20001/ios/web/web_state/block...
ios/web/web_state/blocked_popup_info.h:14: // TODO(crbug.com/692117): Fold this
class into BlockedPopupTabHelper.
I will do this in a separate CL

[rohitrao (ping after 24h), 2017-02-28 22:51:57]

lgtm

https://codereview.chromium.org/2717613006/diff/20001/ios/chrome/browser/web/...
File ios/chrome/browser/web/blocked_popup_tab_helper.mm (right):

https://codereview.chromium.org/2717613006/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.mm:75:
web_state_->OpenURL(params);
Is there any way for this |web_state_| pointer to go stale?  The infobar manager
should be destroyed along with the webstate, so I think the answer is no?

https://codereview.chromium.org/2717613006/diff/20001/ios/chrome/browser/web/...
File ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm (right):

https://codereview.chromium.org/2717613006/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm:30:
WebState::Create(WebState::CreateParams(GetBrowserState()))) {
Why do we need a full WebState for this test now?

https://codereview.chromium.org/2717613006/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm:122:
TEST_F(BlockedPopupTabHelperTest, ShowAndDismissInfoBar) {
Is there value in having a test that adds the infobar and then destroys the
webstate without choosing an infobar button?

[Eugene But (OOO till 7-30), 2017-02-28 23:50:22]

Thanks!

https://codereview.chromium.org/2717613006/diff/20001/ios/chrome/browser/web/...
File ios/chrome/browser/web/blocked_popup_tab_helper.mm (right):

https://codereview.chromium.org/2717613006/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.mm:75:
web_state_->OpenURL(params);
On 2017/02/28 22:51:57, rohitrao wrote:
> Is there any way for this |web_state_| pointer to go stale?  The infobar
manager
> should be destroyed along with the webstate, so I think the answer is no?
Correct, the answer is no. I tested this in debugger.

https://codereview.chromium.org/2717613006/diff/20001/ios/chrome/browser/web/...
File ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm (right):

https://codereview.chromium.org/2717613006/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm:30:
WebState::Create(WebState::CreateParams(GetBrowserState()))) {
On 2017/02/28 22:51:57, rohitrao wrote:
> Why do we need a full WebState for this test now?
To test that OpenURLFromWebState callback is called on WebStateDelegate.
Actually ChromeWebTest already creates a real WebState, so I will just use it.

https://codereview.chromium.org/2717613006/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm:122:
TEST_F(BlockedPopupTabHelperTest, ShowAndDismissInfoBar) {
On 2017/02/28 22:51:57, rohitrao wrote:
> Is there value in having a test that adds the infobar and then destroys the
> webstate without choosing an infobar button?
I think you added this test because you wanted to test IsObservingSources()
call. I would be happy to remove this test because I'm not a fan of testing
private API. Sounds good?

[rohitrao (ping after 24h), 2017-03-01 15:01:17]

lgtm

https://codereview.chromium.org/2717613006/diff/20001/ios/chrome/browser/web/...
File ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm (right):

https://codereview.chromium.org/2717613006/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm:122:
TEST_F(BlockedPopupTabHelperTest, ShowAndDismissInfoBar) {
On 2017/02/28 23:50:22, Eugene But wrote:
> On 2017/02/28 22:51:57, rohitrao wrote:
> > Is there value in having a test that adds the infobar and then destroys the
> > webstate without choosing an infobar button?
> I think you added this test because you wanted to test IsObservingSources()
> call. I would be happy to remove this test because I'm not a fan of testing
> private API. Sounds good?

Sorry, I was asking if we should add a new test that creates an infobar and then
destroys the webstate without first dismissing the infobar.  That might not be
necessary if we're confident that we can't have a dangling web_state_ pointer.

I'd like to keep this ShowAndDismissInfoBar test, because I think it's testing
something useful.  Let's grab baxley and chat about this sometime this week.

[Eugene But (OOO till 7-30), 2017-03-01 16:30:25]

Thanks! Added a test for destroying WebState

https://codereview.chromium.org/2717613006/diff/20001/ios/chrome/browser/web/...
File ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm (right):

https://codereview.chromium.org/2717613006/diff/20001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm:122:
TEST_F(BlockedPopupTabHelperTest, ShowAndDismissInfoBar) {
On 2017/03/01 15:01:16, rohitrao wrote:
> On 2017/02/28 23:50:22, Eugene But wrote:
> > On 2017/02/28 22:51:57, rohitrao wrote:
> > > Is there value in having a test that adds the infobar and then destroys
the
> > > webstate without choosing an infobar button?
> > I think you added this test because you wanted to test IsObservingSources()
> > call. I would be happy to remove this test because I'm not a fan of testing
> > private API. Sounds good?
> 
> Sorry, I was asking if we should add a new test that creates an infobar and
then
> destroys the webstate without first dismissing the infobar.  That might not be
> necessary if we're confident that we can't have a dangling web_state_ pointer.
> 
> I'd like to keep this ShowAndDismissInfoBar test, because I think it's testing
> something useful.  Let's grab baxley and chat about this sometime this week.
Oh, that would be a useful test. Added.

Concerting testing via public v.s. private API. I do believe that testing
private API has more drawbacks then benefits. Those tests are not very useful
for catching regressions (they often fail during refactorings), they require
more maintenance and they rarely catch important things (important things are
usually states verifiable via public API). TabTest is a good example: it almost
always fail during my refactorings, I have to spend time fixing them every time
I touch Tab, those tests never caught a regression after my changes.
go/unit-test-practices?cl=head#public-apis has a good guidance on testing via
private API.

[Eugene But (OOO till 7-30), 2017-03-01 16:30:30]

The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 16:30:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 16:43:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 16:43:20]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-03-02 19:09:00]

The CQ bit was checked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2017-03-02 19:09:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2717613006/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2017-03-02 19:09:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 19:30:48]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488481740939630,
"parent_rev": "cf829c4a52608579f667e6f313a03f412117f926", "commit_rev":
"29ed2a2bcc95599c41501cbf5a1918735bb96d7b"}

[commit-bot: I haz the power, 2017-03-02 19:31:25]

Description was changed from

==========
Moved ios popup opening code from Tab to BlockedPopupTabHelper.

BUG=692117
==========

to

==========
Moved ios popup opening code from Tab to BlockedPopupTabHelper.

BUG=692117

Review-Url: https://codereview.chromium.org/2717613006
Cr-Commit-Position: refs/heads/master@{#454338}
Committed:
https://chromium.googlesource.com/chromium/src/+/29ed2a2bcc95599c41501cbf5a19...
==========

[commit-bot: I haz the power, 2017-03-02 19:31:30]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/29ed2a2bcc95599c41501cbf5a19...