[subresource_filter] Move OnReloadRequested to the ChromeClient

[subresource_filter] Move OnReloadRequested to the ChromeClient

This functionality is only needed in the //chrome layer.

This CL doesn't change behavior.

BUG=717590
Review-Url: https://codereview.chromium.org/2890253002
Cr-Commit-Position: refs/heads/master@{#473453}
Committed: https://chromium.googlesource.com/chromium/src/+/8c425f992cc443505b84998b0e20330592ff6393

[Charlie Harrison, 2017-05-18 22:00:41]

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

[Charlie Harrison, 2017-05-18 22:01:28]

Description was changed from

==========
[subresource_filter] Move OnReloadRequested to the ChromeClient

This functionality is only needed in the //chrome layer.

BUG=717590
==========

to

==========
[subresource_filter] Move OnReloadRequested to the ChromeClient

This functionality is only needed in the //chrome layer.

This CL doesn't change behavior.

BUG=717590
==========

[commit-bot: I haz the power, 2017-05-18 22:03:44]

Dry run: CQ is trying da patch.

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

[Charlie Harrison, 2017-05-18 22:04:21]

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

[commit-bot: I haz the power, 2017-05-18 22:05:09]

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-05-19 01:01:50]

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

[commit-bot: I haz the power, 2017-05-19 01:01:50]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-05-19 12:52:45]

csharrison@chromium.org changed reviewers:
+ bmcquade@chromium.org, pkasting@chromium.org

[Charlie Harrison, 2017-05-19 12:52:46]

bmcquade: Could you PTAL?
pkasting: PTAL at chrome/browser/ui? (trivial)

[Charlie Harrison, 2017-05-19 12:55:50]

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

[commit-bot: I haz the power, 2017-05-19 12:56:07]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-19 13:24:21]

lgtm

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/andro...
File
chrome/browser/ui/android/content_settings/subresource_filter_infobar_delegate.cc
(right):

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/content_settings/subresource_filter_infobar_delegate.cc:81:
ChromeSubresourceFilterClient::FromWebContents(web_contents)
just to be sure - the ChromeSubresourceFilterClient can never be null here,
right?

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1269:
ChromeSubresourceFilterClient::FromWebContents(web_contents())
same

[Charlie Harrison, 2017-05-19 13:35:00]

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/andro...
File
chrome/browser/ui/android/content_settings/subresource_filter_infobar_delegate.cc
(right):

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/content_settings/subresource_filter_infobar_delegate.cc:81:
ChromeSubresourceFilterClient::FromWebContents(web_contents)
On 2017/05/19 13:24:21, Bryan McQuade wrote:
> just to be sure - the ChromeSubresourceFilterClient can never be null here,
> right?

Yes, they are both scoped to the WebContents.

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1269:
ChromeSubresourceFilterClient::FromWebContents(web_contents())
On 2017/05/19 13:24:21, Bryan McQuade wrote:
> same

Actually I'm less sure about this one, it's hard for me to track the lifetimes
here since all the pointers are raw ptrs.

pkasting, do you know if the bubble model is scoped to a WebContents lifetime?

[commit-bot: I haz the power, 2017-05-19 14:10:10]

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

[commit-bot: I haz the power, 2017-05-19 14:10:11]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-05-19 16:52:52]

LGTM

[Peter Kasting, 2017-05-19 17:18:13]

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1269:
ChromeSubresourceFilterClient::FromWebContents(web_contents())
On 2017/05/19 13:35:00, Charlie Harrison wrote:
> On 2017/05/19 13:24:21, Bryan McQuade wrote:
> > same
> 
> Actually I'm less sure about this one, it's hard for me to track the lifetimes
> here since all the pointers are raw ptrs.
> 
> pkasting, do you know if the bubble model is scoped to a WebContents lifetime?

I think you meant to ask about the filter client?  The bubble model is |this|
here and isn't really as connected to a WebContents.

I don't know about the filter client, but this patch isn't changing behavior, so
I'm OK with this code.

[Charlie Harrison, 2017-05-19 17:25:51]

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1269:
ChromeSubresourceFilterClient::FromWebContents(web_contents())
On 2017/05/19 17:18:13, Peter Kasting wrote:
> On 2017/05/19 13:35:00, Charlie Harrison wrote:
> > On 2017/05/19 13:24:21, Bryan McQuade wrote:
> > > same
> > 
> > Actually I'm less sure about this one, it's hard for me to track the
lifetimes
> > here since all the pointers are raw ptrs.
> > 
> > pkasting, do you know if the bubble model is scoped to a WebContents
lifetime?
> 
> I think you meant to ask about the filter client?  The bubble model is |this|
> here and isn't really as connected to a WebContents.
> 
> I don't know about the filter client, but this patch isn't changing behavior,
so
> I'm OK with this code.

The filter client is scoped to a WebContents, and is created with standard tab
helpers.
I'm worried about the following:
1. The bubble is shown
2. The underlying WebContents is destroyed (along with the filter client)
3. OnDoneClicked is run.

I looked through this class and it looks like web_contents_ can be reset to
nullptr here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/content_settings/conte...

So I think it might be possible for the filter client to be null here and
require a null check.

