[subresource_filter] Move OnReloadRequested to the ChromeClient

chrome/browser/ui/content_settings/content_setting_bubble_model.cc

Index: chrome/browser/ui/content_settings/content_setting_bubble_model.cc
diff --git a/chrome/browser/ui/content_settings/content_setting_bubble_model.cc b/chrome/browser/ui/content_settings/content_setting_bubble_model.cc
index 61b1e579511fa09ef6befd8511842d429c4829ce..f8e337ad251a526c749d3f13531ba50b54621806 100644
--- a/chrome/browser/ui/content_settings/content_setting_bubble_model.cc
+++ b/chrome/browser/ui/content_settings/content_setting_bubble_model.cc
@@ -47,7 +47,6 @@
 #include "components/rappor/public/rappor_utils.h"
 #include "components/rappor/rappor_service_impl.h"
 #include "components/strings/grit/components_strings.h"
-#include "components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h"
 #include "components/url_formatter/elide_url.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_frame_host.h"
@@ -1267,9 +1266,8 @@ void ContentSettingSubresourceFilterBubbleModel::OnManageCheckboxChecked(
 
 void ContentSettingSubresourceFilterBubbleModel::OnDoneClicked() {
   if (is_checked_) {
-    auto* driver_factory = subresource_filter::
-        ContentSubresourceFilterDriverFactory::FromWebContents(web_contents());
-    driver_factory->OnReloadRequested();
+    ChromeSubresourceFilterClient::FromWebContents(web_contents())

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

same

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

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?

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

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:50]

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]

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]

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.

+        ->OnReloadRequested();
   }
 }