[ios] Convert BlockedPopupHandler into a tab helper class.

[ios] Convert BlockedPopupHandler into a tab helper class.

BUG=681867
Review-Url: https://codereview.chromium.org/2640463002
Cr-Commit-Position: refs/heads/master@{#445726}
Committed: https://chromium.googlesource.com/chromium/src/+/d6eb8548aca00c0743e5d315e41f481436360dbd

[rohitrao (ping after 24h), 2017-01-17 17:09:26]

rohitrao@chromium.org changed reviewers:
+ eugenebut@chromium.org, marq@chromium.org, sdefresne@chromium.org

[rohitrao (ping after 24h), 2017-01-17 17:09:26]

Checking to see if everyone is ok with the general approach here.  I still need
to rename the class to BlockedPopupTabHelper.

Sylvain, one question specifically for you:  BlockedPopupHandler uses
InfoBarManagerImpl, which is another WebStateUserData/TabHelper class.  How do
we handle dependencies between WebStateUserData?

I chose to create an Initialize() method, with the idea that we'd put all of the
Initialize()s after the calls to CreateFromWebState().

Another option is to make sure CreateFromWebState()s are called in the proper
order, but that's very finicky and easy to break.

A third option is to simply disallow these dependencies, but that seems
unworkable in practice?

[sdefresne, 2017-01-17 17:36:56]

On 2017/01/17 17:09:26, rohitrao wrote:
> Checking to see if everyone is ok with the general approach here.  I still
need
> to rename the class to BlockedPopupTabHelper.
> 
> Sylvain, one question specifically for you:  BlockedPopupHandler uses
> InfoBarManagerImpl, which is another WebStateUserData/TabHelper class.  How do
> we handle dependencies between WebStateUserData?
> 
> I chose to create an Initialize() method, with the idea that we'd put all of
the
> Initialize()s after the calls to CreateFromWebState().
> 
> Another option is to make sure CreateFromWebState()s are called in the proper
> order, but that's very finicky and easy to break.
> 
> A third option is to simply disallow these dependencies, but that seems
> unworkable in practice?

I don't see any mechanism upstream to deal with dependencies between TabHelper.
In fact the design document says that code should support WebContents with only
a subset of the attached TabHelper
(https://chromium.googlesource.com/chromium/src/+/master/docs/tab_helpers.md).
So that would be option 3 (disallow dependencies). If desktop can do it, iOS
should be able to do it too, right?

For your particular code, it looks like you are not interested in the
notification from InfobarManager until you create infobar_ (since the overloaded
method check that the passed infobar is equal to infobar_ first), so I think you
can remove the Initialize method and instead register as a listener when the
infobar_ is initialized / cleared. WDYT?

[Eugene But (OOO till 7-30), 2017-01-17 18:04:53]

https://codereview.chromium.org/2640463002/diff/1/ios/chrome/browser/web/bloc...
File ios/chrome/browser/web/blocked_popup_handler.h (right):

https://codereview.chromium.org/2640463002/diff/1/ios/chrome/browser/web/bloc...
ios/chrome/browser/web/blocked_popup_handler.h:14: #include
"ios/web/public/web_state/web_state_user_data.h"
nit: s/include/import

https://codereview.chromium.org/2640463002/diff/1/ios/chrome/browser/web/bloc...
ios/chrome/browser/web/blocked_popup_handler.h:23: static void
CreateForWebState(web::WebState* web_state);
Do you actually need this method? WebStateUserData has it already:
https://cs.chromium.org/chromium/src/ios/web/public/web_state/web_state_user_...

https://codereview.chromium.org/2640463002/diff/1/ios/chrome/browser/web/bloc...
ios/chrome/browser/web/blocked_popup_handler.h:26: // Initalizes this
BlockedPopupHandler.  Must be called after all tab helpers
Would Sylvain's proposal work for you? Alternatively CreateForWebState could
take InfoBarManager object to enforce the dependency in the code.

[rohitrao (ping after 24h), 2017-01-17 21:43:17]

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

[commit-bot: I haz the power, 2017-01-17 21:43:47]

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

[rohitrao (ping after 24h), 2017-01-17 21:46:24]

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

[commit-bot: I haz the power, 2017-01-17 21:46:31]

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

[rohitrao (ping after 24h), 2017-01-17 21:49:29]

Let's start with Option 3 and see how far we can get without allowing
interdependencies.  I agree that this would be the best approach, if we can get
away with it.

https://codereview.chromium.org/2640463002/diff/1/ios/chrome/browser/web/bloc...
File ios/chrome/browser/web/blocked_popup_handler.h (right):

https://codereview.chromium.org/2640463002/diff/1/ios/chrome/browser/web/bloc...
ios/chrome/browser/web/blocked_popup_handler.h:14: #include
"ios/web/public/web_state/web_state_user_data.h"
On 2017/01/17 18:04:53, Eugene But wrote:
> nit: s/include/import

I thought there was a presubmit that would have caught this.  Is that not
running yet?

https://codereview.chromium.org/2640463002/diff/1/ios/chrome/browser/web/bloc...
ios/chrome/browser/web/blocked_popup_handler.h:23: static void
CreateForWebState(web::WebState* web_state);
On 2017/01/17 18:04:53, Eugene But wrote:
> Do you actually need this method? WebStateUserData has it already:
>
https://cs.chromium.org/chromium/src/ios/web/public/web_state/web_state_user_...

Done.

https://codereview.chromium.org/2640463002/diff/1/ios/chrome/browser/web/bloc...
ios/chrome/browser/web/blocked_popup_handler.h:26: // Initalizes this
BlockedPopupHandler.  Must be called after all tab helpers
On 2017/01/17 18:04:53, Eugene But wrote:
> Would Sylvain's proposal work for you? Alternatively CreateForWebState could
> take InfoBarManager object to enforce the dependency in the code.

Sylvain's suggestion allows me to sidestep this issue for now.

[Eugene But (OOO till 7-30), 2017-01-17 22:10:21]

lgtm. Could you please add a unittest for TabHelper

https://codereview.chromium.org/2640463002/diff/1/ios/chrome/browser/web/bloc...
File ios/chrome/browser/web/blocked_popup_handler.h (right):

https://codereview.chromium.org/2640463002/diff/1/ios/chrome/browser/web/bloc...
ios/chrome/browser/web/blocked_popup_handler.h:14: #include
"ios/web/public/web_state/web_state_user_data.h"
On 2017/01/17 21:49:29, rohitrao wrote:
> On 2017/01/17 18:04:53, Eugene But wrote:
> > nit: s/include/import
> 
> I thought there was a presubmit that would have caught this.  Is that not
> running yet?
Still on review :(
https://codereview.chromium.org/2598173002/

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/blocked_popup_tab_helper.h (right):

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.h:16: #include "url/gurl.h"
nit: is this include still needed?

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.h:27: // Show the popup blocker
infobar for the given popup.
nit: s/Show/Shows

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.h:36: // Show the infobar for
the current popups. Will also handle replacing an
ditto

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

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.mm:94:
BlockedPopupTabHelper::~BlockedPopupTabHelper() {}
nit: 

BlockedPopupTabHelper::~BlockedPopupTabHelper() = default;

[sdefresne, 2017-01-18 10:20:57]

lgtm

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

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.mm:106: popups_.clear();
nit: you can stop observing at that point, can't you?

  popups_.clear();
  scoped_observer_.RemoveAll();

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.mm:121:
RegisterAsInfoBarManagerObserverIfNeeded();
nit: maybe pass the infobar manager as parameter (since the method access it
again as first thing)

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.mm:127:
std::unique_ptr<ConfirmInfoBarDelegate>(
nit: I think you can use base::MakeUnique here

  std::unique_ptr<infobars::InfoBar> infobar =
      infobar_manager->CreateConfirmInfoBar(
          base::MakeUnique<BlockPopupInfoBarDelegate>(
              browser_state, popups_));

[marq (ping after 24h), 2017-01-18 14:22:09]

LGTM

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

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.mm:69: for (it =
popups_.begin(); it != popups_.end(); ++it) {
for (auto& it : popups_)  ?

[rohitrao (ping after 24h), 2017-01-23 15:29:01]

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

[commit-bot: I haz the power, 2017-01-23 15:29:12]

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-01-23 15:30:22]

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

[commit-bot: I haz the power, 2017-01-23 15:30:23]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[rohitrao (ping after 24h), 2017-01-23 15:30:29]

Added a unittest and addressed comments.  Had to add a DEPS exception for
BlockedPopupInfo.

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/blocked_popup_tab_helper.h (right):

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.h:16: #include "url/gurl.h"
On 2017/01/17 22:10:21, Eugene But wrote:
> nit: is this include still needed?

Done.

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.h:27: // Show the popup blocker
infobar for the given popup.
On 2017/01/17 22:10:21, Eugene But wrote:
> nit: s/Show/Shows

Done.

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.h:36: // Show the infobar for
the current popups. Will also handle replacing an
On 2017/01/17 22:10:21, Eugene But wrote:
> ditto

Done.

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

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.mm:69: for (it =
popups_.begin(); it != popups_.end(); ++it) {
On 2017/01/18 14:22:09, marq wrote:
> for (auto& it : popups_)  ?

Done.

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.mm:94:
BlockedPopupTabHelper::~BlockedPopupTabHelper() {}
On 2017/01/17 22:10:21, Eugene But wrote:
> nit: 
> 
> BlockedPopupTabHelper::~BlockedPopupTabHelper() = default;

Is it better to use default rather than {}?

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.mm:106: popups_.clear();
On 2017/01/18 10:20:56, sdefresne wrote:
> nit: you can stop observing at that point, can't you?
> 
>   popups_.clear();
>   scoped_observer_.RemoveAll();

Done.

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.mm:121:
RegisterAsInfoBarManagerObserverIfNeeded();
On 2017/01/18 10:20:57, sdefresne wrote:
> nit: maybe pass the infobar manager as parameter (since the method access it
> again as first thing)

Done.

https://codereview.chromium.org/2640463002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/blocked_popup_tab_helper.mm:127:
std::unique_ptr<ConfirmInfoBarDelegate>(
On 2017/01/18 10:20:57, sdefresne wrote:
> nit: I think you can use base::MakeUnique here
> 
>   std::unique_ptr<infobars::InfoBar> infobar =
>       infobar_manager->CreateConfirmInfoBar(
>           base::MakeUnique<BlockPopupInfoBarDelegate>(
>               browser_state, popups_));

Done.

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

lgtm! Thank you for the tests!

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

https://codereview.chromium.org/2640463002/diff/100001/ios/chrome/browser/web...
ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm:60:
EXPECT_TRUE(IsObservingSources());
From Google Testing Guidelines: "Test code using real APIs - don’t define back
doors only for tests."
https://goto.google.com/unit-test-practices?cl=head#public-apis

For this specific test, it does not look like it is important to know that
InfobarManager is being observed by BlockedPopupTabHelper.

[rohitrao (ping after 24h), 2017-01-24 13:16:03]

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

https://codereview.chromium.org/2640463002/diff/100001/ios/chrome/browser/web...
ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm:60:
EXPECT_TRUE(IsObservingSources());
On 2017/01/23 16:37:47, Eugene But wrote:
> From Google Testing Guidelines: "Test code using real APIs - don’t define back
> doors only for tests."
> https://goto.google.com/unit-test-practices?cl=head#public-apis
> 
> For this specific test, it does not look like it is important to know that
> InfobarManager is being observed by BlockedPopupTabHelper.

Noted.  Leaving this in for now, but will followup via email with you and
baxley.

[rohitrao (ping after 24h), 2017-01-24 13:16:07]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2017-01-24 13:16:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, marq@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2640463002/#ps120001
(title: "Rebased.")

[commit-bot: I haz the power, 2017-01-24 13:16: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-01-24 13:48:38]

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

[commit-bot: I haz the power, 2017-01-24 13:48:39]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[rohitrao (ping after 24h), 2017-01-24 14:01:47]

The CQ bit was checked by rohitrao@chromium.org

[commit-bot: I haz the power, 2017-01-24 14:02:02]

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-01-24 14:44:33]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485266507181580,
"parent_rev": "427645407dc20735347e185b9875b04a26c92f14", "commit_rev":
"d6eb8548aca00c0743e5d315e41f481436360dbd"}

[commit-bot: I haz the power, 2017-01-24 14:45:04]

Description was changed from

==========
[ios] Convert BlockedPopupHandler into a tab helper class.

BUG=681867
==========

to

==========
[ios] Convert BlockedPopupHandler into a tab helper class.

BUG=681867

Review-Url: https://codereview.chromium.org/2640463002
Cr-Commit-Position: refs/heads/master@{#445726}
Committed:
https://chromium.googlesource.com/chromium/src/+/d6eb8548aca00c0743e5d315e41f...
==========

[commit-bot: I haz the power, 2017-01-24 14:45:06]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/d6eb8548aca00c0743e5d315e41f...