[Peter Kasting, 2017-05-19 17:31:52]

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1269:
ChromeSubresourceFilterClient::FromWebContents(web_contents())
On 2017/05/19 17:25:50, Charlie Harrison wrote:
> On 2017/05/19 17:18:13, Peter Kasting wrote:
> > On 2017/05/19 13:35:00, Charlie Harrison wrote:
> > > On 2017/05/19 13:24:21, Bryan McQuade wrote:
> > > > same
> > > 
> > > Actually I'm less sure about this one, it's hard for me to track the
> lifetimes
> > > here since all the pointers are raw ptrs.
> > > 
> > > pkasting, do you know if the bubble model is scoped to a WebContents
> lifetime?
> > 
> > I think you meant to ask about the filter client?  The bubble model is
|this|
> > here and isn't really as connected to a WebContents.
> > 
> > I don't know about the filter client, but this patch isn't changing
behavior,
> so
> > I'm OK with this code.
> 
> The filter client is scoped to a WebContents, and is created with standard tab
> helpers.
> I'm worried about the following:
> 1. The bubble is shown
> 2. The underlying WebContents is destroyed (along with the filter client)
> 3. OnDoneClicked is run.
> 
> I looked through this class and it looks like web_contents_ can be reset to
> nullptr here:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/content_settings/conte...
> 
> So I think it might be possible for the filter client to be null here and
> require a null check.

That listener is really confusing.  I'd look at who added it when and why.  It
doesn't seem offhand like it ought to be needed.

I would expect anything that destroys a WebContents to happen during tab
closure, which should also close open bubbles and the like.  In a UI sense it's
not reasonable to imagine an open bubble for no underlying page.

[Charlie Harrison, 2017-05-19 17:39:06]

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/2890253002/diff/40001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1269:
ChromeSubresourceFilterClient::FromWebContents(web_contents())
On 2017/05/19 17:31:52, Peter Kasting wrote:
> On 2017/05/19 17:25:50, Charlie Harrison wrote:
> > On 2017/05/19 17:18:13, Peter Kasting wrote:
> > > On 2017/05/19 13:35:00, Charlie Harrison wrote:
> > > > On 2017/05/19 13:24:21, Bryan McQuade wrote:
> > > > > same
> > > > 
> > > > Actually I'm less sure about this one, it's hard for me to track the
> > lifetimes
> > > > here since all the pointers are raw ptrs.
> > > > 
> > > > pkasting, do you know if the bubble model is scoped to a WebContents
> > lifetime?
> > > 
> > > I think you meant to ask about the filter client?  The bubble model is
> |this|
> > > here and isn't really as connected to a WebContents.
> > > 
> > > I don't know about the filter client, but this patch isn't changing
> behavior,
> > so
> > > I'm OK with this code.
> > 
> > The filter client is scoped to a WebContents, and is created with standard
tab
> > helpers.
> > I'm worried about the following:
> > 1. The bubble is shown
> > 2. The underlying WebContents is destroyed (along with the filter client)
> > 3. OnDoneClicked is run.
> > 
> > I looked through this class and it looks like web_contents_ can be reset to
> > nullptr here:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/content_settings/conte...
> > 
> > So I think it might be possible for the filter client to be null here and
> > require a null check.
> 
> That listener is really confusing.  I'd look at who added it when and why.  It
> doesn't seem offhand like it ought to be needed.
> 
> I would expect anything that destroys a WebContents to happen during tab
> closure, which should also close open bubbles and the like.  In a UI sense
it's
> not reasonable to imagine an open bubble for no underlying page.

Thanks for the clear explanation, that aligns with my expectations.
I'll go ahead and land this CL and follow up with some code archaeology to
figure out if we can remove that.

[Charlie Harrison, 2017-05-19 17:39:36]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2017-05-19 17:40:04]

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-05-19 17:49:54]

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

[commit-bot: I haz the power, 2017-05-19 17:49:55]

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

[Charlie Harrison, 2017-05-20 21:26:19]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-05-20 21:26:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2890253002/#ps60001
(title: "rebase on #473448")

[commit-bot: I haz the power, 2017-05-20 21:26:25]

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-05-20 22:45:57]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495315579271910,
"parent_rev": "3016414cff4dc6b6520a2812282fb2082e81ae84", "commit_rev":
"8c425f992cc443505b84998b0e20330592ff6393"}

[commit-bot: I haz the power, 2017-05-20 22:46:07]

Description was changed from

==========
[subresource_filter] Move OnReloadRequested to the ChromeClient

This functionality is only needed in the //chrome layer.

This CL doesn't change behavior.

BUG=717590
==========

to

==========
[subresource_filter] Move OnReloadRequested to the ChromeClient

This functionality is only needed in the //chrome layer.

This CL doesn't change behavior.

BUG=717590

Review-Url: https://codereview.chromium.org/2890253002
Cr-Commit-Position: refs/heads/master@{#473453}
Committed:
https://chromium.googlesource.com/chromium/src/+/8c425f992cc443505b84998b0e20...
==========

[commit-bot: I haz the power, 2017-05-20 22:46:08]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8c425f992cc443505b84998b0e20...